-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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 mssql_version module #18907
add mssql_version module #18907
Conversation
415ebb7
to
95671b9
Compare
end | ||
|
||
def run_host(ip) | ||
version = mssql_get_version |
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.
Was it not possible to use mssql_prelogin
directly without copy/pasting 👀
It looks like no methods currently make use out of the mssql_prelogin
result - so updating it to return a hash of metadata doesn't sound too bad to me
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.
Sure, that seems a lot dryer -
Just curious, what do you think about, instead of using all of mssql_prelogin
, I factor out the code of each method into a make_prelogin_packet
helper?
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.
Splitting out the functionality of mssql_prelogin
into smaller reusable blocks also works for me
version = mssql_get_version | ||
if version && !version.empty? | ||
print_status("SQL Server for #{ip}:") | ||
print_good("Version: #{version}") |
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 assume we'll want to report this information into the database similar to the mssql_ping module
metasploit-framework/modules/auxiliary/scanner/mssql/mssql_ping.rb
Lines 57 to 80 in 401cdca
def report_mssql_service(ip,info) | |
mssql_info = "Version: %s, ServerName: %s, InstanceName: %s, Clustered: %s" % [ | |
info['Version'], | |
info['ServerName'], | |
info['InstanceName'], | |
info['IsClustered'] | |
] | |
report_service( | |
:host => ip, | |
:port => 1434, | |
:name => "mssql-m", | |
:proto => "udp", | |
:info => "TCP: #{info['tcp']}, Servername: #{info['ServerName']}" | |
) | |
mssql_tcp_state = (test_connection(ip,info['tcp']) == :up ? "open" : "closed") | |
report_service( | |
:host => ip, | |
:port => info['tcp'], | |
:name => "mssql", | |
:info => mssql_info, | |
:state => mssql_tcp_state | |
) | |
end |
end | ||
|
||
def run_host(ip) | ||
version = mssql_get_version |
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.
Tracking if the server supports encryption or not may be useful too.
lib/rex/proto/mssql/client_mixin.rb
Outdated
pkt | ||
end | ||
|
||
def mssql_get_version |
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.
Instead of this specific mssql_get_version
method; it might be a better long-term implementation to just have this method return a hash of the parsed values that the server has returned
i.e. the version number, and whether encryption is supported - and maybe in the future it would have more details
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.
Looks like we had the same thought, I included this in the last commit
d3b9d34
to
d06df0e
Compare
pkt_hdr[2] = pkt_data.length + 8 | ||
|
||
pkt = pkt_hdr.pack("CCnnCC") + pkt_data | ||
pkt = mssql_prelogin_packet | ||
|
||
resp = mssql_send_recv(pkt) |
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 think if we renamed mssql_get_version
and changed it to just handle parsing, we'd be able to reuse it here below and avoid more of the copy/pasta:
def parse_pre_login_response(data)
It would also be unit-testable which would be nice
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 like the idea, but can you explain what you're envisioning? Where would we reuse it, and what are you suggesting we restrict from the method?
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.
We can get rid of the first block of copy/pasta on lines
metasploit-framework/lib/rex/proto/mssql/client.rb
Lines 491 to 507 in 95283f7
while resp && resp[0, 1] != "\xff" && resp.length > 5 | |
token = resp.slice!(0, 5) | |
token = token.unpack("Cnn") | |
idx -= 5 | |
if token[0] == 0x01 | |
idx += token[1] | |
break | |
end | |
end | |
if idx > 0 | |
encryption_mode = resp[idx, 1].unpack("C")[0] | |
else | |
framework_module.print_error("Unable to parse encryption req " \ | |
"during pre-login, this may not be a MSSQL server") | |
encryption_mode = ENCRYPT_NOT_SUP | |
end |
and
metasploit-framework/lib/rex/proto/mssql/client.rb
Lines 538 to 555 in 95283f7
idx = 0 | |
while resp && resp[0, 1] != "\xff" && resp.length > 5 | |
token = resp.slice!(0, 5) | |
token = token.unpack("Cnn") | |
idx -= 5 | |
if token[0] == 0x01 | |
idx += token[1] | |
break | |
end | |
end | |
if idx > 0 | |
encryption_mode = resp[idx, 1].unpack("C")[0] | |
else | |
framework_module.print_error("Unable to parse encryption req " \ | |
"during pre-login, this may not be a MSSQL server") | |
encryption_mode = ENCRYPT_NOT_SUP | |
end |
] | ||
report_service( | ||
host: ip, | ||
port: 1433, |
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.
port: 1433, | |
port: rport, |
Or if it's not accessible, datastore['RPORT']
super( | ||
'Name' => 'MSSQL Version Utility', | ||
'Description' => 'This module simply queries the MSSQL instance for version information.', | ||
'Author' => 'MC', |
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 assume this needs to be updated, unless it's a hacker handle
def initialize | ||
super( | ||
'Name' => 'MSSQL Version Utility', | ||
'Description' => 'This module simply queries the MSSQL instance for version information.', |
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.
'Description' => 'This module simply queries the MSSQL instance for version information.', | |
'Description' => 'Executes a TDS7 pre-login request against the MSSQL instance to query for version information.', |
d06df0e
to
93e8f87
Compare
lib/msf/core/exploit/remote/mssql.rb
Outdated
@@ -53,6 +53,10 @@ def set_session(client) | |||
@mssql_client = client | |||
end | |||
|
|||
def mssql_get_version |
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 wasn't sure if we needed this method or not, since we already have a mssql_prelogin
method available - which if we changed it to return a hash, would contain all of the information we need for finger printing the remote mssql instance already
@@ -53,6 +53,10 @@ def set_session(client) | |||
@mssql_client = client | |||
end | |||
|
|||
def mssql_get_version | |||
@mssql_client ||= Rex::Proto::MSSQL::Client.new(self, framework, datastore['RHOST'], datastore['RPORT']) |
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.
Maybe we just need a standalone method for creating the mssql client instance here - since the only way to get access to one is via mssql_login
- which we don't have creds for
lib/rex/proto/mssql/client_mixin.rb
Outdated
disconnect if self.sock | ||
connect |
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 think we if we just update the remote/mssql.rb file to allow instantiating a client without needing to perform a login with real creds - we'll be able to get rid of these lines here
But we can still keep a renamed version of this method that focuses on just parsing the pre-login response to return a hash object that the existing code can make use of
lib/rex/proto/mssql/client_mixin.rb
Outdated
return unless resp | ||
|
||
data = {} | ||
while resp |
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.
This parsing logic looks like it could be very brittle, if we're targeting other systems the results may not always be in the order we expect it to be
lib/rex/proto/mssql/client_mixin.rb
Outdated
data = {} | ||
while resp | ||
token = resp.slice!(0, 1) | ||
if token.unpack('C')[0] == 255 |
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.
We could circle back to this to use bindata, but let's skip that requirement for now - we can just get this PR passing with a unit test that tests this function in isolation i.e. expect(subject.parse_prelogin_packet(buf)).to eq({ version: '...', encryption: false })
and in the future we could swap it out for bindata
|
||
def run | ||
if session | ||
set_session(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; we'll still want to rename this on our side - i.e. set_mssql_session
or such
set_session(session.client) | ||
end | ||
|
||
data = mssql_get_version |
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.
def create_mssql_client
@mssql_client ||= Rex::Proto::MSSQL::Client.new(self, framework, datastore['RHOST'], datastore['RPORT'])
end
data = mssql_get_version | |
def | |
if session | |
set_session(session.client) | |
else | |
mssql_client = create_mssql_client | |
mssql_client.connect | |
end | |
# ... | |
begin | |
# WIll be a hash | |
response = client.mssql_prelogin | |
ensure | |
# Ensure there's cleanup of the socket | |
client.close unless session | |
end | |
# ... log to database etc ... | |
end |
c1e87d4
to
669488e
Compare
pkt | ||
end | ||
|
||
def parse_prelogin_response(resp) |
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.
It'd be great to add tests for this as part of this PR so we can circle back to swapping this out with alternative implementations in the future 👍
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.
Already working on it, just pushed this in the meantime 👍
lib/rex/proto/mssql/client_mixin.rb
Outdated
end | ||
|
||
if major && minor && build | ||
data['Version'] = "#{major}.#{minor}.#{build}" |
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.
data['Version'] = "#{major}.#{minor}.#{build}" | |
data[:version] = "#{major}.#{minor}.#{build}" |
Using snake case symbol keys is more idiomatic for modern ruby
lib/rex/proto/mssql/client_mixin.rb
Outdated
build = resp.slice(version_index+2, 2).unpack('n')[0] | ||
|
||
enc_index = resp.slice(6, 2).unpack('n')[0] | ||
data['Encryption'] = resp.slice(enc_index, 1).unpack('C')[0] |
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.
data['Encryption'] = resp.slice(enc_index, 1).unpack('C')[0] | |
data[:encryption] = resp.slice(enc_index, 1).unpack('C')[0] |
lib/rex/proto/mssql/client_mixin.rb
Outdated
end | ||
|
||
|
||
data['Status'] = 'open' if data['Version'] || data['Encryption'] |
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.
data['Status'] = 'open' if data['Version'] || data['Encryption'] |
This seems like a concern that isn't relevant to parsing prelogin response packets 👀
Maybe it's something you want to add to your module, but not here - which offers generic and reusable library code 🤔
20a6c88
to
16b7cc3
Compare
before do | ||
allow(client).to receive(:parse_prelogin_response).and_call_original | ||
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.
I don't think this is needed?
before do | |
allow(client).to receive(:parse_prelogin_response).and_call_original | |
end |
|
||
def run | ||
if session | ||
set_mssql_session(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.
Now that I think about it - the version metadata might be cached already on the session object, so we wouldn't need to resend a prelogin request 🤔
bf79f5e
to
fb72fd8
Compare
end | ||
|
||
report_mssql_service(mssql_client.address, data) | ||
mssql_client.disconnect |
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.
The PR needs rebased; and I think this will be brought up as a bug in the new integration tests
2490915
to
f9cb898
Compare
Co-authored-by: jheysel-r7 <[email protected]>
f9cb898
to
c990542
Compare
c990542
to
95ca80b
Compare
spec/acceptance/mssql_spec.rb
Outdated
"Version: ", | ||
"Encryption is " |
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.
Could we throw regexes here, similar to some of the other snippets you've got:
"Version: ", | |
"Encryption is " | |
/Version: Microsoft SQL Server \d+.\d+/, | |
/Encryption: (?:on|off|or whatever)/ |
else | ||
data[:encryption] = 'unknown' | ||
end | ||
print_good("Encryption is #{data[:encryption]}") |
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.
print_good("Encryption is #{data[:encryption]}") | |
print_good("Encryption: #{data[:encryption]}") |
95ca80b
to
c382066
Compare
@msjenkins-r7 retest this please |
Release NotesAdds a new |
Resolves #18684
mssql_ping
relies on the SQL Server Browser UDP service at 1434 to be running, so we want another way to get some information on the server. This adds themssql_version
module which attempts to connect directly to mssql and retrieve some information surrounding the version number and whether encryption is supported.Verification
List the steps needed to make sure this thing works
msfconsole
use mssql_version
run rhosts=YOUR_RHOST_HERE
This also works with a session instead of rhost
msfconsole
use mssql_login
run CreateSession=true [rest of args here]
use mssql_version
run rhosts=YOUR_RHOST_HERE