Closed kizzx2 closed 9 months ago
Email z niedzieli 26 listopada 2023 od Chrisa Yuena:
WinExec currently errors out when tested on Windows 11 x64. After some diagnosis it appears that
pushstr
aligns to 8 bytes but Windows calling convention requires 16 byte. Perhaps this was not enforced in previous versions of Windows but apparently that is the standard requirement.
Not sure this is correct. I would prefer the stack move to be in a separate variable, and then the extra alignment to be just stack_move & 8
, because the tons of math involved seem too convoluted to read.
I also intend to stress that Windows is not a primary exploitation target for pwntools, as there are better tools for that (metasploit for instance), and there is no easy way to test the shellcraft payloads on the CI. I see that the payloads are used, which is great, because I did not even expect that. Thanks for taking your time to fix it.
-- Wysłane z mojego urządzenia Sailfish
Thanks for the reply!
as there are better tools for that (metasploit for instance)
My experience suggests otherwise. It is extremely convoluted, if not impossible (I never managed to get it work after some trials and errors) to get any Metaploit shellcode to work on modern Windows without Defender flagging it, due to its popularity. (That's how I landed here ;)
Metasploit's address loader (to get position independence) with its customized "hashing algorithm" is probably too popular. Even something like msfvenom -p windows/x64/exec CMD=calc.exe
just wouldn't work even with layers of encoding / encryptions that bloat a 900 byte shellcode into 5000 (Happy to learn if there's something I missed. I guess it might work if the msf payload is broken into parts with some tricky jmps in between -- but then might as well handcode the whole thing)
I would prefer the stack move to be in a separate variable, and then the extra alignment to be just
stack_move & 8
, because the tons of math involved seem too convoluted to read.
Anyway, regarding this PR -- happy to change it in a way that's more obvious but not sure what you meant by a "separate variable"? Anyway to shed some clarity this is what the math is basically trying to do:
align(8, len(cmd) + 1)
This how much amd64.pushstr
would have advanced the stack. I believe the original code was also bugged that the null byte was not accounted for. pushstr
in the default param would have added a null byte so we need to account for it here as well, that's why +1
.align(8, len(cmd) + 1) // 8 % 2
To see what it does:
align(8, len(cmd)+1) |
align(8,len(cmd)+1)//8%2 |
---|---|
8 | 1 |
16 | 0 |
24 | 1 |
32 | 0 |
Basically, if pushstr
moved us forward an 8-byte aligned value on the stack (instead of 16), then the above would evaluate to 1
Now, before pushstr
, the stack is already 8-byte aligned due to the shellcraft not doing the push rbp
"function prolog". Therefore, we can say that the existing code is also bugged in that way that it does not align the stack before calling WinExec
. (for handcoded x64 windows assembly withotu function prologs, people sometimes manually sub rsp, 8
before function calls) (see below for an explanation of why this bug was not noticed earlier)
Thus the logic is
pushstr
pushed us through a 8-byte multiple, then we'll actually be 16-byte aligned, good to gopushstr
pushed us through an 16-byte multiple, then we'll actually be 8-byte aligned. This will cause an error in WinExec
-- we need to push another 8 byte to align ourselvesFrom 5
, we can see that we need to basically invert the result of step 3
. We can do that by ^ 1
on step 3's result. So the final expression is
pad = align(8, len(cmd) + 1) // 8 % 2 ^ 1 * 8 # * 8 because we do 8 bytes multiple. Surprisingly the operator precedences just worked out without extra parentheses
cmd.exe
plus null byte is exactly 8 bytes, and probably that was what this function was tested on. amd64.pushstr
would have pushed the stack 8 bytes thus "accidentally" aligning the stack to 16-byte alignment. However, the existing implementation would only work for a command that is 1-7 characters long (or 16-23 characters long, etc.). calc.exe
plus null byte is incidentally 9 bytes long, thus not accidentally aligning the stack.and there is no easy way to test the shellcraft payloads on the CI
Yes, thus tried to add screenshots and sufficient PoC codes so this can be hopefully easier to verify.
Sure, just pushed that please take a look
Thanks, this looks good now. The only problem I still see with this shellcode (while we're at it) is the add
instruction inserting null bytes for longer commands. How is this solved in similar places in shellcraft? The easiest solution would be to do
${amd64.mov('rcx', stack_frame + stack_frame_align)}
add rsp, rcx
to let mov
handle the value.
Pwntools Pull Request
Thanks for contributing to Pwntools! Take a moment to look at
CONTRIBUTING.md
to make sure you're familiar with Pwntools development.WinExec currently errors out when tested on Windows 11 x64. After some diagnosis it appears that
pushstr
aligns to 8 bytes but Windows calling convention requires 16 byte. Perhaps this was not enforced in previous versions of Windows but apparently that is the standard requirement.Example programs to reproduce the issue:
Screenshot:
As seen above,
rsp
is not 16 byte aligned causesmovaps
to error outAdditionally,
nCmdShow
argument support is addedTesting
Pull Requests that introduce new code should try to add doctests for that code. See
TESTING.md
for more information.Target Branch
Depending on what the PR is for, it needs to target a different branch.
You can always change the branch after you create the PR if it's against the wrong branch.
dev
dev
stable
stable
branchbeta
beta
branch, but notstable
dev
Changelog
After creating your Pull Request, please add and push a commit that updates the changelog for the appropriate branch.
You can look at the existing changelog for examples of how to do this.