Edit report at https://bugs.php.net/bug.php?id=77051&edit=1
ID: 77051
Updated by: stas@php.net
Reported by: php at bohwaz dot net
Summary: Issue with re-binding on SQLite3
Status: Assigned
-Type: Security
+Type: Bug
Package: SQLite related
Operating System: All
PHP Version: 5.6.38
Assigned To: cmb
Block user comment: N
Private report: Y
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