Skip to content

sqlite: reject connection access from authorizer callbacks - #65156

Open
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite-authorizer-reentry
Open

sqlite: reject connection access from authorizer callbacks#65156
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite-authorizer-reentry

Conversation

@TrevorBurnham

@TrevorBurnham TrevorBurnham commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Fixes: #63207

Per sqlite3_set_authorizer(), an authorizer callback must not modify the connection that invoked it, and sqlite3_prepare_v2() and sqlite3_step() both count. node:sqlite allowed the callback to call prepare(), exec(), the statement execution methods, and other connection-mutating APIs on the same DatabaseSync.

Authorizer reentrancy. 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. Depth is per-connection, so a different DatabaseSync stays usable.

Guarded: prepare, exec, serialize, setAuthorizer, createSession, applyChangeset, createTagStore, function, aggregate, enableLoadExtension, enableDefensive, loadExtension, the limits setter; stmt.run/get/all/iterate; iter.next/return; sqlTagStore.run/get/all/iterate; session.changeset/patchset. db.close() and db.deserialize() keep their existing callback-depth messages.

The guard covers every authorizer invocation, not just those from an explicit prepare(): SQLite may re-prepare during sqlite3_step() after a schema change, and serialize() and the session changeset methods prepare internally. Reentry through changeset() never terminated — it recursed until the process died, uncatchable from JavaScript.

Statement reentry. Covering the re-prepare path surfaced a memory-safety bug rather than a contract violation: a statement that is currently being stepped cannot be reentered. Finalizing it frees the virtual machine sqlite3_step() is running, and re-running it resets that virtual machine mid-execution. Neither is authorizer-specific — a user-defined function reaches them:

const { DatabaseSync } = require('node:sqlite');
const db = new DatabaseSync(':memory:');
db.exec('CREATE TABLE t (x INTEGER, txt TEXT)');
for (let i = 0; i < 200; i++) db.exec(`INSERT INTO t VALUES (${i}, '${'z'.repeat(500)}${i}')`);
let stmt;
db.function('boom', (x) => { try { stmt.run(); } catch {} return 1; });
stmt = db.prepare('SELECT boom(x), txt FROM t');
for (const row of stmt.iterate()) { void row.txt; }  // SIGSEGV before this patch

Verified against unpatched bfa3e982ec3: stmt.run(), get(), all(), iterate(), stmt.close(), iter.return(), and re-entering the same cached tagged literal on a tag store each segfault. A single reentrant call on a small result set often returns cleanly, so the crash needs a row payload large enough to force a page fault, or nesting.

iter.next() is the exception: it advances the shared virtual machine rather than resetting or freeing it, so it corrupts iteration instead of crashing (300 nested calls over 800 rows survive). It is guarded alongside the rest because reentering a statement mid-step is not a state a caller can use correctly.

Gating on "any callback is running" would forbid a UDF from preparing, running, and finalizing its own helper statement, which is safe. Instead, track the statements currently being stepped and reject reentry into only those, with statement is already being executed. Tracking is a stack, so a UDF may reenter an inner statement it stepped but not the outer one, and it spans the paired sqlite3_reset() calls, which can run JavaScript through an aggregate's xFinal. statement[Symbol.dispose]() returns early when already finalized, so disposing a statement that is already closed stays a no-op and preserves a pending exception rather than wrapping it in a SuppressedError.

Deliberately unguarded. Three APIs reachable from a callback are left available:

  • backup() and Session.close(). sqlite3_backup_init() runs synchronously but sqlite3_backup_step() takes the source connection's mutex and blocks until the in-progress step finishes; a backup started from inside an authorizer completes and copies every row (also 120 stress iterations at rate: 1, no corruption). Deleting a session doesn't touch the VM under step.
  • sqlTagStore.clear(). Dropping the cache releases strong references but never finalizes a statement synchronously — decrease_refcount() reaching zero calls MakeWeak(), and the statement being stepped is held by a stack BaseObjectPtr regardless — so it touches no SQLite state. Invalidating the cache after a schema change is a legitimate use of an authorizer, so this is allowed rather than guarded.
  • iterator.next() and iterator.return() on a drained iterator. The done_ check precedes the guards, so a drained iterator keeps returning { done: true } instead of throwing.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Aug 9, 2026
@TrevorBurnham
TrevorBurnham force-pushed the sqlite-authorizer-reentry branch 5 times, most recently from e33b0cd to 2a217a9 Compare August 11, 2026 13:31
@TrevorBurnham
TrevorBurnham marked this pull request as ready for review August 11, 2026 14:21
@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.32%. Comparing base (bfa3e98) to head (ca8ee11).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 86.27% 1 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65156      +/-   ##
==========================================
- Coverage   90.32%   90.32%   -0.01%     
==========================================
  Files         760      751       -9     
  Lines      249130   249185      +55     
  Branches    47041    47082      +41     
==========================================
+ Hits       225030   225072      +42     
- Misses      15490    15503      +13     
  Partials     8610     8610              
Files with missing lines Coverage Δ
src/node_sqlite.h 86.81% <100.00%> (+3.47%) ⬆️
src/node_sqlite.cc 81.58% <86.27%> (+0.11%) ⬆️

... and 46 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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 <trevorburnham@gmail.com>
Fixes: nodejs#63207
Assisted-by: claude:opus-5
@TrevorBurnham
TrevorBurnham force-pushed the sqlite-authorizer-reentry branch from 2a217a9 to ca8ee11 Compare August 12, 2026 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sqlite: authorizer callback can modify invoking connection despite SQLite contract

3 participants