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 delimiter selection on powershell payload commands #12463

Merged
merged 5 commits into from Nov 1, 2019

Conversation

@bwatters-r7
Copy link
Contributor

bwatters-r7 commented Oct 17, 2019

While looking at getting some rex-powershell stuff landed, I tried adding powershell payloads to the automated testing, but while the testing system supports it (to my happy surprise), the cmd_exec tests fail.

After some digging, it turns out that the poswershell session was adding the bytestring 0xdad20 after our second delimiter. For some reason (I now see) rather than splitting on the delimiter, we were deleting the second delimiter so that the odd bytestring remained and screwed up the echo tests. Normally, this would be taken care of by our call to chomp, but the space is not eaten by `chomp, so that protected the trailing characters.

One solution was to call "strip" to the response, but then we'd lose any expected trailing whitespace. In this case, I simply split on the second delimiter and threw away any trailing characters.

That approach works, but is is super slow. Significantly slower than I would expect. If someone can find a faster route, I am game. Previously, I tried to combine the regexes and split the substring between the delimiters, and it was not appreciably different in speed than this solution. I also tried using a second buffer under the theory in-place changes might take longer. No such luck.

Verification

List the steps needed to make sure this thing works

  • Start msfconsole
  • use exploit/multi/handler
  • set payload windows/x64/meterpreter/reverse_tcp
  • set lhost <you>
  • set lport <port>
  • run
  • Get session
  • Background session
  • loadpath test/modules
  • use post/test/cmd_exec
  • set session 1
  • set verbose true
  • run
  • Verify that the tests succeed

There are still some things that need attention in the cmd_exec stuff and I've already got an idea on how to fix this for shells, but I wanted to get this up now and see if anyone had feedback on the performance issue.

@bwatters-r7

This comment has been minimized.

Copy link
Contributor Author

bwatters-r7 commented Oct 17, 2019

I marked this as delayed; I'm trying to get a fix in for the command shell errors, too, but that requires a bit more effort and changes to the test module.

inconsistent windows platform type.
* Add extra front chomp to shell_command response string because
there's a leading newline there.. for some reason?
@bwatters-r7

This comment has been minimized.

Copy link
Contributor Author

bwatters-r7 commented Oct 17, 2019

Added changes so that cmd_exec tests pass with shell sessions, too.
Proof:

msf5 post(test/cmd_exec) > set session 2
session => 2
msf5 post(test/cmd_exec) > run

[!] SESSION may not be compatible with this module.
[*] Running against session 2
[*] Session type is powershell and platform is win
[*] Starting cmd_exec tests
[+] should return the result of echo
[+] should return the full response after sleeping
[+] should return the full response after sleeping
[+] should return the result of echo 10 times
[*] Finished cmd_exec tests
[*] Starting cmd_exec quote tests
[+] should return the result of echo with single quotes
[+] should return the result of echo with double quotes
[*] Testing complete in 225.234382416
[*] Passed: 6; Failed: 0
[*] Post module execution completed
msf5 post(test/cmd_exec) > set session 1
session => 1
msf5 post(test/cmd_exec) > run

[!] SESSION may not be compatible with this module.
[*] Running against session 1
[*] Session type is shell and platform is windows
[*] Starting cmd_exec tests
[+] should return the result of echo
[*] Session does not support sleep, skipping sleep tests
[*] Finished cmd_exec tests
[*] Starting cmd_exec quote tests
[+] should return the result of echo with single quotes
[+] should return the result of echo with double quotes
[*] Testing complete in 0.088242649
[*] Passed: 3; Failed: 0
[*] Post module execution completed
msf5 post(test/cmd_exec) > 
@@ -146,7 +146,8 @@ def cmd_exec(cmd, args=nil, time_out=15)
else
o = session.shell_command_token("#{cmd} #{args}", time_out)
end
o.chomp! if o
# this is terrible
o.chomp!.reverse!.chomp!.reverse! if o

This comment has been minimized.

Copy link
@bwatters-r7

bwatters-r7 Oct 17, 2019

Author Contributor

Please; someone tell me there's a better way to do this!

after we pass it around for a while.
@bwatters-r7

This comment has been minimized.

Copy link
Contributor Author

bwatters-r7 commented Oct 18, 2019

Hrm..... The cmd_exec tests pass on Windows shell sessions and powershell sessions, but the lag is still there, and for some reason, the automated tests never recognize the powershell sessions. I need to do some more work on this.

@busterb busterb self-assigned this Oct 21, 2019
Changed the newline remove to a chomp in case the newline is not there
@bwatters-r7

This comment has been minimized.

Copy link
Contributor Author

bwatters-r7 commented Oct 22, 2019

OK; I'm removing the delayed and saying this is ready to get landed....
There's still something off here in the handshake with powershell.....
I'm getting session created, but not completely created. Need to dig more.

I went ahead and guarded against @wvu-r7's cases when shell_command did not have a leading newline by doing the painful string.reverse!.chomp!.reverse! thing, and I fixed where I screwed up the session establishment by thinking I was implicitly returning a string, but was actually returning a bool. 🤷‍♂

There are still some issues related to automated testing, but that's all over in the geppetto repo and I'll iron those out there. I'd still like to see powershell commands be snappier, but as the Rolling Stones remind me, you can't always get what you want.

@bwatters-r7 bwatters-r7 added delayed and removed delayed labels Oct 22, 2019
shell_read_until_token(token, 1, timeout)
res = shell_read_until_token(token, 1, timeout)
# I would like a better way to do this, but I don't know of one
res.reverse!.chomp!.reverse! # the presence of a leading newline is not consistent

This comment has been minimized.

Copy link
@acammack-r7

acammack-r7 Oct 23, 2019

Contributor

Slightly better than this, I would propose: res.sub!(/\A\r?\n/, '')

@bwatters-r7

This comment has been minimized.

Copy link
Contributor Author

bwatters-r7 commented Oct 23, 2019

OK; I give.
The cmd_exec stuff succeeds, now, but there's something wonky with the session creation on Windows 7/8 targets, and I don't have time to dig around anymore. This PR solves the original problem of the cmd_exec tests failing, so let's get it out. The session creation oddity will have to wait for another day. I filed issue #12487 to track it.

@bwatters-r7 bwatters-r7 removed the delayed label Oct 23, 2019
bcook-r7 pushed a commit that referenced this pull request Nov 1, 2019
@bcook-r7 bcook-r7 merged commit 13b54ef into rapid7:master Nov 1, 2019
3 checks passed
3 checks passed
Metasploit Automation - Sanity Test Execution Successfully completed all tests.
Details
Metasploit Automation - Test Execution Successfully completed all tests.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@busterb

This comment has been minimized.

Copy link
Member

busterb commented Nov 1, 2019

Fair enough, thanks @bwatters-r7

msjenkins-r7 added a commit that referenced this pull request Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.