Skip to content

Commit d08fde8

Browse files
mcollinaRafaelGSS
authored andcommitted
sqlite: invalidate tag store iterators on statement reset
SQLTagStore Run/Iterate/Get/All reset the shared cached statement via raw sqlite3_reset(), bypassing ResetStatement() and its reset_generation_ bump, so live iterators were never invalidated. Also mark the iterator done on SQLITE_DONE so it cannot restart the statement. PR-URL: nodejs-private/node-private#896 Refs: https://hackerone.com/reports/3564941 Reviewed-By: Robert Nagy <ronagy@icloud.com> CVE-ID: CVE-2026-58041
1 parent c7ec3dc commit d08fde8

2 files changed

Lines changed: 113 additions & 4 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3365,7 +3365,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {
33653365
}
33663366

33673367
uint32_t n_params = args.Length() - 1;
3368-
int r = sqlite3_reset(stmt->statement_);
3368+
int r = stmt->ResetStatement();
33693369
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
33703370
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
33713371
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
@@ -3398,7 +3398,7 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {
33983398
}
33993399

34003400
uint32_t n_params = args.Length() - 1;
3401-
int r = sqlite3_reset(stmt->statement_);
3401+
int r = stmt->ResetStatement();
34023402
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
34033403
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
34043404
for (int i = 0; i < static_cast<int>(n_params) && i < param_count; ++i) {
@@ -3435,7 +3435,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {
34353435
uint32_t n_params = args.Length() - 1;
34363436
Isolate* isolate = env->isolate();
34373437

3438-
int r = sqlite3_reset(stmt->statement_);
3438+
int r = stmt->ResetStatement();
34393439
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
34403440

34413441
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
@@ -3474,7 +3474,7 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
34743474
uint32_t n_params = args.Length() - 1;
34753475
Isolate* isolate = env->isolate();
34763476

3477-
int r = sqlite3_reset(stmt->statement_);
3477+
int r = stmt->ResetStatement();
34783478
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
34793479

34803480
int param_count = sqlite3_bind_parameter_count(stmt->statement_);
@@ -3718,6 +3718,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
37183718
CHECK_ERROR_OR_THROW(
37193719
env->isolate(), iter->stmt_->db_.get(), r, SQLITE_DONE, void());
37203720
sqlite3_reset(iter->stmt_->statement_);
3721+
iter->done_ = true;
37213722
MaybeLocal<Value> values[] = {Boolean::New(isolate, true), Null(isolate)};
37223723
Local<Object> result;
37233724
if (NewDictionaryInstanceNullProto(env->context(), iter_template, values)

‎test/parallel/test-sqlite-template-tag.js‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,114 @@ test('TagStore capacity, size, and clear', () => {
112112
assert.strictEqual(sql.capacity,10);
113113
});
114114

115+
test('iterator is invalidated when the cached statement is reset',()=>{
116+
constldb=newDatabaseSync(':memory:');
117+
constlsql=ldb.createTagStore();
118+
ldb.exec('CREATE TABLE foo (id INTEGER PRIMARY KEY, text TEXT)');
119+
for(leti=0;i<5;i++){
120+
// eslint-disable-next-line no-unused-expressions
121+
lsql.run`INSERT INTO foo (text) VALUES (${String(i)})`;
122+
}
123+
124+
// Invalidated by sql.get on the same tagged literal
125+
letit=lsql.iterate`SELECT * FROM foo ORDER BY id ASC`;
126+
it.next();
127+
// eslint-disable-next-line no-unused-expressions
128+
lsql.get`SELECT * FROM foo ORDER BY id ASC`;
129+
assert.throws(()=>{it.next();},{
130+
code: 'ERR_INVALID_STATE',
131+
message: /iteratorwasinvalidated/,
132+
});
133+
134+
// Invalidated by sql.all on the same tagged literal
135+
it=lsql.iterate`SELECT * FROM foo ORDER BY id ASC`;
136+
it.next();
137+
// eslint-disable-next-line no-unused-expressions
138+
lsql.all`SELECT * FROM foo ORDER BY id ASC`;
139+
assert.throws(()=>{it.next();},{
140+
code: 'ERR_INVALID_STATE',
141+
message: /iteratorwasinvalidated/,
142+
});
143+
144+
// Invalidated by sql.run on the same tagged literal
145+
it=lsql.iterate`SELECT * FROM foo ORDER BY id ASC`;
146+
it.next();
147+
// eslint-disable-next-line no-unused-expressions
148+
lsql.run`SELECT * FROM foo ORDER BY id ASC`;
149+
assert.throws(()=>{it.next();},{
150+
code: 'ERR_INVALID_STATE',
151+
message: /iteratorwasinvalidated/,
152+
});
153+
154+
// Invalidated by a new sql.iterate on the same tagged literal
155+
it=lsql.iterate`SELECT * FROM foo ORDER BY id ASC`;
156+
it.next();
157+
constit2=lsql.iterate`SELECT * FROM foo ORDER BY id ASC`;
158+
assert.throws(()=>{it.next();},{
159+
code: 'ERR_INVALID_STATE',
160+
message: /iteratorwasinvalidated/,
161+
});
162+
163+
// The fresh iterator still works.
164+
assert.strictEqual(it2.next().done,false);
165+
166+
ldb.close();
167+
});
168+
169+
test('a stale iterator cannot replay a victim-bound write',()=>{
170+
constbank=newDatabaseSync(':memory:');
171+
consttx=bank.createTagStore();
172+
bank.exec(`
173+
CREATE TABLE acct(user TEXT PRIMARY KEY, balance INTEGER);
174+
CREATE TABLE transfers(id INTEGER PRIMARY KEY AUTOINCREMENT,
175+
from_user TEXT, to_user TEXT, amount INTEGER);
176+
CREATE TRIGGER debit AFTER INSERT ON transfers
177+
BEGIN
178+
UPDATE acct SET balance = balance - NEW.amount WHERE user = NEW.from_user;
179+
UPDATE acct SET balance = balance + NEW.amount WHERE user = NEW.to_user;
180+
END;
181+
INSERT INTO acct VALUES ('victim', 1000);
182+
INSERT INTO acct VALUES ('attacker', 0);
183+
`);
184+
185+
// Attacker arms an iterator without stepping it. The static parts of this
186+
// tagged literal must be byte-identical to the victim's below so they share
187+
// the same cached statement.
188+
constarmed=tx.iterate`INSERT INTO transfers(from_user, to_user, amount) VALUES (${'attacker'}, ${'attacker'}, ${0}) RETURNING id`;
189+
190+
// Victim makes one legitimate transfer through the same tagged literal.
191+
// eslint-disable-next-line no-unused-expressions
192+
tx.get`INSERT INTO transfers(from_user, to_user, amount) VALUES (${'victim'}, ${'attacker'}, ${100}) RETURNING id`;
193+
194+
// The stale iterator must not be able to replay the victim's write.
195+
assert.throws(()=>{armed.next();},{
196+
code: 'ERR_INVALID_STATE',
197+
message: /iteratorwasinvalidated/,
198+
});
199+
200+
constbalances=bank.prepare('SELECT user, balance FROM acct ORDER BY user').all();
201+
assert.deepStrictEqual(balances,[
202+
{__proto__: null,user: 'attacker',balance: 100},
203+
{__proto__: null,user: 'victim',balance: 900},
204+
]);
205+
constcount=bank.prepare('SELECT COUNT(*) AS c FROM transfers').get().c;
206+
assert.strictEqual(count,1);
207+
208+
bank.close();
209+
});
210+
211+
test('a finished iterator stays done and does not restart',()=>{
212+
// eslint-disable-next-line no-unused-expressions
213+
sql.run`INSERT INTO foo (text) VALUES (${'bob'})`;
214+
215+
constiter=sql.iterate`SELECT * FROM foo ORDER BY id ASC`;
216+
assert.strictEqual(iter.next().done,false);
217+
assert.strictEqual(iter.next().done,true);
218+
// A subsequent next() must not restart the statement.
219+
assert.strictEqual(iter.next().done,true);
220+
assert.strictEqual(iter.next().done,true);
221+
});
222+
115223
test('sql.db returns the associated DatabaseSync instance',()=>{
116224
assert.strictEqual(sql.db,db);
117225
});

0 commit comments

Comments
 (0)