Add #read_from_file for MSSQL and PostgreSQL, fix the MySQL implementation #16650
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ySQL #read_from_file method
red0xff
force-pushed
the
mssql_postgresql_read_file_sqli
branch
from
e4b886b
to
6d9c789
Compare
| if binary | ||
| # pg_read_binary_file returns bytea | ||
| # an encoder might be needed | ||
| call_function("pg_read_binary_file('#{fpath}')") |
There was a problem hiding this comment.
Does this method handle the scenario where fpath containing unmatched single/double quote? Also, same question for MYSQL and MSSQL.
There was a problem hiding this comment.
Always worth checking if the sql injection library is vulnerable to sql injection 😄
gwillcox-r7
suggested changes
Jun 16, 2022
| def read_from_file(fpath) | ||
| run_sql("select load_file('#{fpath}')") | ||
| def read_from_file(fpath, binary=false) | ||
| call_function("load_file('#{fpath}')") |
There was a problem hiding this comment.
Won't this be an issue if the path contains single quotes at all? You can create paths on both Linux and Windows with single quotes so I'd advise looking at escaping the path before putting it into something like this.
This also applies to the other cases here.
There was a problem hiding this comment.
I'm looking for a way to fix it, not allowing quotes at all seems to be a bad idea (as they might be escaped, and part of filenames on Linux, they might not be escaped, and valid on Windows). But I have yet to find a way to check if a path is valid. I'll look for path specifications before trying to address this.
Comment on lines
+213
to
+214
| # pg_read_binary_file returns bytea | ||
| # an encoder might be needed |
There was a problem hiding this comment.
Is this comment still relevant? This isn't something we should leave up in the air before trying to land this in my opinion.
| expr = @encoder ? @encoder[:encode].sub(/\^DATA\^/, 'BulkColumn') : 'BulkColumn' | ||
| output = if @truncation_length | ||
| truncated_query("select substring(#{expr},^OFFSET^,#{@truncation_length}) " \ | ||
| "from openrowset(bulk N'#{fpath}',SINGLE_CLOB) as #{alias1}") |
There was a problem hiding this comment.
fpath could contain single quotes which would break this query string.
|
Seems to be working with PostgreSQL though did find another bug that should be fixed in another PR whereby unsetting the |
|
MSSQL Results appear mostly fine however there appears to be a weird error with the single quote character getting transformed into some unreadable character in the output when using the |
|
@red0xff Do you have a test for the MySQL implementation? I see your Docker solution at https://github.com/red0xff/sqli_vulnerable has a few implementations but I don't see a MySQL server within that collection. |
Hello, thanks for taking the time to test this. The MariaDB server within that repository can be used for testing MySQL support (as every query the library generates for MySQL should be compatible with MariaDB as well). |
|
To rename MySQL classes as MariaDB? or to add aliases? #read_from_file from MySQL should work just fine with MariaDB (as with any other feature implemented for MySQL): https://mariadb.com/kb/en/load_file/ You can just select MySQL as the DBMS in metasploit, and test everything. |
Sorry I was getting confused as |
|
So one thing I did want to ask is would there be a way to check if the user has the FILE privilege prior to using |
|
Okay yeah so made the following update to Running this however is getting the file however we are still not getting its full contents of the |
gwillcox-r7
reviewed
Jun 16, 2022
If the file privilege isn't granted, Also, I just added a commit to fix base64 encoding on MySQL/MariaDB, removing newlines from the data. |
|
@red0xff Alright last patch partially fixed the issue however you only addressed the case when encoders are used. When no encoders are used the issue is still occuring: |
Thanks for clarification on that, makes sense |
Yes, I fixed the issue with the base64 encoder adding newlines within the data. Without an encoder, it's not the library's fault, it's the test module that is using |
|
Alright @red0xff going to fix up the test module cause honestly I'm kinda annoyed its code is a bit off. Your library changes as you noted are fine so I'll land them with the updates to the test module. |
… we can properly test things like dumping file content
|
Little example for reference: |
|
Will land this once tests pass. |
gwillcox-r7
approved these changes
Jun 17, 2022
Release NotesThis PR implements the method #read_from_file for PostgreSQL and MSSQL, and fixes the MySQL implementation. It also updates the test module to better handle multiline data returned from SQL queries. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the method #read_from_file for PostgreSQL and MSSQL, and fixes the MySQL implementation
Details
The added methods
The PostgreSQL method uses
pg_read_fileorpg_read_binary_fileto read the file.The MSSQL method uses the construct
select BulkColumn from openrowset(bulk N'/etc/passwd',SINGLE_CLOB) as Content.The fix
The MySQL #read_from_file method now takes into consideration the encoder in use, and @truncation_query (it's possible to use it if the output of the query gets truncated, or if an encoder is required to get it to work).
Verification
For MySQL, don't forget to grant the FILE privilege to the user using the
load_filefunction.Just add something like the following to
test/modules/auxiliary/test/sqli_test.rbAnd test it with the different encoders.