From ca8ee116a2e01a41edd1a85610f3758947d71eb4 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Sat, 8 Aug 2026 19:56:16 -0400 Subject: [PATCH 1/2] sqlite: reject connection access from authorizer callbacks SQLite requires that an authorizer callback not modify the connection that invoked it, and counts sqlite3_prepare_v2() and sqlite3_step() as modifications. node:sqlite let the callback call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync. Track authorizer depth on DatabaseSync with an RAII guard around the callback and throw ERR_INVALID_STATE from the affected entry points while it is on the stack. Covering every authorizer invocation, including the re-prepare that SQLite can run during sqlite3_step(), exposed a second and distinct hazard: reentering a statement that is currently being stepped is a use-after-free rather than a contract violation, since finalizing it frees the virtual machine under sqlite3_step() and re-running it resets that machine mid-execution. Any callback SQLite invokes during execution can reach it, so a user-defined function is enough. Track the statements currently being stepped and reject reentry into only those, which leaves a user-defined function free to prepare, run, and finalize its own helper statements. Signed-off-by: Trevor Burnham Fixes: https://github.com/nodejs/node/issues/63207 Assisted-by: claude:opus-5 --- doc/api/sqlite.md | 30 +++ src/node_sqlite.cc | 106 ++++++++- src/node_sqlite.h | 49 +++++ test/parallel/test-sqlite-authz.js | 286 ++++++++++++++++++++++++- test/parallel/test-sqlite-udf-close.js | 197 +++++++++++++++++ 5 files changed, 661 insertions(+), 7 deletions(-) diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md index 97eac5c5a139..59d6a0d4654b 100644 --- a/doc/api/sqlite.md +++ b/doc/api/sqlite.md @@ -442,6 +442,11 @@ wrapper around [`sqlite3_create_function_v2()`][]. * `callback` {Function|null} The authorizer function to set, or `null` to @@ -467,6 +472,31 @@ The callback must return one of the following constants: * `SQLITE_DENY` - Deny the operation (causes an error). * `SQLITE_IGNORE` - Ignore the operation (silently skip). +SQLite requires that the authorizer callback not modify the database connection +that invoked it, which includes preparing and stepping statements. Methods that +would do so throw an error with code `ERR_INVALID_STATE` while the callback is +on the stack, including `database.prepare()`, `database.exec()`, the execution +methods of that connection's statements, iterators, and tag stores, and +`database.setAuthorizer()` itself. Other connections remain usable. + +The callback can also be invoked from within `statement.run()`, +`statement.get()`, and similar methods, because SQLite may re-prepare a +statement during execution after a schema change. + +Separately, a statement that is currently being executed cannot be reentered. +Calling `statement.close()` on it would free the virtual machine that is +running, and re-running it through `statement.run()`, `statement.get()`, +`statement.all()`, `statement.iterate()`, `iterator.next()`, +`iterator.return()`, or the equivalent tag store methods would reset that +virtual machine mid-execution. All of these throw an `ERR_INVALID_STATE` error +instead. This applies to any callback SQLite invokes during execution, such as a +user-defined function. Other statements on the connection remain usable. + +Operations that touch no SQLite state stay available from the callback: +`sqlTagStore.clear()`, which only drops cached statements, and `next()` and +`return()` on an already-drained iterator, which keep returning +`{ done: true }`. + ```cjs const { DatabaseSync, constants } = require('node:sqlite'); const db = new DatabaseSync(':memory:'); diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 9c80d18cdaab..2c817c87035b 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -107,6 +107,27 @@ inline MaybeLocal Utf8StringMaybeOneByte(Isolate* isolate, } \ } while (0) +// SQLite requires that an authorizer callback not modify the connection that +// invoked it. Preparing and stepping statements both count as modifying it. +// See https://www.sqlite.org/c3ref/set_authorizer.html. +#define THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db) \ + THROW_AND_RETURN_ON_BAD_STATE( \ + (env), \ + (db)->IsInAuthorizerCallback(), \ + "database cannot be accessed from an authorizer callback") + +// A statement's virtual machine cannot be reentered while sqlite3_step() is +// running it. Finalizing it frees the VM outright, and re-running it resets the +// VM mid-execution; both are use-after-free rather than merely a contract +// violation. Callbacks that SQLite invokes during execution are therefore +// barred from reaching the statement being stepped, though other statements on +// the connection stay usable. +#define THROW_AND_RETURN_IF_STEPPING(env, stmt) \ + THROW_AND_RETURN_ON_BAD_STATE( \ + (env), \ + (stmt)->db_->IsSteppingStatement((stmt)->statement_.get()), \ + "statement is already being executed") + #define SQLITE_VALUE_TO_JS(from, isolate, use_big_int_args, result, ...) \ do { \ switch (sqlite3_##from##_type(__VA_ARGS__)) { \ @@ -836,6 +857,12 @@ Intercepted DatabaseSyncLimits::LimitsSetter( return Intercepted::kYes; } + if (limits->database_->IsInAuthorizerCallback()) { + THROW_ERR_INVALID_STATE( + env, "database cannot be accessed from an authorizer callback"); + return Intercepted::kYes; + } + if (!value->IsNumber()) { THROW_ERR_INVALID_ARG_TYPE( isolate, "Limit value must be a non-negative integer or Infinity."); @@ -1092,6 +1119,7 @@ void DatabaseSync::CreateTagStore(const FunctionCallbackInfo& args) { THROW_ERR_INVALID_STATE(env, "database is not open"); return; } + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); int capacity = 1000; if (args.Length() > 0 && !args[0]->IsUndefined()) { if (!args[0]->IsNumber()) { @@ -1494,6 +1522,7 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); if (!args[0]->IsString()) { THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -1631,6 +1660,7 @@ void DatabaseSync::Exec(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); if (!args[0]->IsString()) { THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -1655,6 +1685,7 @@ void DatabaseSync::CustomFunction(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); if (!args[0]->IsString()) { THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -1828,6 +1859,7 @@ void DatabaseSync::Serialize(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); std::string db_name = "main"; if (!args[0]->IsUndefined()) { @@ -1962,6 +1994,7 @@ void DatabaseSync::AggregateFunction(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); Utf8Value name(env->isolate(), args[0].As()); Local options = args[1].As(); Local start_v; @@ -2173,6 +2206,7 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { DatabaseSync* db; ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); sqlite3_session* pSession; int r = sqlite3session_create(db->connection_, db_name.c_str(), &pSession); @@ -2342,6 +2376,7 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); if (!args[0]->IsUint8Array()) { THROW_ERR_INVALID_ARG_TYPE( @@ -2477,6 +2512,7 @@ void DatabaseSync::EnableLoadExtension( ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); Isolate* isolate = env->isolate(); if (!args[0]->IsBoolean()) { @@ -2505,6 +2541,7 @@ void DatabaseSync::EnableDefensive(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); Isolate* isolate = env->isolate(); if (!args[0]->IsBoolean()) { @@ -2530,6 +2567,7 @@ void DatabaseSync::LoadExtension(const FunctionCallbackInfo& args) { env, !db->allow_load_extension_, "extension loading is not allowed"); THROW_AND_RETURN_ON_BAD_STATE( env, !db->enable_load_extension_, "extension loading is not allowed"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); if (!args[0]->IsString()) { THROW_ERR_INVALID_ARG_TYPE(env->isolate(), @@ -2558,6 +2596,7 @@ void DatabaseSync::SetAuthorizer(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&db, args.This()); Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db); Isolate* isolate = env->isolate(); @@ -2594,6 +2633,7 @@ int DatabaseSync::AuthorizerCallback(void* user_data, const char* param4) { DatabaseSync* db = static_cast(user_data); CallbackDepthGuard guard(db); + AuthorizerDepthGuard authorizer_guard(db); Environment* env = db->env(); Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); @@ -2706,12 +2746,20 @@ void StatementSync::Close(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_IF_STEPPING(env, stmt); stmt->Close(); } void StatementSync::Dispose(const FunctionCallbackInfo& args) { StatementSync* stmt; ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This()); + Environment* env = Environment::GetCurrent(args); + // Disposal is idempotent, so an already-finalized statement is a no-op even + // inside a callback. + if (stmt->IsFinalized()) { + return; + } + THROW_AND_RETURN_IF_STEPPING(env, stmt); stmt->Close(); } @@ -2993,6 +3041,7 @@ MaybeLocal StatementExecutionHelper::All(Environment* env, LocalVector row_values(isolate); LocalVector row_keys(isolate); + SteppingStatementGuard stepping(db, stmt); while ((r = sqlite3_step(stmt)) == SQLITE_ROW) { if (num_cols == 0) { num_cols = sqlite3_column_count(stmt); @@ -3035,6 +3084,9 @@ MaybeLocal StatementExecutionHelper::Run(Environment* env, bool use_big_ints) { Isolate* isolate = env->isolate(); EscapableHandleScope scope(isolate); + // Declared before the reset below so that it outlives it: sqlite3_reset() + // can run JavaScript through an aggregate's xFinal. + SteppingStatementGuard stepping(db, stmt); bool needs_reset = true; auto reset = OnScopeLeave([&]() { if (needs_reset) sqlite3_reset(stmt); @@ -3122,6 +3174,9 @@ MaybeLocal StatementExecutionHelper::Get(Environment* env, bool use_big_ints) { Isolate* isolate = env->isolate(); EscapableHandleScope scope(isolate); + // Declared before the reset below so that it outlives it: sqlite3_reset() + // can run JavaScript through an aggregate's xFinal. + SteppingStatementGuard stepping(db, stmt); bool needs_reset = true; auto reset = OnScopeLeave([&]() { if (needs_reset) sqlite3_reset(stmt); @@ -3178,6 +3233,8 @@ void StatementSync::All(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get()); + THROW_AND_RETURN_IF_STEPPING(env, stmt); Isolate* isolate = env->isolate(); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void()); @@ -3209,6 +3266,8 @@ void StatementSync::Iterate(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get()); + THROW_AND_RETURN_IF_STEPPING(env, stmt); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3232,6 +3291,8 @@ void StatementSync::Get(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get()); + THROW_AND_RETURN_IF_STEPPING(env, stmt); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3256,6 +3317,8 @@ void StatementSync::Run(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get()); + THROW_AND_RETURN_IF_STEPPING(env, stmt); int r = stmt->ResetStatement(); CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void()); @@ -3538,6 +3601,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get()); BaseObjectPtr stmt = PrepareStatement(args); @@ -3545,6 +3609,8 @@ void SQLTagStore::Run(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_IF_STEPPING(env, stmt.get()); + if (!ResetAndBindStatement(env, stmt.get(), args)) { return; } @@ -3564,6 +3630,7 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get()); BaseObjectPtr stmt = PrepareStatement(args); @@ -3571,6 +3638,8 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_IF_STEPPING(env, stmt.get()); + if (!ResetAndBindStatement(env, stmt.get(), args)) { return; } @@ -3592,6 +3661,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get()); BaseObjectPtr stmt = PrepareStatement(args); @@ -3599,6 +3669,8 @@ void SQLTagStore::Get(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_IF_STEPPING(env, stmt.get()); + if (!ResetAndBindStatement(env, stmt.get(), args)) { return; } @@ -3621,6 +3693,7 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { THROW_AND_RETURN_ON_BAD_STATE( env, !session->database_->IsOpen(), "database is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get()); BaseObjectPtr stmt = PrepareStatement(args); @@ -3628,6 +3701,8 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_IF_STEPPING(env, stmt.get()); + if (!ResetAndBindStatement(env, stmt.get(), args)) { return; } @@ -3653,6 +3728,10 @@ void SQLTagStore::All(const FunctionCallbackInfo& args) { void SQLTagStore::Clear(const FunctionCallbackInfo& args) { SQLTagStore* store; ASSIGN_OR_RETURN_UNWRAP(&store, args.This()); + // Clearing the cache drops strong references to the cached statements but + // never finalizes one synchronously, so it touches no SQLite state and stays + // available from a callback. Invalidating the cache after a schema change is + // a legitimate use of an authorizer. store->sql_tags_.Clear(); } @@ -3860,6 +3939,8 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { auto iter_template = getLazyIterTemplate(env); + // A drained iterator touches no SQLite state, so it stays usable from a + // callback and is checked before the guards below. if (iter->done_) { MaybeLocal values[]{ Boolean::New(isolate, true), @@ -3873,11 +3954,18 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo& args) { return; } + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, iter->stmt_->db_.get()); + THROW_AND_RETURN_IF_STEPPING(env, iter->stmt_.get()); + THROW_AND_RETURN_ON_BAD_STATE( env, iter->statement_reset_generation_ != iter->stmt_->reset_generation_, "iterator was invalidated"); + // sqlite3_reset() can run JavaScript through an aggregate's xFinal, so it + // stays inside the guard. + SteppingStatementGuard stepping(iter->stmt_->db_.get(), + iter->stmt_->statement_.get()); int r = sqlite3_step(iter->stmt_->statement_.get()); if (r != SQLITE_ROW) { CHECK_ERROR_OR_THROW( @@ -3938,11 +4026,18 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo& args) { env, iter->stmt_->IsFinalized(), "statement has been finalized"); Isolate* isolate = env->isolate(); - // Unlike Next(), the reset result is intentionally ignored here: Return() - // is invoked by the language during abrupt completion (e.g. a `throw` - // inside a `for...of` body), and throwing on a deferred SQLite error - // would discard the caller's already-pending exception. - sqlite3_reset(iter->stmt_->statement_.get()); + if (!iter->done_) { + // A language-invoked return() cannot be reached mid-step, since the loop + // body only runs after next() has returned, so these guards only reject an + // explicit call from inside a callback. + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, iter->stmt_->db_.get()); + THROW_AND_RETURN_IF_STEPPING(env, iter->stmt_.get()); + // Unlike Next(), the reset result is intentionally ignored here: Return() + // is invoked by the language during abrupt completion (e.g. a `throw` + // inside a `for...of` body), and throwing on a deferred SQLite error + // would discard the caller's already-pending exception. + sqlite3_reset(iter->stmt_->statement_.get()); + } iter->done_ = true; auto iter_template = getLazyIterTemplate(env); @@ -4018,6 +4113,7 @@ void Session::Changeset(const FunctionCallbackInfo& args) { env, !session->database_->IsOpen(), "database is not open"); THROW_AND_RETURN_ON_BAD_STATE( env, session->session_ == nullptr, "session is not open"); + THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get()); int nChangeset; void* pChangeset; diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 17025a528622..ba22174f37d1 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -9,6 +9,7 @@ #include "sqlite3.h" #include "util.h" +#include #include #include #include @@ -239,6 +240,26 @@ class DatabaseSync : public BaseObject { void DecrementCallbackDepth() { --callback_depth_; } bool IsInCallback() const { return callback_depth_ > 0; } + // SQLite forbids an authorizer callback from doing anything that modifies + // the database connection that invoked it, which includes preparing and + // stepping statements. See https://www.sqlite.org/c3ref/set_authorizer.html. + void IncrementAuthorizerDepth() { ++authorizer_depth_; } + void DecrementAuthorizerDepth() { --authorizer_depth_; } + bool IsInAuthorizerCallback() const { return authorizer_depth_ > 0; } + + // Finalizing a statement frees its virtual machine, so a callback that + // SQLite invokes from inside sqlite3_step() must not finalize the statement + // being stepped. Other statements on the connection are safe to finalize. + void PushSteppingStatement(sqlite3_stmt* stmt) { + stepping_statements_.push_back(stmt); + } + void PopSteppingStatement() { stepping_statements_.pop_back(); } + bool IsSteppingStatement(sqlite3_stmt* stmt) const { + return std::find(stepping_statements_.begin(), + stepping_statements_.end(), + stmt) != stepping_statements_.end(); + } + SET_MEMORY_INFO_NAME(DatabaseSync) SET_SELF_SIZE(DatabaseSync) @@ -253,6 +274,8 @@ class DatabaseSync : public BaseObject { sqlite3* connection_; bool ignore_next_sqlite_error_; int callback_depth_ = 0; + int authorizer_depth_ = 0; + std::vector stepping_statements_; std::set backups_; std::unordered_set sessions_; @@ -432,6 +455,32 @@ class CallbackDepthGuard { DatabaseSync* db_; }; +class SteppingStatementGuard { + public: + SteppingStatementGuard(DatabaseSync* db, sqlite3_stmt* stmt) : db_(db) { + db_->PushSteppingStatement(stmt); + } + ~SteppingStatementGuard() { db_->PopSteppingStatement(); } + SteppingStatementGuard(const SteppingStatementGuard&) = delete; + SteppingStatementGuard& operator=(const SteppingStatementGuard&) = delete; + + private: + DatabaseSync* db_; +}; + +class AuthorizerDepthGuard { + public: + explicit AuthorizerDepthGuard(DatabaseSync* db) : db_(db) { + db_->IncrementAuthorizerDepth(); + } + ~AuthorizerDepthGuard() { db_->DecrementAuthorizerDepth(); } + AuthorizerDepthGuard(const AuthorizerDepthGuard&) = delete; + AuthorizerDepthGuard& operator=(const AuthorizerDepthGuard&) = delete; + + private: + DatabaseSync* db_; +}; + class UserDefinedFunction { public: UserDefinedFunction(Environment* env, diff --git a/test/parallel/test-sqlite-authz.js b/test/parallel/test-sqlite-authz.js index 69c075a57e2e..5de6eeb09874 100644 --- a/test/parallel/test-sqlite-authz.js +++ b/test/parallel/test-sqlite-authz.js @@ -1,7 +1,7 @@ 'use strict'; -const { skipIfSQLiteMissing } = require('../common'); -skipIfSQLiteMissing(); +const common = require('../common'); +common.skipIfSQLiteMissing(); const assert = require('node:assert'); const { DatabaseSync, constants } = require('node:sqlite'); @@ -288,3 +288,285 @@ suite('DatabaseSync.prototype.setAuthorizer()', () => { }); }); }); + +// SQLite forbids an authorizer callback from modifying the connection that +// invoked it, which includes preparing and stepping statements. +// See https://www.sqlite.org/c3ref/set_authorizer.html. +suite('authorizer callback reentrancy', () => { + const expectedError = 'ERR_INVALID_STATE: database cannot be accessed ' + + 'from an authorizer callback'; + const steppingError = + 'ERR_INVALID_STATE: statement is already being executed'; + + // Calls each of `cases` from inside an authorizer callback, and returns a + // `name -> outcome` map of what each one threw. + const runInAuthorizer = (db, cases) => { + const outcomes = {}; + for (const [name, fn] of Object.entries(cases)) { + let ran = false; + db.setAuthorizer(() => { + if (!ran) { + ran = true; + try { + fn(); + outcomes[name] = 'did not throw'; + } catch (err) { + outcomes[name] = `${err.code}: ${err.message}`; + } + } + return constants.SQLITE_OK; + }); + db.exec('SELECT 1'); + db.setAuthorizer(null); + if (!ran) { + outcomes[name] = 'authorizer callback did not run'; + } + } + return outcomes; + }; + + // Builds the expected `name -> outcome` map for the given case names. + const allRejected = (cases) => Object.fromEntries( + Object.keys(cases).map((name) => [name, expectedError]), + ); + + it('rejects database methods', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + const cases = { + prepare: () => db.prepare('SELECT 1'), + exec: () => db.exec('SELECT 1'), + setAuthorizer: () => db.setAuthorizer(null), + createSession: () => db.createSession(), + applyChangeset: () => db.applyChangeset(new Uint8Array([1])), + createTagStore: () => db.createTagStore(), + serialize: () => db.serialize(), + function: () => db.function('noop', () => 1), + aggregate: () => db.aggregate('agg', { start: 0, step: (acc) => acc }), + enableLoadExtension: () => db.enableLoadExtension(false), + limits: () => { db.limits.length = 100; }, + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); + }); + + // close() and deserialize() tear down the connection, so the pre-existing + // callback depth guard already rejects them with its own message. + it('rejects methods the callback depth guard already covers', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + const snapshot = db.serialize(); + const cases = { + close: () => db.close(), + deserialize: () => db.deserialize(snapshot), + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), { + close: 'ERR_INVALID_STATE: database cannot be closed while in a callback', + deserialize: 'ERR_INVALID_STATE: database cannot be deserialized ' + + 'while in a callback', + }); + }); + + it('rejects statement methods', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1)'); + const stmt = db.prepare('SELECT x FROM t'); + const cases = { + run: () => stmt.run(), + get: () => stmt.get(), + all: () => stmt.all(), + iterate: () => stmt.iterate(), + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); + }); + + // Only the statement being stepped is unsafe to finalize. Other statements + // on the connection have their own virtual machines, so finalizing them from + // a callback is allowed. + it('allows finalizing a statement that is not being executed', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1)'); + const closeStmt = db.prepare('SELECT x FROM t'); + const disposeStmt = db.prepare('SELECT x FROM t'); + const cases = { + close: () => closeStmt.close(), + dispose: () => disposeStmt[Symbol.dispose](), + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), { + close: 'did not throw', + dispose: 'did not throw', + }); + }); + + // Disposal is idempotent, so a statement that is already finalized must stay + // a no-op even inside a callback. Throwing here would turn a `using` scope's + // real exception into a SuppressedError. + it('allows disposing an already-finalized statement', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + const stmt = db.prepare('SELECT x FROM t'); + stmt.close(); + const cases = { dispose: () => stmt[Symbol.dispose]() }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), { + dispose: 'did not throw', + }); + }); + + it('rejects session changeset methods', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER PRIMARY KEY, y TEXT)'); + const session = db.createSession({ table: 't' }); + db.exec("INSERT INTO t VALUES (1, 'a')"); + const cases = { + changeset: () => session.changeset(), + patchset: () => session.patchset(), + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); + }); + + // A statement being re-prepared inside sqlite3_step() is the case that + // actually crashes, because that statement's VM is mid-execution. + it('rejects finalizing the statement being stepped', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1)'); + const stmt = db.prepare('SELECT x FROM t'); + stmt.get(); + db.exec('ALTER TABLE t ADD COLUMN y INTEGER'); + + let outcome = 'authorizer callback did not run'; + let ran = false; + db.setAuthorizer(() => { + if (!ran) { + ran = true; + try { + stmt.close(); + outcome = 'did not throw'; + } catch (err) { + outcome = `${err.code}: ${err.message}`; + } + } + return constants.SQLITE_OK; + }); + + stmt.get(); + + assert.strictEqual(outcome, steppingError); + }); + + it('rejects iterator methods', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1), (2)'); + const iter = db.prepare('SELECT x FROM t').iterate(); + const cases = { + next: () => iter.next(), + return: () => iter.return(), + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); + iter.return(); + }); + + // A drained iterator holds no SQLite state, so next() and return() stay + // available and remain idempotent inside a callback. + it('allows iterator methods on a drained iterator', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1)'); + const iter = db.prepare('SELECT x FROM t').iterate(); + for (const row of iter) { + assert.ok(row); + } + const done = {}; + const cases = { + next: () => { done.next = iter.next().done; }, + return: () => { done.return = iter.return().done; }, + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), { + next: 'did not throw', + return: 'did not throw', + }); + assert.deepStrictEqual(done, { next: true, return: true }); + }); + + it('rejects tag store methods', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + const sql = db.createTagStore(10); + const cases = { + run: () => sql.run`SELECT 1`, + get: () => sql.get`SELECT 1`, + all: () => sql.all`SELECT 1`, + iterate: () => sql.iterate`SELECT 1`, + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); + }); + + // clear() only drops cached statements, so invalidating the cache after a + // schema change is allowed from the callback. + it('allows clearing a tag store', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1)'); + const sql = db.createTagStore(10); + assert.strictEqual(sql.all`SELECT x FROM t`.length, 1); + assert.strictEqual(sql.size, 1); + const cases = { clear: () => sql.clear() }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), { + clear: 'did not throw', + }); + assert.strictEqual(sql.size, 0); + }); + + // A statement may be re-prepared during sqlite3_step() after a schema + // change, which invokes the authorizer without an explicit prepare() call. + it('rejects reentry when the authorizer runs during a re-prepare', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1)'); + const stmt = db.prepare('SELECT x FROM t'); + stmt.get(); + db.exec('ALTER TABLE t ADD COLUMN y INTEGER'); + + let outcome = 'authorizer callback did not run'; + let ran = false; + db.setAuthorizer(() => { + if (!ran) { + ran = true; + try { + db.prepare('SELECT 1'); + outcome = 'did not throw'; + } catch (err) { + outcome = `${err.code}: ${err.message}`; + } + } + return constants.SQLITE_OK; + }); + + stmt.get(); + + assert.strictEqual(outcome, expectedError); + }); + + it('allows access again after the authorizer returns', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + const cases = { prepare: () => db.prepare('SELECT 1') }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); + + db.setAuthorizer(() => constants.SQLITE_OK); + assert.deepStrictEqual(db.prepare('SELECT 1 AS v').get(), { __proto__: null, v: 1 }); + }); +}); diff --git a/test/parallel/test-sqlite-udf-close.js b/test/parallel/test-sqlite-udf-close.js index 86794029b457..3c077807cd92 100644 --- a/test/parallel/test-sqlite-udf-close.js +++ b/test/parallel/test-sqlite-udf-close.js @@ -36,4 +36,201 @@ for (const method of ['all', 'get', 'run', 'iterate']) { assert.strictEqual(db.isOpen, true); db.close(); }); + + // Finalizing the statement being stepped frees the virtual machine that + // sqlite3_step() is still running, so this must throw rather than crash. + test(`statement.close() from a UDF during statement.${method}()`, () => { + const db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE data (value INTEGER); + INSERT INTO data VALUES (1), (2), (3); + `); + + let statement; + db.function('close_stmt', (value) => { + statement.close(); + return value; + }); + + statement = db.prepare('SELECT close_stmt(value) FROM data'); + assert.throws(() => { + if (method === 'iterate') { + for (const row of statement.iterate()) { + assert.ok(row); + } + } else { + statement[method](); + } + }, { + code: 'ERR_INVALID_STATE', + message: 'statement is already being executed', + }); + + db.close(); + }); + + // Re-running the statement being stepped resets its virtual machine + // mid-execution, which is the same use-after-free as finalizing it. + for (const reentrant of ['run', 'get', 'all', 'iterate']) { + test(`statement.${reentrant}() from a UDF during ` + + `statement.${method}()`, () => { + const db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE data (value INTEGER, padding TEXT); + INSERT INTO data VALUES (1, '${'x'.repeat(400)}'), + (2, '${'y'.repeat(400)}'), + (3, '${'z'.repeat(400)}'); + `); + + let statement; + let thrown; + db.function('reenter', (value) => { + if (thrown === undefined) { + try { + statement[reentrant](); + thrown = null; + } catch (err) { + thrown = err; + } + } + return value; + }); + + statement = db.prepare('SELECT reenter(value), padding FROM data'); + if (method === 'iterate') { + for (const row of statement.iterate()) { + assert.ok(row); + } + } else { + statement[method](); + } + + assert.ok(thrown, `${reentrant}() was not rejected`); + assert.strictEqual(thrown.code, 'ERR_INVALID_STATE'); + assert.strictEqual(thrown.message, 'statement is already being executed'); + + db.close(); + }); + } + + // Tag store methods resolve to a cached statement, which may be the one + // currently being stepped. + test(`tag store reentry during statement.${method}()`, () => { + const db = new DatabaseSync(':memory:'); + const sql = db.createTagStore(10); + db.exec(` + CREATE TABLE data (value INTEGER, padding TEXT); + INSERT INTO data VALUES (1, '${'x'.repeat(400)}'), + (2, '${'y'.repeat(400)}'); + `); + + let thrown; + db.function('reenter_tag', (value) => { + if (thrown === undefined) { + try { + // The identical tagged literal resolves to the same cached + // statement that is mid-execution. + // eslint-disable-next-line no-unused-expressions + sql.run`SELECT reenter_tag(value), padding FROM data`; + thrown = null; + } catch (err) { + thrown = err; + } + } + return value; + }); + + if (method === 'iterate') { + for (const row of sql.iterate`SELECT reenter_tag(value), padding FROM data`) { + assert.ok(row); + } + } else { + // eslint-disable-next-line no-unused-expressions + sql[method]`SELECT reenter_tag(value), padding FROM data`; + } + + assert.ok(thrown, 'tag store reentry was not rejected'); + assert.strictEqual(thrown.code, 'ERR_INVALID_STATE'); + assert.strictEqual(thrown.message, 'statement is already being executed'); + + db.close(); + }); + + // A UDF may prepare and finalize its own helper statements. Only the + // statement being stepped is off limits. + test(`UDF finalizes its own statement during statement.${method}()`, () => { + const db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE data (value INTEGER); + INSERT INTO data VALUES (1), (2), (3); + CREATE TABLE lookup (key INTEGER, label TEXT); + INSERT INTO lookup VALUES (1, 'one'), (2, 'two'), (3, 'three'); + `); + + db.function('lookup_label', (value) => { + const helper = db.prepare('SELECT label FROM lookup WHERE key = ?'); + const label = helper.get(value).label; + helper.close(); + return label; + }); + + const statement = db.prepare('SELECT lookup_label(value) AS l FROM data'); + if (method === 'iterate') { + const labels = []; + for (const row of statement.iterate()) { + labels.push(row.l); + } + assert.deepStrictEqual(labels, ['one', 'two', 'three']); + } else if (method === 'all') { + assert.deepStrictEqual(statement.all().map((r) => r.l), + ['one', 'two', 'three']); + } else if (method === 'get') { + assert.strictEqual(statement.get().l, 'one'); + } else { + statement.run(); + } + + db.close(); + }); +} + +// iterator.return() resets the statement it is iterating, and next() steps it +// again, so both reach the virtual machine that is mid-execution. +for (const op of ['next', 'return']) { + test(`iterator.${op}() from a UDF during iteration`, () => { + const db = new DatabaseSync(':memory:'); + db.exec(` + CREATE TABLE data (value INTEGER, padding TEXT); + INSERT INTO data VALUES (1, '${'x'.repeat(400)}'), + (2, '${'y'.repeat(400)}'), + (3, '${'z'.repeat(400)}'); + `); + + let iterator; + let thrown; + db.function('reenter_iter', (value) => { + if (thrown === undefined && iterator !== undefined) { + try { + iterator[op](); + thrown = null; + } catch (err) { + thrown = err; + } + } + return value; + }); + + const statement = db.prepare( + 'SELECT reenter_iter(value) AS v, padding FROM data'); + iterator = statement.iterate(); + for (const row of iterator) { + assert.ok(row); + } + + assert.ok(thrown, `iterator.${op}() was not rejected`); + assert.strictEqual(thrown.code, 'ERR_INVALID_STATE'); + assert.strictEqual(thrown.message, 'statement is already being executed'); + + db.close(); + }); } From bd20c8f1c594c25767a59a3e59f7358ddd9e86de Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Wed, 12 Aug 2026 15:58:01 -0400 Subject: [PATCH 2/2] fixup! sqlite: reject connection access from authorizer callbacks Cover the guard paths the existing tests never reached: the authorizer guards on enableDefensive() and loadExtension(), the stepping guard on statement[Symbol.dispose](), and the stepping guards on the tag store's get(), all(), and iterate(). The tag store test looped over the four outer driver methods but always reentered through run(), so three of its four guards never fired. loadExtension() checks that extension loading is enabled before the authorizer guard, so its test opens the database with allowExtension. Signed-off-by: Trevor Burnham --- test/parallel/test-sqlite-authz.js | 44 +++++++++++++++ test/parallel/test-sqlite-udf-close.js | 76 ++++++++++++++------------ 2 files changed, 85 insertions(+), 35 deletions(-) diff --git a/test/parallel/test-sqlite-authz.js b/test/parallel/test-sqlite-authz.js index 5de6eeb09874..f6020ce9047e 100644 --- a/test/parallel/test-sqlite-authz.js +++ b/test/parallel/test-sqlite-authz.js @@ -344,12 +344,26 @@ suite('authorizer callback reentrancy', () => { function: () => db.function('noop', () => 1), aggregate: () => db.aggregate('agg', { start: 0, step: (acc) => acc }), enableLoadExtension: () => db.enableLoadExtension(false), + enableDefensive: () => db.enableDefensive(true), limits: () => { db.limits.length = 100; }, }; assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); }); + // loadExtension() checks that extension loading is enabled before reaching + // the authorizer guard, so it needs a database opened with allowExtension. + it('rejects loadExtension', () => { + const db = new DatabaseSync(':memory:', { allowExtension: true }); + db.enableLoadExtension(true); + db.exec('CREATE TABLE t (x INTEGER)'); + const cases = { + loadExtension: () => db.loadExtension('/nonexistent/extension'), + }; + + assert.deepStrictEqual(runInAuthorizer(db, cases), allRejected(cases)); + }); + // close() and deserialize() tear down the connection, so the pre-existing // callback depth guard already rejects them with its own message. it('rejects methods the callback depth guard already covers', () => { @@ -461,6 +475,36 @@ suite('authorizer callback reentrancy', () => { assert.strictEqual(outcome, steppingError); }); + // Unlike an already-finalized statement, disposing the one being stepped + // would free the running virtual machine, so it throws. + it('rejects disposing the statement being stepped', () => { + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + db.exec('INSERT INTO t VALUES (1)'); + const stmt = db.prepare('SELECT x FROM t'); + stmt.get(); + db.exec('ALTER TABLE t ADD COLUMN y INTEGER'); + + let outcome = 'authorizer callback did not run'; + let ran = false; + db.setAuthorizer(() => { + if (!ran) { + ran = true; + try { + stmt[Symbol.dispose](); + outcome = 'did not throw'; + } catch (err) { + outcome = `${err.code}: ${err.message}`; + } + } + return constants.SQLITE_OK; + }); + + stmt.get(); + + assert.strictEqual(outcome, steppingError); + }); + it('rejects iterator methods', () => { const db = new DatabaseSync(':memory:'); db.exec('CREATE TABLE t (x INTEGER)'); diff --git a/test/parallel/test-sqlite-udf-close.js b/test/parallel/test-sqlite-udf-close.js index 3c077807cd92..cb11e50a7f7a 100644 --- a/test/parallel/test-sqlite-udf-close.js +++ b/test/parallel/test-sqlite-udf-close.js @@ -114,47 +114,53 @@ for (const method of ['all', 'get', 'run', 'iterate']) { } // Tag store methods resolve to a cached statement, which may be the one - // currently being stepped. - test(`tag store reentry during statement.${method}()`, () => { - const db = new DatabaseSync(':memory:'); - const sql = db.createTagStore(10); - db.exec(` - CREATE TABLE data (value INTEGER, padding TEXT); - INSERT INTO data VALUES (1, '${'x'.repeat(400)}'), - (2, '${'y'.repeat(400)}'); - `); + // currently being stepped. Each reentrant method has its own guard, so all + // four are exercised. + for (const reentrant of ['run', 'get', 'all', 'iterate']) { + test(`tag store ${reentrant} reentry during statement.${method}()`, () => { + const db = new DatabaseSync(':memory:'); + const sql = db.createTagStore(10); + db.exec(` + CREATE TABLE data (value INTEGER, padding TEXT); + INSERT INTO data VALUES (1, '${'x'.repeat(400)}'), + (2, '${'y'.repeat(400)}'); + `); - let thrown; - db.function('reenter_tag', (value) => { - if (thrown === undefined) { - try { - // The identical tagged literal resolves to the same cached - // statement that is mid-execution. - // eslint-disable-next-line no-unused-expressions - sql.run`SELECT reenter_tag(value), padding FROM data`; - thrown = null; - } catch (err) { - thrown = err; + let thrown; + db.function('reenter_tag', (value) => { + if (thrown === undefined) { + try { + // The identical tagged literal resolves to the same cached + // statement that is mid-execution. + // All four reject at call time, iterate() included, so the + // result is never consumed. + // eslint-disable-next-line no-unused-expressions + sql[reentrant]`SELECT reenter_tag(value), padding FROM data`; + thrown = null; + } catch (err) { + thrown = err; + } } - } - return value; - }); + return value; + }); - if (method === 'iterate') { - for (const row of sql.iterate`SELECT reenter_tag(value), padding FROM data`) { - assert.ok(row); + if (method === 'iterate') { + for (const row of sql.iterate`SELECT reenter_tag(value), padding FROM data`) { + assert.ok(row); + } + } else { + // eslint-disable-next-line no-unused-expressions + sql[method]`SELECT reenter_tag(value), padding FROM data`; } - } else { - // eslint-disable-next-line no-unused-expressions - sql[method]`SELECT reenter_tag(value), padding FROM data`; - } - assert.ok(thrown, 'tag store reentry was not rejected'); - assert.strictEqual(thrown.code, 'ERR_INVALID_STATE'); - assert.strictEqual(thrown.message, 'statement is already being executed'); + assert.ok(thrown, `tag store ${reentrant} reentry was not rejected`); + assert.strictEqual(thrown.code, 'ERR_INVALID_STATE'); + assert.strictEqual(thrown.message, + 'statement is already being executed'); - db.close(); - }); + db.close(); + }); + } // A UDF may prepare and finalize its own helper statements. Only the // statement being stepped is off limits.