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
Updates MySQL modules to now support the new MySQL session type #18759
Updates MySQL modules to now support the new MySQL session type #18759
Conversation
Your skipped module list looks good; but I think there's a few more modules we could pull in from it to implement:
|
@adfoster-r7 Thanks for taking a look. I can take look into get those updated as well then 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM from a static pov, just a couple questions. Haven't tested for myself yet since this PR doesn't seem to include the MySQL session type addition
6f581c8
to
fc3135a
Compare
@@ -6,6 +6,7 @@ | |||
class MetasploitModule < Msf::Auxiliary | |||
include Msf::Auxiliary::Report | |||
include Msf::Exploit::Remote::MYSQL | |||
include Msf::OptionalSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're moving to using optional sessions like this here:
include Msf::OptionalSession::MySQL
Why wouldn't we just override rport
and rhost
to be something like:
module Msf
module OptionalSession
module MySQL
include Msf::OptionalSession
# ...
def rhost
return session.client.host if session
super # Or potentially default to datastore['RHOST'] etc
end
def rport
return session.client.port if session
super
end
# ...
That way none of these modules would need code changes to replace rhost->mysql_conn.host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dwelch-r7 This one might be a question for yourself I think 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking directly accessing the host/port via the client makes the most sense here, it feels a bit weird to me trying to use rhost
and rport
when we require the client in every situation anyway, I'm just not sure that getting a result for rhost
when the datastore value is empty is expected either
I'm not super against it though, unless there's a scenario where we have an OptionalSession and want to keep rhost/rport datastore options free but that feels like a bit of a stretch
any references to datastore['RHOST']
or datastore['RPORT']
still need to be changed either way whether it's to rhost
/rport
or mysql_conn.host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me 👍 It also stops mixin madness, for modules that would need to target multiple protocols
# If we have a session make use it | ||
if session | ||
print_status("Using existing session #{session.sid}") | ||
self.mysql_conn = session.client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker; and maybe one for a future PR. But maybe we should move all of this logic into the new optional session MySQL mixin:
def use_mysql_session
print_status("Using existing session #{session.sid}")
self.mysql_conn = session.client
self.sock = session.client.sock_or_conn_or_whatever_is_required # This might be required
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably makes sense to explore this approach after the new mixins are available: #18770
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup totally agree, thinking a new PR that handles this across the sessions makes the most sense
fc3135a
to
98f961e
Compare
…e for rhost and rport
98f961e
to
e80f0ef
Compare
It looks like there's a bug here where RPORT isn't being defaulted anymore 👀
|
Yeah good catch. I needed to update the OptionalSession mixin to include the default. As a heads up, I got caught out by having multiple of the new session types features enabled at once and they were overriding each other and the Postgres options being set last was overriding my I believe we have a ticket to move each of these session types out into a separate mixin per session type. |
It looks like from your test resource script: print_warning("******** SESSION ********")
if mod.include?("file_enum")
run_single("run session=1 file_list=/")
elsif mod.include?("writable_dirs")
run_single("run session=1 dir_list=/")
else
run_single("run session=1")
end That the file_list and dir_list are meant to be a local file with a list of directories/files:
After creating the corresponding files:
The old resource script was error'ing out:
|
@mysql_handle = ::Mysql.connect(rhost, user, pass, db, rport, io: sock) | ||
self.mysql_conn = ::Mysql.connect(rhost, user, pass, db, rport, io: sock) | ||
# Deprecating this in favor off `mysql_conn` | ||
@mysql_handle = ActiveSupport::Deprecation::DeprecatedInstanceVariableProxy.new(self, :mysql_conn, :@mysql_handle, ActiveSupport::Deprecation.new) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that this deprecation works, and that old modules would continue to function:
msf6 exploit(multi/mysql/mysql_udf_payload) > git checkout upstream/master -- modules/auxiliary/scanner/mysql/mysql_file_enum.rb
[*] exec: git checkout upstream/master -- modules/auxiliary/scanner/mysql/mysql_file_enum.rb
msf6 exploit(multi/mysql/mysql_udf_payload) > use auxiliary/scanner/mysql/mysql_file_enum
msf6 auxiliary(scanner/mysql/mysql_file_enum) > rerun rhost=127.0.0.1 username=root password=123456 file_list=./files.txt
[*] Reloading module...
[*] 127.0.0.1:3306 - Login...
[+] 127.0.0.1:3306 - 127.0.0.1:3306 MySQL - Logged in to '' with 'root':'123456'
DEPRECATION WARNING: @mysql_handle is deprecated! Call mysql_conn.query instead of @mysql_handle.query. Args: ["USE mysql"] (called from mysql_query_no_handle at /Users/user/Documents/code/metasploit-framework/modules/auxiliary/scanner/mysql/mysql_file_enum.rb:40)
... etc ...
Release NotesUpdates the multiple MySQL modules to work with a provided MySQL session instead of opening a new connection. This functionality is behind a feature flag which can be enabled with |
This PR is a continuation of #18718, make sure it is merged before landing this.
This PR updates existing MySQL modules to know work with an existing session, as well as the original
rhost
functionality:modules/auxiliary/scanner/mysql/mysql_authbypass_hashdump.rb
I updated the MySQL modules to make use of
self.mysql_conn
instead of@mysql_handle
. This module was the only other module that wasn't updated as part of this PR that relied on@mysql_handle
, I updated it to now useself.mysql_conn
and tested it and I believe it works as expected, the module completes but I didn't have an old enough version of MySQL to test it fully.Hopefully that was the right call. Happy to revert if needed 👍
Verification
Targets I used
vulhub/mysql:5.5.23
mariadb:latest
Windows 10 via MySQL installer
Resource script to test modules
Testing steps
msfconsole
mysql_session_type
featurerun session=1
run rhosts=127.0.0.1 username=root password=password rport=3306
run 'mysql://root:password@127.0.0.1'