From 1f1bc1edfd72b95c3ce840956114e6479cdb7e7e Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Sat, 5 Sep 2026 20:16:39 -0400 Subject: [PATCH] sqlite: validate backup() source and URL-like paths backup() checked only that its first argument was an object before unwrapping it as a DatabaseSync, so passing any other object reinterpreted foreign memory as a database handle. Results ranged from SIGSEGV to a spurious ERR_INVALID_STATE, depending on the object's layout. It is the only unwrap site in node_sqlite.cc that takes a value out of args[], and so the only one that V8's signature check for args.This() does not already protect. DatabaseSync had no constructor template on the Environment to test against, so add one alongside the other sqlite classes and use it. ValidateDatabasePath() treats any object with a string href as a URL and asserted that the href parsed, aborting the process on, for example, new DatabaseSync({ href: 'zzz' }). Reject an unparseable href the same way other non-URL objects are rejected, and stop replacing an exception thrown by the href getter with ERR_INVALID_ARG_TYPE. Fixes: https://github.com/nodejs/node/issues/65830 Signed-off-by: Trevor Burnham Assisted-by: Claude Opus 5 --- src/env_properties.h | 1 + src/node_sqlite.cc | 168 +++++++++++---------- src/node_sqlite.h | 2 + test/parallel/test-sqlite-backup.mjs | 34 ++++- test/parallel/test-sqlite-database-sync.js | 18 +++ 5 files changed, 146 insertions(+), 77 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index fc1772f926e5..8a1fa2daa117 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -478,6 +478,7 @@ V(socketaddress_constructor_template, v8::FunctionTemplate) \ V(space_stats_template, v8::DictionaryTemplate) \ V(sqlite_column_template, v8::DictionaryTemplate) \ + V(sqlite_database_sync_constructor_template, v8::FunctionTemplate) \ V(sqlite_limits_template, v8::ObjectTemplate) \ V(sqlite_run_result_template, v8::DictionaryTemplate) \ V(sqlite_statement_sync_constructor_template, v8::FunctionTemplate) \ diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 76accf1c2731..6109c3b401e3 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -82,6 +82,23 @@ inline MaybeLocal Utf8StringMaybeOneByte(Isolate* isolate, isolate, input.data(), NewStringType::kNormal, len); } +static inline void SetSideEffectFreeGetter( + Isolate* isolate, + Local class_template, + Local name, + FunctionCallback fn) { + Local getter = + FunctionTemplate::New(isolate, + fn, + Local(), + v8::Signature::New(isolate, class_template), + /* length */ 0, + ConstructorBehavior::kThrow, + SideEffectType::kHasNoSideEffect); + class_template->InstanceTemplate()->SetAccessorProperty( + name, getter, Local(), DontDelete); +} + BindingData::BindingData(Realm* realm, Local wrap) : BaseObject(realm, wrap) { MakeWeak(); @@ -1279,12 +1296,15 @@ std::optional ValidateDatabasePath(Environment* env, } else if (path->IsObject()) { // When is URL auto url = path.As(); Local href; - if (url->Get(env->context(), env->href_string()).ToLocal(&href) && - href->IsString()) { + if (!url->Get(env->context(), env->href_string()).ToLocal(&href)) { + return std::nullopt; + } + if (href->IsString()) { Utf8Value location_value(env->isolate(), href.As()); auto location = location_value.ToStringView(); - if (!has_null_bytes(location)) { - CHECK(ada::can_parse(location)); + // A real URL always has a parseable href, but any object with a string + // href gets this far, so the value cannot be assumed to be one. + if (!has_null_bytes(location) && ada::can_parse(location)) { if (!location.starts_with("file:")) { THROW_ERR_INVALID_URL_SCHEME(env->isolate()); return std::nullopt; @@ -1303,6 +1323,63 @@ std::optional ValidateDatabasePath(Environment* env, return std::nullopt; } +Local DatabaseSync::GetConstructorTemplate(Environment* env) { + Local tmpl = + env->sqlite_database_sync_constructor_template(); + if (tmpl.IsEmpty()) { + Isolate* isolate = env->isolate(); + tmpl = NewFunctionTemplate(isolate, DatabaseSync::New); + tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "DatabaseSync")); + tmpl->InstanceTemplate()->SetInternalFieldCount( + DatabaseSync::kInternalFieldCount); + SetProtoMethod(isolate, tmpl, "open", DatabaseSync::Open); + SetProtoMethod(isolate, tmpl, "close", DatabaseSync::Close); + SetProtoDispose(isolate, tmpl, DatabaseSync::Dispose); + SetProtoMethod(isolate, tmpl, "prepare", DatabaseSync::Prepare); + SetProtoMethod(isolate, tmpl, "exec", DatabaseSync::Exec); + SetProtoMethod(isolate, tmpl, "function", DatabaseSync::CustomFunction); + SetProtoMethod( + isolate, tmpl, "createTagStore", DatabaseSync::CreateTagStore); + SetProtoMethodNoSideEffect( + isolate, tmpl, "location", DatabaseSync::Location); + SetProtoMethod(isolate, tmpl, "aggregate", DatabaseSync::AggregateFunction); + SetProtoMethod(isolate, tmpl, "createSession", DatabaseSync::CreateSession); + SetProtoMethod( + isolate, tmpl, "applyChangeset", DatabaseSync::ApplyChangeset); + SetProtoMethod(isolate, + tmpl, + "enableLoadExtension", + DatabaseSync::EnableLoadExtension); + SetProtoMethod( + isolate, tmpl, "enableDefensive", DatabaseSync::EnableDefensive); + SetProtoMethod(isolate, tmpl, "loadExtension", DatabaseSync::LoadExtension); + SetProtoMethod(isolate, tmpl, "serialize", DatabaseSync::Serialize); + SetProtoMethod(isolate, tmpl, "deserialize", DatabaseSync::Deserialize); + SetProtoMethod(isolate, tmpl, "setAuthorizer", DatabaseSync::SetAuthorizer); + SetSideEffectFreeGetter(isolate, + tmpl, + FIXED_ONE_BYTE_STRING(isolate, "isOpen"), + DatabaseSync::IsOpenGetter); + SetSideEffectFreeGetter(isolate, + tmpl, + FIXED_ONE_BYTE_STRING(isolate, "isTransaction"), + DatabaseSync::IsTransactionGetter); + SetSideEffectFreeGetter(isolate, + tmpl, + FIXED_ONE_BYTE_STRING(isolate, "limits"), + DatabaseSync::LimitsGetter); + Local sqlite_type_key = + FIXED_ONE_BYTE_STRING(isolate, "sqlite-type"); + Local sqlite_type_symbol = + v8::Symbol::For(isolate, sqlite_type_key); + Local database_sync_string = + FIXED_ONE_BYTE_STRING(isolate, "node:sqlite"); + tmpl->InstanceTemplate()->Set(sqlite_type_symbol, database_sync_string); + env->set_sqlite_database_sync_constructor_template(tmpl); + } + return tmpl; +} + void DatabaseSync::New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args.IsConstructCall()) { @@ -2419,9 +2496,13 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { void Backup(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (args.Length() < 1 || !args[0]->IsObject()) { - THROW_ERR_INVALID_ARG_TYPE(env->isolate(), - "The \"sourceDb\" argument must be an object."); + // Unlike the other unwrap sites in this file, which rely on V8's signature + // check for args.This(), this one takes an argument and must check the type + // itself before unwrapping it. + if (!DatabaseSync::GetConstructorTemplate(env)->HasInstance(args[0])) { + THROW_ERR_INVALID_ARG_TYPE( + env->isolate(), + "The \"sourceDb\" argument must be an instance of DatabaseSync."); return; } @@ -3811,23 +3892,6 @@ SQLTagStore::SQLTagStore(Environment* env, MakeWeak(); } -static inline void SetSideEffectFreeGetter( - Isolate* isolate, - Local class_template, - Local name, - FunctionCallback fn) { - Local getter = - FunctionTemplate::New(isolate, - fn, - Local(), - v8::Signature::New(isolate, class_template), - /* length */ 0, - ConstructorBehavior::kThrow, - SideEffectType::kHasNoSideEffect); - class_template->InstanceTemplate()->SetAccessorProperty( - name, getter, Local(), DontDelete); -} - SQLTagStore::~SQLTagStore() {} Local SQLTagStore::GetConstructorTemplate(Environment* env) { @@ -4569,62 +4633,14 @@ static void Initialize(Local target, } }); } - Local db_tmpl = - NewFunctionTemplate(isolate, DatabaseSync::New); - db_tmpl->InstanceTemplate()->SetInternalFieldCount( - DatabaseSync::kInternalFieldCount); Local constants = Object::New(isolate); DefineConstants(constants); - SetProtoMethod(isolate, db_tmpl, "open", DatabaseSync::Open); - SetProtoMethod(isolate, db_tmpl, "close", DatabaseSync::Close); - SetProtoDispose(isolate, db_tmpl, DatabaseSync::Dispose); - SetProtoMethod(isolate, db_tmpl, "prepare", DatabaseSync::Prepare); - SetProtoMethod(isolate, db_tmpl, "exec", DatabaseSync::Exec); - SetProtoMethod(isolate, db_tmpl, "function", DatabaseSync::CustomFunction); - SetProtoMethod( - isolate, db_tmpl, "createTagStore", DatabaseSync::CreateTagStore); - SetProtoMethodNoSideEffect( - isolate, db_tmpl, "location", DatabaseSync::Location); - SetProtoMethod( - isolate, db_tmpl, "aggregate", DatabaseSync::AggregateFunction); - SetProtoMethod( - isolate, db_tmpl, "createSession", DatabaseSync::CreateSession); - SetProtoMethod( - isolate, db_tmpl, "applyChangeset", DatabaseSync::ApplyChangeset); - SetProtoMethod(isolate, - db_tmpl, - "enableLoadExtension", - DatabaseSync::EnableLoadExtension); - SetProtoMethod( - isolate, db_tmpl, "enableDefensive", DatabaseSync::EnableDefensive); - SetProtoMethod( - isolate, db_tmpl, "loadExtension", DatabaseSync::LoadExtension); - SetProtoMethod(isolate, db_tmpl, "serialize", DatabaseSync::Serialize); - SetProtoMethod(isolate, db_tmpl, "deserialize", DatabaseSync::Deserialize); - SetProtoMethod( - isolate, db_tmpl, "setAuthorizer", DatabaseSync::SetAuthorizer); - SetSideEffectFreeGetter(isolate, - db_tmpl, - FIXED_ONE_BYTE_STRING(isolate, "isOpen"), - DatabaseSync::IsOpenGetter); - SetSideEffectFreeGetter(isolate, - db_tmpl, - FIXED_ONE_BYTE_STRING(isolate, "isTransaction"), - DatabaseSync::IsTransactionGetter); - SetSideEffectFreeGetter(isolate, - db_tmpl, - FIXED_ONE_BYTE_STRING(isolate, "limits"), - DatabaseSync::LimitsGetter); - Local sqlite_type_key = FIXED_ONE_BYTE_STRING(isolate, "sqlite-type"); - Local sqlite_type_symbol = - v8::Symbol::For(isolate, sqlite_type_key); - Local database_sync_string = - FIXED_ONE_BYTE_STRING(isolate, "node:sqlite"); - db_tmpl->InstanceTemplate()->Set(sqlite_type_symbol, database_sync_string); - - SetConstructorFunction(context, target, "DatabaseSync", db_tmpl); + SetConstructorFunction(context, + target, + "DatabaseSync", + DatabaseSync::GetConstructorTemplate(env)); SetConstructorFunction(context, target, "StatementSync", diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 306a47f6c47f..a80ec479fac6 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -226,6 +226,8 @@ class DatabaseSync : public BaseObject { bool open, bool allow_load_extension); void MemoryInfo(MemoryTracker* tracker) const override; + static v8::Local GetConstructorTemplate( + Environment* env); static void New(const v8::FunctionCallbackInfo& args); static void Open(const v8::FunctionCallbackInfo& args); static void IsOpenGetter(const v8::FunctionCallbackInfo& args); diff --git a/test/parallel/test-sqlite-backup.mjs b/test/parallel/test-sqlite-backup.mjs index f995ae3ca72a..dc3ea2d6a260 100644 --- a/test/parallel/test-sqlite-backup.mjs +++ b/test/parallel/test-sqlite-backup.mjs @@ -48,10 +48,31 @@ describe('backup()', () => { backup(); }, { code: 'ERR_INVALID_ARG_TYPE', - message: 'The "sourceDb" argument must be an object.' + message: 'The "sourceDb" argument must be an instance of DatabaseSync.' }); }); + test('throws if the source database is not a DatabaseSync', (t) => { + const database = makeSourceDb(); + const values = [ + {}, + [], + { p0: 1, p1: 2, p2: 3, p3: 4 }, + { __proto__: DatabaseSync.prototype }, + database.prepare('SELECT 1'), + database.createSession(), + ]; + + for (const value of values) { + t.assert.throws(() => { + backup(value, nextDb()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "sourceDb" argument must be an instance of DatabaseSync.' + }); + } + }); + test('throws if path is not a string, URL, or Buffer', (t) => { const database = makeSourceDb(); @@ -88,6 +109,17 @@ describe('backup()', () => { }); }); + test('throws if the database path is an object with an unparseable href', (t) => { + const database = makeSourceDb(); + + t.assert.throws(() => { + backup(database, { href: 'zzz' }); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "path" argument must be a string, Uint8Array, or URL without null bytes.' + }); + }); + test('throws if options is not an object', (t) => { const database = makeSourceDb(); diff --git a/test/parallel/test-sqlite-database-sync.js b/test/parallel/test-sqlite-database-sync.js index 08a636c9cbdc..4a5f12112c16 100644 --- a/test/parallel/test-sqlite-database-sync.js +++ b/test/parallel/test-sqlite-database-sync.js @@ -51,6 +51,24 @@ suite('DatabaseSync() constructor', () => { }); }); + test('throws if the database location is an object with an unparseable href', (t) => { + t.assert.throws(() => { + new DatabaseSync({ href: 'zzz' }); + }, { + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "path" argument must be a string, Uint8Array, or URL without null bytes.', + }); + }); + + test('propagates an exception thrown by the href getter', (t) => { + t.assert.throws(() => { + new DatabaseSync({ get href() { throw new RangeError('boom'); } }); + }, { + name: 'RangeError', + message: 'boom', + }); + }); + test('throws if options is provided but is not an object', (t) => { t.assert.throws(() => { new DatabaseSync('foo', null);