Skip to content

fix(native): bound strfilter output to the searched string bytes - #1489

Open
sahvx655-wq wants to merge 1 commit into
xerial:masterfrom
sahvx655-wq:strfilter-output-overflow
Open

sahvx655-wq wants to merge 1 commit into
xerial:masterfrom
sahvx655-wq:strfilter-output-overflow

Conversation

@sahvx655-wq

Copy link
Copy Markdown
Contributor

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.

@gotson

gotson commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

this is original code from the sqlite repo, has this been checked against what they propose in the extensions ?

@sahvx655-wq

Copy link
Copy Markdown
Contributor Author

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 strncpy(zot, z21, z22-z21) copy from the filter string against a strlen(zi1)+1 buffer, so upstream carries the same overflow. I did try to re-pull the contrib download today to diff it again, but sqlite.org was returning its server overload page, so that comparison is against the in-repo copy.

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.

@gotson

gotson commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator

thanks for the clarification.

I am wondering why the test is passing even though we haven't rebuild the binaries. Would you know ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants