Skip to content

fix GH-13952 sqlite pdo::quote null bytes #13953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

fix GH-13952 sqlite pdo::quote null bytes #13953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfd329c
fix GH-13952 sqlite pdo::quote null bytes
divinity76 Apr 12, 2024
8cbb907
forgot the test file
divinity76 Apr 12, 2024
de48896
update test
divinity76 Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions ext/pdo_sqlite/sqlite_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think adding warnings is appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overflow check is incorrect, leading to a potential integer overflow resulting in a buffer overflow.

php_error_docref(NULL, E_WARNING, "Binary string is too long to be quoted");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment of warnings here.

return NULL;
}
zend_string *quoted_str = zend_string_alloc((ZSTR_LEN(unquoted) * 2) + 4, 0); // 4: x'hex'\x00
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't open-code arithmetic in allocation functions, use safe_alloc variants that can deal with overflow.

// x'hex'
quoted_str->val[0] = 'x';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[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]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #13952 (comment): I agree, this seems like the wrong approach.

}
quoted_str->val[2 + (ZSTR_LEN(unquoted) * 2)] = '\'';
quoted_str->val[3 + (ZSTR_LEN(unquoted) * 2)] = '\0';
quoted_str->len = (ZSTR_LEN(unquoted) * 2) + 3;
return quoted_str;
}
quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3);
/* TODO use %Q format? */
sqlite3_snprintf(2*ZSTR_LEN(unquoted) + 3, quoted, "'%q'", ZSTR_VAL(unquoted));
Expand Down
3 changes: 2 additions & 1 deletion ext/pdo_sqlite/tests/bug81740.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ $pdo = new PDO("sqlite::memory:");
$string = str_repeat("a", 0x80000000);
var_dump($pdo->quote($string));
?>
--EXPECT--
--EXPECTF--
Warning: PDO::quote(): String is too long to be quoted in %s on line %d
bool(false)
12 changes: 12 additions & 0 deletions ext/pdo_sqlite/tests/bug_gh13952_quote_null.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
--TEST--
Bug #GH-13952 (sqlite PDO::quote silently corrupts strings with null bytes)
--EXTENSIONS--
pdo
pdo_sqlite
--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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more tests, and actually print maybe even what the quote output is.

?>
--EXPECT--
bool(true)
Loading