Repository navigation
fix(native): bound strfilter output to the searched string bytes - #1489
sahvx655-wq wants to merge 1 commit into
Conversation
|
this is original code from the sqlite repo, has this been checked against what they propose in the extensions ? |
|
Yes, I looked. This file isn't part of the SQLite source tree itself, it's Liam Healy's contributed extension-functions.c from sqlite.org/contrib, and that upload hasn't been revised since the 2009/2010 entries in its History header. SQLite's own tree (including ext/misc) has no strfilter at all, so there's no upstream variant of this function to align with. The strfilterFunc loop here before my change is the contrib code as imported (only #1454 has touched this file since, and that was the reader macro), and it has the same So the fix stays a local deviation, same as #1454 was. I kept it to the loop body for that reason, so any future resync against the contrib file is a trivial three line conflict. For well formed UTF-8 the output is byte for byte what upstream produces, because matching code points share one encoding. It only differs on invalid input, where upstream writes past the allocation and this returns the searched string's own bytes. |
|
thanks for the clarification. I am wondering why the test is passing even though we haven't rebuild the binaries. Would you know ? |
strfilter() in the bundled extension-functions.c sizes its output buffer from the byte length of the searched string (sqlite3_malloc(strlen(zi1)+1)), yet for every character it keeps it copied the bytes of the matching character taken from the filter argument. Those two strings only agree in length for well formed input, where equal code points share one encoding. A one byte searched character that decodes to the same code point as a wider filter character then makes the per match copy write past the allocation. The invalid byte 0xff is the easy trigger: the reader maps it to U+FFFD, which matches a genuine three byte U+FFFD in the filter, so
select strfilter(cast(x'ff' as text), char(65533))copies three bytes into a two byte buffer. An AddressSanitizer build of the amalgamation flags a heap write past the malloc'd region at the strncpy on that line. strfilter is reachable from any SQL this driver runs, so a string value an attacker controls reaches the overflow; left unfixed it is heap corruption rather than a clean crash.The kept character is by definition the searched character, so the fix copies its own bytes (z1 up to the next character boundary) instead of the filter's. Those bytes always sit inside the strlen(zi1) allocation, which removes the overflow, and for well formed UTF-8 the output is byte for byte identical to before because the matched code points share an encoding. Sizing and copy now key off the one string, so the bound holds in the function itself rather than relying on the two arguments happening to encode alike. I reused the existing z22 cursor to keep the change inside the loop body, and checked the sibling allocators in this file (replicate, padl/padr/padc, leftstr, rightstr, reverse, proper, replace): each copies from the same string it measured, so strfilter was the only site with this mismatch.
Confirmed with an AddressSanitizer harness around the file that aborts on the SQL above before the change and runs clean after, with the well formed cases unchanged. The regression test in ExtensionTest follows the shape PR #1454 used for the sibling reader fix in this file.