fix GH-13952 sqlite pdo::quote null bytes #13953
Conversation
| @@ -224,8 +224,27 @@ static zend_string* sqlite_handle_quoter(pdo_dbh_t *dbh, const zend_string *unqu | |||
| { | |||
| char *quoted; | |||
| if (ZSTR_LEN(unquoted) > (INT_MAX - 3) / 2) { | |||
| php_error_docref(NULL, E_WARNING, "String is too long to be quoted"); | |||
There was a problem hiding this comment.
I don't think adding warnings is appropriate?
There was a problem hiding this comment.
I actually think a warning here is appropriate: when quote() fails, an error of some kind should be generated to explain why quote failed. Having no error generated is inappropriate IMO
There was a problem hiding this comment.
Right, I overlooked this was targeted to master
| return NULL; | ||
| } | ||
| // check if it contains a null byte | ||
| if (memchr(ZSTR_VAL(unquoted), '\0', ZSTR_LEN(unquoted))) { | ||
| if(((ZSTR_LEN(unquoted) * 2) + 3) > (INT_MAX - 3) / 2) { |
There was a problem hiding this comment.
The overflow check is incorrect, leading to a potential integer overflow resulting in a buffer overflow.
| // check if it contains a null byte | ||
| if (memchr(ZSTR_VAL(unquoted), '\0', ZSTR_LEN(unquoted))) { | ||
| if(((ZSTR_LEN(unquoted) * 2) + 3) > (INT_MAX - 3) / 2) { | ||
| php_error_docref(NULL, E_WARNING, "Binary string is too long to be quoted"); |
There was a problem hiding this comment.
Same comment of warnings here.
| php_error_docref(NULL, E_WARNING, "Binary string is too long to be quoted"); | ||
| return NULL; | ||
| } | ||
| zend_string *quoted_str = zend_string_alloc((ZSTR_LEN(unquoted) * 2) + 4, 0); // 4: x'hex'\x00 |
There was a problem hiding this comment.
Don't open-code arithmetic in allocation functions, use safe_alloc variants that can deal with overflow.
| } | ||
| zend_string *quoted_str = zend_string_alloc((ZSTR_LEN(unquoted) * 2) + 4, 0); // 4: x'hex'\x00 | ||
| // x'hex' | ||
| quoted_str->val[0] = 'x'; |
There was a problem hiding this comment.
Don't open-code ->val and ->len, use the ZSTR_VAL and ZSTR_LEN macros, which will be robust if the data structure ever changes.
| quoted_str->val[0] = 'x'; | ||
| quoted_str->val[1] = '\''; | ||
| for (size_t i = 0; i < ZSTR_LEN(unquoted); i++) { | ||
| sprintf(quoted_str->val + 2 + (i * 2), "%02x", (unsigned char)ZSTR_VAL(unquoted)[i]); |
There was a problem hiding this comment.
See #13952 (comment): I agree, this seems like the wrong approach.
| --FILE-- | ||
| <?php | ||
| $pdo = new PDO("sqlite::memory:", null, null, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]); | ||
| var_dump($pdo->query("SELECT " . $pdo->quote("foo\x00bar"))->fetch(PDO::FETCH_NUM)[0] === "foo\x00bar"); |
There was a problem hiding this comment.
We need more tests, and actually print maybe even what the quote output is.
|
@nielsdos is cooking up a better fix, closing. |
resolves #13952