Skip to content
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

Null (`\0`) bytes in strings cause truncation in MSSQL driver #329

Open
rmc47 opened this Issue Feb 28, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@rmc47
Copy link
Contributor

rmc47 commented Feb 28, 2019

A bit of an edge case, but I promise we hit it in real life! Feel free to close as too far-fetched.

If any user input includes a null byte (char(0) / \0) in a string, the MSSQL ODBC driver will truncate the SQL statement at that point, presumably because it's using null-terminated strings somewhere internally:

    $new_post = [
            'post_title' => "Testing " . time(),
            'post_content' => "Test with \0 null",
            'post_author' => 1,
            'post_status' => 'publish',
            'post_type' => 'post',
            'post_excerpt' => 'Hello world',
    ];

    $id = wp_insert_post($new_post);

This results in a prepared SQL statement that includes the \0 byte, at which point the ODBC driver truncates the string at that point, resulting in an incomplete SQL statement, with a complaint that the string literal isn't closed (thankfully!). The behaviour is undefined in the ODBC driver: https://docs.microsoft.com/en-us/sql/odbc/reference/develop-app/character-data-and-c-strings?view=sql-server-2017

Using prepared statements at the SQL driver level avoids this, for example:

$query = "SELECT ?";
$stmt = sqlsrv_query($conn, $query, array("Hello \0 world"));

will work, but:

$query = "SELECT 'Hello \0 world';";
$stmt = sqlsrv_query($conn, $query);

will fail.

However, I guess that wouldn't be too easy to drop into the Wordpress framework of returning a single string from wpdb::prepare().

I wonder if it's worth rejecting any queries up front that contain \0, in case it's possible to construct one where the presence of that character might result in a prematurely-truncated but still valid statement, and therefore a potential attack?

(In our case, we hit this by importing some data from an external RSS feed which contained the offending byte. I haven't been instantly able to submit it through the front-end, which is encouraging, but I've not tried extensively.)

@patrickebates

This comment has been minimized.

Copy link
Member

patrickebates commented Mar 1, 2019

There is so much that needs to be said right here...

First, I believe I could count on two hands the installs which might have been able to find this (one if I dedupe for their dev sites), so bravo. And I'll have to review the pull request and determine if there was a reason for the original order before merging.

Second, given that limited case I do wonder if a solution for you might be a filter on post save that replaces the nulls with a salt and on post display converts it back. There are similar examples of that throughout the Wordpress ecosystem.

And third
headwall
Yet another reason I really wish that SqlSrv acted more like traditional SQL Server drivers with named parameters, etc. So many problems we could solve in Wordpress just with that alone.

@rmc47

This comment has been minimized.

Copy link
Contributor Author

rmc47 commented Mar 1, 2019

To be honest, I have no good reason to store null bytes at all: it was data that happened to come in from elsewhere, and I was as surprised by it as anyone, so I'm just stripping those and dumping them in that workflow - not a big deal.

It was more the idle wondering about whether it could be an attack vector that made me throw this issue in!

(#327 and its associated PR are way more interesting, and real-world-relevant, than this issue!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.