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

[rmc47]

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]

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 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]

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!)