From 797c6d6899a31d7b8cbbd1954d5f9fb05983f06b Mon Sep 17 00:00:00 2001 From: pbbadenhorst <17087278+pbbadenhorst@users.noreply.github.com> Date: Mon, 18 May 2026 23:18:23 -0700 Subject: [PATCH 1/2] fix(cpp): bind native ATTACH/DETACH parameters and use SQLCipher key API --- android/CMakeLists.txt | 14 ++- cpp/DBHostObject.cpp | 17 ++++ cpp/bridge.cpp | 30 +++++-- cpp/libsql/bridge.cpp | 9 +- cpp/turso_bridge.cpp | 7 +- example/src/tests/dbsetup.ts | 166 +++++++++++++++++++++++++++++++++++ 6 files changed, 225 insertions(+), 18 deletions(-) diff --git a/android/CMakeLists.txt b/android/CMakeLists.txt index 89a5580d..885c47d2 100644 --- a/android/CMakeLists.txt +++ b/android/CMakeLists.txt @@ -3,14 +3,20 @@ cmake_minimum_required(VERSION 3.9.0) set (PACKAGE_NAME "op-sqlite") -include_directories( - ../cpp -) - if (USE_SQLCIPHER) + # Put ../cpp/sqlcipher ahead of ../cpp on the include path so the SQLCipher + # header (which exposes sqlite3_key_v2 and friends) wins over the plain + # sqlite3.h that also lives in ../cpp. Quoted #include search has its own + # rules, but with this ordering both quoted and angle-bracket forms resolve + # to the SQLCipher header under USE_SQLCIPHER; without it the SQLCipher + # build was empirically picking up the plain header. include_directories(../cpp/sqlcipher) endif() +include_directories( + ../cpp +) + if (USE_LIBSQL) include_directories(src/main/jniLibs/include) endif() diff --git a/cpp/DBHostObject.cpp b/cpp/DBHostObject.cpp index 97b56225..4c98f5ad 100644 --- a/cpp/DBHostObject.cpp +++ b/cpp/DBHostObject.cpp @@ -251,6 +251,19 @@ void DBHostObject::create_jsi_functions(jsi::Runtime &rt) { secondary_db_path = secondary_db_path + location; } + // Reject zero bytes uniformly: libsql's libsql_bind_string takes a + // C-string and would silently truncate at the first zero byte; + // SQLite and Turso bind with explicit lengths. Failing loudly across + // all backends keeps behaviour consistent. + if (secondary_db_name.find('\0') != std::string::npos) { + throw std::runtime_error( + "[op-sqlite] attach secondaryDbFileName must not contain a zero byte"); + } + if (alias.find('\0') != std::string::npos) { + throw std::runtime_error( + "[op-sqlite] attach alias must not contain a zero byte"); + } + #ifdef OP_SQLITE_USE_LIBSQL opsqlite_libsql_attach(db, secondary_db_path, secondary_db_name, alias); #else @@ -266,6 +279,10 @@ void DBHostObject::create_jsi_functions(jsi::Runtime &rt) { } std::string alias = args[0].asString(rt).utf8(rt); + if (alias.find('\0') != std::string::npos) { + throw std::runtime_error( + "[op-sqlite] detach alias must not contain a zero byte"); + } #ifdef OP_SQLITE_USE_LIBSQL opsqlite_libsql_detach(db, alias); #else diff --git a/cpp/bridge.cpp b/cpp/bridge.cpp index 7d6b9948..adafe298 100644 --- a/cpp/bridge.cpp +++ b/cpp/bridge.cpp @@ -103,7 +103,23 @@ sqlite3 *opsqlite_open(std::string const &name, std::string const &path, #ifdef OP_SQLITE_USE_SQLCIPHER if (!encryption_key.empty()) { - opsqlite_execute(db, "PRAGMA key = '" + encryption_key + "'", nullptr); + // Use the SQLCipher C API directly instead of `PRAGMA key = '...'`. + // Primary reason: removes the SQL-injection shape — if the key string + // is ever attacker-influenced, a payload like `'; ATTACH ...; --` + // would otherwise execute arbitrary SQL on the freshly-opened + // connection. Secondary benefits: the key is passed as a binary + // buffer with explicit length so embedded zero bytes in the key are + // preserved, and it never enters trace/log surfaces — defense in + // depth, not an exploit class on its own. + int key_status = sqlite3_key_v2( + db, "main", encryption_key.data(), + static_cast(encryption_key.size())); + if (key_status != SQLITE_OK) { + const char *message = sqlite3_errmsg(db); + throw std::runtime_error( + "[op-sqlite] failed to set encryption key: " + + std::string(message != nullptr ? message : "unknown error")); + } } #endif @@ -161,15 +177,17 @@ void opsqlite_close(sqlite3 *db) { void opsqlite_attach(sqlite3 *db, std::string const &doc_path, std::string const &secondary_db_name, std::string const &alias) { + // ATTACH/DETACH's filename and schema-name slots both accept parameters + // (verified on SQLite 3.50.6, behavior present across the 3.x series). + // Binding sidesteps every escaping/identifier-quoting concern. auto secondary_db_path = opsqlite_get_db_path(secondary_db_name, doc_path); - auto statement = "ATTACH DATABASE '" + secondary_db_path + "' AS " + alias; - - opsqlite_execute(db, statement, nullptr); + std::vector params = {secondary_db_path, alias}; + opsqlite_execute(db, "ATTACH DATABASE ? AS ?", ¶ms); } void opsqlite_detach(sqlite3 *db, std::string const &alias) { - std::string statement = "DETACH DATABASE " + alias; - opsqlite_execute(db, statement, nullptr); + std::vector params = {alias}; + opsqlite_execute(db, "DETACH DATABASE ?", ¶ms); } void opsqlite_remove(sqlite3 *db, std::string const &name, diff --git a/cpp/libsql/bridge.cpp b/cpp/libsql/bridge.cpp index 869f94b9..444dbe65 100644 --- a/cpp/libsql/bridge.cpp +++ b/cpp/libsql/bridge.cpp @@ -155,14 +155,13 @@ void opsqlite_libsql_attach(DB const &db, std::string const &docPath, std::string const &databaseToAttach, std::string const &alias) { std::string dbPath = opsqlite_get_db_path(databaseToAttach, docPath); - std::string statement = "ATTACH DATABASE '" + dbPath + "' AS " + alias; - - opsqlite_libsql_execute(db, statement, nullptr); + std::vector params = {dbPath, alias}; + opsqlite_libsql_execute(db, "ATTACH DATABASE ? AS ?", ¶ms); } void opsqlite_libsql_detach(DB const &db, std::string const &alias) { - std::string statement = "DETACH DATABASE " + alias; - opsqlite_libsql_execute(db, statement, nullptr); + std::vector params = {alias}; + opsqlite_libsql_execute(db, "DETACH DATABASE ?", ¶ms); } int32_t opsqlite_libsql_get_reserved_bytes(DB const &db) { diff --git a/cpp/turso_bridge.cpp b/cpp/turso_bridge.cpp index 9556e8b7..43e410cc 100644 --- a/cpp/turso_bridge.cpp +++ b/cpp/turso_bridge.cpp @@ -593,12 +593,13 @@ void opsqlite_attach(sqlite3 *db, std::string const &doc_path, std::string const &secondary_db_name, std::string const &alias) { auto secondary_db_path = opsqlite_get_db_path(secondary_db_name, doc_path); - auto statement = "ATTACH DATABASE '" + secondary_db_path + "' AS " + alias; - opsqlite_execute(db, statement, nullptr); + std::vector params = {secondary_db_path, alias}; + opsqlite_execute(db, "ATTACH DATABASE ? AS ?", ¶ms); } void opsqlite_detach(sqlite3 *db, std::string const &alias) { - opsqlite_execute(db, "DETACH DATABASE " + alias, nullptr); + std::vector params = {alias}; + opsqlite_execute(db, "DETACH DATABASE ?", ¶ms); } sqlite3_stmt *opsqlite_prepare_statement(sqlite3 *db, diff --git a/example/src/tests/dbsetup.ts b/example/src/tests/dbsetup.ts index 04500a07..18dbf448 100644 --- a/example/src/tests/dbsetup.ts +++ b/example/src/tests/dbsetup.ts @@ -270,6 +270,172 @@ it("Can attach/dettach database", () => { db2.delete(); }); +it("Neutralizes SQL injection payload in attach alias", () => { + if (isTurso()) { + return; + } + const db = open({ + name: "attachInjectionTest.sqlite", + encryptionKey: "test", + }); + let db2 = open({ + name: "attachInjectionTest2.sqlite", + encryptionKey: "test", + }); + db2.close(); + + // Pre-fix, `opsqlite_execute` walked remainingStatement, so the trailing + // `ATTACH ... AS pwned` would have run as a second prepared statement. + // Post-fix the alias is passed via parameter binding (`ATTACH DATABASE ? + // AS ?`), so the whole payload is a single TEXT value used as the + // schema-name; neither `pwned` nor `evil` ends up attached. The + // `:memory:` target in the payload keeps the proof side-effect-free + // across sandboxes in case the pre-fix code path is ever reintroduced. + const maliciousAlias = "evil; ATTACH DATABASE ':memory:' AS pwned; --"; + + db.attach({ + secondaryDbFileName: "attachInjectionTest2.sqlite", + alias: maliciousAlias, + }); + + let pwnedAttached = false; + try { + db.executeSync("SELECT 1 FROM pwned.sqlite_master LIMIT 1;"); + pwnedAttached = true; + } catch { + pwnedAttached = false; + } + expect(pwnedAttached).toBe(false); + + let evilAttached = false; + try { + db.executeSync("SELECT 1 FROM evil.sqlite_master LIMIT 1;"); + evilAttached = true; + } catch { + evilAttached = false; + } + expect(evilAttached).toBe(false); + + db.detach(maliciousAlias); + + db.delete(); + + db2 = open({ + name: "attachInjectionTest2.sqlite", + encryptionKey: "test", + }); + db2.delete(); +}); + +it("Neutralizes SQL injection payload in attach path", () => { + if (isTurso()) { + return; + } + // `opsqlite_get_db_path` just concatenates location + filename, so any + // quote in the filename used to escape the surrounding string literal + // in `ATTACH DATABASE '...'`. Post-fix the path is passed via parameter + // binding, so the embedded quote is just data — no SQL involvement. + const quirkyFileName = "attach'Injection.sqlite"; + const db = open({ + name: "attachPathHostDb.sqlite", + encryptionKey: "test", + }); + const db2 = open({ + name: quirkyFileName, + encryptionKey: "test", + }); + db2.close(); + + db.attach({ + secondaryDbFileName: quirkyFileName, + alias: "quirky", + }); + db.executeSync("DROP TABLE IF EXISTS quirky.canary;"); + db.executeSync( + "CREATE TABLE IF NOT EXISTS quirky.canary (id INTEGER PRIMARY KEY);", + ); + db.executeSync("INSERT INTO quirky.canary (id) VALUES (1);"); + const rows = db.executeSync("SELECT id FROM quirky.canary;").rows; + expect(rows[0]!.id).toBe(1); + + db.detach("quirky"); + db.delete(); + + open({ name: quirkyFileName, encryptionKey: "test" }).delete(); +}); + +it("Detach with injection payload does not execute trailing SQL", () => { + if (isTurso()) { + return; + } + const db = open({ + name: "detachInjectionTest.sqlite", + encryptionKey: "test", + }); + const secondary = open({ + name: "detachInjectionTest2.sqlite", + encryptionKey: "test", + }); + secondary.close(); + + db.executeSync("DROP TABLE IF EXISTS canary;"); + db.executeSync("CREATE TABLE canary (id INTEGER PRIMARY KEY);"); + db.executeSync("INSERT INTO canary (id) VALUES (42);"); + + // Attach a *real* schema named `safe` so the leading DETACH actually + // resolves pre-fix. The trailing `DROP TABLE canary; --` would then + // run as a second prepared statement and the canary row would be gone. + // Post-fix the alias is passed via parameter binding, so the whole + // payload is the schema-name to detach; no such schema exists, DETACH + // errors with "no such database", canary survives. + db.attach({ + secondaryDbFileName: "detachInjectionTest2.sqlite", + alias: "safe", + }); + + let threw = false; + try { + db.detach("safe; DROP TABLE canary; --"); + } catch { + threw = true; + } + expect(threw).toBe(true); + + const rows = db.executeSync("SELECT id FROM canary;").rows; + expect(rows.length).toBe(1); + expect(rows[0]!.id).toBe(42); + + // Clean up the real `safe` schema we attached above. The detach above + // failed, so `safe` is still attached. + db.detach("safe"); + db.delete(); + + open({ name: "detachInjectionTest2.sqlite", encryptionKey: "test" }).delete(); +}); + +if (isSQLCipher()) { + it("Encryption key with single quote survives a round-trip", () => { + // Prior code embedded the key directly into `PRAGMA key = ''`, + // so a quote in the key would either error out or silently set a + // truncated key. Post-fix the key is set via `sqlite3_key_v2`, + // which takes a binary buffer + length — preserving the key + // exactly, so reopening with the same key still decrypts. + const trickyKey = "p'a''ss\"wrd"; + const dbName = "pragmaKeyInjectionTest.sqlite"; + + let db = open({ name: dbName, encryptionKey: trickyKey }); + db.executeSync("DROP TABLE IF EXISTS secret;"); + db.executeSync("CREATE TABLE secret (value TEXT);"); + db.executeSync("INSERT INTO secret (value) VALUES ('ok');"); + db.close(); + + db = open({ name: dbName, encryptionKey: trickyKey }); + const rows = db.executeSync("SELECT value FROM secret;").rows; + expect(rows[0]!.value).toBe("ok"); + db.delete(); + }); +} + it("Can get db path", () => { const db = open({ name: "pathTest.sqlite", From 48bd37dfb0193e2ecb9fada137e6d3a2fc3f7f7c Mon Sep 17 00:00:00 2001 From: pbbadenhorst <17087278+pbbadenhorst@users.noreply.github.com> Date: Tue, 19 May 2026 00:13:17 -0700 Subject: [PATCH 2/2] test: relax detach injection assertion for libsql missing-schema semantics --- example/src/tests/dbsetup.ts | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/example/src/tests/dbsetup.ts b/example/src/tests/dbsetup.ts index 18dbf448..976849ba 100644 --- a/example/src/tests/dbsetup.ts +++ b/example/src/tests/dbsetup.ts @@ -386,28 +386,35 @@ it("Detach with injection payload does not execute trailing SQL", () => { // resolves pre-fix. The trailing `DROP TABLE canary; --` would then // run as a second prepared statement and the canary row would be gone. // Post-fix the alias is passed via parameter binding, so the whole - // payload is the schema-name to detach; no such schema exists, DETACH - // errors with "no such database", canary survives. + // payload is the schema-name to detach; nothing matches. The core + // proof is that `canary` survives — backends differ on whether a + // missing-schema DETACH errors (sqlite/sqlcipher) or returns cleanly + // (libsql), so we don't assert on the throw shape. db.attach({ secondaryDbFileName: "detachInjectionTest2.sqlite", alias: "safe", }); - let threw = false; try { db.detach("safe; DROP TABLE canary; --"); } catch { - threw = true; + // Ignored — only the canary check below is load-bearing. } - expect(threw).toBe(true); const rows = db.executeSync("SELECT id FROM canary;").rows; expect(rows.length).toBe(1); expect(rows[0]!.id).toBe(42); - // Clean up the real `safe` schema we attached above. The detach above - // failed, so `safe` is still attached. - db.detach("safe"); + // Best-effort cleanup of `safe`. On backends where the malicious + // detach above did not throw, libsql may also have detached the + // `safe` schema (treating the bound text as a literal alias-name + // that didn't match) or left it attached; either way, swallow the + // error so cleanup doesn't fail the test. + try { + db.detach("safe"); + } catch { + // Already detached or never attached — ignore. + } db.delete(); open({ name: "detachInjectionTest2.sqlite", encryptionKey: "test" }).delete();