Skip to content

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

Open
TrevorBurnham wants to merge 1 commit into
nodejs:mainfrom
TrevorBurnham:sqlite-option-getter-toctou
Open

sqlite: re-validate database state after reading options#65595
TrevorBurnham wants to merge 1 commit 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
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