Extract SMB, PostgreSQL, MySQL and MSSQL optional sessions into their own mixins #18770

dwelch-r7 · 2024-01-30T17:27:43Z

Spotted an issue where having any of the new SMB/MySQL/PostgreSQL/MSSQL session type features enabled would end up enabling the new feature for each which was unintended, also fixes an issue where if the postgres session feature is enabled it starts registering postgres specific datastore options in the included other session type modules, this PR resolves both of these issues by splitting the Msf::OptionalSession out into individual mixins for each session type.

The Msf::OptionalSession mixin is left quite bare at the minute, but it will fill up in the near future with stuff common to all sessions (like the SESSION datastore option as an example) once the sessions are no longer behind individual feature flags

Verification steps

Continue the above steps for each new session type

zgoldman-r7

This all works for and looks good to me and - seems to be an issue in #18763 as well, I'll add a commit to that following this pattern

lib/msf/core/optional_session/smb.rb

adfoster-r7 · 2024-02-12T18:00:47Z

@dwelch-r7 I think this needs a rebase 👀

cgranleese-r7 · 2024-02-13T12:25:14Z

Tested the steps outlined in the verification steps above and repeated those for the MySQL scenarios as well and everything appears to be working as expected 👍

Assuming we wont want to land this until we have the MSSQL modules in and tested against this as well?

cgranleese-r7

Everything worked when tested but noticed a few things that may need updated due to lots of overlapping PRs and things needing updated since this was opened.

cgranleese-r7 · 2024-02-13T12:27:00Z

lib/msf/core/optional_session/mysql.rb

+            [
+              Msf::OptInt.new('SESSION', [ false, 'The session to run this module on' ]),
+              Msf::Opt::RHOST(nil, false),
+              Msf::Opt::RPORT(nil, false)


Context: I think some final changes were made after this PR was put up.

Suggested change

Msf::Opt::RPORT(nil, false)

Msf::Opt::RPORT(3306, false)

cgranleese-r7 · 2024-02-13T12:29:27Z

lib/msf/core/optional_session/mysql.rb

+        super(
+          update_info(
+            info,
+            'SessionTypes' => %w[MySQL]


Was this added to address this comment?
Also, I have a PR that updates these session types to be aligned with the rest of framework to use lowercase. Makes sense to align it here as well to avoid a race condition on which PR lands first:

Suggested change

'SessionTypes' => %w[MySQL]

'SessionTypes' => %w[mysql]

cgranleese-r7 · 2024-02-13T12:31:14Z

lib/msf/core/optional_session/postgresql.rb

+        super(
+          update_info(
+            info,
+            'SessionTypes' => %w[PostgreSQL]


Same as above

Suggested change

'SessionTypes' => %w[PostgreSQL]

'SessionTypes' => %w[postgresql]

cgranleese-r7 · 2024-02-13T12:31:24Z

lib/msf/core/optional_session/postgresql.rb

+              Msf::OptString.new('DATABASE', [ false, 'The database to authenticate against', 'postgres']),
+              Msf::OptString.new('USERNAME', [ false, 'The username to authenticate as', 'postgres']),
+              Msf::Opt::RHOST(nil, false),
+              Msf::Opt::RPORT(nil, false)


Same as MySQL port

Suggested change

Msf::Opt::RPORT(nil, false)

Msf::Opt::RPORT(5432, false)

lib/msf/core/optional_session/mssql.rb

cgranleese-r7 · 2024-02-13T12:36:18Z

lib/msf/core/optional_session/smb.rb

+        super(
+          update_info(
+            info,
+            'SessionTypes' => %w[SMB]


Suggested change

'SessionTypes' => %w[SMB]

'SessionTypes' => %w[smb]

cgranleese-r7 · 2024-02-13T12:38:19Z

lib/msf/core/optional_session/smb.rb

+            [
+              Msf::OptInt.new('SESSION', [ false, 'The session to run this module on' ]),
+              Msf::Opt::RHOST(nil, false),
+              Msf::Opt::RPORT(nil, false)


Just curious, the other session types needed the RPORT value defaulted, is there a reason as to why the SMB session type doesn't require a default?

yea not sure what happened, I added it elsewhere in the default options values part of the info hash but it's not there now and I'm not sure what happened but makes sense to add it in here

cgranleese-r7 · 2024-02-13T12:42:15Z

modules/auxiliary/scanner/postgres/postgres_schemadump.rb

@@ -18,7 +18,6 @@ def initialize
      ),
      'Author' => ['theLightCosine'],
      'License' => MSF_LICENSE,
-      'SessionTypes' => %w[PostgreSQL]


Looks like this was done here, but lots of other modules didn't have this removed. If we're going ahead with initialising the session type within the mixin, I'm assuming we want all of these removed?

Looks like it's mostly the MySQL and PostgreSQL modules were this was the case, so I assume you just forgot to remove them after updating the mxin 🤷

Example:
https://github.com/rapid7/metasploit-framework/pull/18770/files#diff-eeaec8f2c44f2baca8313b0d7849a46daf67c71d0d2ac8eef3a7a92692280468R23

Thinking more about this, do we really want the session type not being on the module? As a user I'd expect to be able to view that kinda of module info within the module.

Just thinking removing the existing module information could cause confusion, whereas the call in the module is just more information for the user to work with. Not super strongly opinionated on this just thought I'd call it out 😄

it still appears when you do info -d though right? I think that's how a majority of users would see it rather than looking at the module code itself

I suppose that's true. Maybe I'm looking at it too much from a developer perspective were I'd be working with the module and just get my information there. Whereas you say, users would probably get it from console.

Dean's current pattern looks good to me 👍

adfoster-r7 · 2024-02-14T11:52:19Z

This branch has conflicts that must be resolved

Needs a rebase, and probably some changes back-ported to the recently changed mssql modules 👍

adfoster-r7 · 2024-02-15T13:22:06Z

Release Notes

Fixes a bug when multiple new session types (SMB, PostgreSQL, MSSQL, MySQL) were enabled with the features set postgresql_session_type true command

zgoldman-r7 self-requested a review January 30, 2024 18:36

zgoldman-r7 approved these changes Jan 30, 2024

View reviewed changes

dwelch-r7 mentioned this pull request Jan 31, 2024

Add MySQL session type #18718

Merged

7 tasks

smcintyre-r7 reviewed Feb 1, 2024

View reviewed changes

lib/msf/core/optional_session/smb.rb Show resolved Hide resolved

dwelch-r7 force-pushed the individual-optional-session-mixins branch 3 times, most recently from 995322f to 60291ec Compare February 2, 2024 16:20

adfoster-r7 mentioned this pull request Feb 6, 2024

Updates MySQL modules to now support the new MySQL session type #18759

Merged

7 tasks

dwelch-r7 force-pushed the individual-optional-session-mixins branch from 60291ec to 448313e Compare February 11, 2024 22:55

dwelch-r7 mentioned this pull request Feb 11, 2024

Improve options display optional session types #18817

Merged

3 tasks

dwelch-r7 force-pushed the individual-optional-session-mixins branch from 4addfab to 5b10b92 Compare February 13, 2024 11:02

cgranleese-r7 self-assigned this Feb 13, 2024

dwelch-r7 force-pushed the individual-optional-session-mixins branch from 5b10b92 to 1e76bbc Compare February 13, 2024 11:09

cgranleese-r7 changed the title ~~Extract SMB and PostgreSQL optional sessions into their own mixins~~ Extract SMB, PostgreSQL, MySQL and MSSQL optional sessions into their own mixins Feb 13, 2024

cgranleese-r7 added the rn-fix release notes fix label Feb 13, 2024

cgranleese-r7 reviewed Feb 13, 2024

View reviewed changes

dwelch-r7 force-pushed the individual-optional-session-mixins branch from 1e76bbc to d258c52 Compare February 14, 2024 12:16

dwelch-r7 added 4 commits February 14, 2024 15:37

Extract SMB and PostgreSQL optional sessions into their own mixins

0f319bd

Add session type to info hash in the mixin

08872d0

Use individual session mixins

587a869

Use mssql option session mixin with mssql modules

0d4e1ed

dwelch-r7 force-pushed the individual-optional-session-mixins branch from 9b9a175 to d73293d Compare February 14, 2024 15:37

lowercase session types

fa5c4c0

dwelch-r7 force-pushed the individual-optional-session-mixins branch from d73293d to fa5c4c0 Compare February 14, 2024 15:45

adfoster-r7 merged commit e49c6a7 into rapid7:master Feb 15, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract SMB, PostgreSQL, MySQL and MSSQL optional sessions into their own mixins #18770

Extract SMB, PostgreSQL, MySQL and MSSQL optional sessions into their own mixins #18770

dwelch-r7 commented Jan 30, 2024 •

edited by cgranleese-r7

Loading

zgoldman-r7 left a comment

adfoster-r7 commented Feb 12, 2024

cgranleese-r7 commented Feb 13, 2024

cgranleese-r7 left a comment

cgranleese-r7 Feb 13, 2024

cgranleese-r7 Feb 13, 2024

cgranleese-r7 Feb 13, 2024

cgranleese-r7 Feb 13, 2024

cgranleese-r7 Feb 13, 2024

cgranleese-r7 Feb 13, 2024

dwelch-r7 Feb 14, 2024

cgranleese-r7 Feb 13, 2024

cgranleese-r7 Feb 13, 2024 •

edited

Loading

dwelch-r7 Feb 13, 2024

cgranleese-r7 Feb 13, 2024 •

edited

Loading

adfoster-r7 Feb 14, 2024

adfoster-r7 commented Feb 14, 2024

adfoster-r7 commented Feb 15, 2024

	'SessionTypes' => %w[PostgreSQL]
	'SessionTypes' => %w[postgresql]

Extract SMB, PostgreSQL, MySQL and MSSQL optional sessions into their own mixins #18770

Extract SMB, PostgreSQL, MySQL and MSSQL optional sessions into their own mixins #18770

Conversation

dwelch-r7 commented Jan 30, 2024 • edited by cgranleese-r7 Loading

Verification steps

zgoldman-r7 left a comment

Choose a reason for hiding this comment

adfoster-r7 commented Feb 12, 2024

cgranleese-r7 commented Feb 13, 2024

cgranleese-r7 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

cgranleese-r7 Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgranleese-r7 Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adfoster-r7 commented Feb 14, 2024

adfoster-r7 commented Feb 15, 2024

Release Notes

dwelch-r7 commented Jan 30, 2024 •

edited by cgranleese-r7

Loading

cgranleese-r7 Feb 13, 2024 •

edited

Loading

cgranleese-r7 Feb 13, 2024 •

edited

Loading