-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Fix reference URLs for several modules and update tools/modules/module_reference.rb code with fixes #16093
Conversation
Linting will fail on this because i'm touching so many modules. I have no intention of doing rubocop on them. #dealwithit |
@h00die To help with reviewing/landing, it'd be great to revert the rubocop autocorrections and keep this PR limited to the URL fixes 👀 Or if that's overhead, I can create a separate PR to rubocop these modules to help reduce the noise a bit |
To save some code somewhere, i'm using a really quick and dirty python script to go through the csv output of
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
modules/auxiliary/admin/mssql/mssql_enum_domain_accounts_sqli.rb
Outdated
Show resolved
Hide resolved
@bcoles don't start looking through this yet. I'll be more or less updating all of this hopefully today with the final run |
This is now ready for review. Obvi it isn't exhaustive, but it does update quite a few old and really old links to work again. I think the funniest one is http://www.smbloris.com/ is now an office furniture store or something. |
I think this closes #4039 ? This issue is 7 years old and mostly done. It was re-opened until the results were "easy to prune". Presumably your CSV output is easy to parse. |
modules/auxiliary/scanner/http/wp_modern_events_calendar_sqli.rb
Outdated
Show resolved
Hide resolved
@@ -36,7 +36,7 @@ def initialize | |||
}, | |||
'References' => | |||
[ | |||
[ 'URL', 'http://labs.mwrinfosecurity.com' ] | |||
[ 'URL', 'https://labs.f-secure.com/' ] |
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.
Should this be 'https://labs.f-secure.com/tools/sap-metasploit-modules/'
or 'https://labs.f-secure.com/archive/mwr-sap-metasploit-modules/'
like the others?
I downloaded the zip, but didn't find this module in there (perhaps the file name has changed).
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.
I tried to do 1:1 replacements, investigating 2,600 broken URLs intelligently was entirely too time consuming. I'm keen to leave this as is since that's what was originally there
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.
I read through the URL changes.
I noticed that all r-7.co
links were changed (presumably because they 30x redirected). Presumably, the purpose of this domain was for conventiant URL shortening. On the other hand, 30x status codes should be considered an error. 🤷
There are a few references like these which simply point to a vendor or researcher's domain/blog, rather than anything relevant to the module:
[ 'URL', 'https://blog.c22.cc' ]
[ 'URL', 'https://labs.f-secure.com/' ]
These references aren't useful and are more akin to advertising.
agree on the |
Thanks for your pull request! Before this pull request can be merged, it must pass the checks of our automated linting tools. We use Rubocop and msftidy to ensure the quality of our code. This can be ran from the root directory of Metasploit:
You can automate most of these changes with the
Please update your branch after these have been made, and reach out if you have any problems. |
@gwillcox-r7 Looks like in this scenario we should be good to ignore the linting failures. The pull request is just updating the URLs of the modules - and it would just muddy the waters to request linting 367 modules as part of this pull request 👀 |
@h00die If you're good for this to be reviewed/landed, it'd be great to squash this down to a single commit 🎉 |
No problem I was just going through PRs that are open and was assigning tags to those that were open but didn't have labels attached to them yet. Wasn't sure if we needed that given the changes being made here so thanks for clarifying. |
squishy squashy, all in one. |
Will need to rebase this as we removed the VSS modules so they don't need to be updated anymore, but rest looks like it should be easy to check and then land. |
fix URLs not resolving add csv export to references fix URLs not resolving pdf not pd missed a url change remove extra recirectedfrom fields remove extra file fix ovftool url accidental replacement
80ee6f9
to
d5ba1af
Compare
Fixed up some of the errors encountered when running the tool in d7b442f |
… used in a gsub operation. This prevents errors where sometimes the script can crash when r.ctx_val is considered to be a integer due to it containing only numbers and nothing else
Running a complete check now. Is taking a while (been nearly an hour now) but hopefully it should tell us if the latest changes work and if there are any remaining URLs that need fixing. |
tools/modules/module_reference.rb
Outdated
return false | ||
ensure | ||
cli.close | ||
end | ||
|
||
if res.nil? || res.code == 404 || res.body =~ /<title>.*not found<\/title>/i | ||
if res.nil? || res.body =~ %r{<title>.*not found</title>}i || res.code != 200 |
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.
I'm in disagreement with this and the prime reason here is that if the code is a redirect code, your essentially going to end up with hundreds of links that end up being reported as being "down" when in reality they aren't actually down.
Case in point I ran this against all URLs in the database, and ended up with this result:
post/windows/manage/sticky_ Down URL-https://blog.carnal0wnage.com/2012/04/pri
keys vilege-escalation-via-sticky-keys.html
post/windows/manage/vmdk_mo Down URL-http://www.shelliscoming.com/2017/05/post
unt -exploitation-mounting-vmdk-files.html
post/windows/manage/vss Alive URL-https://web.archive.org/web/2020111121295
2/https://securityweekly.com/2011/11/02/safel
y-dumping-hashes-from-liv/
post/windows/recon/outbound Down URL-http://www.shelliscoming.com/2014/11/gett
_ports ing-outbound-filtering-rules-by.html
Number of bad references found: 8663
Note here that http://www.shelliscoming.com/2017/05/post-exploitation-mounting-vmdk-files.html
is considered down but in reality if you visit that page it just upgrades to a SSL version of that page so its not really "down" per say.
Now that being said I'm all for updating these old links to https links, but this labeling of a link as "down" doesn't really serve to help anyone and only causes more confusion.
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.
I also have concerns r.e the accuracy of this script given that https://blog.carnal0wnage.com/2012/04/privilege-escalation-via-sticky-keys.html was incorrectly labeled as being down
. I'll have to look into this further.
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.
Alright so the response to the link above was HTTP/2 200 OK
so its possible that maybe the handler doesn't handle HTTP/2 responses correctly? Idk.
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.
I also have concerns r.e the accuracy of this script given that https://blog.carnal0wnage.com/2012/04/privilege-escalation-via-sticky-keys.html was incorrectly labeled as being
down
. I'll have to look into this further.
This has been addressed with 550a625 which does a major rework in an attempt to support redirections by defining a new status type for redirects and updating the code to report the status appropriately whilst also still not labeling the URL as down.
The HTTP/2 issue still exists however.
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.
Our Rex library appears to be pulling some weird shenanigans on SSL versions that honestly is leading me scratching my head. Seems to default to SSLv3 if a version is not supplied but if I try change it then its still throwing errors about trying to perform a handshake with SSLv3 that failed.
Ideally as per some discussions with @smcintyre-r7 (sorry CCing you in here as I think this may be an issue with Rex itself), Rex would actually bother to renegotiate the SSL version but this doesn't seem to be the case right now.
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.
yea i wasn't happy about calling redirects down
, but was also trying to keep it more Boolean. I also figured if it isn't perfect, I'd rather call it out to get fixed. regardless, this is a vast improvement regardless.
I noticed the SSL issues as well, didn't want to go down an additional rabbit hole as this was already enough
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.
No worries @h00die, I think our main concern is if this could affect other modules if this is affecting the Rex library. Also personally I really don't like the fact that this module probably hasn't been working for a long time now so forgive my continuous editing to try get it working again 😄
SSL related issues are now being tracked at #16213 as this appears to be a bigger issue affecting much more of our code base than originally anticipated. As this is an unrelated issue to this code though, which seems to be working fine minus that issue, I'm going to go ahead and land this. |
Fixed up last RuboCop issues cause it wasn't that many and was an easy fix. Will land this now, future work on SSL will be another PR as that is a larger job. |
Release NotesA number of broken URL references have been fixed in Metasploit modules. In addition the |
fixes #4039
Updated
tools/modules/module_reference.rb
with a rubocop run, removed the SSL version reference to use the default. Made it so 300s now mark as down, and a CSV export.This gave ~2600 URLs with issues (some seem to work and were false positives).
Mainly just either putting in the new URLs (if it forwards), looking for the real URL via google search, or worst case going with wayback machine references. If all those cases fail, I just leave it as is.