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.

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

smb_version module should support RPORT #19072

Closed
jeffmcjunkin opened this issue Apr 10, 2024 · 17 comments · Fixed by #19163
Closed

smb_version module should support RPORT #19072

jeffmcjunkin opened this issue Apr 10, 2024 · 17 comments · Fixed by #19163
Assignees
Labels
suggestion-feature New feature suggestions

Comments

@jeffmcjunkin
Copy link
Contributor

Summary

The auxiliary/scanner/smb/smb_version module currently takes great pains to exclude the RPORT variable (amongst several others):

https://github.com/rapid7/metasploit-framework/blob/master/modules/auxiliary/scanner/smb/smb_version.rb#L50

Motivation

In some pivoting scenarios (for example, ssh -L) the redirected SMB server may not be listening on the normal TCP port 445. Allowing the option (with the default of 445, of course) will enable these scenarios.

In my quick testing, removing line 50, as well as lines 53-59, re-enabled those options and enabled the workflow I'm describing.

Hopefully this isn't too much of an xkcd 1172 scenario.

Before removing those lines:

msf6 auxiliary(scanner/smb/smb_version) > show options 

Module options (auxiliary/scanner/smb/smb_version):

   Name     Current Setting  Required  Description
   ----     ---------------  --------  -----------
   RHOSTS   localhost        yes       The target host(s), see https://github.com/rapid7/metasploit-f
                                       ramework/wiki/Using-Metasploit
   THREADS  1                yes       The number of concurrent threads (max one per host)

Afterwards:

msf6 auxiliary(scanner/smb/smb_version) > show options 

Module options (auxiliary/scanner/smb/smb_version):

   Name     Current Setting  Required  Description
   ----     ---------------  --------  -----------
   RHOSTS   localhost        yes       The target host(s), see https://github.com/rapid7/metasploit-f
                                       ramework/wiki/Using-Metasploit
   RPORT    445              yes       The SMB service port (TCP)
   THREADS  1                yes       The number of concurrent threads (max one per host)
@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Apr 10, 2024

Interesting! We coincidentally needed something similar for our acceptance testing, i.e. running test targets in docker on arbitrary ports. So this is also useful from just a generic engineering perspective.

I guess the problem is for a chunk of our SMB modules, what would the user expect to have happen if you supply RPORT - should it skip scanning 139 and 445, and scan only the user's RPORT? 🤔 Would need a few more brain cycles to work out the best backwards-compatible approach here

@nrathaus
Copy link
Contributor

We can do a non-breaking fix where RPORT would add to the array and change it to a set - so that there is no duplicate:
smb_ports = [445, 139]

This way we will test 445, 139 by default, and the user's non-default port if one is provided

@jeffmcjunkin
Copy link
Contributor Author

@nrathaus I like that approach for the flexibility, but I'm not sure what exact code you would want. I think you can add a commit to the PR though!

@nrathaus
Copy link
Contributor

@jeffmcjunkin done - #19076

@smcintyre-r7
Copy link
Contributor

So I like the idea of making the ports configurable, it makes a lot of sense. My concern is that what is currently defaulting to 139 as well as 445 should both be configurable. Since the negotiation to the service on 139 is different than what is used to 445 we should test to ensure that negotiating is automatic. If it's not automatic, we'll likely need different data store options (RPORT_SMB and RPORT_NETBIOS probably?) for each so the framework can be told what negotiating mechanism to use.

This should ensure that it works in the scenario where the Metasploit user is forwarding and connecting to 445/tcp on whatever proxy/pivot host they're using to 139/tcp on the Windows target and vice versa.

@nrathaus
Copy link
Contributor

I see that there is some references to SMBDirect - The target port is a raw SMB service (not NetBIOS)

Which is auto-true set on port 445:

  def smb_direct
    (@smb_port == 445)
  end

Which means that if you put a custom port, this smb_direct wouldn't be enabled, causing a potential cascade of an issue, especially it used in the def login function inside smb/simple_client.rb

So I think you are correct, fixing that 445 SMBDirect would require a "bigger" fix - I can do that if you feel its needed, let me know.

@jeffmcjunkin
Copy link
Contributor Author

FWIW with Impacket I PR'd a separate argument for the protocol for that scenario, @smcintyre-r7 : fortra/impacket#1730

@nrathaus
Copy link
Contributor

The problem with this approach on Metasploit, is that there are many cases that need to be fixed where == 445 or == 139 need to be fixed (10 results, and 7 results, respectively) - so its not a one place fix - it is doable, but requires more fixes

@smcintyre-r7
Copy link
Contributor

So after looking into this a bit more, I think we can do it in a way that's backwards compatible and wouldn't require too many changes.

Right now, smb_version iterates through ports 445, and 139 in order until it's able to collect some information and it determines whether it should be "direct" or not based on the port number.

If the Msf::Exploit::Remote::SMB::Client mixin's #connect method were updated to allow a direct keyword argument, we could control it independently of the port. By having it default to nil, it could retain the existing functionality and thus backwards compatibility where it looks at the #smb_direct method, the SMBDirect datastore option and port, but only when it's nil.

With that in place, it should be relatively easy to update smb_version to add an optional RPORT option and either read the SMBDirect option or register a new one. The smb_version module could then determine if the RPORT option has been set and if so use that in place of the default values it currently has. Something like this:

if datastore['RPORT'].present? && datastore['RPORT'] != 0
  smb_services = [
    { port: datastore['RPORT'], direct: datastore['SMBDirect'] }
  ]
else
  smb_services = [
    { port: 445, direct: true },
    { port: 139, direct: false }
  ]
end

With that in place we could pass it down to the updated #connect method by updating the loop logic a bit and users should be able to use this module to scan any port using either the direct or indirect setup.

@nrathaus
Copy link
Contributor

@smcintyre-r7 I made a bit simpler modification with two additional options for users, SMB_RPORT and RPC_RPORT

Let me know what you think

@nrathaus
Copy link
Contributor

Metasploit run lines:


use auxiliary/scanner/smb/smb_version
set SMB_RPORT 4445
set RHOSTS 127.0.0.1
run

@smcintyre-r7
Copy link
Contributor

I'd kinda prefer what I had originally suggested to keep the datastore options and their significance intuitive and consistent. I think a lot of users are inclined to just use the muscle memory of setting RPORT and in the case of an SMB module, it makes sense for that to default to RPORT=445 and SMBDirect=true.

@nrathaus
Copy link
Contributor

@smcintyre-r7 ok

@nrathaus
Copy link
Contributor

#19076

@smcintyre-r7 smcintyre-r7 self-assigned this Apr 16, 2024
@nrathaus
Copy link
Contributor

@smcintyre-r7 please look at it now, I made the suggested changes - PR #19076

@smcintyre-r7
Copy link
Contributor

The changes I see in #19076 are not quite what I had in mind. Those changes add two new datastore options instead of reusing the existing RPORT and SMBDirect. I've been waiting for #19095 to land to incorporate the changes from a008288 which adds the direct keyword argument to #connect. Once those changes are landed, this could be rebased and the direct keyword argument could be passed to the connection method. In the same commit, you can also see the pattern applied to smb_enumusers that I'm hoping we can use in all of the applicable modules including smb_version.

@nrathaus
Copy link
Contributor

@smcintyre-r7 there is no need to wait, as setting SMBDirect is sufficient to change the behavior of the underlying code, but if you want to wait, we will wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion-feature New feature suggestions
Projects
None yet
4 participants