-
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
Implement a method for command shells to register a post-session cleanup #9353
Implement a method for command shells to register a post-session cleanup #9353
Conversation
@wvu you may be interested in reviewing this as well, since I believe you and @mkienow-r7 had a discussion about it earlier. |
], self.class) | ||
OptString.new('InitialAutoRunScript', "An initial script to run on session creation (before AutoRunScript)"), | ||
OptString.new('AutoRunScript', "A script to run automatically on session creation."), | ||
OptString.new('CommandShellCleanupCommand', "A command to run before the session is closed") |
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.
We could also leave this unregistered, and simply allow the payloads that need it to define it. This also brought up the question of the best way for a payload module to override the default value without having to entirely redefine the datastore option, which would be a nice feature.
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.
What about DefaultOptions
?
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.
that's perfect, thanks
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.
For the record, we discussed this and think it should stay registered here so users can see that it's an option (and it'll be tab-completable). Module authors can set a default in DefaultOptions
.
def cleanup | ||
if rstream | ||
if !@cleanup_command.blank? | ||
shell_command_token(@cleanup_command, 0) |
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.
A funny thing we noticed about shell_command_token is it always waits for the timeout before returning, even if the token is returned. Feels like a bug to me, since what's the point of waiting the full timeout if you're reading the token in the first place?
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.
Have noticed this as well. Definitely wrong 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.
OK, I'll take a stab at fixing it while here.
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.
It works with #9356
I didn't bother trying to untangle the ringbuffer code, since it worked without it, and makes the Meterpreter shell and command shell paths identical.
@@ -193,12 +196,28 @@ def shell_write(buf) | |||
# :category: Msf::Session::Provider::SingleCommandShell implementors | |||
# | |||
# Closes the shell. | |||
# Note: parent's 'self.kill' method calls cleanup below. |
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.
This is important to note, since the parent's kill command will call cleanup, which means we didn't need to continue doing a direct rstream.close
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.
This is a nauseatingly common problem with our setups and cleanups. Good catch.
You beat me to the no datastore fix! I was testing the change locally. :) |
Tested successfully against a Windows target |
thanks @mkienow-r7 , good luck with your exploit |
This is precisely what I had in mind over the initial |
Release NotesEnhancement adds CommandShellCleanupCommand advanced option that provides a method for a payload module to register a post-session command that is run after a command shell session has ended, but before the connection is actually closed. This is useful for when a payload needs to cleanup after it executes, for instance, killing a telnetd after the session completes. |
This adds a way for a method for a payload module to register a post-session command that gets run after a the session has ended, but before the connection is actually closed. This is useful for when a payload needs to cleanup after it executes (for instance, killing a telnetd after the session completes).
Example usage:
./msfconsole -qx 'use multi/handler; set payload linux/x86/shell_reverse_tcp; set lhost 127.0.0.1; set CommandShellCleanupCommand pkill telnetd; run'
Created to support a new payload type from @mkienow-r7