Skip to content

sqlite PDO::quote silently corrupts strings with null bytes #13952

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

Closed
divinity76 opened this issue Apr 12, 2024 · 11 comments
Closed

sqlite PDO::quote silently corrupts strings with null bytes #13952

divinity76 opened this issue Apr 12, 2024 · 11 comments

Comments

@divinity76
Copy link
Contributor

Description

The following code:

<?php

declare(strict_types=1);

$db = new \PDO('sqlite::memory:', null, null, array(
    \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES => false,
));
var_dump($db->quote("foo\x00bar"));

Resulted in this output:

string(5) "'foo'"

But I expected this output instead:

string(17) "x'666f6f00626172'"

-or-

Fatal error: Uncaught ValueError: PDO::quote(): Argument #1 ($string) must not contain any null bytes

(but preferably the former)

  • but SILENTLY CORRUPTING THE STRING is certainly not what I expected, and not the appropriate course of action.

PHP Version

PHP 8.3.4

Operating System

Ubuntu 24.04-beta

@nielsdos
Copy link
Member

Lovely, the implementation even has a comment above it stating it is broken:

/* NB: doesn't handle binary strings... use prepared stmts for that */
static zend_string* sqlite_handle_quoter(pdo_dbh_t *dbh, const zend_string *unquoted, enum pdo_param_type paramtype)
{
char *quoted;
if (ZSTR_LEN(unquoted) > (INT_MAX - 3) / 2) {
return NULL;
}
quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3);
/* TODO use %Q format? */
sqlite3_snprintf(2*ZSTR_LEN(unquoted) + 3, quoted, "'%q'", ZSTR_VAL(unquoted));
zend_string *quoted_str = zend_string_init(quoted, strlen(quoted), 0);
efree(quoted);
return quoted_str;
}

@divinity76
Copy link
Contributor Author

divinity76 commented Apr 12, 2024

Any idea what (INT_MAX - 3) / 2 is supposed to be? It's 73 million bytes higher than SQLite's SQLITE_MAX_SQL_LENGTH default value,

((0x7FFFFFFF - 3) / 2) - 1,000,000,000 = 73741822

Maybe PHP max string length? Idk

@nielsdos
Copy link
Member

It's protection against integer overflow causing a buffer overflow.

@mvorisek
Copy link
Contributor

mvorisek commented Apr 12, 2024

@nielsdos
Copy link
Member

I think the best solution is to just implement this thing ourselves, looking at the sqlite code it doesn't seem difficult at all.

@divinity76
Copy link
Contributor Author

proposed fix: #13953

@mvorisek
Copy link
Contributor

I meant concat using x'00' might be better as otherwise the payload/used bandwith can be 2x as high.

@nielsdos
Copy link
Member

I agree with @mvorisek
This is what I was writing as a prototype: https://gist.github.com/nielsdos/1fc34c55b2ae7d24e7b2759ae26ce586
It seems to work but I need to do extra tests and cleanup. I'll take a look at @divinity76 's approach now

@divinity76
Copy link
Contributor Author

I actually tried doing concat-optimizations, and was very surprised to learn that SQLite's concat operator does not support nulls:

$ sqlite3 '' "SELECT LENGTH('foo' || x'00' || 'bar')"
3
$ sqlite3 '' "SELECT LENGTH(printf('%s%s%s', 'foo', x'00', 'bar'));"
6

@nielsdos have you actually tested your approach with null bytes? it seems to me like your approach tries using the || operator, which (to my surprise) does not support null bytes

@nielsdos
Copy link
Member

@divinity76 Yes, my approach works (test program below).

Interesting results though! Even the printf one doesn't properly work as it shows 6 instead of 7...
When doing SELECT 'foo' || x'00' || 'bar' it even outputs foo.
However, when doing the exact same thing from PHP, it appears to work:

$stmt = $db->prepare("SELECT 'foo' || x'00' || 'bar'");
$stmt->execute();
var_dump($stmt->fetchAll());

outputs:

array(1) {
  [0]=>
  array(1) {
    ["'foo' || x'00' || 'bar'"]=>
    string(7) "foobar"
  }
}

Perhaps the sqlite3 command line client also has issues with NUL byte processing.

This is the test program I'm using:

<?php

declare(strict_types=1);

$db = new \PDO('sqlite::memory:', null, null, array(
    \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES => false,
));

$db->exec('CREATE TABLE test (name TEXT)');
$db->exec("INSERT INTO test (name) VALUES (" . $db->quote("foo'bar") . ")");
$db->exec("INSERT INTO test (name) VALUES (" . $db->quote("\x00foo\x00\x00\x00bar\x00") . ")");

$stmt = $db->prepare('SELECT * FROM test');
$stmt->execute();
var_dump($stmt->fetchAll());

This shows:

array(2) {
  [0]=>
  array(1) {
    ["name"]=>
    string(7) "foo'bar"
  }
  [1]=>
  array(1) {
    ["name"]=>
    string(11) "foobar"
  }
}

which is expected (note the length 11 for foobar).

nielsdos added a commit to nielsdos/php-src that referenced this issue Apr 13, 2024
…l bytes

The built-in way to quote string using sqlite3's custom printf does not
support NULL bytes in a string. Reimplement it ourselves. This also gets
rid of the integer-based length limit.
divinity76 added a commit to divinity76/php-src that referenced this issue Apr 14, 2024
Implements x'hex' encoding in pdo::quote for handling strings with null bytes,
 providing a reliable workaround for issue phpGH-13952.
An alternative fix is discussed in PR php#13956

PR php#13956 does something interesting, it avoids the overhead of copying to/from sqlite3_snprintf,
probably speeding up PDO::quote,
but right now I just want to keep things simple.
divinity76 added a commit to divinity76/php-src that referenced this issue Apr 15, 2024
Implements x'hex' encoding in pdo::quote for handling strings with null bytes,
 providing a reliable workaround for issue phpGH-13952.
An alternative fix is discussed in PR php#13956

PR php#13956 does something interesting, it avoids the overhead of copying to/from sqlite3_snprintf,
probably speeding up PDO::quote,
but right now I just want to keep things simple.

Co-authored-by: Niels Dossche <nielsdos@php.net>
divinity76 added a commit to divinity76/php-src that referenced this issue Apr 15, 2024
Implements x'hex' encoding in pdo::quote for handling strings with null bytes,
 providing a reliable workaround for issue phpGH-13952.
An alternative fix is discussed in PR php#13956

PR php#13956 does something interesting, it avoids the overhead of copying to/from sqlite3_snprintf,
probably speeding up PDO::quote,
but right now I just want to keep things simple.

Co-authored-by: Niels Dossche <nielsdos@php.net>
@SakiTakamachi
Copy link
Member

I have reported it to sqlite along with the reproduction code.
https://sqlite.org/forum/forumpost/e6d6779ed2

divinity76 added a commit to divinity76/php-src that referenced this issue Apr 15, 2024
resolves issue phpGH-13952,
this is basically a smart_str() version of PR php#13956

per php#13962 (comment)
this is 1 of the 4 proposed alternatives to the problem, and the
pros of this solution is that it produces smaller queries than the alternatives,
and retains the sqlite datatype 'string' (instead of changing it to blob),
and should make PDO::quote faster as we now avoid the overhead of copying data to/from sqlite3_snprintf.

The cons of this solution, that I can think of right now,
 is that the implementation is non-trivial,
 involves a bunch of php-allocator-reallocs() (PR php#13956 does not invovle reallocs, as it pre-computes the length.
also worth noting that php allocator's reallocs() are faster than libc realloc, often avoiding talking to the OS),
 and SQLite's LENGTH(('foo'||x'00'||'bar')) returns 3 instead of 7,
 and binary strings gets the datatype 'string' instead of 'blob' (that can be considered both a pro and a con)

Co-authored-by: Niels Dossche <nielsdos@php.net>
divinity76 added a commit to divinity76/php-src that referenced this issue Apr 13, 2025
fix a corruption issue where PDO::quote for SQLite would silently truncate strings with
null bytes in them,
 by throwing.

resolve phpGH-13952
close phpGH-13972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment