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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Fixing multiple bugs in credential generation + refactoring #19653

Changes from 1 commit
Commits
Show all changes
6 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
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
250 changes: 250 additions & 0 deletions spec/lib/metasploit/framework/credential_collection_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@
Metasploit::Framework::Credential.new(public: "foo", private: "bar"),
)
end

# REMOVE BEFORE COMMIT: question for the userpass file: do we want to make options work with it or not?
end

context "when given a pass_file and user_file" do
Expand Down Expand Up @@ -311,6 +313,20 @@
Metasploit::Framework::Credential.new(public: username, private: password),
)
end

context "when using password spraying" do
let(:password_spray) { true }

# REMOVE BEFORE COMMIT: yields nothings, fails because of bug in method
context "without password" do
Copy link
Contributor

Choose a reason for hiding this comment

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

What would your thoughts be on not nesting the contexts here. My thinking is it would make searching for failing tests much harder, as the sting would be broken across multiple lines whereas if it was a single context it would be much easier to find.

Here is where I usually check "best practices" when writing tests, just in case it's useful:
https://www.betterspecs.org/#contexts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok with me 👍
I struggled a bit to find a proper organization of the cases, given the already existing specs.

Do you want to merge only those two, or do you think I should also do the same for the other cases?
(meaning having only 1-level deep contexts)

Copy link
Contributor

@cgranleese-r7 cgranleese-r7 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you want to merge only those two, or do you think I should also do the same for the other cases? (meaning having only 1-level deep contexts)

I think having only one context for each test would be better when combined with this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fine with me. I'll change that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bit painful to do but I agree, once it is done, it is much clearer to read and to debug 🎉

let(:password) { nil }
specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: username, private: nil),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fails because the collection yields nothing in case of "password spraying".

)
end
end
end
end

context "when :blank_passwords is true" do
Expand All @@ -323,6 +339,240 @@
end
end

context "when given additional_publics" do
let(:username) { nil }
let(:password) { nil }
let(:additional_publics) { [ "test_public" ] }

context "when :user_as_pass is true" do
let(:user_as_pass) { true }

# REMOVE BEFORE COMMIT currently failing
specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: "test_public", private: "test_public"),
Copy link
Contributor Author

@Mathiou04 Mathiou04 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Crashes because of a non-existent variable.

)
end


context "when using password spraying" do
let(:password_spray) { true }

# REMOVE BEFORE COMMIT currently failing
specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: "test_public", private: "test_public"),
Copy link
Contributor Author

@Mathiou04 Mathiou04 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Crashes because of a non-existent variable.

)
end
end
end

context "when :nil_passwords is true" do
let(:nil_passwords) { true }

# REMOVE BEFORE COMMIT: this option is ignored currently for additional_publics
specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: "test_public", private: nil),
Copy link
Contributor Author

@Mathiou04 Mathiou04 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This case yields nothing: nil passwords are ignored in additional_public credential generation. (is there a reason for this?)

)
end
end

context "when using password spraying" do
let(:password_spray) { true }

context "when :blank_passwords and :nil_password are true" do
let(:blank_passwords) { true }
let(:nil_passwords) { true }

context "with 2 additional_publics" do
let(:additional_publics) { [ "test_public1", "test_public2" ] }

# REMOVE BEFORE COMMIT: fails because no pwd spraying
specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: "test_public1", private: ""),
Metasploit::Framework::Credential.new(public: "test_public2", private: ""),
Metasploit::Framework::Credential.new(public: "test_public1", private: nil),
Metasploit::Framework::Credential.new(public: "test_public2", private: nil),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails because the password spraying option does not work with additional_publics.
It generates the correct list of credentials, but not in the correct order.

)
end
end
end

context "when given a user file" do
let(:user_file) do
filename = "user_file"
stub_file = StringIO.new("asdf\njkl\n")
allow(File).to receive(:open).with(filename, /^r/).and_return stub_file

filename
end

# REMOVE BEFORE COMMIT: this also yields the usernames as passwords for the additional_public
context "when given a password" do
let(:password) { "password" }

specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: "adsf", private: "password"),
Metasploit::Framework::Credential.new(public: "jkl", private: "password"),
Metasploit::Framework::Credential.new(public: "test_public", private: "password"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails because for some reason, in addition to those 3 credentials, it also uses the additional_publics ("test_public") as a password.

)
end
end
end
end
end

context "when using password spraying" do
let(:password_spray) { true }
let(:username) { nil }
let(:password) { nil }

context "when :blank_passwords is true" do
let(:blank_passwords) { true }

context "with password (but no username)" do
let(:password) { "pass" }

# REMOVE BEFORE COMMIT: this yields empty creds (no username, no pass)
specify do
expect { |b| collection.each(&b) }.to yield_successive_args()
Copy link
Contributor Author

@Mathiou04 Mathiou04 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This fails because in this case (no username defined!) it generates a set of empty credentials.

end
end

# REMOVE BEFORE COMMIT: yields nothings, fails because of bug in method
context "with username (but no password)" do
let(:username) { "user" }

specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: username, private: ''),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails because it generates no credential.
But in this case we have a username and are asking for a blank password so it should generate this. (this is the case I chose to describe in the issue)

)
end
end

context "when given a user_file" do
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming from the comment below you want to have blank_passwords set here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually set up on line 434, but with the nested contexts it is not easy to follow along...
I am wondering if it is better to duplicate setup code so as to have 1-level context where you can properly see all the setup, or be a bit more dry and nest.

Happy to follow what you feel is best.

Copy link
Contributor

Choose a reason for hiding this comment

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

better to duplicate setup code so as to have 1-level context where you can properly see all the setup

I would personally be in favor of that as it is much easier to see the configuration for each test.

let(:user_file) do
filename = "foo"
stub_file = StringIO.new("asdf\njkl\n")
allow(File).to receive(:open).with(filename,/^r/).and_return stub_file

filename
end

# REMOVE BEFORE COMMIT: yields nothing, same for blank passwords option
specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
Metasploit::Framework::Credential.new(public: "asdf", private: ''),
Metasploit::Framework::Credential.new(public: "jkl", private: ''),
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fails also because it yields nothing. The given file is completely ignored even if we would like to have the usernames with the blank passwords generated.

end
end
end
end

context "when every possible option is used" do
Copy link
Contributor Author

@Mathiou04 Mathiou04 Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tried to create some generic non-regression tests to demonstrate how the credentials should be yielded in case of various user/password data source (in fact in case of all them being active).

The idea is mainly to check that all credentials are present, but also to have the proper order.
I am not sure they should be kept after the fixes/refacto have been made (as there may be some question regarding the order, many are probably valid?) but here they will help demonstrate what we expect.

let(:nil_passwords) { true }
let(:blank_passwords) { true }
let(:username) { "user" }
let(:password) { "pass" }
let(:user_file) do
filename = "user_file"
stub_file = StringIO.new("userfile")
allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file

filename
end
let(:pass_file) do
filename = "pass_file"
stub_file = StringIO.new("passfile\n")
allow(File).to receive(:open).with(filename,/^r/).and_return stub_file

filename
end
let(:user_as_pass) { false }
let(:userpass_file) do
filename = "userpass_file"
stub_file = StringIO.new("userpass_user userpass_pass\n")
allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file

filename
end
let(:prepended_creds) { ['test_prepend'] }
let(:additional_privates) { ['test_private'] }
let(:additional_publics) { ['test_public'] }

# REMOVE BEFORE COMMIT: fails because of the useraspass error, then fails because of the nil value for addiitonal publics and should be ok then
specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
"test_prepend",
Metasploit::Framework::Credential.new(public: "user", private: nil),
Metasploit::Framework::Credential.new(public: "user", private: "pass"),
Metasploit::Framework::Credential.new(public: "user", private: "user"),
Metasploit::Framework::Credential.new(public: "user", private: ""),
Metasploit::Framework::Credential.new(public: "user", private: "passfile"),
Metasploit::Framework::Credential.new(public: "user", private: "test_private"),
Metasploit::Framework::Credential.new(public: "userfile", private: nil),
Metasploit::Framework::Credential.new(public: "userfile", private: "pass"),
Metasploit::Framework::Credential.new(public: "userfile", private: "userfile"),
Metasploit::Framework::Credential.new(public: "userfile", private: ""),
Metasploit::Framework::Credential.new(public: "userfile", private: "passfile"),
Metasploit::Framework::Credential.new(public: "userfile", private: "test_private"),
Metasploit::Framework::Credential.new(public: "userpass_user", private: "userpass_pass"),
Metasploit::Framework::Credential.new(public: "test_public", private: nil), # missing this case
Metasploit::Framework::Credential.new(public: "test_public", private: "pass"),
Metasploit::Framework::Credential.new(public: "test_public", private: "test_public"),
Metasploit::Framework::Credential.new(public: "test_public", private: ""),
Metasploit::Framework::Credential.new(public: "test_public", private: "passfile"),
Metasploit::Framework::Credential.new(public: "test_public", private: "test_private")
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently failing because of the various bugs that make the other tests fail.

end

context "when using password spraying" do
let(:password_spray) { true }
let(:user_file) do
filename = "user_file"
stub_file = StringIO.new("userfile")
allow(File).to receive(:open).with(filename,/^r/).and_return stub_file

filename
end
let(:pass_file) do
filename = "pass_file"
stub_file = StringIO.new("passfile\n")
allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file

filename
end

specify do
expect { |b| collection.each(&b) }.to yield_successive_args(
"test_prepend",
Metasploit::Framework::Credential.new(public: "user", private: nil),
Metasploit::Framework::Credential.new(public: "userfile", private: nil),
Metasploit::Framework::Credential.new(public: "test_public", private: nil),
Metasploit::Framework::Credential.new(public: "user", private: "pass"),
Metasploit::Framework::Credential.new(public: "userfile", private: "pass"),
Metasploit::Framework::Credential.new(public: "test_public", private: "pass"),
Metasploit::Framework::Credential.new(public: "user", private: "user"),
Metasploit::Framework::Credential.new(public: "userfile", private: "userfile"),
Metasploit::Framework::Credential.new(public: "test_public", private: "test_public"),
Metasploit::Framework::Credential.new(public: "user", private: ""),
Metasploit::Framework::Credential.new(public: "userfile", private: ""),
Metasploit::Framework::Credential.new(public: "test_public", private: ""),
Metasploit::Framework::Credential.new(public: "user", private: "passfile"),
Metasploit::Framework::Credential.new(public: "userfile", private: "passfile"),
Metasploit::Framework::Credential.new(public: "test_public", private: "passfile"),
Metasploit::Framework::Credential.new(public: "userpass_user", private: "userpass_pass"),
Metasploit::Framework::Credential.new(public: "user", private: "test_private"),
Metasploit::Framework::Credential.new(public: "userfile", private: "test_private"),
Metasploit::Framework::Credential.new(public: "test_public", private: "test_private"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently failing because of the various bugs that make the other tests fail.

)
end
end
end
end

describe "#empty?" do
Expand Down
Loading