Skip to content

Comments

Fix for issue 20310 with module php_fpm_rce#20311

Closed
tijldeneut wants to merge 3 commits intorapid7:masterfrom
tijldeneut:master
Closed

Fix for issue 20310 with module php_fpm_rce#20311
tijldeneut wants to merge 3 commits intorapid7:masterfrom
tijldeneut:master

Conversation

@tijldeneut
Copy link
Contributor

Fix bug with exploit "php_fpm_rce", bug was cased by the addition of new PHP encoders, bug described in Issue:
#20310

Verification

List the steps needed to make sure this thing works

  • Start msfconsole
  • use exploit/multi/http/php_fpm_rce
  • set rhosts 192.168.1.1
  • exploit

Bug fix, the (new) default PHP encoder is HEX which is not compatible with this exploit
end

def send_payload
encoded_payload = framework.encoders.create('php/base64').encode(payload.raw, nil, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would be quite the right fix - maybe this is a regression from the recent PHP changes? 🤔

#20160

cc @zeroSteiner

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that the payload changed and the the bad characters that are defined caused an encoder to take over. The target definition does not note that there is a maximum size which could explain why the now larger payload is being used when it's invalid.

I can bisect it and take a look but I agree, this doesn't look like the fix I'd expect. We should be able to fix it by updating the target definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I agree, the default exploit just mentioned "payload.encoded" but since there was only 1 PHP encoder at the time that was probably not the proper way.
A better solution would indeed bo to have an (advanced) exploit option to specify the encoder and to have it default to Base64 instead of Hex.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should already be an Encoder option that the user can set but it shouldn't be necessary if the target definition is correct because the framework should handle it automatically to produce a payload that's compatible with the exploit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression but not a recent one. I bisected it back to determine it was broken in 24750de from #19420.

@smcintyre-r7 smcintyre-r7 self-assigned this Jun 24, 2025
@smcintyre-r7
Copy link
Contributor

I'm going to close this and get a replacement PR up to fix the issue. @tijldeneut in the future you shouldn't submit PRs from the master branch and instead use a topic branch. In this case it matters because I'd rebase this to remove commits and if I do that for you, it's going to introduce issues with your branch and local copy.

Thanks for reporting the issue to us, I'll tag this in the replacement PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants