Skip to content

Commit dff1c04

Browse files
authored
fix(cpp): bind native ATTACH/DETACH parameters and use SQLCipher key API (#409)
2 parents bc1b3b7 + 48bd37d commit dff1c04

6 files changed

Lines changed: 232 additions & 18 deletions

File tree

android/CMakeLists.txt

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,20 @@ cmake_minimum_required(VERSION 3.9.0)
33

44
set (PACKAGE_NAME "op-sqlite")
55

6-
include_directories(
7-
../cpp
8-
)
9-
106
if (USE_SQLCIPHER)
7+
# Put ../cpp/sqlcipher ahead of ../cpp on the include path so the SQLCipher
8+
# header (which exposes sqlite3_key_v2 and friends) wins over the plain
9+
# sqlite3.h that also lives in ../cpp. Quoted #include search has its own
10+
# rules, but with this ordering both quoted and angle-bracket forms resolve
11+
# to the SQLCipher header under USE_SQLCIPHER; without it the SQLCipher
12+
# build was empirically picking up the plain header.
1113
include_directories(../cpp/sqlcipher)
1214
endif()
1315

16+
include_directories(
17+
../cpp
18+
)
19+
1420
if (USE_LIBSQL)
1521
include_directories(src/main/jniLibs/include)
1622
endif()

cpp/DBHostObject.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,19 @@ void DBHostObject::create_jsi_functions(jsi::Runtime &rt) {
251251
secondary_db_path = secondary_db_path + location;
252252
}
253253

254+
// Reject zero bytes uniformly: libsql's libsql_bind_string takes a
255+
// C-string and would silently truncate at the first zero byte;
256+
// SQLite and Turso bind with explicit lengths. Failing loudly across
257+
// all backends keeps behaviour consistent.
258+
if (secondary_db_name.find('\0') != std::string::npos) {
259+
throw std::runtime_error(
260+
"[op-sqlite] attach secondaryDbFileName must not contain a zero byte");
261+
}
262+
if (alias.find('\0') != std::string::npos) {
263+
throw std::runtime_error(
264+
"[op-sqlite] attach alias must not contain a zero byte");
265+
}
266+
254267
#ifdef OP_SQLITE_USE_LIBSQL
255268
opsqlite_libsql_attach(db, secondary_db_path, secondary_db_name, alias);
256269
#else
@@ -266,6 +279,10 @@ void DBHostObject::create_jsi_functions(jsi::Runtime &rt) {
266279
}
267280

268281
std::string alias = args[0].asString(rt).utf8(rt);
282+
if (alias.find('\0') != std::string::npos) {
283+
throw std::runtime_error(
284+
"[op-sqlite] detach alias must not contain a zero byte");
285+
}
269286
#ifdef OP_SQLITE_USE_LIBSQL
270287
opsqlite_libsql_detach(db, alias);
271288
#else

cpp/bridge.cpp

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,23 @@ sqlite3 *opsqlite_open(std::string const &name, std::string const &path,
103103

104104
#ifdef OP_SQLITE_USE_SQLCIPHER
105105
if (!encryption_key.empty()) {
106-
opsqlite_execute(db, "PRAGMA key = '" + encryption_key + "'", nullptr);
106+
// Use the SQLCipher C API directly instead of `PRAGMA key = '...'`.
107+
// Primary reason: removes the SQL-injection shape — if the key string
108+
// is ever attacker-influenced, a payload like `'; ATTACH ...; --`
109+
// would otherwise execute arbitrary SQL on the freshly-opened
110+
// connection. Secondary benefits: the key is passed as a binary
111+
// buffer with explicit length so embedded zero bytes in the key are
112+
// preserved, and it never enters trace/log surfaces — defense in
113+
// depth, not an exploit class on its own.
114+
int key_status = sqlite3_key_v2(
115+
db, "main", encryption_key.data(),
116+
static_cast<int>(encryption_key.size()));
117+
if (key_status != SQLITE_OK) {
118+
const char *message = sqlite3_errmsg(db);
119+
throw std::runtime_error(
120+
"[op-sqlite] failed to set encryption key: " +
121+
std::string(message != nullptr ? message : "unknown error"));
122+
}
107123
}
108124
#endif
109125

@@ -161,15 +177,17 @@ void opsqlite_close(sqlite3 *db) {
161177
void opsqlite_attach(sqlite3 *db, std::string const &doc_path,
162178
std::string const &secondary_db_name,
163179
std::string const &alias) {
180+
// ATTACH/DETACH's filename and schema-name slots both accept parameters
181+
// (verified on SQLite 3.50.6, behavior present across the 3.x series).
182+
// Binding sidesteps every escaping/identifier-quoting concern.
164183
auto secondary_db_path = opsqlite_get_db_path(secondary_db_name, doc_path);
165-
auto statement = "ATTACH DATABASE '" + secondary_db_path + "' AS " + alias;
166-
167-
opsqlite_execute(db, statement, nullptr);
184+
std::vector<JSVariant> params = {secondary_db_path, alias};
185+
opsqlite_execute(db, "ATTACH DATABASE ? AS ?", &params);
168186
}
169187

170188
void opsqlite_detach(sqlite3 *db, std::string const &alias) {
171-
std::string statement = "DETACH DATABASE " + alias;
172-
opsqlite_execute(db, statement, nullptr);
189+
std::vector<JSVariant> params = {alias};
190+
opsqlite_execute(db, "DETACH DATABASE ?", &params);
173191
}
174192

175193
void opsqlite_remove(sqlite3 *db, std::string const &name,

cpp/libsql/bridge.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,14 +155,13 @@ void opsqlite_libsql_attach(DB const &db, std::string const &docPath,
155155
std::string const &databaseToAttach,
156156
std::string const &alias) {
157157
std::string dbPath = opsqlite_get_db_path(databaseToAttach, docPath);
158-
std::string statement = "ATTACH DATABASE '" + dbPath + "' AS " + alias;
159-
160-
opsqlite_libsql_execute(db, statement, nullptr);
158+
std::vector<JSVariant> params = {dbPath, alias};
159+
opsqlite_libsql_execute(db, "ATTACH DATABASE ? AS ?", &params);
161160
}
162161

163162
void opsqlite_libsql_detach(DB const &db, std::string const &alias) {
164-
std::string statement = "DETACH DATABASE " + alias;
165-
opsqlite_libsql_execute(db, statement, nullptr);
163+
std::vector<JSVariant> params = {alias};
164+
opsqlite_libsql_execute(db, "DETACH DATABASE ?", &params);
166165
}
167166

168167
int32_t opsqlite_libsql_get_reserved_bytes(DB const &db) {

cpp/turso_bridge.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -593,12 +593,13 @@ void opsqlite_attach(sqlite3 *db, std::string const &doc_path,
593593
std::string const &secondary_db_name,
594594
std::string const &alias) {
595595
auto secondary_db_path = opsqlite_get_db_path(secondary_db_name, doc_path);
596-
auto statement = "ATTACH DATABASE '" + secondary_db_path + "' AS " + alias;
597-
opsqlite_execute(db, statement, nullptr);
596+
std::vector<JSVariant> params = {secondary_db_path, alias};
597+
opsqlite_execute(db, "ATTACH DATABASE ? AS ?", &params);
598598
}
599599

600600
void opsqlite_detach(sqlite3 *db, std::string const &alias) {
601-
opsqlite_execute(db, "DETACH DATABASE " + alias, nullptr);
601+
std::vector<JSVariant> params = {alias};
602+
opsqlite_execute(db, "DETACH DATABASE ?", &params);
602603
}
603604

604605
sqlite3_stmt *opsqlite_prepare_statement(sqlite3 *db,

example/src/tests/dbsetup.ts

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,179 @@ it("Can attach/dettach database", () => {
270270
db2.delete();
271271
});
272272

273+
it("Neutralizes SQL injection payload in attach alias", () => {
274+
if (isTurso()) {
275+
return;
276+
}
277+
const db = open({
278+
name: "attachInjectionTest.sqlite",
279+
encryptionKey: "test",
280+
});
281+
let db2 = open({
282+
name: "attachInjectionTest2.sqlite",
283+
encryptionKey: "test",
284+
});
285+
db2.close();
286+
287+
// Pre-fix, `opsqlite_execute` walked remainingStatement, so the trailing
288+
// `ATTACH ... AS pwned` would have run as a second prepared statement.
289+
// Post-fix the alias is passed via parameter binding (`ATTACH DATABASE ?
290+
// AS ?`), so the whole payload is a single TEXT value used as the
291+
// schema-name; neither `pwned` nor `evil` ends up attached. The
292+
// `:memory:` target in the payload keeps the proof side-effect-free
293+
// across sandboxes in case the pre-fix code path is ever reintroduced.
294+
const maliciousAlias = "evil; ATTACH DATABASE ':memory:' AS pwned; --";
295+
296+
db.attach({
297+
secondaryDbFileName: "attachInjectionTest2.sqlite",
298+
alias: maliciousAlias,
299+
});
300+
301+
let pwnedAttached = false;
302+
try {
303+
db.executeSync("SELECT 1 FROM pwned.sqlite_master LIMIT 1;");
304+
pwnedAttached = true;
305+
} catch {
306+
pwnedAttached = false;
307+
}
308+
expect(pwnedAttached).toBe(false);
309+
310+
let evilAttached = false;
311+
try {
312+
db.executeSync("SELECT 1 FROM evil.sqlite_master LIMIT 1;");
313+
evilAttached = true;
314+
} catch {
315+
evilAttached = false;
316+
}
317+
expect(evilAttached).toBe(false);
318+
319+
db.detach(maliciousAlias);
320+
321+
db.delete();
322+
323+
db2 = open({
324+
name: "attachInjectionTest2.sqlite",
325+
encryptionKey: "test",
326+
});
327+
db2.delete();
328+
});
329+
330+
it("Neutralizes SQL injection payload in attach path", () => {
331+
if (isTurso()) {
332+
return;
333+
}
334+
// `opsqlite_get_db_path` just concatenates location + filename, so any
335+
// quote in the filename used to escape the surrounding string literal
336+
// in `ATTACH DATABASE '...'`. Post-fix the path is passed via parameter
337+
// binding, so the embedded quote is just data — no SQL involvement.
338+
const quirkyFileName = "attach'Injection.sqlite";
339+
const db = open({
340+
name: "attachPathHostDb.sqlite",
341+
encryptionKey: "test",
342+
});
343+
const db2 = open({
344+
name: quirkyFileName,
345+
encryptionKey: "test",
346+
});
347+
db2.close();
348+
349+
db.attach({
350+
secondaryDbFileName: quirkyFileName,
351+
alias: "quirky",
352+
});
353+
db.executeSync("DROP TABLE IF EXISTS quirky.canary;");
354+
db.executeSync(
355+
"CREATE TABLE IF NOT EXISTS quirky.canary (id INTEGER PRIMARY KEY);",
356+
);
357+
db.executeSync("INSERT INTO quirky.canary (id) VALUES (1);");
358+
const rows = db.executeSync("SELECT id FROM quirky.canary;").rows;
359+
expect(rows[0]!.id).toBe(1);
360+
361+
db.detach("quirky");
362+
db.delete();
363+
364+
open({ name: quirkyFileName, encryptionKey: "test" }).delete();
365+
});
366+
367+
it("Detach with injection payload does not execute trailing SQL", () => {
368+
if (isTurso()) {
369+
return;
370+
}
371+
const db = open({
372+
name: "detachInjectionTest.sqlite",
373+
encryptionKey: "test",
374+
});
375+
const secondary = open({
376+
name: "detachInjectionTest2.sqlite",
377+
encryptionKey: "test",
378+
});
379+
secondary.close();
380+
381+
db.executeSync("DROP TABLE IF EXISTS canary;");
382+
db.executeSync("CREATE TABLE canary (id INTEGER PRIMARY KEY);");
383+
db.executeSync("INSERT INTO canary (id) VALUES (42);");
384+
385+
// Attach a *real* schema named `safe` so the leading DETACH actually
386+
// resolves pre-fix. The trailing `DROP TABLE canary; --` would then
387+
// run as a second prepared statement and the canary row would be gone.
388+
// Post-fix the alias is passed via parameter binding, so the whole
389+
// payload is the schema-name to detach; nothing matches. The core
390+
// proof is that `canary` survives — backends differ on whether a
391+
// missing-schema DETACH errors (sqlite/sqlcipher) or returns cleanly
392+
// (libsql), so we don't assert on the throw shape.
393+
db.attach({
394+
secondaryDbFileName: "detachInjectionTest2.sqlite",
395+
alias: "safe",
396+
});
397+
398+
try {
399+
db.detach("safe; DROP TABLE canary; --");
400+
} catch {
401+
// Ignored — only the canary check below is load-bearing.
402+
}
403+
404+
const rows = db.executeSync("SELECT id FROM canary;").rows;
405+
expect(rows.length).toBe(1);
406+
expect(rows[0]!.id).toBe(42);
407+
408+
// Best-effort cleanup of `safe`. On backends where the malicious
409+
// detach above did not throw, libsql may also have detached the
410+
// `safe` schema (treating the bound text as a literal alias-name
411+
// that didn't match) or left it attached; either way, swallow the
412+
// error so cleanup doesn't fail the test.
413+
try {
414+
db.detach("safe");
415+
} catch {
416+
// Already detached or never attached — ignore.
417+
}
418+
db.delete();
419+
420+
open({ name: "detachInjectionTest2.sqlite", encryptionKey: "test" }).delete();
421+
});
422+
423+
if (isSQLCipher()) {
424+
it("Encryption key with single quote survives a round-trip", () => {
425+
// Prior code embedded the key directly into `PRAGMA key = '<key>'`,
426+
// so a quote in the key would either error out or silently set a
427+
// truncated key. Post-fix the key is set via `sqlite3_key_v2`,
428+
// which takes a binary buffer + length — preserving the key
429+
// exactly, so reopening with the same key still decrypts.
430+
const trickyKey = "p'a''ss\"wrd";
431+
const dbName = "pragmaKeyInjectionTest.sqlite";
432+
433+
let db = open({ name: dbName, encryptionKey: trickyKey });
434+
db.executeSync("DROP TABLE IF EXISTS secret;");
435+
db.executeSync("CREATE TABLE secret (value TEXT);");
436+
db.executeSync("INSERT INTO secret (value) VALUES ('ok');");
437+
db.close();
438+
439+
db = open({ name: dbName, encryptionKey: trickyKey });
440+
const rows = db.executeSync("SELECT value FROM secret;").rows;
441+
expect(rows[0]!.value).toBe("ok");
442+
db.delete();
443+
});
444+
}
445+
273446
it("Can get db path", () => {
274447
const db = open({
275448
name: "pathTest.sqlite",

0 commit comments

Comments
 (0)