Extract reusable core session commands #18516

dwelch-r7 · 2023-11-06T15:49:59Z

Refactoring some commands out of the Meterpreter specific command dispatcher into a new generic Sessions command dispatcher that all sessions will be able to leverage.

The common commands I pulled out were: help background sessions resource irb pry exit
I didn't see any others that made a tonne of sense to move out but if there's any you'd like to see added feel free to dorp a comment calling it out

Also added a bunch of tests that should let us make sure any new session types have correctly implemented the api that the session command dispatcher is expecting

bcoles

code quality issues with existing code

bcoles · 2023-11-06T16:06:49Z

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

+              end
+                # let's check to see if it's in the scripts/resource dir (like when tab completed)
+              end


where's end's friend? he shares an indentation level with another end.

huh, most of this was copied over from the meterpreter command_dispatcher, I swear I rubocopped this but there's definitely still issues, I'll get it sorted cheers

bcoles · 2023-11-06T16:06:51Z

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

+              if good_res
+                client.console.load_resource(good_res)
+              else
+                print_error("#{res} is not a valid resource file")
+                next
+              end


Suggested change

if good_res

client.console.load_resource(good_res)

else

print_error("#{res} is not a valid resource file")

next

end

unless good_res

print_error("#{res} is not a valid resource file")

next

end

client.console.load_resource(good_res)

bcoles · 2023-11-06T16:08:30Z

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

+              return tab_complete_filenames(str, words)
+            elsif (!(words[1]) || !words[1].match(%r{^/}))


Suggested change

return tab_complete_filenames(str, words)

elsif (!(words[1]) || !words[1].match(%r{^/}))

return tab_complete_filenames(str, words)

end

if (!(words[1]) || !words[1].match(%r{^/}))

bcoles · 2023-11-06T16:08:42Z

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

+                    ::File.file?(path) and ::File.readable?(path)
+                  end
+                end
+              rescue Exception


spec/lib/msf/base/sessions/command_shell_spec.rb

adfoster-r7 · 2023-11-06T16:16:56Z

spec/lib/msf/base/sessions/command_shell_spec.rb

+
+COMMANDS = %i[help background sessions resource shell download upload source irb pry]
+
+RSpec.describe Msf::Sessions::CommandShell do


It'd be nice to clean these up to align with some of the conventions from https://www.betterspecs.org/

this should follow the conventions of https://www.betterspecs.org/#describe

i.e.

describe '.type' do # ... end describe '.can_cleanup_files' do # ... end # ... etc ...

And ensure context blocks follow the naming convention of context 'when ...' do

https://www.betterspecs.org/#contexts

adfoster-r7 · 2023-11-06T16:22:23Z

spec/lib/msf/base/sessions/command_shell_spec.rb

+          datastore
+        end
+        it 'should execute the script' do
+          is_expected.to receive(:execute_script).with(initial_auto_run_script)


Not a blocker; expect to receive is an antipattern as it doesn't follow the normal pattern of arrange/act/assert

https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/

i.e. prefer adding a expect(...).to have_received(...) after the 'act', instead of expect(...). to receive(...) before your 'act'

spec/lib/rex/post/meterpreter/ui/console/command_dispatcher/core_spec.rb

adfoster-r7 · 2023-11-06T19:32:05Z

lib/msf/base/sessions/command_shell.rb

+    session_id = args[0].to_i
+    if session_id <= 0
+      print_status 'Invalid session id'
+      cmd_sessions_help


Does this need a return?

jvoisin

It would be nice to normalise the return part in cmd_…_help: do we want a return false after it? do we want to return cmd_…_help? do we want a naked return instead?

jvoisin · 2023-11-07T00:28:43Z

lib/msf/base/sessions/command_shell.rb

@@ -30,7 +30,7 @@ class CommandShell
  include Rex::Ui::Text::Resource

  @@irb_opts = Rex::Parser::Arguments.new(
-    '-h' => [false, 'Help menu.'             ],
+    ['-h', '--help'] => [false, 'Help menu.'             ],


Suggested change

['-h', '--help'] => [false, 'Help menu.' ],

['-h', '--help'] => [false, 'Help menu.'],

jvoisin · 2023-11-07T00:30:23Z

lib/msf/base/sessions/command_shell.rb

-      # One arg, and args[1] => '-h' '-H' 'help'
-      return cmd_sessions_help
+    if args.length == 1 && (args[0] == '-h' || args[0] == '--help')
+      # One arg, and args[0] => '-h' '--help'


Suggested change

# One arg, and args[0] => '-h' '--help'

I don't think this comment is necessary.

jvoisin · 2023-11-07T00:30:56Z

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

+          include Rex::Ui::Text::DispatcherShell::CommandDispatcher
+
+          @@irb_opts = Rex::Parser::Arguments.new(
+            %w[-h --help] => [false, 'Help menu.' ],


Suggested change

%w[-h --help] => [false, 'Help menu.' ],

%w[-h --help] => [false, 'Help menu.'],

jvoisin · 2023-11-07T00:33:28Z

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

+          #
+          # Terminates the session.
+          #
+          def cmd_exit(*args)
+            print_status("Shutting down session: #{client.sid}")
+            client.exit
+          end
+
+          alias cmd_quit cmd_exit


A session can be terminated, exit'ed, shut down and quit'ted. It would be nice to reduce the amount of terms for this action I think.

I don't necessarily disagree but I don't think it belongs in this PR, this is just a bit of a refactor and moving things around, the intention is not to alter the way someone would interact with a session here

jvoisin · 2023-11-07T00:34:29Z

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

+            end
+
+            print_status('Starting Pry shell...')
+            print_status("You are in the \"client\" (session) object\n")


Suggested change

print_status("You are in the \"client\" (session) object\n")

print_status('You are in the "client" (session) object')

print_line

jvoisin · 2023-11-07T00:35:18Z

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

+          end
+
+          def cmd_sessions(*args)
+            if args.empty? || args[0].to_i == 0


Suggested change

if args.empty? || args[0].to_i == 0

if args.empty? || args[0].to_i == 0 || args.include?('-h') || args.include?('--help')

sjanusz-r7 · 2023-11-09T14:49:05Z

lib/msf/base/sessions/command_shell.rb

-    if args.length != 1
-      # More than one argument
+    session_id = args[0].to_i
+    if session_id <= 0


Could we check if this works the same as before when provided with negative session ids?

@dwelch-r7 Is there anything actionable here?

No, the previous implementation didn't accept negative session IDs so it's a nice to have but I don't think we should block the PR on it

adfoster-r7 · 2023-11-14T23:13:40Z

Some of the remaining comments are about existing code that's just been shuffled, will treat as not a blocker for now

Add test for command shell sessions

3f3531d

dwelch-r7 marked this pull request as draft November 6, 2023 16:04

bcoles reviewed Nov 6, 2023

View reviewed changes

dwelch-r7 force-pushed the core-session-commands-tests branch from 8303d8c to 06e066b Compare November 6, 2023 16:31

adfoster-r7 reviewed Nov 6, 2023

View reviewed changes

Add core session commands and test suite

c7e0e09

dwelch-r7 force-pushed the core-session-commands-tests branch from 06e066b to c7e0e09 Compare November 6, 2023 16:34

adfoster-r7 reviewed Nov 6, 2023

View reviewed changes

jvoisin reviewed Nov 7, 2023

View reviewed changes

dwelch-r7 added 3 commits November 7, 2023 14:10

Fix thread count

67a3432

log threads with no uuid

8205717

Fix tests leaving behind threads

ea41ec7

dwelch-r7 marked this pull request as ready for review November 8, 2023 11:34

sjanusz-r7 reviewed Nov 9, 2023

View reviewed changes

dwelch-r7 mentioned this pull request Nov 14, 2023

Add Smb session type #18539

Merged

10 tasks

adfoster-r7 merged commit e011fbe into rapid7:master Nov 15, 2023
32 checks passed

adfoster-r7 added the rn-no-release-notes no release notes label Nov 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract reusable core session commands #18516

Extract reusable core session commands #18516

dwelch-r7 commented Nov 6, 2023

bcoles left a comment

bcoles Nov 6, 2023

dwelch-r7 Nov 6, 2023

bcoles Nov 6, 2023

bcoles Nov 6, 2023

bcoles Nov 6, 2023

adfoster-r7 Nov 6, 2023

adfoster-r7 Nov 6, 2023

adfoster-r7 Nov 6, 2023

jvoisin left a comment

jvoisin Nov 7, 2023

jvoisin Nov 7, 2023

jvoisin Nov 7, 2023

jvoisin Nov 7, 2023

dwelch-r7 Nov 7, 2023

jvoisin Nov 7, 2023

jvoisin Nov 7, 2023

sjanusz-r7 Nov 9, 2023

adfoster-r7 Nov 13, 2023

dwelch-r7 Nov 14, 2023

adfoster-r7 commented Nov 14, 2023 •

edited

Loading

		return tab_complete_filenames(str, words)
		elsif (!(words[1]) \|\| !words[1].match(%r{^/}))


		COMMANDS = %i[help background sessions resource shell download upload source irb pry]

		RSpec.describe Msf::Sessions::CommandShell do

	['-h', '--help'] => [false, 'Help menu.' ],
	['-h', '--help'] => [false, 'Help menu.'],

	%w[-h --help] => [false, 'Help menu.' ],
	%w[-h --help] => [false, 'Help menu.'],

	print_status("You are in the \"client\" (session) object\n")
	print_status('You are in the "client" (session) object')
	print_line

	if args.empty? \|\| args[0].to_i == 0
	if args.empty? \|\| args[0].to_i == 0 \|\| args.include?('-h') \|\| args.include?('--help')

Extract reusable core session commands #18516

Extract reusable core session commands #18516

Conversation

dwelch-r7 commented Nov 6, 2023

bcoles left a 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

jvoisin left a 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 commented Nov 14, 2023 • edited Loading

adfoster-r7 commented Nov 14, 2023 •

edited

Loading