Bug #77051 [Com]: Issue with re-binding on SQLite3

Edit report at https://bugs.php.net/bug.php?id=77051&edit=1 ID: 77051 Comment by: php at bohwaz dot net Reported by: php at bohwaz dot net Summary: Issue with re-binding on SQLite3 Status: Assigned Type: Bug Package: SQLite related Operating System: All PHP Version: 5.6.38 Assigned To: cmb Block user comment: N Private report: N New Comment: 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. Previous Comments: ------------------------------------------------------------------------ [2018-11-12 16:22:17] 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-12 16:00:15] 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 15:25:29] 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 13:24:56] 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-11 18:15:15] stas@php.net Doesn't look like security issue though - it requires specific targeted developer code to trigger. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=77051 -- Edit this bug report at https://bugs.php.net/bug.php?id=77051&edit=1