SQLite Forum

Concurrent `PRAGMA {data,temp}_store_directory = ...` is very dangerous when docs say even malicious SQL should be safe
Login

Concurrent `PRAGMA {data,temp}_store_directory = ...` is very dangerous when docs say even malicious SQL should be safe

(1) By Zuurr (zuuurrr) on 2022-09-01 11:18:17 [source]

Hi, I'm the maintainer of rusqlite, one of the SQLite bindings libraries for Rust.

I recently came across the documentation for PRAGMA data_store_directory / PRAGMA temp_store_directory, which says things like:

Changing the data_store_directory setting writes to the sqlite3_data_directory global variable and that global variable is not protected by a mutex

Taking a look at the code in pragma.c, I see things like

if( !zRight ){
  returnSingleText(v, sqlite3_data_directory);
}else{
  // ... snip ...
  sqlite3_free(sqlite3_data_directory);
  if( zRight[0] ){
    sqlite3_data_directory = sqlite3_mprintf("%s", zRight); 
  }else{
    sqlite3_data_directory = 0;
  }
}

Unless I'm missing something (obviously possible), it seems very dangerous for this to have no synchronization (far worse than a something like a benign-in-practice data race); if run by on multiple threads at the same time (on unrelated connections) it could easy lead to a use after free, double free, or other very bad memory error.

While this is mentioned in the docs of each PRAGMA as something not to do, it seems to me that it goes against the documentation in https://www.sqlite.org/security.html, which says:

SQLite should never crash, overflow a buffer, leak memory, or exhibit any other harmful behavior, even when presented with maliciously malformed SQL inputs...

IMO this would not even take malicious code, just careless code that didn't read the docs close enough. Concretely, I would like the statement in the security docs to be accuate, and for there to be no way to cause that sort of extremely dangerous memory error by running a PRAGMA (or any other SQL statement).


This is problematic for me as I expose a safe API for running SQL statements, and that ofc includes PRAGMAs. APIs that are "safe" ideally should be insulated from this sort of thing (with obvious exceptions like hitting an unknown bug somewhere or using using an outdated versions, etc...)

So I'd like it if there's no UB from SQL statements, even if users use hammer on these PRAGMAs from connections across multiple threads in terrible ways.

(That said, I don't care about the danger for code that directly uses sqlite3_temp_directory, sqlite3_data_directory, sqlite_win32_set_directory, and so on. It's more reasonable for global variables and API calls to have UB than SQL statements -- I also don't expose these, and if I did it would be via an API which requires unsafe to use)

If this is WONTFIX, I guess it's not the end of the world for me to prevent this using an auth hook or something, but it would be worth mentioning on the security page.

(2) By anonymous on 2022-09-01 12:59:27 in reply to 1 [link] [source]

There's a configuration option to exclude those pragmas (and some other bits and pieces).

(3) By Dan Kennedy (dan) on 2022-09-01 17:42:11 in reply to 1 [link] [source]

for there to be no way to cause that sort of extremely dangerous memory error by running a PRAGMA

Quite reasonable, that.

The plan is to take two measures for 3.39.3:

  • Protect all accesses to these variables within the library using a global mutex, and

  • Disable these two pragmas when SQLITE_DB_CONFIG_DEFENSIVE is set.

https://www.sqlite.org/c3ref/c_dbconfig_defensive.html#sqlitedbconfigdefensive

This isn't quite perfect, because as you say applications may still access the global variables directly in a non-threadsafe manner, but there isn't much can be done about that now.

Dan.