-
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
Debug cmd_exec again #15362
Debug cmd_exec again #15362
Conversation
TestingThe goodThe badWhile checking for regressions on the *nix side, it turns out it is all broken, but through testing, it was broken in the main branch before I changed anything.
|
# Platform-agnostic file write. Writes given object content to a remote file. | ||
# | ||
# NOTE: *This is not binary-safe on Windows shell sessions!* | ||
# |
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.
🎉
end | ||
end | ||
end | ||
end | ||
rescue | ||
# nothing, just continue |
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.
Bit on the fence about removing the rescues here, as we might have modules relying on nil being returned - rather than exploding. Even if exploding is better/expected behavior 🤔
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.
Let's test this a bit more against linux targets too (not specifically file read/writes), and if it's good - we'll land it, and wait for the screaming 😱
That's some epic windows shell-fu. |
@sempervictus I completely agree on certutil. I was hoping for something less obvious than invoking certutil, because it is the go-to for base64 interaction on the Windows side, and I figured it was likely to be monitored. Unfortunately, the reason it is the go-to is because it is one of the only ways covered in Windows 7-10 I've found to do base64 encode/decode. |
@agalway-r7 and myself have tested the above code and it seems to work great! We did encounter one bug that we think is unrelated. When we get an initial shell with the We haven't tested this with other initial sessions. We wanted to check if this is a known issue before testing further. FYI, our environment:
|
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.
Looks good. This seems to fix #15311 and fixes the cmd_exec tests on both windows/x64/shell_reverse_tcp
and cmd/windows/reverse_powershell
sessions.
Double checked this hasn't broken linux 👍 |
Release NotesFixes a regression issue with |
Please wait for me to do some automated testing to verify this works across all supported Windows versions and no regressions were introduced to Linux. Automated testing is easier on PRs, so I've only done some basic tests locally. I'll leave it in draft until I finish the automated testing.This is yet another attempt to fix some of the bugs in cmd_exec. I noticed that the cmd_exec tests were failing on Windows shells and after digging a bit, I don't think the
write_file
orappend_file
functionality ever worked except under some very contrived circumstances, namely, it would not work on binary files that contained non-ansi characters or any file larger than ~ 7K.Further, because of some interesting sharing of code between *nix and windows shells, the tokenization is sloppy with regard to newlines- sometimes Windows newlines, sometimes *nix-style newlines, depending on whether we send the token, or Windows echos the token. Rather than go back and split the code that sends the token, I just normalize the newlines before processing responses. It is certainly feasible to fix this on the sending end, but this approach also works. It also allows me to remove the abomination I added in #12463 because the trailing newlines are corrected upstream. 🎉
Finally, there was a rescue that hid everything rather than just the expected timeout errors, so when any number or errors got thrown, it defaulted to that stupid
no method reverse! for nil
message because it was the first thing following that empty rescue all.One concern I have is that we're making a awful lot of files on target for binary file appends- file_name.b64, file_name.tmp, and file_name and I am slightly concerned we might run into some collisions, especially with the *.tmp. I'd be curious to know what others think about that and if I'm just being overly paranoid.
Verification
List the steps needed to make sure this thing works
msfconsole
use payload/windows/x64/shell_reverse_tcp
generate -f exe -o <payloadname>
to_handler
loadpath test/modules
use post/test/file
set session <session>
set verbose true
run
Old and busted
New Hotness
Also, I just realized I need to update some copy/pasta comments.....