php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #77051 Issue with re-binding on SQLite3
Submitted: 2018-10-23 21:26 UTC Modified: 2018-11-29 00:25 UTC
From: php at bohwaz dot net Assigned: cmb (profile)
Status: Closed Package: SQLite related
PHP Version: 5.6.38 OS: All
Private report: No CVE-ID: None
 [2018-10-23 21:26 UTC] php at bohwaz dot net
Description:
------------
I have discovered this bug while chatting with Christoph about details of bindValue and bindParam implementation in the SQLite3 extension.

As highlighted in these examples:
https://3v4l.org/vkcnQ
https://3v4l.org/R4CbZ

Binding a parameter (with ::bindParam or ::bindValue), then executing the statement, and then rebinding to a different value and re-executing the statement (without calling ::reset, as it should be according to the SQLite3 C library doc), causes PHP to return what is apparently incorrect info in some cases. It returns either the name of the binding (here ":a") in PHP 7.0 & 7.3, or random chunks of data (5.6 and some versions of 7.1).

Returning random chunks of data looks like it is returning parts of the memory. But not sure about that. This might be a potential security flaw.

@cmb proposes this patch to fix the issue: https://gist.github.com/cmb69/950613abe8b554502bf9749efd5df9c9

And says:

> TL;DR: after execution of the statement, libsqlite3 returns
> SQLITE_MISUSE if any attempt to re-bind is made.  The SQLite docs make
> it clear that SQLITE_MISUSE may not be reliably detected, and that any
> SQLITE_MISUSE return value has to be regarded as potentially serious bug.
>
> Obviously, it is hard for a PHP developer to notice this misuse, since
> it is not detected by the SQLite3 binding, so we should apply something
> like my patch, and prominently document the behavior. 

I agree with that proposal. I would just add that maybe the error message should contain something about the fact that you should use ::reset if you want to re-bind the same parameter, to help users figure out what's happening.

Note that the same issue might apply to PDO SQLite driver as well but I didn't have time to check that yet.

Test script:
---------------
<?php

$db = new SQLite3(':memory:');

$stmt = $db->prepare('SELECT :a;');

$a = 42;
$stmt->bindParam(':a', $a, SQLITE3_INTEGER);
$stmt->bindValue(':b', 'php');
$stmt->bindValue(':b', 'PHP');
$stmt->bindValue(3, 424242);

echo "Execute statement\n";
var_dump($res = $stmt->execute());

echo "Statement result\n";
var_dump($res->fetchArray(SQLITE3_NUM));

//$stmt->reset();
echo "Change binded values\n";
$stmt->bindValue(':a', 'TEST');
$stmt->bindValue(':b', '!!!');
$stmt->bindValue(3, 40, SQLITE3_INTEGER);

echo "Execute statement\n";
var_dump($res = $stmt->execute());

echo "Statement result\n";
var_dump($res->fetchArray(SQLITE3_NUM));

Expected result:
----------------
Not sure.

Actual result:
--------------
Random chunks of binary data

Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-10-24 14:31 UTC] php at bohwaz dot net
Seems that PDO is not affected, the code handles SQLite returns correctly. Eg:

if (sqlite3_bind_null(S->stmt, param->paramno + 1) == SQLITE_OK) {
	return 1;
}
pdo_sqlite_error_stmt(stmt);
return 0;
 [2018-11-11 18:11 UTC] stas@php.net
-Assigned To: +Assigned To: scottmac
 [2018-11-11 18:13 UTC] stas@php.net
-Assigned To: scottmac +Assigned To: cmb
 [2018-11-11 18:15 UTC] stas@php.net
Doesn't look like security issue though - it requires specific targeted developer code to trigger.
 [2018-11-12 13:24 UTC] cmb@php.net
> Doesn't look like security issue though - it requires specific
> targeted developer code to trigger.

Isn't PHP supposed to shield from use after free szenarios?
However, the given test script causes an UAF (tested with current
PHP-7.3 with bundled libsqlite):

    ==25219== Memcheck, a memory error detector
    ==25219== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
    ==25219== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
    ==25219== Command: sapi/cli/php -d error_reporting=-1 ../77051.php
    ==25219==
    Execute statement
    object(SQLite3Result)#3 (0) {
    }
    Statement result
    array(1) {
    [0]=>
    string(2) "42"
    }
    Change binded values
    Execute statement
    object(SQLite3Result)#4 (0) {
    }
    Statement result
    ==25219== Invalid read of size 2
    ==25219==    at 0x4C30198: memcpy@GLIBC_2.2.5 (vg_replace_strmem.c:1017)
    ==25219==    by 0x2A937C: sqlite3VdbeMemGrow (sqlite3.c:73201)
    ==25219==    by 0x2A9793: vdbeMemAddTerminator (sqlite3.c:73243)
    ==25219==    by 0x3074D3: sqlite3VdbeMemNulTerminate (sqlite3.c:73314)
    ==25219==    by 0x3074D3: sqlite3VdbeExec (sqlite3.c:83310)
    ==25219==    by 0x30C27F: sqlite3Step (sqlite3.c:80302)
    ==25219==    by 0x30C27F: sqlite3_step (sqlite3.c:80365)
    ==25219==    by 0x280CA6: zim_sqlite3result_fetchArray (sqlite3.c:1802)
    ==25219==    by 0x4D867D: ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER (zend_vm_execute.h:1102)
    ==25219==    by 0x4D867D: execute_ex (zend_vm_execute.h:55442)
    ==25219==    by 0x4D8E7F: zend_execute (zend_vm_execute.h:60834)
    ==25219==    by 0x44FC2A: zend_execute_scripts (zend.c:1568)
    ==25219==    by 0x3EFEEF: php_execute_script (main.c:2630)
    ==25219==    by 0x4DB1CD: do_cli (php_cli.c:997)
    ==25219==    by 0x1EE09C: main (php_cli.c:1390)
    ==25219==  Address 0x6384858 is 24 bytes inside a block of size 32 free'd
    ==25219==    at 0x4C2CDDB: free (vg_replace_malloc.c:530)
    ==25219==    by 0x48A86A: zend_assign_to_variable (zend_execute.h:108)
    ==25219==    by 0x48A86A: ZEND_ASSIGN_SPEC_CV_CONST_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:40951)
    ==25219==    by 0x4D3339: execute_ex (zend_vm_execute.h:59726)
    ==25219==    by 0x4D8E7F: zend_execute (zend_vm_execute.h:60834)
    ==25219==    by 0x44FC2A: zend_execute_scripts (zend.c:1568)
    ==25219==    by 0x3EFEEF: php_execute_script (main.c:2630)
    ==25219==    by 0x4DB1CD: do_cli (php_cli.c:997)
    ==25219==    by 0x1EE09C: main (php_cli.c:1390)
    ==25219==  Block was alloc'd at
    ==25219==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
    ==25219==    by 0x427CE8: __zend_malloc (zend_alloc.c:2904)
    ==25219==    by 0x447502: zend_string_alloc (zend_string.h:133)
    ==25219==    by 0x447502: zend_string_init (zend_string.h:155)
    ==25219==    by 0x447502: zend_long_to_str (zend_operators.c:2992)
    ==25219==    by 0x4476F7: _convert_to_string (zend_operators.c:556)
    ==25219==    by 0x280850: zim_sqlite3stmt_execute (sqlite3.c:1624)
    ==25219==    by 0x4D867D: ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER (zend_vm_execute.h:1102)
    ==25219==    by 0x4D867D: execute_ex (zend_vm_execute.h:55442)
    ==25219==    by 0x4D8E7F: zend_execute (zend_vm_execute.h:60834)
    ==25219==    by 0x44FC2A: zend_execute_scripts (zend.c:1568)
    ==25219==    by 0x3EFEEF: php_execute_script (main.c:2630)
    ==25219==    by 0x4DB1CD: do_cli (php_cli.c:997)
    ==25219==    by 0x1EE09C: main (php_cli.c:1390)
    ==25219==
    array(1) {
    [0]=>
    string(2) "42"
    }
    ==25219==
    ==25219== HEAP SUMMARY:
    ==25219==     in use at exit: 0 bytes in 0 blocks
    ==25219==   total heap usage: 7,161 allocs, 7,161 frees, 1,728,973 bytes allocated
    ==25219==
    ==25219== All heap blocks were freed -- no leaks are possible
    ==25219==
    ==25219== For counts of detected and suppressed errors, rerun with: -v
    ==25219== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Anyhow, while the linked patch[1] basically seems to be a
reasonable improvement, it actually doesn't solve the problem that
we do not reset the current bindings before we (re-)bind all
registered bound variables[2].  So the appropriate fix for this
issue is rather something like clear-before-bind.patch[3] (not
sure if error handling for sqlite3_clear_binding() should be added
right away, or only as improvement).

@BohwaZ what do you think?

[1] <https://gist.github.com/cmb69/950613abe8b554502bf9749efd5df9c9>
[2] <https://github.com/php/php-src/blob/php-7.3.0RC5/ext/sqlite3/sqlite3.c#L1568-L1663>
[3] <https://gist.github.com/cmb69/1c41991df75f307a760ae473a2e9345b>
 [2018-11-12 15:25 UTC] php at bohwaz dot net
IMHO the best solution would be to reset the statement when needed, like PDO_SQLite is doing:

bindValue/bindParam:
https://github.com/php/php-src/blob/php-7.3.0RC5/ext/pdo_sqlite/sqlite_statement.c#L84-L87

and

execute:
https://github.com/php/php-src/blob/php-7.3.0RC5/ext/pdo_sqlite/sqlite_statement.c#L48-L50

We have discussed this option earlier while discussing issues with reset/clear behavior consistencies. It seems to be a good solution in that case as it will solve the issue and not produce an error message/exception if you happen to fail to know the correct order the execute/bindValue/reset methods should be called.

However this doesn't change the fact that we still need to check the result of the sqlite3_bind_* functions, just in case.

If you just call sqlite3_clear_bindings without calling sqlite3_reset before, you will end up with all your binded params with NULL values, even if you do call bindValue after. I don't think that calling sqlite3_clear_bindings is actually useful. The reset should be enough.

As for the memory issue, according to @cmb memcheck the issue seem to happen inside the SQLite3 library.
 [2018-11-12 16:00 UTC] cmb@php.net
> If you just call sqlite3_clear_bindings without calling
> sqlite3_reset before, you will end up with all your binded params
> with NULL values, even if you do call bindValue after. I don't
> think that calling sqlite3_clear_bindings is actually useful. The
> reset should be enough.

Calling sqlite3_reset() without sqlite3_clear_bindings() is
definitely insufficient[1].  The sole addition of
sqlite3_clear_bindings() lets the supplied test script succeed,
but additionally calling sqlite3_reset() might be more
appropriate.

> As for the memory issue, according to @cmb memcheck the issue
> seem to happen inside the SQLite3 library.

From what I can tell, the formerly bound parameter is written to
by SQLite3 (because the second binding was rejected), although it
has already been released in the meantime.

[1] <https://www.sqlite.org/c3ref/clear_bindings.html>
 [2018-11-12 16:22 UTC] php at bohwaz dot net
PDO_SQLite is not calling sqlite3_clear_bindings at any point so I'm not sure if it is necessary?

I just tried and just doing a reset seems to be enough.

Though calling clear_bindings is not an issue anyway as all the values are stored in a list and re-binded every time execute is called, so it wouldn't change anything.

But ideally the binding should happen when the call to bindValue is done, and we shouldn't have to re-bind every value every time we execute (but it is currently required because of bindParam), in that context calling clear_bindings would not be adequate as it would set to NULL any value previously binded. (but as I said it would be "ideally", and not the current way it is handled)
 [2018-11-18 20:55 UTC] stas@php.net
-Type: Security +Type: Bug
 [2018-11-22 16:05 UTC] php at bohwaz dot net
Made a PR to fix this issue https://github.com/php/php-src/pull/3675

I also recommend applying @cmbs' patch to check the return value of sqlite3_bind_ functions.
 [2018-11-29 00:25 UTC] cmb@php.net
> Though calling clear_bindings is not an issue anyway as all the
> values are stored in a list and re-binded every time execute is
> called, so it wouldn't change anything.

Indeed, you're right!  Since we don't allow to unbind parameters
(except via SQLite3Stmt::clear(), which calls
sqlite3_clear_bindings() anyway), there is no need to temporarily
bind the parameters to null values.

> I also recommend applying @cmbs' patch to check the return value
> of sqlite3_bind_ functions.

I agree, but unless that would really fix a bug, I think this
should target master only.
 [2018-11-29 01:21 UTC] cmb@php.net
Automatic comment on behalf of bohwaz@github.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=94ec262fca2e832ab2e1c4f03bc68cbda6aa42ae
Log: Fix #77051: Issue with re-binding on SQLite3
 [2018-11-29 01:21 UTC] cmb@php.net
-Status: Assigned +Status: Closed
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Mon Sep 23 07:01:27 2019 UTC