Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing multiple bugs in credential generation + refactoring #19653

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
+403 −261
Open

Fixing multiple bugs in credential generation + refactoring #19653

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7cab903
CredentialCollection: adding various tests that currently fails to de…
Mathiou04 Nov 15, 2024
5d7d8cf
Fixes trivial bugs when combining additional_publics with user_as_pass
Mathiou04 Nov 18, 2024
397f466
Fixes the fact that no nil credential is generated for additional_pub…
Mathiou04 Nov 18, 2024
5fce8d2
Fix incorrect expectations of currently failing tests
Mathiou04 Nov 18, 2024
383dc0d
Re-implement the each_unfiltered_password_first method (used in case …
Mathiou04 Nov 18, 2024
e03f2e2
Inline all specs to avoid nesting context
Mathiou04 Dec 13, 2024
b2cc863
Reverts some test expectations
cgranleese-r7 Jan 28, 2025
2047671
Some final tidy up
cgranleese-r7 Jan 28, 2025
033dd2f
First refactoring pass in order to dry the code that iterates on pass…
Mathiou04 Dec 13, 2024
7612b31
Re-revert tests expectations
Mathiou04 Feb 7, 2025
66ce020
Merge branch 'master' into fix_bugs_in_credentials_collection_enumera…
Mathiou04 Feb 11, 2025
b08a5f6
Change how ldap_login generate its specific credentials for SCHANNEL …
Mathiou04 Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 63 additions & 35 deletions lib/metasploit/framework/credential_collection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,52 +280,98 @@ def each_unfiltered_password_first
yield Metasploit::Framework::Credential.new(public: '', private: '', realm: realm, private_type: :password)
end

if user_as_pass
if nil_passwords
if username.present?
yield Metasploit::Framework::Credential.new(public: username, private: nil, realm: realm, private_type: :password)
end

if user_fd
user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: user_from_file, private: user_from_file, realm: realm, private_type: private_type(password))
yield Metasploit::Framework::Credential.new(public: user_from_file, private: nil, realm: realm, private_type: :password)
end
user_fd.seek(0)
end

additional_publics.each do |add_public|
yield Metasploit::Framework::Credential.new(public: add_public, private: nil, realm: realm, private_type: :password)
end
end

if password.present?
if nil_passwords
yield Metasploit::Framework::Credential.new(public: username, private: nil, realm: realm, private_type: :password)
end
if username.present?
yield Metasploit::Framework::Credential.new(public: username, private: password, realm: realm, private_type: private_type(password))
end
if user_as_pass

if user_fd
user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: user_from_file, private: password, realm: realm, private_type: private_type(password))
end
user_fd.seek(0)
end

additional_publics.each do |add_public|
yield Metasploit::Framework::Credential.new(public: add_public, private: password, realm: realm, private_type: private_type(password))
end
end

if user_as_pass
if username.present?
yield Metasploit::Framework::Credential.new(public: username, private: username, realm: realm, private_type: :password)
end
if blank_passwords

if user_fd
user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: user_from_file, private: user_from_file, realm: realm, private_type: private_type(password))
end
user_fd.seek(0)
end

additional_publics.each do |add_public|
yield Metasploit::Framework::Credential.new(public: add_public, private: add_public, realm: realm, private_type: :password)
end
Copy link
Contributor Author

@Mathiou04 Mathiou04 Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can easily see the potential for a refactoring here as many blocks are almost complete c/p of previous blocks.

Copy link
Contributor

@cgranleese-r7 cgranleese-r7 Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, just my opinion but after having another look at this I think refactoring these sections may actually reduce readability.

I find this code hard to read anyway due to all of the nuances required for each possible configuration of password_spray (not something that you have introduced, just the nature of password_spray's requirements itself).

I may be misunderstanding what you had in mind though, so feel free to let me know if that's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a clear direction in mind, I just see potential because of the duplication of this big block:

	if username.present?
          	yield Metasploit::Framework::Credential.new(public: username, private: username, realm: realm, private_type: :password)
        end

        if user_fd
          user_fd.each_line do |user_from_file|
            user_from_file.chomp!
            yield Metasploit::Framework::Credential.new(public: user_from_file, private: user_from_file, realm: realm, private_type: private_type(password))
          end
          user_fd.seek(0)
        end

        additional_publics.each do |add_public|
          yield Metasploit::Framework::Credential.new(public: add_public, private: add_public, realm: realm, private_type: :password)
        end

It bothers me as if we have one slight modification of behaviour, we will have to modify the 3 (or 4, or 5?) of them.

A better target is not clear for me, but I had an idea that I didn't investigate yet: it would be to introduce two new classes, that would essentialy be iterator: one to iterate on usernames, and another one on passwords.

The code would then simply look something like this:

usernames_iterator.each do |username|
  passwords_iterator.each do |password|
    yield Metasploit::Framework::Credential.new...
  end
end

And the password spraying branch would simply look like this:

passwords_iterator.each do |username|
  usernames_iterator.each do |password|
    yield Metasploit::Framework::Credential.new...
  end
end

The main problem would be to handle the user_as_pass parameter in a clean way.
(anyway, maybe we are good enough for now?)

end

if blank_passwords
if username.present?
yield Metasploit::Framework::Credential.new(public: username, private: "", realm: realm, private_type: :password)
end

if user_fd
user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: user_from_file, private: password, realm: realm, private_type: private_type(password))
yield Metasploit::Framework::Credential.new(public: user_from_file, private: "", realm: realm, private_type: :password)
end
user_fd.seek(0)
end

additional_publics.each do |add_public|
yield Metasploit::Framework::Credential.new(public: add_public, private: "", realm: realm, private_type: :password)
end
end

if pass_file.present?
File.open(pass_file, 'r:binary') do |pass_fd|
pass_fd.each_line do |pass_from_file|
pass_from_file.chomp!

if username.present?
yield Metasploit::Framework::Credential.new(public: username, private: pass_from_file, realm: realm, private_type: :password)
yield Metasploit::Framework::Credential.new(public: username, private: pass_from_file, realm: realm, private_type: private_type(pass_from_file))
end

if user_fd
user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: user_from_file, private: pass_from_file, realm: realm, private_type: private_type(pass_from_file))
end
user_fd.seek(0)
end
next unless user_fd

user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: user_from_file, private: pass_from_file, realm: realm, private_type: private_type(pass_from_file))
additional_publics.each do |add_public|
yield Metasploit::Framework::Credential.new(public: add_public, private: pass_from_file, realm: realm, private_type: private_type(pass_from_file))
end
user_fd.seek(0)
end
end
end
Expand All @@ -348,34 +394,16 @@ def each_unfiltered_password_first
if username.present?
yield Metasploit::Framework::Credential.new(public: username, private: add_private, realm: realm, private_type: private_type(add_private))
end
user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: user_from_file, private: add_private, realm: realm, private_type: private_type(add_private))
end
user_fd.seek(0)
end

additional_publics.each do |add_public|
if password.present?
yield Metasploit::Framework::Credential.new(public: add_public, private: password, realm: realm, private_type: private_type(password) )
end
if user_as_pass
yield Metasploit::Framework::Credential.new(public: add_public, private: add_public, realm: realm, private_type: :password)
end
if blank_passwords
yield Metasploit::Framework::Credential.new(public: add_public, private: "", realm: realm, private_type: :password)
end
if nil_passwords
yield Metasploit::Framework::Credential.new(public: add_public, private: nil, realm: realm, private_type: :password)
end
if user_fd
user_fd.each_line do |user_from_file|
user_from_file.chomp!
yield Metasploit::Framework::Credential.new(public: add_public, private: user_from_file, realm: realm, private_type: private_type(user_from_file))
yield Metasploit::Framework::Credential.new(public: user_from_file, private: add_private, realm: realm, private_type: private_type(add_private))
end
user_fd.seek(0)
end
additional_privates.each do |add_private|

additional_publics.each do |add_public|
yield Metasploit::Framework::Credential.new(public: add_public, private: add_private, realm: realm, private_type: private_type(add_private))
end
end
Expand Down
Loading