Fix #17602 - Update basic_discovery.rc to support commas in RHOSTS values #17650

samsepi0x0 · 2023-02-15T16:51:28Z

This change parses the RHOSTS before passing the arguments to nmap scan in basic_recovery.rc. The commas are removed from the RHOSTS (in case they are present).

Verification

Steps to verify

export METASPLOIT_RHOSTS='172.31.2.3/32, 172.31.4.5/32, 172.31.6.7/32'
export METASPLOIT_RESOURCE_DIR=/usr/share/metasploit-framework/scripts/resource
msfconsole --quiet --logger Stdout \
  --execute-command \
  "setg RHOSTS ${METASPLOIT_RHOSTS}; resource ${METASPLOIT_RESOURCE_DIR}/basic_discovery.rc; exit"

Output

RHOSTS => 172.31.2.3/32, 172.31.4.5/32, 172.31.6.7/32
[*] Processing /usr/share/metasploit-framework/scripts/resource/basic_discovery.rc for ERB directives.
[*] resource (/usr/share/metasploit-framework/scripts/resource/basic_discovery.rc)> Ruby Code (20400 bytes)
THREADS => 15

============================================
starting discovery scanners ... stage 1
============================================


starting portscanners ...

udp_sweep
[*] Auxiliary module running as background job 0.
Module: db_nmap
Using Nmap with the following options: -n -PN -P0 -O -sSV 172.31.2.3/32 172.31.4.5/32 172.31.6.7/32

Metasploit Version:

v6.3.0-dev

(Submitting from the new branch, apologies for the earlier confusion, created the pull request without checking the branch).

gwillcox-r7 · 2023-02-15T17:02:36Z

Thanks @samsepi0x0 will get these changes landed once tests pass, apologies for the earlier inconvenience!

gwillcox-r7 · 2023-02-15T17:54:40Z

Looks like our backend checks probably failed for some silly reason again. One sec....

gwillcox-r7 · 2023-02-15T17:56:13Z

@msjenkins-r7 retest this please

adfoster-r7 · 2023-02-15T18:10:22Z

scripts/resource/basic_discovery.rc

@@ -19,6 +19,9 @@ if (framework.datastore['RHOSTS'] == nil)
 	return
 end

+#Remove any commas present in the RHOSTS
+framework.datastore['RHOSTS'] = framework.datastore['RHOSTS'].gsub(',','')


Just to confirm, won't this break things? If there's no spaces present? i.e. ip1,ip2 will become ip1ip2 which nmap won't be able to parse

Or does the datastore normalize this value on being set?

let me run and check the output once.

Also another point we just discussed that I hadn't thought about is that this is modifying datastore options. This may end up introducing bugs if validation takes place in one part of the code and then you update the value here which causes a violation of assumptions down the line. We've seen bugs like this happen in the past. Is there a way we could perhaps modify this closer to where the issue occurs vs modifying the datastore itself?

Nmap wont be able to parse it, it should be gsub(',',' ') instead to make IP1,IP2 to IP1 IP2.

In the regular case of IP1, IP2 that would be IP1 IP2, which wouldnt give any problem to nmap.

Maybe instead of the datastore mutation (i.e. the reassignment of RHOSTS), we could add the code change closer to where it needs to be used like these lines:

if (verbose == 1) print_line("Using Nmap with the following options: -v -n #{nmapopts} #{framework.datastore['RHOSTS']}") run_single("db_nmap -v -n #{nmapopts} #{framework.datastore['RHOSTS'].gsub...etc...}") else print_line("Using Nmap with the following options: -n #{nmapopts} #{framework.datastore['RHOSTS']}") run_single("db_nmap -n #{nmapopts} #{framework.datastore['RHOSTS'].gsub...etc...}") end

Or add a temporary variable for readability and removing code duplication

nmap_rhosts = datastore..gsub..join..etc ... run_single("db_nmap -v -n #{nmapopts} #{nmap_rhosts}") ...

Maybe instead of the datastore mutation (i.e. the reassignment of RHOSTS), we could add the code change closer to where it needs to be used like these lines:

run_single("db_nmap -v -n #{nmapopts} #{framework.datastore['RHOSTS'].gsub...etc...}")

that does make more sense than changing the datastore, let me try this out.

Maybe instead of the datastore mutation (i.e. the reassignment of RHOSTS), we could add the code change closer to where it needs to be used like these lines:

run_single("db_nmap -v -n #{nmapopts} #{framework.datastore['RHOSTS'].gsub...etc...}")

This would run the nmap scan without changing the datastore, and since RHOSTS are set for only stage 1, global values remains unchanged.

I will edit the file to make the necessary changes in this pull request.

gwillcox-r7 · 2023-02-15T21:05:12Z

scripts/resource/basic_discovery.rc

 run_single("use auxiliary/scanner/discovery/udp_sweep")
 run_single("run -j")


Unfortunately this is still erroring out:

~/git/metasploit-framework │ bucket ?24  export METASPLOIT_RHOSTS='172.31.2.3/32,172.31.4.5/32'  ✔ │ 4m 49s │ 3.0.5 Ruby │ 02:55:34 PM ~/git/metasploit-framework │ bucket ?24  ./msfconsole --quiet --logger Stdout \  ✔ │ 3.0.5 Ruby │ 02:56:04 PM --execute-command \ "setg RHOSTS ${METASPLOIT_RHOSTS}; resource /home/gwillcox/git/metasploit-framework/scripts/resource/basic_discovery.rc; exit" RHOSTS => 172.31.2.3/32,172.31.4.5/32 [*] Processing /home/gwillcox/git/metasploit-framework/scripts/resource/basic_discovery.rc for ERB directives. [*] resource (/home/gwillcox/git/metasploit-framework/scripts/resource/basic_discovery.rc)> Ruby Code (20356 bytes) THREADS => 15 ============================================ starting discovery scanners ... stage 1 ============================================ starting portscanners ... udp_sweep [-] Msf::OptionValidateError The following options failed to validate: RHOSTS Module: db_nmap Using Nmap with the following options: -n -PN -P0 -O -sSV 172.31.2.3/32 172.31.4.5/32 [*] Nmap: 'You requested a scan type which requires root privileges.' [!] Running Nmap with sudo [*] Nmap: Starting Nmap 7.93 ( https://nmap.org/ ) at 2023-02-15 14:56 CST

After speaking to colleagues about this I think we may want to just add in more run_single('set RHOSTS XXXX') calls here to fix this up.

RHOSTS is set setg RHOSTS ${METASPLOIT_RHOSTS}; here. So something like:

nmap_rhosts = datastore..gsub...

then set RHOSTS using run_single("set RHOSTS #{nmap_hosts}") explicitly?

Did the following changes: (in Stage 1)

Created a temp variable for storing the space separated values.

Added run_single to set RHOSTS again.

Additional: (in Stage 2)
3. Changed auxiliary/admin/ftp/titanftp_xcrc_traversal to auxiliary/scanner/ftp/titanftp_xcrc_traversal as the earlier one does not exist anymore.

adfoster-r7 · 2023-02-15T22:50:24Z

scripts/resource/basic_discovery.rc

@@ -80,6 +80,10 @@ print_line("starting discovery scanners ... stage 1")
 print_line("============================================")
 print_line("")

+# Temp variable to store space separated RHOSTS
+nmap_rhosts = framework.datastore['RHOSTS'].gsub(',',' ')
+run_single("set RHOSTS #{nmap_rhosts}")


Is this line needed? 👀

yes, without it the MSF validate RHOSTS error would come up, even the auxiliary scan requires space separated values not comma separated so...

Ah, thanks. I wonder if it gets treated as a URL when there are no spaces and CIDR is defined. Will try and take a quick look tomorrow

@adfoster-r7 Any updates on the test cases? I tested the when CIDR is defined but couldn't seem to find anything out of the ordinary.

adfoster-r7 · 2023-02-15T22:52:22Z

scripts/resource/basic_discovery.rc

@@ -613,7 +617,7 @@ framework.db.workspace.hosts.each do |host|
 			jobwaiting(maxjobs,verbose)

 			print_line("Module: titanftp_xcrc_traversal")
-			run_single("use auxiliary/admin/ftp/titanftp_xcrc_traversal")
+			run_single("use auxiliary/scanner/ftp/titanftp_xcrc_traversal")


Nice catch, thanks! 👍

adfoster-r7 · 2023-02-20T09:41:45Z

Release Notes

Updates the script/resource/basic_discovery.rc script to support commas in RHOST values

adfoster-r7 · 2023-02-20T09:47:16Z

Thanks! I've also created a separate PR to better handle the database not being connected - #17674

gwillcox-r7 approved these changes Feb 15, 2023

View reviewed changes

gwillcox-r7 self-assigned this Feb 15, 2023

adfoster-r7 reviewed Feb 15, 2023

View reviewed changes

gwillcox-r7 approved these changes Feb 15, 2023

View reviewed changes

gwillcox-r7 changed the title ~~Fixed Issue #17602~~ Fix #17602 - Update basic_discovery.rc to support commas in RHOST values Feb 15, 2023

Correctly parsed before the scan.

86e8f5c

gwillcox-r7 force-pushed the bucket branch from 3b501bd to 86e8f5c Compare February 15, 2023 20:48

gwillcox-r7 reviewed Feb 15, 2023

View reviewed changes

Update basic_discovery.rc

12c739b

adfoster-r7 reviewed Feb 15, 2023

View reviewed changes

adfoster-r7 merged commit db29036 into rapid7:master Feb 20, 2023

smcintyre-r7 added the rn-fix release notes fix label Feb 24, 2023

smcintyre-r7 changed the title ~~Fix #17602 - Update basic_discovery.rc to support commas in RHOST values~~ Fix #17602 - Update basic_discovery.rc to support commas in RHOSTS values Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #17602 - Update basic_discovery.rc to support commas in RHOSTS values #17650

Fix #17602 - Update basic_discovery.rc to support commas in RHOSTS values #17650

samsepi0x0 commented Feb 15, 2023

gwillcox-r7 commented Feb 15, 2023

gwillcox-r7 commented Feb 15, 2023

gwillcox-r7 commented Feb 15, 2023

adfoster-r7 Feb 15, 2023

samsepi0x0 Feb 15, 2023

gwillcox-r7 Feb 15, 2023

samsepi0x0 Feb 15, 2023

adfoster-r7 Feb 15, 2023 •

edited

samsepi0x0 Feb 15, 2023

samsepi0x0 Feb 15, 2023

gwillcox-r7 Feb 15, 2023

samsepi0x0 Feb 15, 2023

samsepi0x0 Feb 15, 2023

adfoster-r7 Feb 15, 2023

samsepi0x0 Feb 15, 2023

adfoster-r7 Feb 15, 2023

samsepi0x0 Feb 19, 2023

adfoster-r7 Feb 15, 2023

adfoster-r7 commented Feb 20, 2023 •

edited

adfoster-r7 commented Feb 20, 2023

		run_single("use auxiliary/scanner/discovery/udp_sweep")
		run_single("run -j")

Fix #17602 - Update basic_discovery.rc to support commas in RHOSTS values #17650

Fix #17602 - Update basic_discovery.rc to support commas in RHOSTS values #17650

Conversation

samsepi0x0 commented Feb 15, 2023

Verification

Output

Metasploit Version:

gwillcox-r7 commented Feb 15, 2023

gwillcox-r7 commented Feb 15, 2023

gwillcox-r7 commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 Feb 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 commented Feb 20, 2023 • edited

Release Notes

adfoster-r7 commented Feb 20, 2023

adfoster-r7 Feb 15, 2023 •

edited

adfoster-r7 commented Feb 20, 2023 •

edited