Skip to content
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 an edge case in .to_win32pe #18094

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

zeroSteiner
Copy link
Contributor

When the entry point is after the payload, there would occasionally be cases where poff and eidx to be invalid, causing entry to be truncated. poff should never be negative and eidx should reserve the 256 bytes that entry may occupy.

Testing

  • Generate multiple x86 payload EXEs and ensure they always work
    • Alternatively you can modify the rand() calls to initialize eloc to 1 and poff to 100. This would have triggered the bug every time.

When the entry point is after the payload, there woud occassionally be
cases where `poff` and `eidx` to be invalid, causing `entry` to be
truncated. `poff` should never be negative and `eidx` should reserve the
256 bytes that `entry` may occupy.
Comment on lines +331 to +332
poff -= [256, poff].min
eidx = rand(block[1] - (poff + payload.length + 256)) + poff + payload.length
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two lines are the real fix for the boundary issues.

Comment on lines +309 to 311
if payload.length + 256 >= block[1]
raise RuntimeError, "The largest block in .text does not have enough contiguous space (need:#{payload.length+257} found:#{block[1]})"
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was switched to ensure that block[1] is greater than payload.length + 256. 256 is the max size of the entry as you can see on L324. 5 bytes are reserved for the jump instruction.

This should prevent future calls to rand() from receiving 0 in the event that payload.length + 256 == block[1]. When Ruby's rand() function is called with 0, it will return a float which is unexpected.

Copy link
Contributor

@adfoster-r7 adfoster-r7 left a comment

Choose a reason for hiding this comment

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

Will wait for another team member to also approve before landing; but as a datapoint I was running into this issue once or twice every 200 generations or so on CI, to not having seen the original segfault crash after 1000+ generations. Thanks for the fix! 👍

@adfoster-r7 adfoster-r7 self-assigned this Jun 14, 2023
@adfoster-r7 adfoster-r7 merged commit 50832be into rapid7:master Jun 15, 2023
33 checks passed
@adfoster-r7
Copy link
Contributor

Release Notes

Fixes an edgecase with windows/meterpreter/reverse_tcp where there was a small chance of an invalid stager being created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug library rn-fix release notes fix
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants