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.

Sign up for GitHub

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

Jump to bottom

Implement a method for command shells to register a post-session cleanup #9353

Merged
merged 3 commits into from
Dec 29, 2017
Merged

Implement a method for command shells to register a post-session cleanup #9353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e546598
Implement a method for command shells to register a post-session clea…
busterb Dec 29, 2017
198aeda
rename option
busterb Dec 29, 2017
a444bdb
handle no datastore
busterb Dec 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions lib/msf/base/sessions/command_shell.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,14 @@ def self.type
"shell"
end

def initialize(*args)
def initialize(conn, opts = {})
self.platform ||= ""
self.arch ||= ""
self.max_threads = 1
datastore = opts[:datastore]
if datastore && !datastore["CommandShellCleanupCommand"].blank?
@cleanup_command = opts[:datastore]["CommandShellCleanupCommand"]
end
super
end

Expand Down Expand Up @@ -193,12 +197,28 @@ def shell_write(buf)
# :category: Msf::Session::Provider::SingleCommandShell implementors
#
# Closes the shell.
# Note: parent's 'self.kill' method calls cleanup below.
Copy link
Member Author

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

Copy link
Contributor

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.

#
def shell_close()
rstream.close rescue nil
self.kill
end

##
# :category: Msf::Session implementors
#
# Closes the shell.
#
def cleanup
if rstream
if !@cleanup_command.blank?
shell_command_token(@cleanup_command, 0)
Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

end
rstream.close
rstream = nil
end
super
end

#
# Execute any specified auto-run scripts for this session
#
Expand Down
8 changes: 5 additions & 3 deletions lib/msf/base/sessions/command_shell_options.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ def initialize(info = {})

register_advanced_options(
[
OptString.new('InitialAutoRunScript', [false, "An initial script to run on session creation (before AutoRunScript)", '']),
OptString.new('AutoRunScript', [false, "A script to run automatically on session creation.", ''])
], 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")
Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about DefaultOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's perfect, thanks

Copy link
Contributor

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.

]
)
end

def on_session(session)
Expand Down