-
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 NText column parsing to MSSQL #19054
Conversation
83f3cee
to
f00cc0d
Compare
Smaller replication steps 🟢
Looks like it crashes with null though 🔴
Other examples of failing types, not a blocker for this PR
List from: https://learn.microsoft.com/en-us/sql/t-sql/data-types/data-types-transact-sql?view=sql-server-ver16 |
f00cc0d
to
72bdc7e
Compare
col[:cflags] = data.slice!(0, 2).unpack('v')[0] | ||
col[:charset_id] = data.slice!(0, 1).unpack('C')[0] | ||
col[:namelen] = data.slice!(0, 1).unpack('C')[0] | ||
col[:table_name] = data.slice!(0, (col[:namelen] * 2) + 1).gsub("\x00", '') |
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 there's a directive for null terminated strings as an alternative to this gsub
approach 👀
https://apidock.com/ruby/String/unpack
Z | String | null-terminated string
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.
unless this is needed for utf16 support?
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 it is, but I'm not positive. Do you have any suggestions for verifying this?
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.
Circling back - this had some unexpected behavior. ntext doesn't just end in null, but separates characters with it, and unpack with Z was removing characters to the right of null values
lib/rex/proto/mssql/client_mixin.rb
Outdated
@@ -328,6 +337,18 @@ def mssql_parse_tds_row(data, info) | |||
end | |||
row << str.gsub("\x00", '') | |||
|
|||
when :ntext | |||
str = "" |
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 looks here like str
will always be a string, either empty or with data. I'm wondering though how it will handle a column from the target SQL server with nullable values where there's a mix of some set, some empty and some are null. On the ruby side I would expect nil
for a null value, and strings for the rest.
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.
So from what I can tell, it looks like when we're outputting the data, our format outputs both empty string and nil as blank. Do you think this is worth changing? @adfoster-r7 thoughts? I believe we've had a similar discussion before, but I can't remember.
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 client should return real ruby values, regardless of our presentation layer
it's the same feedback as this: #18872 (comment) 📈
So you'll want to implement @smcintyre-r7's suggestion 💯
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.
@zgoldman-r7 If in your testing the output data looks correct for NULL and empty string values without crashing, then it's probably just a matter of tweaking the Ruby code to identify when it should be nil
and change the path from when it's an empty string.
I'm totally shooting in the dark here because I can't find the T-SQL docs you shared with me at one point, but I'd guess it might be something like this:
str = nil
ptrlen = data.slice!(0, 1).unpack("C")[0]
ptr = data.slice!(0, ptrlen)
unless ptrlen == 0
str = "" # at this point, ptrlen being non-zero implies there's data
timestamp = data.slice!(0, 8)
datalen = data.slice!(0, 4).unpack("V")[0] # this is probably zero if the string is empty?
str = data.slice!(0, datalen)
end
I could be very very wrong though. Could you also add a link to the docs you've been reading into this file? That would help future travelers and myself.
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.
@smcintyre-r7 As far as that pattern goes, I implemented something similar locally and it seems to work, so that looks good - I'll tweak the presentation layer too 👍
As far as the docs link, I have a link to https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-tds/ce3183a6-9d89-47e8-a02f-de5a1a1303de from a recent PR on line 234 - is that what you're referring to?
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 might have been a different page we were looking at. Based on those docs though
Null is represented by a length of 65535 (0xFFFF). A non-nullable char or binary can still have a length of zero (for example, an empty value).
It looks like something should be compared to 0xffff which is different than what I'd suggested.
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.
@smcintyre-r7 interesting catch - especially considering that doesn't seem to be what I get on wireshark running run rhost=127.0.0.1 username=sa password=P4$$w0rd123 database='' sql="select cast(null as ntext);"
- I wonder if/how that's different.
Looking at our :string
parse though, it seems to have a similar check for both (line 326):
if len > 0 && len < 65535
str << data.slice!(0, len)
end
995d609
to
ac54e92
Compare
Looks like test failures 👀 |
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.
For the docs:
hashcat --force -m 1731 ./hashes.txt ./passwords.txt
131 MSSQL (2000) 0x01002702560500000000000000000000000000000000000000008db43dd9b1972a636ad0c7d4b8c515cb8ce46578
132 MSSQL (2005) 0x010018102152f8f28c8499d8ef263c53f8be369d799f931b2fbe
1731 MSSQL (2012, 2014, and above) 0x02000102030434ea1b17802fd95ea6316bd61d2c94622ca3812793e8fb1672487b5c904a45a31b2ab4a78890d563d2fcf5663e46fe797d71550494be50cf4915d3f4d55ec375
Fixing that mssql hashdump module: diff --git a/modules/auxiliary/scanner/mssql/mssql_hashdump.rb b/modules/auxiliary/scanner/mssql/mssql_hashdump.rb
index cfac867a4f..a1fd781fbc 100644
--- a/modules/auxiliary/scanner/mssql/mssql_hashdump.rb
+++ b/modules/auxiliary/scanner/mssql/mssql_hashdump.rb
@@ -28,6 +28,12 @@ class MetasploitModule < Msf::Auxiliary
set_session(session.client)
elsif !mssql_login(datastore['USERNAME'], datastore['PASSWORD'])
print_error('Invalid SQL Server credentials')
+ info = self.mssql_client.initial_connection_info
+ if info[:errors] && !info[:errors].empty?
+ info[:errors].each do |err|
+ print_error(err)
+ end
+ end
return
end
@@ -98,6 +104,8 @@ class MetasploitModule < Msf::Auxiliary
hashtype = "mssql05"
when "2012", "2014"
hashtype = "mssql12"
+ else
+ hashtype = "mssql12"
end
this_service = report_service(
@@ -125,12 +133,15 @@ class MetasploitModule < Msf::Auxiliary
next if row[0].nil? or row[1].nil?
next if row[0].empty? or row[1].empty?
+ username = row[0]
+ upcase_hash = "0x#{row[1].upcase}"
+
credential_data = {
module_fullname: self.fullname,
origin_type: :service,
private_type: :nonreplayable_hash,
- private_data: "0x#{row[1]}",
- username: row[0],
+ private_data: upcase_hash,
+ username: username,
jtr_format: hashtype
}
@@ -146,8 +157,8 @@ class MetasploitModule < Msf::Auxiliary
login_data.merge!(service_data)
login = create_credential_login(login_data)
- tbl << [row[0], row[1]]
- print_good("Saving #{hashtype} = #{row[0]}:#{row[1]}")
+ tbl << [username, upcase_hash]
+ print_good("Saving #{hashtype} = #{username}:#{upcase_hash}")
end
end
@@ -167,6 +178,8 @@ class MetasploitModule < Msf::Auxiliary
when "2005", "2008", "2012", "2014"
results = mssql_query(mssql_2k5_password_hashes())[:rows]
+ else
+ results = mssql_query(mssql_2k5_password_hashes())[:rows]
end
return results
Will also need to update the user docs if that hasn't been added 👀 |
diff --git a/modules/auxiliary/scanner/mssql/mssql_hashdump.rb b/modules/auxiliary/scanner/mssql/mssql_hashdump.rb
index cfac867a4f..6656c83744 100644
--- a/modules/auxiliary/scanner/mssql/mssql_hashdump.rb
+++ b/modules/auxiliary/scanner/mssql/mssql_hashdump.rb
@@ -107,12 +107,6 @@ class MetasploitModule < Msf::Auxiliary
:proto => 'tcp'
)
- tbl = Rex::Text::Table.new(
- 'Header' => 'MS SQL Server Hashes',
- 'Indent' => 1,
- 'Columns' => ['Username', 'Hash']
- )
-
service_data = {
address: ::Rex::Socket.getaddress(mssql_client.peerhost,true),
port: mssql_client.peerport,
@@ -146,7 +140,6 @@ class MetasploitModule < Msf::Auxiliary
login_data.merge!(service_data)
login = create_credential_login(login_data)
- tbl << [row[0], row[1]]
print_good("Saving #{hashtype} = #{row[0]}:#{row[1]}")
end
end
This can be deleted 👍 |
ac54e92
to
f3cb214
Compare
Looks like there's still some test failures 👀 |
Thanks for your pull request! Before this can be merged, we need the following documentation for your module: |
yep - schemadump adjustments I made need tweaking for the tests, looking shortly |
f3cb214
to
3e0b5eb
Compare
3e0b5eb
to
d357484
Compare
|
||
The `mssql_hashdump` module queries an MSSQL instance or session and returns hashed user:pass pairs. These pairs can be decripted via or `hashcat`. | ||
|
||
## Available Options |
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; I don't think this follows the format that's expected for module documentation: #19054 (comment)
@@ -41,6 +41,15 @@ def test_console_query | |||
end | |||
end | |||
|
|||
def test_datatypes | |||
it "should support ntext TDS datatype" do | |||
stdout = with_mocked_console(session) {|console| console.run_single(%{ query "select cast('foo' as ntext);"})} |
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'll probably want this to use the convention of the other PR, but can fix separately
I think there's some follow up work here, but this seems like a good first step |
Release NotesAdds NText column parsing to MSSQL modules |
Addresses
Unsupported column type: 99.
error for mssqlVerification:
CREATE TABLE xml_table(Col1 int primary key, Col2 xml);
INSERT INTO xml_table values (3, '<ProductDescription ProductID="1"/>');
mssql_sql
module, runselect * from xml_table