|
|
php.net | support | documentation | report a bug | advanced search | search howto | statistics | random bug | login |
[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 PatchesPull Requests
Pull requests:
HistoryAllCommentsChangesGit/SVN commits
|
|||||||||||||||||||||||||||
Copyright © 2001-2024 The PHP GroupAll rights reserved. |
Last updated: Sat Dec 21 15:01:29 2024 UTC |
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;> 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>