Improves detection of the MS15-034 #5386
Conversation
|
cc @firefart for comments :D |
|
Should /cc @wchen-r7 as well since he originally wrote the module |
|
That looks smarter and more thoughtful, overall I like it. |
| next unless uri.host == vhost || uri.host == rhost | ||
|
|
||
| uris << uri.path if uri.path =~ /\.[a-z]{2,}$/i # Only keep path with a file | ||
| rescue => e |
There was a problem hiding this comment.
Swallowing all exceptions is not a good coding practice. Do you know if there's something particular you're trying to rescue? Maybe you shouldn't rescue at all?
There was a problem hiding this comment.
I just don't want the scanner to be stopped due to something weird like an encoding issue or pretty much anything
There was a problem hiding this comment.
I understand, but you're taking over some console functionalities when you rescue everything. Interrupt is an example, you don't want to deny the user the ability to hit the e-brakes at anytime. You've also lost the ability to log errors to framework.log.
It looks like the block you're rescuing shouldn't be raising anything anyway (not even URI.encode, I guess). If you can handle the nils properly, you should not need exception handling at all.
|
Looking good. I will test this tomorrow, thanks @erwanlr |
|
Tried this PR. So the check part works, but the dos part doesn't anymore because whatever potential_static_files_uris finds isn't shared with the dos_host and get_file_size method. |
|
|
||
| return Exploit::CheckCode::Vulnerable | ||
| elsif res && res.body.include?('The request has an invalid header name') | ||
| vprint_status("#{vmessage} - Safe") |
There was a problem hiding this comment.
These messages aren't very necessary. If you run the check command, basically it will tell you the check result twice.
There was a problem hiding this comment.
I think you meant the 'run command' instead of 'check command' ?
If those are removed, the vulnerable file or tested ones are never displayed, leading to the same behaviour as Nessus: it just tells you if it's vulnerable or not, w/o any relevant output (i.e the files tested) which is extremely annoying when you want to double check or provide an easiest way for a client to verify the issue (using curl for example)
There was a problem hiding this comment.
There is a check command. It handles our check codes. It's different from the run command. <-- wait never mind, you know this. You demo'd this in the PR description.
Sorry I wasn't being clear enough. I only meant the safe/vulnerable messages. You can vprint which files are tried or useful, definitely handy, but please avoid the safe/vulnerable messages because the check handler is doing that already.
There was a problem hiding this comment.
Hmmm you know what.... if you don't say vulnerable/safe it's kind of weird too. Please disregard my feedback.
You do need to vprint print_status("#{peer} - #{uri} is vulnerable") though. We started enforcing this last year.
There was a problem hiding this comment.
Thanks. I should be able to land this today.
|
the target_uri is now shared between the check and dos methods (thought is was by default but not huhu) |
|
Module works perfectly. Excellent work @erwanlr , thanks again! |
|
Yay ! You're welcome :) |
Currently only the /welcome.png is checked, however in practise, this file is barely there.
Furthermore, some scanners like Nessus often fail to properly detect the issue, and when it's detected, the vulnerable file is not provided.
I've improved the detection based on a script I created at work: https://github.com/RandomStorm/scripts/blob/master/ms15-034-checker.rb
The idea is to extract all potential static files from the index page and test them rather than just check the /welcome.png.
Output examples:
Vulnerable:
w/o Verbose:
with verbose:
Not vulnerable:
w/o verbose:
with verbose:
Unknown (Debian Server):
w/o verbose:
with verbose
Some questions/remarks: