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
Multi-threaded check command, plus aux scanner support #2922
Conversation
Auxiliary modules can use check, too. Not just exploits.
If threads aren't killed, then when the user triggers interrupt, the console will keep the threads (vuln checks) running, which looks weird.
Code changes address these feature requests: [SeeRM rapid7#8737] [SeeRM rapid7#8752] [SeeRM rapid7#8755]
@@ -32,6 +32,17 @@ def initialize(info = {}) | |||
end | |||
|
|||
|
|||
def check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The returns are unnecessary as the begin rescue
will be treated as a single statement and if there is no error then the check_host's return will be return value and if there is an error then the rescue's value will be the return value. Also remove the unnecessary self
when calling replicant
.
def check
nmod = replicant
begin
nmod.check_host(datastore['RHOST])
rescue NoMethodError
Exploit::CheckCode::Unsupported
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in eec01e7
break unless ip | ||
|
||
@tl << framework.threads.spawn("CheckHost-#{ip}", false, ip.dup) { |tip| | ||
mod.datastore['RHOST'] = tip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not thread-safe, but github's hiding our discussion because it was on an outdated diff. To fix this you need to make the threads not share mutable data or only share read-only data. The easiest way I can think of to do this is to create a replicant on line 85 in the parent thread and then use that replicant in the child thread block of 87 to 88. This will be inefficient since check_simple itself calls replicant internally, but modifying the datastore has to happen on a separate module instance for each thread:
instance = mod.replicant
instance.datastore['RHOST'] = ip.dup
thread = framework.threads.spawn("CheckHost-#{ip}", false) {
instance.check_simple
}
@tl << thread
I moved setting the datastore outside the thread since it doesn't take that long and doesn't really need to be threaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... actually calling replicant
should not affect the original mod
, but could take some time since it's dup
ing a lot of variables, so it's probably better to move it into the thread.
thread = framework.threads.spawn("CheckHost-#{ip}", false) {
instance = mod.replicant
instance.datastore['RHOST'] = ip.dup
instance.check_simple
}
@tl << thread
The ip.dup
is still necessary in case any of the code that uses 'RHOST'
uses mutators like strip!
that would modify the String
object instead of just replacing it in the data store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this solution doesn't actually work because "instance" doesn't have check_simple(). Luke, what was the other solution you were talking about at the airport? Did you say extend a module in there? So check_simple() exists in both exploit and auxiliary, not sure how you're supposed to extend that in a more universal way.... could you please repeat what you said? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to emulate what happens when you call Msf::ModuleSet#create
: The instance is initialized from the class and then the creation event is sent to the framework:
instance = mod.replicant
instance.datastore['RHOST'] = ip.dup
framework.events.on_module_created(instance)
instance.check_simple
The framework.events.on_module_created
is implemented as Msf::Simple::Framework#on_module_created:
def on_module_created(instance)
Msf::Simple::Framework.simplify_module(instance)
end
Msf::Simple::Framework.simplify_module extends the instance
with a module_type specific Simple
module:
instance.extend(ModuleSimplifiers[instance.type])
It is this extend
that finally gives your instance
the check_simple
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, no, it wasn't immediately obvious that the event system would add the simple methods to me either. I only figured it out when I left out the framework.events.on_module_created
call on my module caching branches. The Simple namespace in lib/msf/base
should probably be merged into lib/msf/core
's normal modules and classes since nothing really works with non-simplified modules and frameworks, but that's for later for me at least after rounds 1 and 2 of the module cache are complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, that works. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in 9f669a8
So if I'm not mistaken, all concerns are addressed at this point. I'm gonna go ahead and retest everything now. And Luke, if you see any more (or things that still aren't properly addressed), please let me know. And thanks again. |
I finished testing and only had to fix ::Interrupt exception handling. Also passed Travis again. This PR is back on track. |
Testing... |
Checking boxes, rather. :) |
I checked all the boxes. Phew. |
Scenario 1:
Scenario 2:
Scenario 3:
Scenario 4:
Scenario 5:
Scenario 6:
Scenario 7:
Scenario 8:
Scenario 9:
|
well done sir. |
Looks like testing is complete. Please merge. Thanks. |
Talked to Tod. Since there's an issue with other things in framework, we're holding this one to make sure we have a safe upcoming release. Should probably go in the same time as #2930, since they're two non-small effort features. |
Roger. |
Thanks!! |
This PR addresses the following redmine tickets:
It is also the final completion of this ticket:
http://dev.metasploit.com/redmine/issues/8737
Usage:
Basically there are two ways to use this command:
Example 1:
check [an IP or an IP range]
Example 2:
If you wish to have more threads, then do:
set threads [number of threads]
Please note: Having more threads may result in missed opportunities (meaning that even though a host is vulnerable, it may not be flagged).
Verifications
Before you begin, please make sure you have the following for testing:
ruby -run -e httpd . -p 8181
And now let's move on to these scenarios:
Scenario 1: With ms08_067_netapi, set the RHOST and run check
msfconsole
use exploit/windows/smb/ms08_067_netapi
set rhost [IP]
check
Scenario 2: With ms08_067_netapi, check multiple hosts
msfconsole
use exploit/windows/smb/ms08_067_netapi
check [range]
(keep the range small or it will run for a long time)Scenario 3: With ms08_067_netapi, set threads, and run multiple hosts
msfconsole
use exploit/windows/smb/ms08_067_netapi
set threads 10
check [range]
(ideally try to keep the range small for faster testing)Scenario 4: With ms08_067_netapi, abort scanning while on a multi-threaded scan
msfconsole
use exploit/windows/smb/ms08_067_netapi
set threads 10
check [range]
(ideally try to keep the range small for faster testing)Scenario 5: With ms08_067_netapi, run check without any arguments
msfconsole
use exploit/windows/smb/ms08_067_netapi
check
Scenario 6: Run nodejs_pipelining.rb against the fake web server
msfconsole
use auxiliary/dos/http/nodejs_pipelining
set rport [fake web server PORT]
check [fake web server IP]
Scenario 7: Run aux_scanner_check_test against the web server
msfconsole
use [path to aux_scanner_check_test.rb]
set rport [fake web server rport]
check [web server IP]
Scenario 8: With aux_scanner_check_test, run check without any arguments
msfconsole
use [path to aux_scanner_check_test.rb]
check
Scenario 9: Run a module that does not support check at all
msfconsole
use auxiliary/scanner/http/soap_xml
set rhosts [IP]
check