Add support for killing sessions based on last checkin time #18364

zgoldman-r7 · 2023-09-13T18:18:43Z

Implements existing --search (or -S) flag to address #5664
closes #5664
Uses:
list sessions that have checked in within 10 seconds:
sessions --search "last_checkin:less_than:10s"

list stale sessions that havent within 10 seconds:
sessions --search "last_checkin:greater_than:10s"

kill all stale sessions older than 20 seconds:
sessions --search "last_checkin:greater_than:10s" -K
sessions -K --search "last_checkin:greater_than:10s"

search for a session with specific ids
sessions --search "session_id:1 session_id:2"

search for sessions of a particular type
sessions --search "session_type:meterpreter"

search for sessions with multiple query parameters
sessions --search "session_id:1 session_type:meterpreter last_checkin:after:10s"

kill sessions that match a range of query parameters
sessions --search "session_id:1 session_type:meterpreter last_checkin:after:10s" -K

run a command on sessions that match a range of query parameters
sessions -c 'whoami' --search "session_id:1 session_type:meterpreter last_checkin:after:10s"

Verification

List the steps needed to make sure this thing works

Start msfconsole
launch a module
start any number of active sessions
list active sessions with sessions -v to see what metadata exists
try filtering the sessions in accordance with the types of search terms listed above and make sure the results are expected

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

adfoster-r7 · 2023-09-18T13:25:25Z

I don't think this will be a scalable approach adding a one off flag for check-in functionality. It's not immediately clear how a user will filter/search for sessions that are checked in before or after a point in time - i.e. only run this command on sessions checked in less than 1 hour ago, or kill sessions greater than 1 day old

Maybe we need to consider adding additional keyword search terms to the sessions command, similar to the prior art of the search command:

search cve:2009 type:exploit

Taking inspiration from Docker's filter options - it has similar support for filtering:

images

before ([:], or image@digest) - filter images created before given id or references
since ([:], or image@digest) - filter images created since
given id or references

prune

until () - only remove images created before given timestamp
The until filter can be Unix timestamps, date formatted timestamps, or Go duration strings (e.g. 10m, 1h30m) computed relative to the daemon machine’s time. Supported formats for date formatted time stamps include RFC3339Nano, RFC3339, 2006-01-02T15:04:05, 2006-01-02T15:04:05.999999999,

A similar DSL in msfconsole might be:

sessions --kill --search last_checkin:after:12h
sessions -c 'whoami' --search last_checkin:before:2h

It would be out of scope, but we should do some pen and paper exercises first to ensure that the semantics would work for the search table:

search disclosure_date:after:2020-01-01 type:exploit

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

dwelch-r7 · 2023-10-05T11:44:39Z

lib/msf/base/serializer/readable_text.rb

+
+      framework.sessions.each { |k|
+        next unless session_ids.nil? || session_ids.include?(k[0])
+        session = k[1]
        row = create_msf_session_row(session, show_extended)
        tbl << row
      }


Suggested change

framework.sessions.each { |k|

next unless session_ids.nil? || session_ids.include?(k[0])

session = k[1]

row = create_msf_session_row(session, show_extended)

tbl << row

}

framework.sessions.each do |id , session|

next unless session_ids.nil? || session_ids.include?(id)

row = create_msf_session_row(session, show_extended)

tbl << row

}

I know you're just using the existing code here but we can tidy this up a little since we're making changes

@adfoster-r7 this conflicts with going back to each_sorted as that only iterates on the ids - thoughts on where to go?

We didn't realise that each_sorted only gave session ids rather than the sessions themselves
Taking a look at the each function though shows that it is sorted anyway

metasploit-framework/lib/msf/core/session_manager.rb

Lines 191 to 197 in 2f0509f

def each(&block)

list = []

self.keys.sort.each do |sidx|

list << [sidx, self[sidx]]

end

list.each(&block)

end

so IMO I think we're good to stick with each since you need the session objects anyway and it keeps the same sorting

dwelch-r7 · 2023-10-05T11:48:45Z

lib/msf/base/serializer/readable_text.rb

-        session = framework.sessions[k]
+
+      framework.sessions.each { |k|
+        next unless session_ids.nil? || session_ids.include?(k[0])


If sessions_ids is nil then it will be nil for every iteration of the loop, no need to check it over and over

you can also probably be a little smarter about filtering the sessions you want to include here, instead of looping over all the frameworks sessions and checking the id it might make more sense to loop over your list of sessions_ids and then grab the session with framework.sessions.get(id)

dwelch-r7 · 2023-10-05T11:50:43Z

lib/msf/base/serializer/readable_text.rb

@@ -987,6 +990,11 @@ def self.dump_sessions_verbose(framework, opts={})
    framework.sessions.each_sorted do |k|
      session = framework.sessions[k]

+      if opts[:session_ids]


similar to my previous comment, you should only need to check if opts[:session_ids] has a value once rather than every loop

dwelch-r7 · 2023-10-05T11:58:50Z

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

@@ -1425,6 +1431,7 @@ def cmd_sessions(*args)
    begin
    method   = nil
    quiet    = false
+    search   = false


Suggested change

search = false

I don't think this is being used

dwelch-r7 · 2023-10-05T12:00:32Z

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

+      if search_term
+        matching_sessions = get_matching_sessions(search_term)
+        if matching_sessions.empty?
+          print_error("No matching sessions.") 


Suggested change

print_error("No matching sessions.")

print_status("No matching sessions.")

Not having sessions match your search term isn't an error, this also aligns with a later print_status for not matching any sessions

dwelch-r7 · 2023-10-05T12:34:37Z

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

+    matching_sessions = {}
+
+    framework.sessions.each do |session_id, session|
+      case search_term.split(":")[0]


Let's make sure to check if the search term is valid and feed that back to the user

dwelch-r7 · 2023-10-05T12:37:21Z

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

+      case search_term.split(":")[0]
+      when "last_checkin"
+        if session.respond_to?(:last_checkin) && session.last_checkin && evaluate_search_criteria(session, search_term)
+          matching_sessions.store(session_id, session)


Suggested change

matching_sessions.store(session_id, session)

matching_sessions[session_id] = session

This might just be a personal preference thing but I think this reads better and is much more common in the code base

dwelch-r7 · 2023-10-05T12:38:27Z

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

+      when "session_type"
+        matching_sessions.store(session_id, session) if evaluate_search_criteria(session, search_term)
+      when "session_id"
+        matching_sessions.store(session_id, session) if evaluate_search_criteria(session, search_term)
+      end


all these cases in the switch statement don't seem to do anything different?

dwelch-r7 · 2023-10-05T12:40:29Z

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

+      matches.each do |session_id, session|
+        matching_sessions.store(session_id, session)
+      end


https://apidock.com/ruby/Hash/merge you can use merge or merge! instead of looping over your hashes

dwelch-r7 · 2023-10-05T12:43:24Z

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

+      matches = filter_sessions_by_search(term)
+      matches.each do |session_id, session|


am I correct in thinking that this results in your filter being an or filter rather than and? so if you search for before a certain checkin time and for session to be a meterpreter type rather than getting only meterpreter sessions checking in before a certain time you get all sessions that have checked in and all meterpreter sessions regardless of checkin time?

I would have expected the search to be an and and I think that's what users would expect too, but let me know if you disagree

I was thinking an or filter for the case of someone searching for multiple different ids or session types, and having it be consistent from there. Seems like a few options -

keep it as an or, which might lead to some users on the search results if they try to query on multiple fields as you pointed out

change it to an and, effectively disabling the ability to search for multiple different types of the same field

add the logic to make the or/and context sensitive, which might be more convoluted/inconsistent, but has the most functionality
Happy to defer to you here, lmk what you think

Let's try and base our implementation off of how docker does it https://docs.docker.com/engine/reference/commandline/images/#filter since thye seem to have a fairly good UX for this kinda thing already
sessions IDs would be an or then anything else would be an and
I really think the ability to provide a before time and an after time to select sessions from a certain range could be a pretty nice feature to have and I think it's more intuitive that way
Probably best to write up the test cases first for what we would expect to happen and maybe get a few more opinions on that when people can see what the workflow will be more easily

adfoster-r7 · 2023-10-05T13:08:09Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+        Killing matching sessions...
+        Active sessions
+        ===============
+
+          Id  Name   Type         Information  Connection
+          --  ----   ----         -----------  ----------
+          2   sesh2  meterpreter  info         tunnel (127.0.0.1)
+          3   sesh3  java         info         tunnel (127.0.0.1)
+        TABLE


Let's indent this for readability, similar to

metasploit-framework/spec/lib/msf/base/serializer/readable_text_spec.rb

Lines 111 to 117 in 1ee7f03

expect(described_class.dump_datastore('Table name', Msf::DataStore.new, indent_length)).to match_table <<~TABLE

Table name

==========

No entries in data store.

TABLE

end

for context heredoc can be aligned/extra indented: https://www.rubyguides.com/2018/11/ruby-heredoc/

adfoster-r7 · 2023-10-05T13:11:32Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+
+      let(:sessions) do 
+        {
+          1 => double('Session', last_checkin: Time.now, type: 'meterpreter', sid:1, sname: "sesh1", info: "info", session_host: "127.0.0.1", tunnel_to_s: "tunnel"),


Not tested: I believe you should be able to use instance_double(::Msf::Session, ...) here. instance_double is nicer as it will verify your mocked calls against a real class instance - so if you mock values that are deleted from the real object your tests will start failing. Whilst a mock would continue to work.

This should work the methods that are called aren't defined with metaprogramming/method_missing logic

In RSpec 3.0 and higher, instance_double is an implementation of verifying doubles. Verifying doubles are defined in RSpec as "a stricter alternative to normal doubles that provide guarantees about what is being verified. When using verifying doubles, RSpec will check that the methods being stubbed are actually present on the underlying object, if it is available."
https://www.simplybusiness.co.uk/about-us/tech/2020/05/rspec-instance-doubles/

adfoster-r7 · 2023-10-05T13:14:01Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+        {
+          1 => double('Session', last_checkin: Time.now, type: 'meterpreter', sid:1, sname: "sesh1", info: "info", session_host: "127.0.0.1", tunnel_to_s: "tunnel"),
+          2 => double('Session', last_checkin: (Time.now - 90), type: 'meterpreter', sid:2, sname: "sesh2", info: "info", session_host: "127.0.0.1", tunnel_to_s: "tunnel"),
+          3 => double('Session', last_checkin: Time.now, type: 'java', sid:3, sname: "sesh3", info: "info", session_host: "127.0.0.1", tunnel_to_s: "tunnel")


For anything timing related, it's probably best to mock time with TimeCop to ensure that your tests are deterministic on every test run. It also provides a nice API for time travel

metasploit-framework/spec/lib/msf/ui/console/command_dispatcher/db/klist_spec.rb

Lines 195 to 202 in 1ee7f03

before(:each) do

Timecop.freeze(Time.parse('Dec 18, 2022 12:33:40.000000000 GMT'))

create_tickets

end

after do

Timecop.return

end

adfoster-r7 · 2023-10-05T13:15:54Z

lib/msf/base/serializer/readable_text.rb

@@ -834,6 +834,7 @@ def self.dump_datastore(name, ds, indent = DefaultIndent, col = DefaultColumnWra
  def self.dump_sessions(framework, opts={})
    output = ""
    verbose = opts[:verbose] || false
+    session_ids = opts[:session_ids] || nil


I'd be worried about race conditions in IDs changing over time or being deleted by threads due to race conditions in code etc.

What are your thoughts on passing in the sessions object here in the opts that you would like to have serialized? Of course if the user doesn't provide the sessions, it'll grab them from framework.sessions. I think this would be a nicer API - and we wouldn't need this logic that's been added below either 🤔

framework.sessions.each { |k| next unless session_ids.nil? || session_ids.include?(k[0])

I think that makes sense - I'll change it up!

adfoster-r7 · 2023-10-05T13:22:23Z

lib/msf/base/serializer/readable_text.rb

@@ -987,6 +990,11 @@ def self.dump_sessions_verbose(framework, opts={})
    framework.sessions.each_sorted do |k|


Similar to above, maybe this https://github.com/rapid7/metasploit-framework/pull/18364/files#r1347404847

adfoster-r7 · 2023-10-05T13:22:46Z

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

@@ -115,6 +114,13 @@ class Core
    ["-g", "--global"] => [ false, "Operate on global datastore variables"]
  )

+  VALID_PARAMS =


Suggested change

VALID_PARAMS =

VALID_SESSION_SEARCH_PARAMS =

And can we use private_constant :VALID_SESSION_SEARCH_PARAMS too to ensure this constant isn't available externally, as currently this constant would now be available as part of this classes public API 👍

https://aaronlasseigne.com/2016/10/26/know-ruby-private_constant/

adfoster-r7 · 2023-10-05T13:26:15Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+    context "with no sessions" do
+      it "should show an empty table" do
+        core.cmd_sessions
+        expect(@output).to eq([


It'd be great to be consistent with the use of match_table for these newly added tests 👍

Looked into this a bit, this case doesn't return a table so using match table results in failure

match_table is a misnomer, it's mostly just a match_multiline_string matcher

Interesting. There does seem to be some sort of formatting mismatch with the expecation though.

1) Msf::Ui::Console::CommandDispatcher::Core#cmd_sessions with no sessions should show an empty table Failure/Error: expect(@output).to match_table <<~TABLE Active sessions =============== No active sessions. TABLE Expected: 'Active sessions' '===============' '' 'No active sessions.' Received: '["Active sessions", "===============", "", "No active sessions."]' Raw Result: ["Active sessions", "===============", "", "No active sessions."] Diff: @@ -1,5 +1,2 @@ -Active sessions -=============== - -No active sessions. +["Active sessions", "===============", "", "No active sessions."]

I'll dive deeper and see what I can uncover

It looks like you're expecting the string to equal an array:

Expected: 'Active sessions' '===============' '' 'No active sessions.' Received: '["Active sessions", "===============", "", "No active sessions."]' <----

i.e. you're missing the @output.join("\n") that's being used in the other test examples

Yep, thank ya!

adfoster-r7 · 2023-10-05T13:30:18Z

lib/msf/base/serializer/readable_text.rb

-      framework.sessions.each_sorted { |k|
-        session = framework.sessions[k]
+
+      framework.sessions.each { |k|


I imagine we'll still want each_sorted here, not sure if it was dropped intentionally or not 👀

It didn't seem necessary and was causing issues with testing, but I'll fix it 💪

dwelch-r7 · 2023-10-12T15:17:30Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+    before(:each) do
+      allow(driver).to receive(:active_session=)
+      allow(framework).to receive(:sessions).and_return(sessions)
+      Timecop.freeze(Time.parse('Dec 18, 2022 12:33:40.000000000 GMT'))


we don''t need to set up timecop before each test, we can do it once for this section of tests and also make sure to cleanup and tell timecop to return things to normal
https://github.com/travisjeffery/timecop#:~:text=You%20can%20mock%20the%20time%20for%20a%20set%20of%20tests%20easily%20via%20setup/teardown%20methods

dwelch-r7 · 2023-10-12T15:18:51Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+      end
+    end
+
+    context "searches with sessions with different ids" do


this context is good english but that might just be me

...isn't? 😅

🤦 yup isn't, maybe I shouldn't comment on english if I can't do it myself 🙃

dwelch-r7 · 2023-10-12T15:46:31Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+    context "searches with sessions with different checkin values" do
+      let(:sessions) do 
+        {
+          1 => instance_double(::Msf::Sessions::Meterpreter_x64_Win, last_checkin: Time.parse('Dec 18, 2022 12:33:40.000000000 GMT'), type: 'meterpreter', sid:1, sname: "session1", info: "info", session_host: "127.0.0.1", tunnel_to_s: "tunnel"),


Since you're using Timecop you don't need to be parsing dates anymore

dwelch-r7 · 2023-10-12T15:54:13Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+    context "with sessions" do
+      let(:sessions) do 
+        {
+          1 => instance_double(::Msf::Sessions::Meterpreter_x64_Win, last_checkin: Time.now, type: 'meterpreter', sid:1, sname: "sesh1", info: "info", session_host: "127.0.0.1", tunnel_to_s: "tunnel")


Just thinking out loud here but would it make more sense/be easier to read to just have a common set of session you could use across most of your tests here? rather than making a bespoke set for each test case you have

I agree it would be easier to read, I put it this way since we don't want them to exist for the "no active sessions" context which exists at the same level as all the other contexts. But if we're fine separating them, or there's another rspec trick I'm not privy to, happy to change that up

Additionally, I suppose using the same sessions might be an issue when testing sessions that don't respond to last_checkin, if we want to see what a query returns when none of the sessions do

In my head I was thinking you could have two contexts, with and without sessions but it's entirely possible there's some technical issues you might or have already ran into that I'm not thinking of why you can't do that

dwelch-r7 · 2023-10-12T15:55:37Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+      end
+
+      it "returns matching session when searching for id" do
+        core.cmd_sessions("--search", "session_id:1")


Are there any tests for what happens when you combine searching for a session id and something else like type or checkin?

I'm still not seeing it 😅 I can see where you're testing type with checking but not with session id
it might be somewhere and the line numbers have changed since you replied and I just can't see though

dwelch-r7 · 2023-10-12T16:31:07Z

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

+      print_error("Cannot search for last_checkin with two #{operator1} arguments.")
+      return false
+    end
+    if (operator1 == "before" && operator2 == "after" && parse_duration(value2) < parse_duration(value1)) || (operator1 == "after" && operator2 == "before" && parse_duration(value1) < parse_duration(value2))


use single quotes '' over double quotes "" when not needed, I think rubocop should throw you a warning about that

dwelch-r7 · 2023-10-12T16:34:24Z

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

+    # Make sure user is properly querying checkin
+    unless checkin_searches.length() < 2
+      unless validate_checkin_searches(checkin_searches)
+        return
+      end
+    end


you should validate the checkin searches regardless of how many you have since if you only have a single checkin search you'll miss out on the rest of the validation you have (like that only before and after are valid)

that validation occurs later, but you're right it'd be good to consolidate it into that function to clean up the logic!

Circling back - checkin_searches.length() < 2 is itself a validation, so the logic does need to be tweaked but we need to keep that line somewhere

isn't that validation redundant since if you have 2 or more searches you'll either have an invalid operator or a duplicate? but if you think it's necessary then can't it be moved inside the validate_checkin_searches function?

dwelch-r7 · 2023-10-12T16:38:44Z

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

+    # Retrieve checkin search results. AND filter with a max length of 2
+    unless checkin_searches.empty?
+      checkin_matches = {}
+      checkin_matches = filter_sessions_by_search(checkin_searches.first)
+      if checkin_searches[1]
+        matches = filter_sessions_by_search(checkin_searches[1])
+        checkin_matches = checkin_matches.select{ |session_id, session| matches[session_id] == session }
+      end
+      searches << checkin_matches
+    end


checkin_matches = {} checkin_searches.each do | search_query | ... end

you should just be able to loop over the contents and achieve the same thing you're trying to do here

I'm a little confused what a loop would add here since checkin_searches can't be more than two elements and all we're trying to do is get the intersection of the two if there are two, can you elaborate?

uhhh yea I was thinking it could look a bit tidier and maybe more future proof but don't worry about it sure

dwelch-r7 · 2023-10-12T16:40:01Z

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

+      if matching_sessions
+        print_status("Killing matching sessions...")
+        print_line
+        print(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))


should this be a print_status?

Suggested change

print(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))

print_status(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))

Was just going with the existing functionality, but that's no reason not to give it a swap if we think it's better

I think this question became automagically unresolved; It shouldn't be print_status here as the table would be printed with a [*] prefix which isn't needed

dwelch-r7 · 2023-10-12T16:40:32Z

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

+      unless matching_sessions
+        return
+      end
+      if matching_sessions.empty?
+        print_error("No matching sessions.")
+        return
+      end


Suggested change

unless matching_sessions

return

end

if matching_sessions.empty?

print_error("No matching sessions.")

return

end

if matching_sessions.empty?

print_error("No matching sessions.")

return

end

assuming it can't return nil, but if it can you'll need to check that too

It returns nil if the user enters invalid input (basically anywhere we'd hit print_error() and the nil bubbles up as a flag to cut off the search, which is why the unless matching_sessions is there - should I go about this differently?

I think in that case you could change my suggestion here to
if matching_sessions.nil? || matching_sessions.empty?
I would also explicitly return nil if that's what you're expecting

adfoster-r7 · 2023-10-12T16:50:45Z

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

@@ -56,7 +55,7 @@ class Core
    ["-k", "--kill"]            => [ true,  "Terminate the specified thread ID.", "<id>"             ],
    ["-K", "--kill-all"]        => [ false, "Terminate all non-critical threads."                    ],
    ["-i", "--info"]            => [ true,  "Lists detailed information about a thread.", "<id>"     ],
-    ["-l", "--list"] => [ false, "List all background threads."                           ],
+    ["-l", "--list"]            => [ false, "List all background threads."                           ],
    ["-v", "--verbose"]         => [ false, "Print more detailed info.  Use with -i and -l"          ])


I can't comment on the line directly, but updating the --search options above:

["-S", "--search"] => [ true, "Row search filter.", "<filter>" ],

To include an example of the filter would be useful for users to discover this functionality 👍

The thought being - if the feature isn't documented for users, it doesn't necessarily exist

adding in session_type would be useful too, just to make sure all the options are visible to the user

adfoster-r7 · 2023-10-12T17:48:53Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+    expected_values = {
+          "1s" => 1,
+          "2s" => 2,
+          "1.5m" => 90,
+          "1.5d" => 129600,
+          "1d1h1m1s" => 90061,
+          "1.5d1.5h1.5m1.5s" => 135091,
+          "1.75m70s" => 175
+    }
+
+    it_behaves_like "parses time values correctly", expected_values


This isn't quite how you use it_behaves_like, which is more useful for testing the different behavior of instances/objects

Shared examples let you describe behaviour of classes or modules. When declared, a shared group’s content is stored. It is only realized in the context of another example group, which provides any context the shared group needs to run.
https://rspec.info/features/3-12/rspec-core/example-groups/shared-examples/

Paired with the above knowledge - and that it's generally best practice is to have 1 expectation for each it block - https://www.betterspecs.org/#single

This pattern might be cleaner:

Suggested change

expected_values = {

"1s" => 1,

"2s" => 2,

"1.5m" => 90,

"1.5d" => 129600,

"1d1h1m1s" => 90061,

"1.5d1.5h1.5m1.5s" => 135091,

"1.75m70s" => 175

}

it_behaves_like "parses time values correctly", expected_values

{

"1s" => 1,

"2s" => 2,

"1.5m" => 90,

"1.5d" => 129600,

"1d1h1m1s" => 90061,

"1.5d1.5h1.5m1.5s" => 135091,

"1.75m70s" => 175

}.each do |input, expected|

it "returns #{expected} seconds for the input #{input}" do

expect(core.parse_duration(input)).to eq(output)

end

end

As it allows you to run failed tests individually from the command line, whilst with your current pattern that's not possible

dwelch-r7 · 2023-10-13T11:36:06Z

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

 require 'optparse'
-
+require 'pry-byebug'


Suggested change

require 'pry-byebug'

This is what's causing the test failures

dwelch-r7 · 2023-10-13T11:38:50Z

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

+      unless matching_sessions
+        return
+      end
+      if matching_sessions.empty?
+        print_error("No matching sessions.")
+        return
+      end


I think in that case you could change my suggestion here to
if matching_sessions.nil? || matching_sessions.empty?
I would also explicitly return nil if that's what you're expecting

dwelch-r7 · 2023-10-13T11:45:36Z

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

+  def validate_checkin_searches(checkin_searches)
+    checkin_searches.each do |search_term|
+      unless search_term.split(":").length == 3
+        print_error("Please only specify last_checkin, before or after, and a time. Ex: last_checkin:before:1m3os")


Suggested change

print_error("Please only specify last_checkin, before or after, and a time. Ex: last_checkin:before:1m3os")

print_error("Please only specify last_checkin, before or after, and a time. Ex: last_checkin:before:1m30s")

dwelch-r7 · 2023-10-13T11:47:34Z

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

+        return false
+      end
+      operator = checkin_searches[0].split(":")[1]
+      unless operator == "before" || operator == "after"


still have these "before" and "after" strings, you should be able to check if VALID_OPERATORS includes your operator here

dwelch-r7 · 2023-10-13T11:49:00Z

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

+        return false
+      end
+    end    
+    return true


Suggested change

return true

true

pretty sure this is what rubocop would want

dwelch-r7 · 2023-10-13T11:53:06Z

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

+      end
+    when "session_id"
+      return session.sid.to_s == operator
+    when "session_type"


dropping this comment here but it applies in a few places
when you're using a string like 'session_type' here multiple times you should consider making it a constant

dwelch-r7 · 2023-10-13T11:54:49Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+      end
+    end
+
+    context "searches with sessions with different ids" do


🤦 yup isn't, maybe I shouldn't comment on english if I can't do it myself 🙃

dwelch-r7 · 2023-10-13T11:56:48Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+      end
+
+      it "returns matching session when searching for id" do
+        core.cmd_sessions("--search", "session_id:1")


I'm still not seeing it 😅 I can see where you're testing type with checking but not with session id
it might be somewhere and the line numbers have changed since you replied and I just can't see though

dwelch-r7 · 2023-10-13T11:58:26Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+    context "with sessions" do
+      let(:sessions) do 
+        {
+          1 => instance_double(::Msf::Sessions::Meterpreter_x64_Win, last_checkin: Time.now, type: 'meterpreter', sid:1, sname: "sesh1", info: "info", session_host: "127.0.0.1", tunnel_to_s: "tunnel")


In my head I was thinking you could have two contexts, with and without sessions but it's entirely possible there's some technical issues you might or have already ran into that I'm not thinking of why you can't do that

dwelch-r7 · 2023-10-13T12:06:03Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+
+  describe "#parse_duration" do
+    expected_values = {
+          "1s" => 1,


I think a warning is a good shout, I wouldn't expect it to combine them, I reckon it's more likely someone typo'd a second d instead on an h and would appreciate knowing they made a mistake

dwelch-r7 · 2023-10-13T12:39:11Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+      end
+    end
+
+    context "searches for checkin with sessions that do not respond to checkin" do


what do you think about adding a test to verify the session doesn't respond to checkin? that way if commandshell gets that feature or some future traveller updates this test unknowingly to a session that does respond to checkin it'll warn them

dwelch-r7 · 2023-10-16T13:20:06Z

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

+  SESSION_TYPE = "session_type"
+  SESSION_ID = "session_id"
+  LAST_CHECKIN = "last_checkin"


Suggested change

SESSION_TYPE = "session_type"

SESSION_ID = "session_id"

LAST_CHECKIN = "last_checkin"

SESSION_TYPE = 'session_type'

SESSION_ID = 'session_id'

LAST_CHECKIN = 'last_checkin'

should these also be private constants?

dwelch-r7 · 2023-10-16T13:37:10Z

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

+      end
+      time_value = search_term.split(":")[2]
+      time_unit_string = time_value.gsub(/[^a-zA-Z]/, '')
+      unless time_unit_string == time_unit_string.squeeze


squeeze only removes duplicate letters in a row right? so dds becomes ds but it doesn't work with say for example dmd it would remain dmd right?

Yes, but at that point it'll get caught by the parse_time function and print print_error("Unrecognized time format: #{value}")

dwelch-r7 · 2023-10-16T13:43:46Z

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

+
+  def parse_duration(duration)
+    total_time = 0
+    time_tokens = duration.scan(/(?:\d+\.?\d*|\.\d+)/).zip(duration.scan(/[a-zA-Z]+/))


What happens here if the user puts in capital letters? I don't see them being downcase'd anywhere and the switch statement only deals with the lower case characters

added a downcase to the switch 👍

adfoster-r7 · 2023-10-17T10:15:46Z

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

@@ -1794,6 +1843,177 @@ def cmd_sessions(*args)
    true
  end

+  def get_matching_sessions(search_term)


@zgoldman-r7 I think @dwelch-r7 has called it out a few times now separately - but could we run Rubocop on all new code that's being added? i.e. these chunks of code, and the code in your test. You can run it from the command line with bundle exec rubocop -A file_1.rb file_2.rb

The rest of the file hasn't been rubocop'ed, so you'll just need to make sure the new code is rubocop'ed

dwelch-r7 · 2023-10-17T10:23:03Z

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

@@ -1755,7 +1804,7 @@ def cmd_sessions(*args)
      end
    when 'list', 'list_inactive', nil
      print_line
-      print(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, search_term: search_term))
+      print_status(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))


Sorry dude I know I asked you to change this but it needs to go back, I thought it was a mistake but the tables shouldn't be print_status since it appends a little extra to the output

Suggested change

print_status(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))

print(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))

dwelch-r7 · 2023-10-17T10:23:22Z

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

+      if matching_sessions
+        print_status('Killing matching sessions...')
+        print_line
+        print_status(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))


Same here as below, just regular print for tables, my bad

Suggested change

print_status(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))

print(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))

adfoster-r7 · 2023-10-17T10:28:16Z

spec/lib/msf/ui/console/command_dispatcher/core_spec.rb

+          '1.5d1.5h1.5m1.5s' => 135091,
+          '1.75m70s' => 175
+    }.each do |input, expected|
+      it 'returns #{expected} seconds for the input #{input}' do


String interpolation only works with double quotes, and the indentation is a bit off above; I think Rubocop might've caught both of these

Original comment:
#18364 (comment)

dwelch-r7 · 2023-10-19T11:37:41Z

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

+      when GREATER_THAN
+        return checkin_time < threshold_time
+      when LESS_THAN
+        return checkin_time > threshold_time


Just thinking out loud here, what do we think about swapping the order of the comparisons here so the switch case lines up with the > and < operators and renaming checkin_time to last_checkin_time to make it clear which one is the user input and which is the sessions actual last checkin time

when GREATER_THAN return threshold_time > last_checkin_time when LESS_THAN return threshold_time < last_checkin_time

Ohh I like it

dwelch-r7 · 2023-10-19T11:38:06Z

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

+  VALID_SESSION_SEARCH_PARAMS =
+    %w[
+      last_checkin
+      session_id
+      session_type
+    ]
+  VALID_OPERATORS =
+    %w[
+      less_than
+      greater_than
+    ]


Let's use the constants defined above in here aswell

remove single flag pivot to search flag added support for search session type adds search session id support remove stale references reshuffle code fix time parsing, add command support fix search list, reduce duplicated code testing added killall with search lists table of killed sessions sessions are no longer represented by ids addresses feedback on code structure and search behavior some test reshuffling, switch raised errors to printed ones add checkin validation, rest of cmd_sessions tests add time parsing test refactoring test reformatting and adjusted error validation make error handling more explicit, add test context fixes sub quotes, make constant rubocopping switch before and after to greater than and less than mbetter incorporate constants update example

dwelch-r7 · 2023-10-19T15:41:12Z

Release Notes

Add support for filtering sessions based on last checkin time, session type and id

adfoster-r7 · 2023-10-19T19:23:35Z

Looks like the verification steps on this PR are wrong 👀

adfoster-r7 · 2023-10-19T19:27:54Z

Looks like the existing issue is still open, I'm not sure if that's intentional or not - or if there's more work required here? 👀

If it was meant to be closed, a trick that we use is following the convention of adding closes #ticket_number in your PR description and it will close the associated issues automatically 🎉

adfoster-r7 · 2023-10-19T19:33:01Z

I think we're missing an associated wiki entry for this functionality so folk can search how to kill stale sessions from the docs site 📚

adfoster-r7 · 2023-10-19T20:08:20Z

A small UX niggle I ran into when trying to use this functionality for the weekly wraup;

It looks like there's no user affordance given when the user provides an incorrect search term:

msf6 payload(osx/x64/meterpreter_reverse_tcp) > sessions -S 'id:1 id:2'
[-] Please provide valid search term. Given: id
                                      ^ Error generated, but it's not clear how the user can resolve the issue on their own

For MVP we could just add the list of available valid terms so that the user can self service:

msf6 payload(osx/x64/meterpreter_reverse_tcp) > sessions -S 'id:1 id:2'
[-] Please provide valid search term. Given: id. Supported keywords are: session_id, session_type, last_checkin
                                                  ^ List of available keywords provided to the user

Out of scope -

If the filter functionality ever became complex enough, we could follow the pattern of how the search command works - and output a pretty intuitive help menu:

msf6 payload(osx/x64/meterpreter_reverse_tcp) > search session_id:10
[-] Invalid argument(s)

Usage: search [<options>] [<keywords>:<value>]

Prepending a value with '-' will exclude any matching results.
If no options or keywords are provided, cached results are displayed.


OPTIONS:

    -h, --help                      Help banner
    -I, --ignore                    Ignore the command if the only match has the same name as the search
    -o, --output <filename>         Send output to a file in csv format
    -r, --sort-descending <column>  Reverse the order of search results to descending order
    -S, --filter <filter>           Regex pattern used to filter search results
    -s, --sort-ascending <column>   Sort search results by the specified column in ascending order
    -u, --use                       Use module if there is one result

Keywords:
  adapter          :  Modules with a matching adater reference name
  aka              :  Modules with a matching AKA (also-known-as) name
  author           :  Modules written by this author
  arch             :  Modules affecting this architecture
  bid              :  Modules with a matching Bugtraq ID
  cve              :  Modules with a matching CVE ID
  edb              :  Modules with a matching Exploit-DB ID
  check            :  Modules that support the 'check' method
  date             :  Modules with a matching disclosure date
  description      :  Modules with a matching description
  fullname         :  Modules with a matching full name
  mod_time         :  Modules with a matching modification date
  name             :  Modules with a matching descriptive name
  path             :  Modules with a matching path
  platform         :  Modules affecting this platform
  port             :  Modules with a matching port
  rank             :  Modules with a matching rank (Can be descriptive (ex: 'good') or numeric with comparison operators (ex: 'gte400'))
  ref              :  Modules with a matching ref
  reference        :  Modules with a matching reference
  stage            :  Modules with a matching stage reference name
  stager           :  Modules with a matching stager reference name
  target           :  Modules affecting this target
  type             :  Modules of a specific type (exploit, payload, auxiliary, encoder, evasion, post, or nop)

Supported search columns:
  rank             :  Sort modules by their exploitabilty rank
  date             :  Sort modules by their disclosure date. Alias for disclosure_date
  disclosure_date  :  Sort modules by their disclosure date
  name             :  Sort modules by their name
  type             :  Sort modules by their type
  check            :  Sort modules by whether or not they have a check method

Examples:
  search cve:2009 type:exploit
  search cve:2009 type:exploit platform:-linux
  search cve:2009 -s name
  search type:exploit -s type -r

adfoster-r7 reviewed Sep 14, 2023

View reviewed changes

lib/msf/ui/console/command_dispatcher/core.rb Outdated Show resolved Hide resolved

adfoster-r7 assigned dwelch-r7 Sep 15, 2023

zgoldman-r7 marked this pull request as draft September 26, 2023 17:22

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch 5 times, most recently from 8cd5fb5 to 6938597 Compare September 26, 2023 17:56

adfoster-r7 reviewed Sep 26, 2023

View reviewed changes

lib/msf/ui/console/command_dispatcher/core.rb Outdated Show resolved Hide resolved

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch from 4969912 to d7abe5d Compare September 28, 2023 17:58

adfoster-r7 changed the title ~~add -e flag for stale sessions~~ Add support for killing sessions based on last checkin time Sep 29, 2023

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch 7 times, most recently from ca6ebd7 to 9683743 Compare October 4, 2023 20:11

zgoldman-r7 marked this pull request as ready for review October 4, 2023 20:12

dwelch-r7 reviewed Oct 5, 2023

View reviewed changes

adfoster-r7 reviewed Oct 5, 2023

View reviewed changes

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch from 85c9cfb to 6642afd Compare October 10, 2023 21:50

dwelch-r7 reviewed Oct 12, 2023

View reviewed changes

adfoster-r7 reviewed Oct 12, 2023

View reviewed changes

dwelch-r7 reviewed Oct 13, 2023

View reviewed changes

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch from 224a457 to 742aa78 Compare October 13, 2023 15:24

dwelch-r7 reviewed Oct 16, 2023

View reviewed changes

adfoster-r7 reviewed Oct 17, 2023

View reviewed changes

dwelch-r7 reviewed Oct 17, 2023

View reviewed changes

adfoster-r7 reviewed Oct 17, 2023

View reviewed changes

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch from 432650a to 565e528 Compare October 17, 2023 14:58

dwelch-r7 reviewed Oct 19, 2023

View reviewed changes

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch 4 times, most recently from 21f18a1 to c134908 Compare October 19, 2023 14:40

zgoldman-r7 force-pushed the ms9056-list-stale-sessions branch from c134908 to b4b7352 Compare October 19, 2023 14:41

dwelch-r7 merged commit 7baabd0 into rapid7:master Oct 19, 2023
34 checks passed

bwatters-r7 added the rn-enhancement release notes enhancement label Oct 19, 2023

zgoldman-r7 mentioned this pull request Oct 19, 2023

Feature request: add ability to list "stale" sessions #5664

Closed

zgoldman-r7 mentioned this pull request Oct 19, 2023

Adds wiki guide for session searching #18470

Merged

3 tasks

	def each(&block)
	list = []
	self.keys.sort.each do \|sidx\|
	list << [sidx, self[sidx]]
	end
	list.each(&block)
	end

	print_error("No matching sessions.")
	print_status("No matching sessions.")

	matching_sessions.store(session_id, session)
	matching_sessions[session_id] = session

		matches = filter_sessions_by_search(term)
		matches.each do \|session_id, session\|

	expect(described_class.dump_datastore('Table name', Msf::DataStore.new, indent_length)).to match_table <<~TABLE
	Table name
	==========

	No entries in data store.
	TABLE
	end

	before(:each) do
	Timecop.freeze(Time.parse('Dec 18, 2022 12:33:40.000000000 GMT'))
	create_tickets
	end

	after do
	Timecop.return
	end

		@@ -987,6 +990,11 @@ def self.dump_sessions_verbose(framework, opts={})
		framework.sessions.each_sorted do \|k\|

	print(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))
	print_status(Serializer::ReadableText.dump_sessions(framework, show_active: show_active, show_inactive: show_inactive, show_extended: show_extended, verbose: verbose, sessions: matching_sessions))

	print_error("Please only specify last_checkin, before or after, and a time. Ex: last_checkin:before:1m3os")
	print_error("Please only specify last_checkin, before or after, and a time. Ex: last_checkin:before:1m30s")

Add support for killing sessions based on last checkin time #18364

Add support for killing sessions based on last checkin time #18364

Conversation

zgoldman-r7 commented Sep 13, 2023 • edited

Verification

adfoster-r7 commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwelch-r7 Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zgoldman-r7 commented Sep 13, 2023 •

edited

adfoster-r7 Oct 5, 2023 •

edited

adfoster-r7 Oct 5, 2023 •

edited

adfoster-r7 Oct 5, 2023 •

edited

dwelch-r7 Oct 13, 2023 •

edited