Fixing multiple bugs in credential generation + refactoring

[Mathiou04]

Fixes #19652

Summary

In this PR, I added various tests on the CredentialCollection class, all of them currently failing and illustrating what I think are bugs.

Issues can be grouped in two categories:

• those linked to the additional_publics array
• those linked to the password_spray option

I added some comments on each test explaining what it is currently yielding (instead of the result I think should be expected - let me know if I misunderstood anything).

Refactoring opportunity

You will see that there are many bugs linked to the password_spray option.

It seems that this branch of the code has been copy/pasted from the original branch (that didn't handle password spraying) and adapted.
There was not a lot of tests on this part of the code + the structure is difficult to maintain which probably explains those issues.

The original code branch (without spraying: #each_unfiltered_username_first) is also a bit complex with a lot of duplicated code.

I think there are some opportunities to simplify and clarify the code a lot with some refactoring.

Non-regression tests

For this, I created 2 additional "non-regression" tests that activate all options and show how the credentials should be yielded.
There is one test with password spraying, one without.

I don't know if we want to keep them after the refactoring, but they will surely help.

Also, even if a part of the order in which the credentials are yielded should be "fixed", there are questions around others.
For example:

• when do we want to yield the credentials generated by the user_as_pass option in case of password_spraying?
• when do we want to yield credentials generated by the "userpass" file?

Additional question

As a side note, the nil_passwords, blank_passwords and user_as_pass options do not apply to the userpass file.
Is it expected behaviour or should we extend this behaviour to users yielded in the userpass file?

Next steps

• 1. I first plan to fix the easiest bugs (wrong var names for example) to make more tests pass
• 2. Then I would tackle the cleaning of the password spraying code branch, so as to make all tests pass.
• 3. Then in a third commit, I would refactor both branches to simplify the code and reduce the probability to have such bugs in the future (also facilitating maintenance)

[Mathiou04]

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.

[Mathiou04]

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

[cgranleese-r7]

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.

[Mathiou04]

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?)

[cgranleese-r7]

I know it's still in draft but I thought I'd give it a once over anyway. Looks great and thanks for following up on the last pull request 🚀

[cgranleese-r7]

@msjenkins-r7 retest this please

[cgranleese-r7]

Hi @Mathiou04. Only getting back to this now as I was off for the holidays.

I took at look at these failing tests and it seem in the last commit some off the test expectations were changed. Mostly and_returns being changed to and_yields.

I have tested this locally and left suggestions that got everything passing for me.

I'll also add the diff, incase I missed any suggestions on the code or if you want to use it to patch in the changes 👍

diff --git a/spec/lib/metasploit/framework/credential_collection_spec.rb b/spec/lib/metasploit/framework/credential_collection_spec.rb
index dcb1682796..55b4bc2b12 100644
--- a/spec/lib/metasploit/framework/credential_collection_spec.rb
+++ b/spec/lib/metasploit/framework/credential_collection_spec.rb
@@ -68,7 +68,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:pass_file) do
         filename = "foo"
         stub_file = StringIO.new("asdf\njkl\n")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -113,7 +113,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:pass_file) do
         filename = "pass_file"
         stub_file = StringIO.new("asdf\njkl\n")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -142,7 +142,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:user_file) do
         filename = "user_file"
         stub_file = StringIO.new("user1\nuser2\nuser3\n")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -173,7 +173,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:user_file) do
         filename = "user_file"
         stub_file = StringIO.new("user1\nuser2\nuser3\n")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -220,7 +220,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:user_file) do
         filename = "user_file"
         stub_file = StringIO.new("user1\nuser2\nuser3\n")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -255,7 +255,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:user_file) do
         filename = "user_file"
         stub_file = StringIO.new("user1\nuser2\nuser3\n")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -293,7 +293,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:pass_file) do
         filename = "pass_file"
         stub_file = StringIO.new("asdf\njkl\n")
-        allow(File).to receive(:open).with(filename, /^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename, /^r/).and_return stub_file
 
         filename
       end
@@ -473,7 +473,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:user_file) do
         filename = "foo"
         stub_file = StringIO.new("asdf\njkl\n")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -502,7 +502,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       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
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end
@@ -554,7 +554,7 @@ RSpec.describe Metasploit::Framework::CredentialCollection do
       let(:user_file) do
         filename = "user_file"
         stub_file = StringIO.new("userfile")
-        allow(File).to receive(:open).with(filename,/^r/).and_yield stub_file
+        allow(File).to receive(:open).with(filename,/^r/).and_return stub_file
 
         filename
       end

[cgranleese-r7]

@Mathiou04 I went ahead and made some changes to get the tests passing and resolved any comments I believed had been covered with the changes.

I also marked the PR over to ready for review as I believe apart from the refactoring you mentioned (I left some feedback on that), everything else is now covered 🤞. Feel free to let me know if that is not the case.

[Mathiou04]

Hi @Mathiou04. Only getting back to this now as I was off for the holidays.

I took at look at these failing tests and it seem in the last commit some off the test expectations were changed. Mostly and_returns being changed to and_yields.

I have tested this locally and left suggestions that got everything passing for me.

Thanks for looking into this, sorry I didn't came back earlier.
It's weird, because I thought the tests were passing locally and those changes in expectation seemed to make sense given the changes I made to the code

In fact I just realized I had one more commit, that made the tests pass, and I didn't push it...
I was trying to extract a few methods to DRY the code a bit

I introduced a method each_username to groupe this repeated code and use in each_unfiltered_password_first:

  def each_username
      if username.present?
        yield username
      end

      if user_file.present?
        File.open(user_file, 'r:binary') do |user_fd|
          user_fd.each_line do |user_from_file|
            user_from_file.chomp!
            yield user_from_file
          end
          user_fd.seek(0)
        end
      end

      additional_publics.each do |add_public|
        yield add_public
      end
    end

And a method each_password(user) to groupe the code in 'each_unfiltered_username_first`:

def each_password(user)
     if nil_passwords
       yield [nil, :password]
     end

     if password.present?
       yield [password, private_type(password)]
     end

     if user_as_pass
       yield [user, :password]
     end

     if blank_passwords
       yield ["", :password]
     end

     if pass_file
       File.open(pass_file, 'r:binary') do |pass_fd|
         pass_fd.each_line do |pass_from_file|
           pass_from_file.chomp!
           yield [pass_from_file, private_type(pass_from_file)]
         end
         pass_fd.seek(0)
       end
     end

     additional_privates.each do |add_private|
       yield [add_private, private_type(add_private)]
     end
   end

Is it ok if I try to rebase/merge with your work, to show you?
We may remove it afterward if you don't think it adds value.

[cgranleese-r7]

Is it ok if I try to rebase/merge with your work, to show you? We may remove it afterward if you don't think it adds value.

@Mathiou04 Absolutely, that would be great!

[Mathiou04]

@cgranleese-r7 Here is he final version I worked on, sorry for the delay!
So I pushed my last commit and re-reverted the changes in the tests 😓
They should all pass now 🤞

I had other ideas for further refactoring and DRYing (mainly creating custom Iterators class for username and password), but I am not sure it is worth it and am struggling to ind the time to work on it.
So let me know what you think of this version!

[cgranleese-r7]

I'll handle these changes as I know you mentioned you don't have the cycles 👍

[Mathiou04]

I'll handle these changes as I know you mentioned you don't have the cycles 👍

Thx for that.
Just one question though, about those changes: why does ignore_private assumes there is only one username in the list?

I am not sure I understand all the implications, but those new parameters feels kind of specific to a certain usage (and don't interact well with other params, like password spraying for example).

Wouldn't it be cleaner for the CredentialCollection to have an interface with some kind of enum: ignore: [public, private, both] ?
This way we could have something more generic that can still ignore either usernames, passwords or both and this would be properly integrated with the rest of the code (and the behaviour would be less surprising).

Note: I can try to do the changes, they are not as time consuming as the ones I mentioned before 😉

[cdelafuente-r7]

Hi @Mathiou04, thank you for looking into this. I just wanted to give you a bit more of context and try to answer your questions.

The PR that @cgranleese-r7 mentioned introduced these ignore_private and ignore_public options to better handle the use of smb_login and ldap_login modules with sessions. These modules can be used to get a session (LDAP or SMB sessions) and the user can run them without providing username/password and set the authentication mode to Kerberos or Schannel. The module will automatically fetch the right Kerberos tickets or Pkcs12 certificate to authenticate the user.

These two new options are set automatically by the modules in this case, depending on if Kerberos or Schannel is used without providing username/password. This will avoid errors since the *_login modules originally were requiring at least a username and a password (or an option like anonymous login set). In this context, a single username is provided when ignore_private is set. It is used to look for the right Kerberos ticket or Pkcs12 certificate in the database.

So, you're right, these new parameters are only used for this specific use-case: the user wants a session and don't want to bruteforce anything. For this reason, when ignore_private is set, #each_unfiltered in lib/metasploit/framework/credential_collection.rb should completely bypass the other cases and just yield the proper Credential object with private and/or public set to nil. That's why there was a if-elsif-end block. Now, with the current implementation, I suggest to move the if ignore_private block to the top of the method and return early in case this option is set. For example (untested):

def each_unfiltered(&block)
  prepended_creds.each { |c| yield c }

  if ignore_private
    if ignore_public
      yield Metasploit::Framework::Credential.new(public: nil, private: nil, realm: realm)
    else
      yield Metasploit::Framework::Credential.new(public: username, private: nil, realm: realm)
    end
    return
  end

  if anonymous_login
    yield Metasploit::Framework::Credential.new(public: '', private: '', realm: realm, private_type: :password)
  end

  if password_spray
    each_unfiltered_password_first(&block)
  else
    each_unfiltered_username_first(&block)
  end
end

[Mathiou04]

Hi @Mathiou04, thank you for looking into this. I just wanted to give you a bit more of context and try to answer your questions.

Thanks for the detailed explanations @cdelafuente-r7 🙏

I understand that this is a specific use case and how it is used by other "clients" (classes) in the library at the moment.
From the CredentialCollection perspective, I have the impression that the names ignore_private/ignore_public could be a bit misleading.

If I were trying to iterate on many usernames and have only a nil password, I think that given the name I would try to pass a collection of usernames, set the ignore_private to true, and expect the code to yield the list of usernames with nil passwords.

Does what I say makes sense?

I think there is a simple implementation that preserves the current behaviour when passing only a username, but that would also work with multiple ones.
I'll push a new commit just so you can see what I mean.

[cdelafuente-r7]

Thanks! It makes sense. One more detail, the ignore_private and ignore_public parameters cannot be set by the user. It is the module's responsibility to set them accordingly.

For example, in the ldap_login module:
https://github.com/rapid7/metasploit-framework/blob/master/modules/auxiliary/scanner/ldap/ldap_login.rb#L92-L105

I'll have a look at your commit, thank you!

[Mathiou04]

Thanks! It makes sense. One more detail, the ignore_private and ignore_public parameters cannot be set by the user. It is the module's responsibility to set them accordingly.

Yes sorry, I meant "user" in the sense of "client code using the class" 😅

I'll have a look at your commit, thank you!

Here it is 🎉

I first did a simple merge commit with the straightforward solution given the current code.

Then as I wrote before, I tried to see if I could make the change more generic. But after looking at a few options I realized that we might be looking at it the wrong way.

It seemed to me that the responsibility of handling those options should not be in the CredentialCollection: it feels weird to pass collections of usernames, passwords, prepend from DB etc, but then bypass everything with a single option.

So I had the impression that we would be better off with the client library making better use of the different available options to actually retrieve what it wants and not pass wasted parameters.

Therefore I introduced a new method in ldap_login that depending on the configuration instantiate a PrivateCredentialCollection (if it doesn't want a username) or a CredentialCollection with very few options (just what is needed).

There weren't any specs on the ldap_login class, and I wasn't sure how to introduce some or actually test it so I am not 100% sure the current implementation is actually what we expect.

I also renamed/changed the parameters to have one reflecting the SCHANNEL case, that necessitates a fully void login attempt, and the KERBEROS case that simply need a void password.
The conditions felt more natural that way (ignore_public set to true was useless if ignore_password wasn't true as well).

Another option that I explored what to replace the ignore flags with a new nil_usernames flag and play with that.
We would still have to instantiate different CredentialCollections in the ldap_login code though.
I went with the PrivateCredentialCollection instead, even if I am not 100% sure it works. It seemed to be done for that from what I read.

Finally, I think there was a bug in the KERBEROS condition, but I am not 100% sure.
The change in the conditions for determining the variable is not 100% equivalent to what we had initially, so let me know if I am wrong.

Sorry for the long comment, happy to hear your thoughts on this.