Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Add alert to show user the new session options available in Metasploit 6.4 #18761

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

dwelch-r7
Copy link
Contributor

With the introduction of the new SMB, Postgresql, etc. sessions we thought it would be a good idea to add in some messaging to the user when they use a module that supports ether the creation or use of one of these modules that supports these new session types

Draft PR for the moment so we can discuss some UX details
Open questions:

  • Do we like the wording?
  • Should we only display these messages if the corresponding feature is turned on?
  • Should we always print it and give instruction on how to enable the feature in the message?
  • The message currently prints everytime a user uses the module, personally I don't think that's too intrusive but I could look into what it takes to only print the first time a particular module is used

Example output:
image

@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Jan 30, 2024

Desired end goals:

msf6 auxiliary(scanner/mssql/mssql_hashdump) > use postgres_login

Matching Modules
================

   #  Name                                       Disclosure Date  Rank    Check  Description
   -  ----                                       ---------------  ----    -----  -----------
   0  auxiliary/scanner/postgres/postgres_login  .                normal  No     PostgreSQL Login Utility


Interact with a module by name or index. For example info 0, use 0 or use auxiliary/scanner/postgres/postgres_login

[*] Using auxiliary/scanner/postgres/postgres_login
[*] New in Metasploit 6.4 - The CreateSession option within this module can open an interactive session
msf6 auxiliary(scanner/postgres/postgres_login) > 

And we want to remove the blank line when using the module:

msf6 auxiliary(admin/kerberos/forge_ticket) > use scanner/postgres/postgres_login
[*] New in Metasploit 6.4 - The CreateSession option within this module can open an interactive session
msf6 auxiliary(scanner/postgres/postgres_login) > 

We also want the messages to only appear when we've got the feature flag enabled

@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Jan 30, 2024

It looks like this is printing an extra blank line when using the module:

msf6 auxiliary(admin/kerberos/forge_ticket) > use scanner/postgres/postgres_login

[*] New in Metasploit 6.4 - The CreateSession option within this module can open an interactive session
msf6 auxiliary(scanner/postgres/postgres_login) > 

Looks like the new line is caused by the prompting value, will need to look deeper why this isn't unset correctly

diff --git a/lib/msf/core/module/alert.rb b/lib/msf/core/module/alert.rb
index 1bc609e356..904000c638 100644
--- a/lib/msf/core/module/alert.rb
+++ b/lib/msf/core/module/alert.rb
@@ -243,25 +243,36 @@ module Msf::Module::Alert
   def alert_user
     self.you_have_been_warned ||= {}
 
-    errors.each do |msg|
-      if msg && !self.you_have_been_warned[msg.hash]
-        print_error(msg)
-        self.you_have_been_warned[msg.hash] = true
+    self.without_prompt do
+      errors.each do |msg|
+        if msg && !self.you_have_been_warned[msg.hash]
+          print_error(msg)
+          self.you_have_been_warned[msg.hash] = true
+        end
       end
-    end
 
-    warnings.each do |msg|
-      if msg && !self.you_have_been_warned[msg.hash]
-        print_warning(msg)
-        self.you_have_been_warned[msg.hash] = true
+      warnings.each do |msg|
+        if msg && !self.you_have_been_warned[msg.hash]
+          print_warning(msg)
+          self.you_have_been_warned[msg.hash] = true
+        end
       end
-    end
 
-    infos.each do |msg|
-      if msg && !self.you_have_been_warned[msg.hash]
-        print_line(msg)
-        self.you_have_been_warned[msg.hash] = true
+      infos.each do |msg|
+        if msg && !self.you_have_been_warned[msg.hash]
+          print_status("i am updated, hello")
+          print_status(msg)
+          self.you_have_been_warned[msg.hash] = true
+        end
       end
     end
   end
+
+  # Temporarily set the prompt mode to false to ensure that there are not additional lines printed
+  # A workaround for the prompting bug spotted in https://github.com/rapid7/metasploit-framework/pull/18761#issuecomment-1916645095
+  def without_prompt(&block)
+    previous_prompting_value = user_output.prompting
+    user_output.prompting(false)
+    yield
+  ensure
+    user_output.prompting(previous_prompting_value)
+  end
 end

Edit: Looks like the prompting logic was removed here: ecc853d

I'm not sure that's correct, I believe it's meant to be this:

    def pgets

      line = nil
      orig = Thread.current.priority

      begin
        Thread.current.priority = -20

        output.prompting
        line = readline_with_output(prompt, true)
        ::Readline::HISTORY.pop if (line and line.empty?)
      ensure
        Thread.current.priority = orig || 0
+       output.prompting(false)
      end

      line
    end

@dwelch-r7 dwelch-r7 marked this pull request as ready for review January 31, 2024 13:47
Copy link
Contributor

@smcintyre-r7 smcintyre-r7 left a comment

Choose a reason for hiding this comment

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

Would you mind also adding a callout for this new functionality in the tips array that shown on startup and by the tips command.

infos.each do |msg|
if msg && !self.you_have_been_warned[msg.hash]
# Make prefix an empty string to avoid adding clutter (timestamps, rhost, rport, etc.) to the output
print_status(msg, prefix: '')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that all of the calls to print_* shouldn't have a prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean for the warnings and errors too? ummm I'm not sure, I don't think we particularly want the prefixes for them but I wouldn't be 100% on that

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this might be leaving around tech debt/inconsistencies for the next person 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind making the change, can sort that now

end

print_status("Using #{used_module}") if used_module
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we talked about moving this above the cmd_use call - did we leave that out intentionally? 👀

Current:

msf6 auxiliary(scanner/mysql/mysql_login) > use smb_login

Matching Modules
================

   #  Name                             Disclosure Date  Rank    Check  Description
   -  ----                             ---------------  ----    -----  -----------
   0  auxiliary/scanner/smb/smb_login  .                normal  No     SMB Login Check Scanner


Interact with a module by name or index. For example info 0, use 0 or use auxiliary/scanner/smb/smb_login

[*] New in Metasploit 6.4 - The CreateSession option within this module can open an interactive session
[*] Using auxiliary/scanner/smb/smb_login

Expected:

msf6 auxiliary(scanner/mysql/mysql_login) > use smb_login

Matching Modules
================

   #  Name                             Disclosure Date  Rank    Check  Description
   -  ----                             ---------------  ----    -----  -----------
   0  auxiliary/scanner/smb/smb_login  .                normal  No     SMB Login Check Scanner


Interact with a module by name or index. For example info 0, use 0 or use auxiliary/scanner/smb/smb_login

[*] Using auxiliary/scanner/smb/smb_login
[*] New in Metasploit 6.4 - The CreateSession option within this module can open an interactive session

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope just forgot to move it

@adfoster-r7 adfoster-r7 merged commit 372b792 into rapid7:master Feb 2, 2024
55 of 57 checks passed
@adfoster-r7
Copy link
Contributor

adfoster-r7 commented Feb 2, 2024

Release Notes

Adds a user notification that new modules support a CreateSession option. This functionality is currently behind a feature flag which can be enables with the features command

@sjanusz-r7 sjanusz-r7 added the rn-enhancement release notes enhancement label Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn-enhancement release notes enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants