Skip to content

sqlite: re-validate database state after reading options - #65595

Open
TrevorBurnham wants to merge 2 commits into
nodejs:mainfrom
TrevorBurnham:sqlite-option-getter-toctou
Open

sqlite: re-validate database state after reading options#65595
TrevorBurnham wants to merge 2 commits into
nodejs:mainfrom
TrevorBurnham:sqlite-option-getter-toctou

Conversation

@TrevorBurnham

@TrevorBurnham TrevorBurnham commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes #65586

Several DatabaseSync methods check IsOpen() and then read their options bag with Object::Get(). A user-supplied property getter runs arbitrary JavaScript at that point, so a getter that calls close() invalidates the check before the resulting sqlite3* is used.

function(), aggregate(), deserialize(), applyChangeset(), backup(). All but prepare() all segfault in this scenario. prepare() reports a spurious ERR_SQLITE_ERROR: out of memory instead, because sqlite3_prepare_v3() null-checks its argument.

The fix here is to re-check IsOpen() after option parsing, immediately before the SQLite call. The existing early check is kept: moving it would make attacker-controlled getters run on calls that are already doomed to throw, which is the wrong direction for a hardening change, and would reorder existing error precedence.

createSession() already parsed its options before validating. It gains the matching early check here for consistency.

Two related fixes in the same code:

  • deserialize() latched input->ByteLength() before reading options.dbName. A getter that shrinks the backing ArrayBuffer leaves the latched length too large, CopyContents() copies only what remains, its return value is discarded, and the uninitialized remainder is handed to SQLite, where serialize() returns it to JavaScript. The CopyContents() result is now checked.
  • function() and aggregate() cast the callback's length with As<Int32>() and no IsInt32() guard. length is configurable, so 'abc', {}, NaN and 1.5 all reached the cast and registered a silently wrong arity. Now validated, matching the five sites in this file that already do.

Verification

New test/parallel/test-sqlite-options-getter-reentry.js covers all seven sites, buffer shrink and detach, and the length type checks.

Compatibility note

Rejecting a non-integer fn.length with ERR_INVALID_ARG_TYPE is a behavior change, though it only affects functions whose length has been redefined.

@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 27, 2026
prepare(), function(), aggregate(), deserialize(), applyChangeset() and
backup() validated the connection, then read their options bag with
Object::Get(). A property getter runs arbitrary JavaScript at that
point, so a getter calling close() invalidates what was just checked.
Five of the six then passed a null sqlite3* to SQLite and crashed;
prepare() reported a spurious "out of memory".

Re-check IsOpen() after option parsing, immediately before the SQLite
call, keeping the early check so invalid calls still fail before any
user code runs. IsOpen() is the only condition a getter can change:
authorizer and callback depths are RAII-managed. createSession()
already parsed options first, so it only gains the early check.

deserialize() also latched the buffer length before reading
options.dbName. A getter that shrank the backing store left the length
too large; CopyContents() then handed the uninitialized remainder to
SQLite, from where serialize() returned it to JavaScript. Check the
CopyContents() result instead of discarding it.

function() and aggregate() cast the callback's length property with
As<Int32>() and no IsInt32() guard. length is configurable, so any type
reached the cast and produced a silently wrong arity.

Fixes: nodejs#65586
Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@TrevorBurnham
TrevorBurnham force-pushed the sqlite-option-getter-toctou branch from 39de389 to d3e10a8 Compare August 27, 2026 15:58
@TrevorBurnham
TrevorBurnham marked this pull request as ready for review August 27, 2026 16:56
Add the skipIfSQLiteMissing() guard. Builds configured with shared
libraries omit node:sqlite, so requiring it unconditionally failed the
x86_64-darwin and aarch64-darwin shared-library CI jobs.

Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.07%. Comparing base (9baabd4) to head (8f7d36d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/node_sqlite.cc 91.17% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65595      +/-   ##
==========================================
- Coverage   90.07%   90.07%   -0.01%     
==========================================
  Files         751      751              
  Lines      254875   254908      +33     
  Branches    48108    48140      +32     
==========================================
+ Hits       229579   229602      +23     
- Misses      16466    16486      +20     
+ Partials     8830     8820      -10     
Files with missing lines Coverage Δ
src/node_sqlite.cc 82.23% <91.17%> (+0.19%) ⬆️

... and 30 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.

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: option getters run user JS mid-call, then stale state is used

2 participants