-
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
php_cgi_arg_injection: Fix check regex match to detect code html tag #17823
Conversation
return Exploit::CheckCode::Unknown | ||
return CheckCode::Unknown('Connection failed.') unless response | ||
|
||
if response.code == 200 && response.body.to_s.lstrip =~ /^<code><span style/i && !datastore['PLESK'] |
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.
Would it make sense to use xpath with response.get_html_document.xpath(...)
instead?
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.
For what reason? Would xpath detect code
at the start of line?
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 specific reason, xpath is usually preferred when HTML parsing is required. It was just a suggestion, I'm fine with regex too.
Thanks @bcoles for fixing this. It looks good to me. I tested against Metasploitable 2 and verified this fixed the issue. I'll go ahead and land it. Before
After
|
Release NotesThis fixes an issue in the |
Fixes #17822 by replacing:
With:
It seems likely that the entire response body would start with the
<code>
block due to invoking the PHP pre-processor before returning content. However, without access to a bunch of test boxes (and without bothering to dig through 10 year old PHP source), simple checking for the<code>
block at start-of-line (instead of start-of-document) seems like a quick and easy approach.lstrip
is unlikely to be necessary, but makes the check even more reliable just in case.Performing case-insensitive matching also seems unnecessary, but that's what the original module used.
Before
After