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

Session Management: Allow killing multiple specific sessions #4063

Merged
merged 5 commits into from Nov 10, 2014

Conversation

Projects
None yet
7 participants
@TomSellers
Contributor

TomSellers commented Oct 23, 2014

The intent of this change is to allow the user to specify multiple sessions to kill when using the sessions -k switch. This uses the build_sessions_array function added in PR #3401 which is now landed.

sessions -k <session id range> 
sessions -k 1,3,5-8

EDIT:
With the help and feedback from @kernelsmith and @wvu-r7 this PR has been expanded to support specifying ranges for the command, detach, and script options of sessions as well as the kill option of jobs. There has been additional code cleanup performed in the areas of the module that were touched in this PR.

Here is an example that specifies a single session, a range, and an invalid session ID.

msf exploit(psexec) > sessions 

Active sessions
===============

  Id  Type                   Information                    Connection
  --  ----                   -----------                    ----------
  1   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49526 (192.168.190.210)
  2   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49528 (192.168.190.210)
  3   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49529 (192.168.190.210)
  4   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49530 (192.168.190.210)
  5   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49532 (192.168.190.210)
  6   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49533 (192.168.190.210)

msf exploit(psexec) > sessions -k 1,3-5,7
[*] Killing the following session(s): [1, 3, 4, 5, 7]
[*] Killing session 1
[*]192.168.190.210 - Meterpreter session 1 closed.
[*] Killing session 3
[*]192.168.190.210 - Meterpreter session 3 closed.
[*] Killing session 4
[*]192.168.190.210 - Meterpreter session 4 closed.
[*] Killing session 5
[*]192.168.190.210 - Meterpreter session 5 closed.
[-] Invalid session identifier: 7
msf exploit(psexec) > sessions 

Active sessions
===============

  Id  Type                   Information                    Connection
  --  ----                   -----------                    ----------
  2   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49528 (192.168.190.210)
  6   meterpreter x86/win32  NT AUTHORITY\SYSTEM @ WINDC01 192.168.180.102:8343 ->192.168.190.210:49533 (192.168.190.210)

@TomSellers TomSellers changed the title from Allow killing multiple specific sessions to Session Management: Allow killing multiple specific sessions Oct 23, 2014

@wvu-r7 wvu-r7 self-assigned this Oct 23, 2014

Show outdated Hide outdated lib/msf/ui/console/command_dispatcher/core.rb
session_list = build_sessions_array(sid)
print_status("Killing the following session(s): #{session_list}")
session_list.each do |sess|
if ((session = framework.sessions.get(sess)))

This comment has been minimized.

@kernelsmith

kernelsmith Oct 24, 2014

Contributor

you have 2 extra sets of parens here and I think you left out an '='.
I believe this should be

if session == framework.sessions.get(sess)
@kernelsmith

kernelsmith Oct 24, 2014

Contributor

you have 2 extra sets of parens here and I think you left out an '='.
I believe this should be

if session == framework.sessions.get(sess)

This comment has been minimized.

@kernelsmith

kernelsmith Oct 24, 2014

Contributor

never mind, you are doing assignment here on purpose

@kernelsmith

kernelsmith Oct 24, 2014

Contributor

never mind, you are doing assignment here on purpose

This comment has been minimized.

@kernelsmith

kernelsmith Oct 24, 2014

Contributor

these are generally considered evil. I would recommend:

session = framework.sessions.get(sess)
if session
  print_status("Killing session #{sess}")
  session.kill
else
  print_error("Invalid session identifier: #{sess}")
end
@kernelsmith

kernelsmith Oct 24, 2014

Contributor

these are generally considered evil. I would recommend:

session = framework.sessions.get(sess)
if session
  print_status("Killing session #{sess}")
  session.kill
else
  print_error("Invalid session identifier: #{sess}")
end

@kernelsmith kernelsmith assigned kernelsmith and unassigned wvu-r7 Oct 24, 2014

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Oct 24, 2014

Contributor

@TomSellers if you fix that if statement, I can merge this

Contributor

kernelsmith commented Oct 24, 2014

@TomSellers if you fix that if statement, I can merge this

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Oct 24, 2014

Contributor

Thanks for the review and feedback @kernelsmith
I used that syntax in order to stay consistent with the rest of the file. I can certainly change it to be less evil.

Contributor

TomSellers commented Oct 24, 2014

Thanks for the review and feedback @kernelsmith
I used that syntax in order to stay consistent with the rest of the file. I can certainly change it to be less evil.

@mubix

This comment has been minimized.

Show comment
Hide comment
@mubix

mubix Oct 24, 2014

Contributor

Just saying, but I would love to see an "all" option, as well as a sessions -d 1,2,3,4 or something like it to detach HTTP(s) sessions

Contributor

mubix commented Oct 24, 2014

Just saying, but I would love to see an "all" option, as well as a sessions -d 1,2,3,4 or something like it to detach HTTP(s) sessions

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Oct 24, 2014

Contributor

@mubix - If it doesn't introduce any functionality issues I'll submit a patch for that this weekend. I haven't worked with detached sessions before so it will be a chance to learn something.

Contributor

TomSellers commented Oct 24, 2014

@mubix - If it doesn't introduce any functionality issues I'll submit a patch for that this weekend. I haven't worked with detached sessions before so it will be a chance to learn something.

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Oct 24, 2014

Contributor

mubix, you mean like sessions -K (kills all sessions) or you mean more like sessions -K https which would kill only all https sessions?

On Oct 24, 2014, at 1:54 PM, Rob Fuller notifications@github.com wrote:

Just saying, but I would love to see an "all" option, as well as a sessions -d 1,2,3,4 or something like it to detach HTTP(s) sessions


Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Oct 24, 2014

mubix, you mean like sessions -K (kills all sessions) or you mean more like sessions -K https which would kill only all https sessions?

On Oct 24, 2014, at 1:54 PM, Rob Fuller notifications@github.com wrote:

Just saying, but I would love to see an "all" option, as well as a sessions -d 1,2,3,4 or something like it to detach HTTP(s) sessions


Reply to this email directly or view it on GitHub.

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Oct 25, 2014

Contributor

@kernelsmith Syntax changes per request. Thanks for the review and feedback.

Contributor

TomSellers commented Oct 25, 2014

@kernelsmith Syntax changes per request. Thanks for the review and feedback.

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
Contributor

wvu-r7 commented Oct 28, 2014

Show outdated Hide outdated lib/msf/ui/console/command_dispatcher/core.rb
end
end
return job_list.uniq.sort

This comment has been minimized.

@kernelsmith

kernelsmith Oct 28, 2014

Contributor

explicit return is not required here as it's the last thing evaluated in the method
I can clear this during merge, the detach question is more important

@kernelsmith

kernelsmith Oct 28, 2014

Contributor

explicit return is not required here as it's the last thing evaluated in the method
I can clear this during merge, the detach question is more important

This comment has been minimized.

@wvu-r7

wvu-r7 Oct 31, 2014

Contributor

Yup. If you want to fix it, it should be fixed in build_sessions_array directly above, too.

@wvu-r7

wvu-r7 Oct 31, 2014

Contributor

Yup. If you want to fix it, it should be fixed in build_sessions_array directly above, too.

Show outdated Hide outdated lib/msf/ui/console/command_dispatcher/core.rb
print_line("Stopping job: #{val}...")
framework.jobs.stop_job(val)
job_list = build_jobs_array(val)
print_status("Stopping the following job(s): #{job_list.join(', ')}")

This comment has been minimized.

@kernelsmith

kernelsmith Oct 28, 2014

Contributor

do you think this should be a vprint_status? It's not like you'll have time to stop the process if you don't like the list that gets displayed

@kernelsmith

kernelsmith Oct 28, 2014

Contributor

do you think this should be a vprint_status? It's not like you'll have time to stop the process if you don't like the list that gets displayed

This comment has been minimized.

@wvu-r7

wvu-r7 Oct 31, 2014

Contributor

Good idea. Then it should be changed in cmd_sessions as well.

@wvu-r7

wvu-r7 Oct 31, 2014

Contributor

Good idea. Then it should be changed in cmd_sessions as well.

This comment has been minimized.

@TomSellers

TomSellers Oct 31, 2014

Contributor

vprint_status is not valid in this context. We would have to define it, add Module.rb (?) or adjust the code to

print_status("Stopping the following job(s): #{job_list.join(', ')}") if datastore['VERBOSE'] || framework.datastore['VERBOSE']

@TomSellers

TomSellers Oct 31, 2014

Contributor

vprint_status is not valid in this context. We would have to define it, add Module.rb (?) or adjust the code to

print_status("Stopping the following job(s): #{job_list.join(', ')}") if datastore['VERBOSE'] || framework.datastore['VERBOSE']

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Oct 28, 2014

Contributor

I think the usage for cmd_sessions/jobs should be slightly updated as well, but I can PR that separately once this goes in

Contributor

kernelsmith commented Oct 28, 2014

I think the usage for cmd_sessions/jobs should be slightly updated as well, but I can PR that separately once this goes in

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@wvu-r7

wvu-r7 Oct 31, 2014

Contributor

@kernelsmith: Re TomSellers@5547890#commitcomment-8337475: I accidentally a line. It should check session.interactive? like it did before.

Contributor

wvu-r7 commented Oct 31, 2014

@kernelsmith: Re TomSellers@5547890#commitcomment-8337475: I accidentally a line. It should check session.interactive? like it did before.

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Oct 31, 2014

Contributor

@toms I started to land this last night, was just gonna do some slight cleanup, then I saw something in the rest of core.rb to clean up, then something else, then 30 other things...3hours later I was done. I didn't have the energy to test at that point so I'll do that today. If you can, don't push any changes unless you really need to. I have incorporated any and all comments to this point including the session.interactive from wvu (modified) and comments I made after your fixes

-Josh

On Oct 31, 2014, at 00:39, wvu-r7 notifications@github.com wrote:

Re TomSellers/metasploit-framework@5547890#commitcomment-8337475: I accidentally a line. It should check session.interactive?.


Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Oct 31, 2014

@toms I started to land this last night, was just gonna do some slight cleanup, then I saw something in the rest of core.rb to clean up, then something else, then 30 other things...3hours later I was done. I didn't have the energy to test at that point so I'll do that today. If you can, don't push any changes unless you really need to. I have incorporated any and all comments to this point including the session.interactive from wvu (modified) and comments I made after your fixes

-Josh

On Oct 31, 2014, at 00:39, wvu-r7 notifications@github.com wrote:

Re TomSellers/metasploit-framework@5547890#commitcomment-8337475: I accidentally a line. It should check session.interactive?.


Reply to this email directly or view it on GitHub.

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Oct 31, 2014

Contributor

@kernelsmith Doh, I made quite a few changes this morning. I collapsed the two new functions, changed some of the error handling, dealt with trash in the session/job list, and expanded the functionality to the sessions -c command. If you want, make your changes and I will drop any of my outstanding changes on top of that.

Contributor

TomSellers commented Oct 31, 2014

@kernelsmith Doh, I made quite a few changes this morning. I collapsed the two new functions, changed some of the error handling, dealt with trash in the session/job list, and expanded the functionality to the sessions -c command. If you want, make your changes and I will drop any of my outstanding changes on top of that.

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Oct 31, 2014

Contributor

@kernelsmith Here's the diff of my current local code. https://gist.github.com/TomSellers/9f96819ec787396c7b77

In addition to what was already done it:

  • Added range support to sessions -c and sessions -s
  • Added check for undetachable sessions
  • Added back the check for session.interactive? when detaching sessions
  • Collapse build_jobs_array and build_sessions_array to build_range_array
  • Added check for empty or invalid parameters to detach, script, command, and kill [session | job]
  • Reworked session id sanity check around line 1660
  • RuboCop/Style guide change: Array.new -> []
  • Fixed usage of parens in multiple IF statements
  • Misc RuboCop/Style guide spacing changes
Contributor

TomSellers commented Oct 31, 2014

@kernelsmith Here's the diff of my current local code. https://gist.github.com/TomSellers/9f96819ec787396c7b77

In addition to what was already done it:

  • Added range support to sessions -c and sessions -s
  • Added check for undetachable sessions
  • Added back the check for session.interactive? when detaching sessions
  • Collapse build_jobs_array and build_sessions_array to build_range_array
  • Added check for empty or invalid parameters to detach, script, command, and kill [session | job]
  • Reworked session id sanity check around line 1660
  • RuboCop/Style guide change: Array.new -> []
  • Fixed usage of parens in multiple IF statements
  • Misc RuboCop/Style guide spacing changes
@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Oct 31, 2014

Contributor

One of us is gonna have to deal w/merge conflicts and I don’t it’s fair to put that on you, so why don’t you push your changes, looks like many of them are the same anyways as many of your additions are things I fixed as well. I was also doing this:

if session
  if session.interactive?
    blah
  else
    # warn not interactive, can’t do whatever
  end
else
  # warn invalid session id
end

On Oct 31, 2014, at 11:06 AM, Tom Sellers notifications@github.com wrote:

@kernelsmith Here's the diff of my current local code. https://gist.github.com/TomSellers/9f96819ec787396c7b77

In addition to what was already done it:

Added range support to sessions -c and sessions -s
Added check for undetachable sessions
Added back the check for session.interactive? when detaching sessions
Collapse build_jobs_array and build_sessions_array to build_range_array
Added check for empty or invalid parameters to detach, script, command, and kill [session | job]
Reworked session id sanity check around line 1660
RuboCop/Style guide change: Array.new -> []
Fixed usage of parens in multiple IF statements
Misc RuboCop/Style guide spacing changes

Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Oct 31, 2014

One of us is gonna have to deal w/merge conflicts and I don’t it’s fair to put that on you, so why don’t you push your changes, looks like many of them are the same anyways as many of your additions are things I fixed as well. I was also doing this:

if session
  if session.interactive?
    blah
  else
    # warn not interactive, can’t do whatever
  end
else
  # warn invalid session id
end

On Oct 31, 2014, at 11:06 AM, Tom Sellers notifications@github.com wrote:

@kernelsmith Here's the diff of my current local code. https://gist.github.com/TomSellers/9f96819ec787396c7b77

In addition to what was already done it:

Added range support to sessions -c and sessions -s
Added check for undetachable sessions
Added back the check for session.interactive? when detaching sessions
Collapse build_jobs_array and build_sessions_array to build_range_array
Added check for empty or invalid parameters to detach, script, command, and kill [session | job]
Reworked session id sanity check around line 1660
RuboCop/Style guide change: Array.new -> []
Fixed usage of parens in multiple IF statements
Misc RuboCop/Style guide spacing changes

Reply to this email directly or view it on GitHub.

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Oct 31, 2014

Contributor

The changes have been committed. Please let me know if you see anything that I you want me to address.

Contributor

TomSellers commented Oct 31, 2014

The changes have been committed. Please let me know if you see anything that I you want me to address.

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 2, 2014

Contributor

@TomSellers I have a PR for you. I will finish testing it and send it tomorrow. There's only one structural change, and that was to add a verify_session method as I noticed we were duplicating the if session and if session_interactive? code alot, I think it's much nicer now

Contributor

kernelsmith commented Nov 2, 2014

@TomSellers I have a PR for you. I will finish testing it and send it tomorrow. There's only one structural change, and that was to add a verify_session method as I noticed we were duplicating the if session and if session_interactive? code alot, I think it's much nicer now

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 2, 2014

Contributor

@TomSellers are you getting this or just me? Specifically, Error when running -c on a valid session. I'm going to test master here in a sec too

msf exploit(psexec) > sessions -i 1-2 -c getuid
[*] Running 'getuid' on meterpreter session 1 (192.168.66.177)
[-] Failed: Rex::Post::Meterpreter::RequestError stdapi_sys_process_execute: Operation failed: The system cannot find the file specified.
[-] Invalid session identifier: 2, skipping
msf exploit(psexec) > sessions -i 1-2 -c pwd
[*] Running 'pwd' on meterpreter session 1 (192.168.66.177)
[-] Failed: Rex::Post::Meterpreter::RequestError stdapi_sys_process_execute: Operation failed: The system cannot find the file specified.
[-] Invalid session identifier: 2, skipping
msf exploit(psexec) > si 1
[*] Starting interaction with 1...

meterpreter > pwd
C:\Windows\system32
meterpreter >
Contributor

kernelsmith commented Nov 2, 2014

@TomSellers are you getting this or just me? Specifically, Error when running -c on a valid session. I'm going to test master here in a sec too

msf exploit(psexec) > sessions -i 1-2 -c getuid
[*] Running 'getuid' on meterpreter session 1 (192.168.66.177)
[-] Failed: Rex::Post::Meterpreter::RequestError stdapi_sys_process_execute: Operation failed: The system cannot find the file specified.
[-] Invalid session identifier: 2, skipping
msf exploit(psexec) > sessions -i 1-2 -c pwd
[*] Running 'pwd' on meterpreter session 1 (192.168.66.177)
[-] Failed: Rex::Post::Meterpreter::RequestError stdapi_sys_process_execute: Operation failed: The system cannot find the file specified.
[-] Invalid session identifier: 2, skipping
msf exploit(psexec) > si 1
[*] Starting interaction with 1...

meterpreter > pwd
C:\Windows\system32
meterpreter >
@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 2, 2014

Contributor

same error on master too, so at least not our fault ;)
Opened #4118 to track that problem

Contributor

kernelsmith commented Nov 2, 2014

same error on master too, so at least not our fault ;)
Opened #4118 to track that problem

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Nov 2, 2014

Contributor

@kernelsmith Try sessions -i 1-2 -c hostname

Contributor

TomSellers commented Nov 2, 2014

@kernelsmith Try sessions -i 1-2 -c hostname

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@wvu-r7

wvu-r7 Nov 2, 2014

Contributor

You guys are awesome. :3

Contributor

wvu-r7 commented Nov 2, 2014

You guys are awesome. :3

TomSellers added some commits Nov 5, 2014

Merge pull request #3 from kernelsmith/landing/4063-DRYer
modernizes & DRYs session/job ranges from kernelsmith
Minor fixes and status update
Minor tweaks after the PR from @kernelsmith

Remaining items:

1. Handle empty session IDs correctly, for example 'sessions -d' or 'sessions -k'
2. Find a method of explaining the range options in the help text
3. Retest all changed code areas
4. Edit PR Summary to reflect changes to the scope
@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Nov 5, 2014

Contributor

Remaining items:

  1. Handle empty session IDs correctly, for example 'sessions -d' or 'sessions -k'
  2. Find a method of explaining the range options in the help text
  3. Retest all changed code areas
  4. Edit PR Summary to reflect changes to the scope
Contributor

TomSellers commented Nov 5, 2014

Remaining items:

  1. Handle empty session IDs correctly, for example 'sessions -d' or 'sessions -k'
  2. Find a method of explaining the range options in the help text
  3. Retest all changed code areas
  4. Edit PR Summary to reflect changes to the scope
@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 5, 2014

Contributor

@TomSellers Don't u just get an invalid session identifier message if u do sessions -k or -d etc?

-Josh

On Nov 5, 2014, at 06:50, Tom Sellers notifications@github.com wrote:

Remaining items:

Handle empty session IDs correctly, for example 'sessions -d' or 'sessions -k'
Find a method of explaining the range options in the help text
Retest all changed code areas
Edit PR Summary to reflect changes to the scope

Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Nov 5, 2014

@TomSellers Don't u just get an invalid session identifier message if u do sessions -k or -d etc?

-Josh

On Nov 5, 2014, at 06:50, Tom Sellers notifications@github.com wrote:

Remaining items:

Handle empty session IDs correctly, for example 'sessions -d' or 'sessions -k'
Find a method of explaining the range options in the help text
Retest all changed code areas
Edit PR Summary to reflect changes to the scope

Reply to this email directly or view it on GitHub.

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Nov 5, 2014

Contributor

@kernelsmith No, it bypasses the session validation here because sid is nil

    unless sid.blank? || method == 'interact'
      session_list = build_range_array(sid)
      if session_list.blank?
        print_error("Please specify valid session identifier(s)")
        return false
      end
    end

This means it generates an error here because it tries to .join a nilClass

when 'detach'
      print_status("Detaching the following session(s): #{session_list.join(', ')}")
      session_list.each do |sess_id|

Its a logic flaw that will affect any of the newly modified code. I haven't had a chance to apply adequate levels of brain to handle this cleanly.

Contributor

TomSellers commented Nov 5, 2014

@kernelsmith No, it bypasses the session validation here because sid is nil

    unless sid.blank? || method == 'interact'
      session_list = build_range_array(sid)
      if session_list.blank?
        print_error("Please specify valid session identifier(s)")
        return false
      end
    end

This means it generates an error here because it tries to .join a nilClass

when 'detach'
      print_status("Detaching the following session(s): #{session_list.join(', ')}")
      session_list.each do |sess_id|

Its a logic flaw that will affect any of the newly modified code. I haven't had a chance to apply adequate levels of brain to handle this cleanly.

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 5, 2014

Contributor

If (sid && !sid.blank?) || method == 'interact'
Perhaps?

-Josh

On Nov 5, 2014, at 09:20, Tom Sellers notifications@github.com wrote:

@kernelsmith No, it bypasses the session validation here because sid is nil

unless sid.blank? || method == 'interact'
  session_list = build_range_array(sid)
  if session_list.blank?
    print_error("Please specify valid session identifier(s)")
    return false
  end
end

This means it generates an error here because it tries to .join a nilClass

when 'detach'
print_status("Detaching the following session(s): #{session_list.join(', ')}")
session_list.each do |sess_id|
Its a logic flaw that will affect any of the newly modified code. I haven't had a chance to apply adequate levels of brain to handle this cleanly.


Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Nov 5, 2014

If (sid && !sid.blank?) || method == 'interact'
Perhaps?

-Josh

On Nov 5, 2014, at 09:20, Tom Sellers notifications@github.com wrote:

@kernelsmith No, it bypasses the session validation here because sid is nil

unless sid.blank? || method == 'interact'
  session_list = build_range_array(sid)
  if session_list.blank?
    print_error("Please specify valid session identifier(s)")
    return false
  end
end

This means it generates an error here because it tries to .join a nilClass

when 'detach'
print_status("Detaching the following session(s): #{session_list.join(', ')}")
session_list.each do |sess_id|
Its a logic flaw that will affect any of the newly modified code. I haven't had a chance to apply adequate levels of brain to handle this cleanly.


Reply to this email directly or view it on GitHub.

@jvennix-r7

This comment has been minimized.

Show comment
Hide comment
@jvennix-r7

jvennix-r7 Nov 5, 2014

Contributor

.blank?/.present? are defined on nil so no need for the nil check. And !obj.blank? is the same as obj.present?, so maybe: sid.present? || method == 'interact'

Contributor

jvennix-r7 commented Nov 5, 2014

.blank?/.present? are defined on nil so no need for the nil check. And !obj.blank? is the same as obj.present?, so maybe: sid.present? || method == 'interact'

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Nov 5, 2014

Contributor

In this logic flow using sessions -d results in `sidbeingnil`` and so the problem would remain.

In my mind this can be adjusted in one of two ways:

  1. Make sure sid is always set to some value for those operations requiring it or
  2. in the block for detach, kill etc. we check for a valid value of sid. This could be dangerous because of how some values are handled by code in build_range_array. For example if a string or negative value is provided this is translated into the integer 0.

Either can be done, I just needed to work out the most reliable method with in the smallest amount of code.

Contributor

TomSellers commented Nov 5, 2014

In this logic flow using sessions -d results in `sidbeingnil`` and so the problem would remain.

In my mind this can be adjusted in one of two ways:

  1. Make sure sid is always set to some value for those operations requiring it or
  2. in the block for detach, kill etc. we check for a valid value of sid. This could be dangerous because of how some values are handled by code in build_range_array. For example if a string or negative value is provided this is translated into the integer 0.

Either can be done, I just needed to work out the most reliable method with in the smallest amount of code.

Fix empty session ID and cleanup
- Fixed handling of empty session IDs for those commands that required them
- Added help text for ranges with examples
Show outdated Hide outdated lib/msf/ui/console/command_dispatcher/core.rb
if session.type == 'meterpreter'
# If session.sys is nil, dont even try..
print_good "Checking if session.sys"

This comment has been minimized.

@TomSellers

TomSellers Nov 6, 2014

Contributor

@kernelsmith Do we need this as default output?

@TomSellers

TomSellers Nov 6, 2014

Contributor

@kernelsmith Do we need this as default output?

This comment has been minimized.

@kernelsmith

kernelsmith Nov 6, 2014

Contributor

No. Leftover. It was actually there to help pinpoint the problem with -c getuid etc but I forgot to pursue that

-Josh

On Nov 6, 2014, at 07:22, Tom Sellers notifications@github.com wrote:

In lib/msf/ui/console/command_dispatcher/core.rb:

  •          end
    
  •          if process and process.channel and (data = process.channel.read)
    
  •            print_line(data)
    
  •          end
    
  •        elsif session.type == "shell"
    
  •          if (output = session.shell_command(cmd))
    
  •            print_line(output)
    
  •          end
    
  •    sessions.each do |s|
    
  •      session = verify_session(s)
    
  •      next unless session
    
  •      print_status("Running '#{cmd}' on #{session.type} session #{s} (#{session.session_host})")
    
  •      if session.type == 'meterpreter'
    
  •        # If session.sys is nil, dont even try..
    
  •        print_good "Checking if session.sys"
    
    @kernelsmith Do we need this as default output?


Reply to this email directly or view it on GitHub.

@kernelsmith

kernelsmith Nov 6, 2014

Contributor

No. Leftover. It was actually there to help pinpoint the problem with -c getuid etc but I forgot to pursue that

-Josh

On Nov 6, 2014, at 07:22, Tom Sellers notifications@github.com wrote:

In lib/msf/ui/console/command_dispatcher/core.rb:

  •          end
    
  •          if process and process.channel and (data = process.channel.read)
    
  •            print_line(data)
    
  •          end
    
  •        elsif session.type == "shell"
    
  •          if (output = session.shell_command(cmd))
    
  •            print_line(output)
    
  •          end
    
  •    sessions.each do |s|
    
  •      session = verify_session(s)
    
  •      next unless session
    
  •      print_status("Running '#{cmd}' on #{session.type} session #{s} (#{session.session_host})")
    
  •      if session.type == 'meterpreter'
    
  •        # If session.sys is nil, dont even try..
    
  •        print_good "Checking if session.sys"
    
    @kernelsmith Do we need this as default output?


Reply to this email directly or view it on GitHub.

# @TODO: Not interactive sessions can or cannot have scripts run on them?
if session == false # specifically looking for false
# if verify_session returned false, sess_id is valid, but not interactive
session = framework.sessions.get(sess_id)

This comment has been minimized.

@TomSellers

TomSellers Nov 6, 2014

Contributor

@kernelsmith Was the intent here to just provide a warning on a non-interactive session because this code checks for interactive and, if false, binds anyway. After taking a quick glance at the scripts folder I don't see anything that stands out to me as something that would work if the session is non-interactive.

Who would be a good person to query on this?

@TomSellers

TomSellers Nov 6, 2014

Contributor

@kernelsmith Was the intent here to just provide a warning on a non-interactive session because this code checks for interactive and, if false, binds anyway. After taking a quick glance at the scripts folder I don't see anything that stands out to me as something that would work if the session is non-interactive.

Who would be a good person to query on this?

This comment has been minimized.

@kernelsmith

kernelsmith Nov 6, 2014

Contributor

Yeah, that's why I put the @todo in there. The old code didn't check for interactive, so I made this version act the same way, but like you, thought it was wonky

On Nov 6, 2014, at 07:27, Tom Sellers notifications@github.com wrote:

In lib/msf/ui/console/command_dispatcher/core.rb:

  •    if sid
    
  •      print_status("Running script #{script} on session #{sid}...")
    
  •      sessions = [ sid ]
    
  •    else
    
  •      print_status("Running script #{script} on all sessions...")
    
  •      sessions = framework.sessions.keys.sort
    
  •  sessions.each do |sess_id|
    
  •    session = verify_session(sess_id, true)
    
  •    # @TODO: Not interactive sessions can or cannot have scripts run on them?
    
  •    if session == false # specifically looking for false
    
  •      # if verify_session returned false, sess_id is valid, but not interactive
    
  •      session = framework.sessions.get(sess_id)
    
    Was the intent here to just provide a warning on a non-interactive session because this code checks for interactive and, if false, binds anyway. After taking a quick glance at the scripts folder I don't see anything that stands out to me as something that would work if the session is non-interactive.

Who would be a good person to query on this?


Reply to this email directly or view it on GitHub.

@kernelsmith

kernelsmith Nov 6, 2014

Contributor

Yeah, that's why I put the @todo in there. The old code didn't check for interactive, so I made this version act the same way, but like you, thought it was wonky

On Nov 6, 2014, at 07:27, Tom Sellers notifications@github.com wrote:

In lib/msf/ui/console/command_dispatcher/core.rb:

  •    if sid
    
  •      print_status("Running script #{script} on session #{sid}...")
    
  •      sessions = [ sid ]
    
  •    else
    
  •      print_status("Running script #{script} on all sessions...")
    
  •      sessions = framework.sessions.keys.sort
    
  •  sessions.each do |sess_id|
    
  •    session = verify_session(sess_id, true)
    
  •    # @TODO: Not interactive sessions can or cannot have scripts run on them?
    
  •    if session == false # specifically looking for false
    
  •      # if verify_session returned false, sess_id is valid, but not interactive
    
  •      session = framework.sessions.get(sess_id)
    
    Was the intent here to just provide a warning on a non-interactive session because this code checks for interactive and, if false, binds anyway. After taking a quick glance at the scripts folder I don't see anything that stands out to me as something that would work if the session is non-interactive.

Who would be a good person to query on this?


Reply to this email directly or view it on GitHub.

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Nov 6, 2014

Contributor

I've made my final commits and finished testing. Other than a review by @kernelsmith of the two items I linked above I think this is read for final review / testing.

Examples of some of the testing commands:
sessions
sessions -h

sessions -c
sessions -c hostname
sessions -c hostname -i toast
-- Throwing strings into -i in a couple places broke the code in interesting ways
sessions -c hostname -i 1,3-4

sessions -s
sessions -s checkvm
sessions -s checkvm -i toast
sessions -s checkvm -i 1,3-4

sessions -d
sessions -d toast
sessions -d 1,3-4

sessions -k
sessions -k toast
sessions -k 1,3-4

jobs -k
jobs -k toast
jobs -k 6,8-10

Contributor

TomSellers commented Nov 6, 2014

I've made my final commits and finished testing. Other than a review by @kernelsmith of the two items I linked above I think this is read for final review / testing.

Examples of some of the testing commands:
sessions
sessions -h

sessions -c
sessions -c hostname
sessions -c hostname -i toast
-- Throwing strings into -i in a couple places broke the code in interesting ways
sessions -c hostname -i 1,3-4

sessions -s
sessions -s checkvm
sessions -s checkvm -i toast
sessions -s checkvm -i 1,3-4

sessions -d
sessions -d toast
sessions -d 1,3-4

sessions -k
sessions -k toast
sessions -k 1,3-4

jobs -k
jobs -k toast
jobs -k 6,8-10

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 6, 2014

Contributor

Ideally, we'd change the parsing and usage so it would be

sessions [options] [sessionid-range]

Then given certain options that require a sid, we could immediately pop off the last arg and validate the sessions

On Nov 6, 2014, at 07:30, Tom Sellers notifications@github.com wrote:

I've made my final commits and finished testing. Other than a review by @kernelsmith of the two items I linked above I think this is read for final review / testing.

Examples of some of the testing commands:
sessions
sessions -h

sessions -c
sessions -c hostname
sessions -c hostname -i toast
-- Throwing strings into -i in a couple places broke the code in interesting ways
sessions -c hostname -i 1,3-4

sessions -s
sessions -s checkvm
sessions -s checkvm -i toast
sessions -s checkvm -i 1,3-4

sessions -d
sessions -d toast
sessions -d 1,3-4

sessions -k
sessions -k toast
sessions -k 1,3-4

jobs -k
jobs -k toast
jobs -k 6,8-10


Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Nov 6, 2014

Ideally, we'd change the parsing and usage so it would be

sessions [options] [sessionid-range]

Then given certain options that require a sid, we could immediately pop off the last arg and validate the sessions

On Nov 6, 2014, at 07:30, Tom Sellers notifications@github.com wrote:

I've made my final commits and finished testing. Other than a review by @kernelsmith of the two items I linked above I think this is read for final review / testing.

Examples of some of the testing commands:
sessions
sessions -h

sessions -c
sessions -c hostname
sessions -c hostname -i toast
-- Throwing strings into -i in a couple places broke the code in interesting ways
sessions -c hostname -i 1,3-4

sessions -s
sessions -s checkvm
sessions -s checkvm -i toast
sessions -s checkvm -i 1,3-4

sessions -d
sessions -d toast
sessions -d 1,3-4

sessions -k
sessions -k toast
sessions -k 1,3-4

jobs -k
jobs -k toast
jobs -k 6,8-10


Reply to this email directly or view it on GitHub.

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 6, 2014

Contributor

Right now -i is handled separately, hence the -i toast blowup, what we should do, is the parsing I just mentioned, then if -i was given, check that session_list.length == 1, if not error saying you must specify 1 valid sid. Or we could check for 0 as well and give more detailed errors etc

On Nov 6, 2014, at 07:30, Tom Sellers notifications@github.com wrote:

I've made my final commits and finished testing. Other than a review by @kernelsmith of the two items I linked above I think this is read for final review / testing.

Examples of some of the testing commands:
sessions
sessions -h

sessions -c
sessions -c hostname
sessions -c hostname -i toast
-- Throwing strings into -i in a couple places broke the code in interesting ways
sessions -c hostname -i 1,3-4

sessions -s
sessions -s checkvm
sessions -s checkvm -i toast
sessions -s checkvm -i 1,3-4

sessions -d
sessions -d toast
sessions -d 1,3-4

sessions -k
sessions -k toast
sessions -k 1,3-4

jobs -k
jobs -k toast
jobs -k 6,8-10


Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Nov 6, 2014

Right now -i is handled separately, hence the -i toast blowup, what we should do, is the parsing I just mentioned, then if -i was given, check that session_list.length == 1, if not error saying you must specify 1 valid sid. Or we could check for 0 as well and give more detailed errors etc

On Nov 6, 2014, at 07:30, Tom Sellers notifications@github.com wrote:

I've made my final commits and finished testing. Other than a review by @kernelsmith of the two items I linked above I think this is read for final review / testing.

Examples of some of the testing commands:
sessions
sessions -h

sessions -c
sessions -c hostname
sessions -c hostname -i toast
-- Throwing strings into -i in a couple places broke the code in interesting ways
sessions -c hostname -i 1,3-4

sessions -s
sessions -s checkvm
sessions -s checkvm -i toast
sessions -s checkvm -i 1,3-4

sessions -d
sessions -d toast
sessions -d 1,3-4

sessions -k
sessions -k toast
sessions -k 1,3-4

jobs -k
jobs -k toast
jobs -k 6,8-10


Reply to this email directly or view it on GitHub.

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 6, 2014

Contributor

The more I think about it the more I like this. Want me to PR it to u? I can also drop the print_good if u haven't already

Contributor

kernelsmith commented Nov 6, 2014

The more I think about it the more I like this. Want me to PR it to u? I can also drop the print_good if u haven't already

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Nov 6, 2014

Contributor

@kernelsmith Currently there is enough error checking to handle 'toast' but I am down with cleaning up the handling of ranges with sessions -c or -s .

win = true if CLEAN and CONSISTENT

Contributor

TomSellers commented Nov 6, 2014

@kernelsmith Currently there is enough error checking to handle 'toast' but I am down with cleaning up the handling of ranges with sessions -c or -s .

win = true if CLEAN and CONSISTENT

@TomSellers

This comment has been minimized.

Show comment
Hide comment
@TomSellers

TomSellers Nov 6, 2014

Contributor

@kernelsmith That being said, I've been considering adding the ability to perform filters on the session list for example:
sessions -u os=linux
sessions -k type=shell
sessions -c 'uname -a' -i os=linux

I don't know the general interest or utility for that though. If you think it would be useful to do this lets account for this in the thought process, even if it is in a future PR.

Contributor

TomSellers commented Nov 6, 2014

@kernelsmith That being said, I've been considering adding the ability to perform filters on the session list for example:
sessions -u os=linux
sessions -k type=shell
sessions -c 'uname -a' -i os=linux

I don't know the general interest or utility for that though. If you think it would be useful to do this lets account for this in the thought process, even if it is in a future PR.

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 6, 2014

Contributor

That's a cool idea. The nice thing is that since we have everything running through the build array and verify session methods, we won't have to change lot's of code to accommodate. We might be able to pass a block or other filtering mechanism to those methods as necessary to identify and/or verify sessions that meet the filter

-Josh

On Nov 6, 2014, at 08:43, Tom Sellers notifications@github.com wrote:

@kernelsmith That being said, I've been considering adding the ability to perform filters on the session list for example:

sessions -u os=linux
sessions -k type=shell
sessions -c 'uname -a' -i os=linux

I don't know the general interest or utility for that though. If you think it would be useful to do this lets account for this in the though process, even if it is in a future PR.


Reply to this email directly or view it on GitHub.

Contributor

kernelsmith commented Nov 6, 2014

That's a cool idea. The nice thing is that since we have everything running through the build array and verify session methods, we won't have to change lot's of code to accommodate. We might be able to pass a block or other filtering mechanism to those methods as necessary to identify and/or verify sessions that meet the filter

-Josh

On Nov 6, 2014, at 08:43, Tom Sellers notifications@github.com wrote:

@kernelsmith That being said, I've been considering adding the ability to perform filters on the session list for example:

sessions -u os=linux
sessions -k type=shell
sessions -c 'uname -a' -i os=linux

I don't know the general interest or utility for that though. If you think it would be useful to do this lets account for this in the though process, even if it is in a future PR.


Reply to this email directly or view it on GitHub.

@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 7, 2014

Contributor

@TomSellers I'll work on this some more tonight

Contributor

kernelsmith commented Nov 7, 2014

@TomSellers I'll work on this some more tonight

@kernelsmith kernelsmith merged commit 9295d90 into rapid7:master Nov 10, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

kernelsmith added a commit that referenced this pull request Nov 10, 2014

Land #4063 allow session lists
Note: the parsing for cmd_sessions  needs to be revamped and DRYd up in
a separate PR.
@kernelsmith

This comment has been minimized.

Show comment
Hide comment
@kernelsmith

kernelsmith Nov 10, 2014

Contributor

the cmd_sessions method needs improved option parsing, but it wasn't worth holding up this PR for that. Esp since core.rb in general needs a steam bath.

Contributor

kernelsmith commented Nov 10, 2014

the cmd_sessions method needs improved option parsing, but it wasn't worth holding up this PR for that. Esp since core.rb in general needs a steam bath.

@wvu-r7

This comment has been minimized.

Show comment
Hide comment
@wvu-r7

wvu-r7 Nov 10, 2014

Contributor

Bam, it's done. Nice work, gentlemen.

Contributor

wvu-r7 commented Nov 10, 2014

Bam, it's done. Nice work, gentlemen.

@TomSellers TomSellers deleted the TomSellers:kill_list/command_dispatcher branch Nov 12, 2014

wvu-r7 added a commit that referenced this pull request Jun 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment