Fix #81742: open_basedir bypass in SQLite3 by using file URI

[cmb69]

A previous fix[1] was not sufficient to catch all potential file URIs, because the patch did not cater to URL encoding. Properly parsing and decoding the URI may yield a different result than the handling of SQLite3, so we play it safe, and reject any file URIs if open_basedir is configured.

[1] https://bugs.php.net/bug.php?id=77967

---

Obviously, this can break existing code using (maybe even hard-coded) file URIs. As such, we should consider to postpone this fix to PHP-8.2.

cc @bohwaz

[bohwaz]

Thanks for the notification.

I had trouble understanding how this can affect the sqlite3 extension as it does not support URIs by default. It is in PDO SQLite (see a8dd009 ), but not in SQLite3. A SQLITE_OPEN_URI is not set in the opening flags of the database, and there is no SQLITE3_OPEN_URI constant defined: there is no way to have URIs enabled.

So doing new SQLite3('file:/tmp/a') results in an error.

But it seems that if you compile SQLite3 with SQLITE_USE_URI then URIs are then allowed inside SQL statements by default.

So yes that fix is good, and is consistent with that exists in PDO.

I think we should merge this fix in the current versions, and not wait for 8.2, as:

• URI support in SQLite3 extension was never documented in the first place
• PDO SQLite extension also states that URIs and open_basedir cannot work together
• this can lead to security issues (even though open_basedir is not a security feature, I know, it's still a useful measure against attacks)
• this feature wasn't documented, and most likely this is not used widely

[bohwaz]

Maybe we should also enable SQLITE_OPEN_URI in SQLite3::open by default as well like PDO, for 8.2 or 8.3, to make it consistent? I can do a PR for that.

[cmb69]

Thanks for the analysis!

Maybe we should also enable SQLITE_OPEN_URI in SQLite3::open by default as well like PDO, for 8.2 or 8.3, to make it consistent? I can do a PR for that.

Yeah, that appears to be reasonable. That PR should probably target master, to have a clean cut (especially for documentation purposes).

[arnaud-lb]

It's unfortunate that sqlite doesn't make it possible to use a white list approach here. The check seems to be consistent with how sqlite parses the filename, but we may be missing something, and this is not future proof.

I'm wondering if we could use a custom VFS to hook at a lower level.

Otherwise it looks reasonable.

[cmb69]

I'm wondering if we could use a custom VFS to hook at a lower level.

Oh, I wasn't aware of these. That might be an option, but on the other hand, it is apparently possible to specify the desired VFS as URI parameter, so that may end up in a mess.