Skip to content

sqlite: reject connection access from authorizer callbacks #65156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
TrevorBurnham wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

sqlite: reject connection access from authorizer callbacks #65156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions doc/api/sqlite.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,11 @@ wrapper around [`sqlite3_create_function_v2()`][].

<!-- YAML
added: v24.10.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/65156
description: Accessing the invoking database connection from the authorizer
callback now throws.
-->

* `callback` {Function|null} The authorizer function to set, or `null` to
Expand All @@ -464,6 +469,21 @@ The callback must return one of the following constants:
* `SQLITE_DENY` - Deny the operation (causes an error).
* `SQLITE_IGNORE` - Ignore the operation (silently skip).

SQLite requires that the authorizer callback not modify the database connection
that invoked it, which includes preparing and stepping statements. Methods that
would do so throw an error with code `ERR_INVALID_STATE` while the callback is
on the stack, including `database.prepare()`, `database.exec()`, the execution
methods of that connection's statements, iterators, and tag stores, and
`database.setAuthorizer()` itself. Other connections remain usable.

The callback can also be invoked from within `statement.run()`,
`statement.get()`, and similar methods, because SQLite may re-prepare a
statement during execution after a schema change.

Separately, `statement.close()` throws if called from any callback SQLite
invokes during execution, such as a user-defined function, because finalizing a
statement that is mid-execution would free the virtual machine that is running.

```cjs
const { DatabaseSync, constants } = require('node:sqlite');
const db = new DatabaseSync(':memory:');
Expand Down
61 changes: 61 additions & 0 deletions src/node_sqlite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,24 @@ inline MaybeLocal<String> Utf8StringMaybeOneByte(Isolate* isolate,
} \
} while (0)

// SQLite requires that an authorizer callback not modify the connection that
// invoked it. Preparing and stepping statements both count as modifying it.
// See https://www.sqlite.org/c3ref/set_authorizer.html.
#define THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db) \
THROW_AND_RETURN_ON_BAD_STATE( \
(env), \
(db)->IsInAuthorizerCallback(), \
"database cannot be accessed from an authorizer callback")

// Finalizing a statement frees its virtual machine. Any callback that SQLite
// invokes from inside sqlite3_step() may be running on that very statement, so
// finalizing from one is a use-after-free rather than a contract violation.
#define THROW_AND_RETURN_IF_IN_CALLBACK(env, db) \
THROW_AND_RETURN_ON_BAD_STATE( \
(env), \
(db)->IsInCallback(), \
"statement cannot be finalized from a callback")

#define SQLITE_VALUE_TO_JS(from, isolate, use_big_int_args, result, ...) \
do { \
switch (sqlite3_##from##_type(__VA_ARGS__)) { \
Expand Down Expand Up @@ -825,6 +843,12 @@ Intercepted DatabaseSyncLimits::LimitsSetter(
return Intercepted::kYes;
}

if (limits->database_->IsInAuthorizerCallback()) {
THROW_ERR_INVALID_STATE(
env, "database cannot be accessed from an authorizer callback");
return Intercepted::kYes;
}

if (!value->IsNumber()) {
THROW_ERR_INVALID_ARG_TYPE(
isolate, "Limit value must be a non-negative integer or Infinity.");
Expand Down Expand Up @@ -1081,6 +1105,7 @@ void DatabaseSync::CreateTagStore(const FunctionCallbackInfo<Value>& args) {
THROW_ERR_INVALID_STATE(env, "database is not open");
return;
}
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);
int capacity = 1000;
if (args.Length() > 0 && !args[0]->IsUndefined()) {
if (!args[0]->IsNumber()) {
Expand Down Expand Up @@ -1483,6 +1508,7 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

if (!args[0]->IsString()) {
THROW_ERR_INVALID_ARG_TYPE(env->isolate(),
Expand Down Expand Up @@ -1606,6 +1632,7 @@ void DatabaseSync::Exec(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

if (!args[0]->IsString()) {
THROW_ERR_INVALID_ARG_TYPE(env->isolate(),
Expand All @@ -1630,6 +1657,7 @@ void DatabaseSync::CustomFunction(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

if (!args[0]->IsString()) {
THROW_ERR_INVALID_ARG_TYPE(env->isolate(),
Expand Down Expand Up @@ -1803,6 +1831,7 @@ void DatabaseSync::Serialize(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

std::string db_name = "main";
if (!args[0]->IsUndefined()) {
Expand Down Expand Up @@ -1858,6 +1887,7 @@ void DatabaseSync::Deserialize(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

if (!args[0]->IsUint8Array()) {
THROW_ERR_INVALID_ARG_TYPE(env->isolate(),
Expand Down Expand Up @@ -1933,6 +1963,7 @@ void DatabaseSync::AggregateFunction(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);
Utf8Value name(env->isolate(), args[0].As<String>());
Local<Object> options = args[1].As<Object>();
Local<Value> start_v;
Expand Down Expand Up @@ -2144,6 +2175,7 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo<Value>& args) {
DatabaseSync* db;
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

sqlite3_session* pSession;
int r = sqlite3session_create(db->connection_, db_name.c_str(), &pSession);
Expand Down Expand Up @@ -2313,6 +2345,7 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

if (!args[0]->IsUint8Array()) {
THROW_ERR_INVALID_ARG_TYPE(
Expand Down Expand Up @@ -2447,6 +2480,7 @@ void DatabaseSync::EnableLoadExtension(
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

Isolate* isolate = env->isolate();
if (!args[0]->IsBoolean()) {
Expand Down Expand Up @@ -2475,6 +2509,7 @@ void DatabaseSync::EnableDefensive(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

Isolate* isolate = env->isolate();
if (!args[0]->IsBoolean()) {
Expand All @@ -2500,6 +2535,7 @@ void DatabaseSync::LoadExtension(const FunctionCallbackInfo<Value>& args) {
env, !db->allow_load_extension_, "extension loading is not allowed");
THROW_AND_RETURN_ON_BAD_STATE(
env, !db->enable_load_extension_, "extension loading is not allowed");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

if (!args[0]->IsString()) {
THROW_ERR_INVALID_ARG_TYPE(env->isolate(),
Expand Down Expand Up @@ -2528,6 +2564,7 @@ void DatabaseSync::SetAuthorizer(const FunctionCallbackInfo<Value>& args) {
ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, db);

Isolate* isolate = env->isolate();

Expand Down Expand Up @@ -2564,6 +2601,7 @@ int DatabaseSync::AuthorizerCallback(void* user_data,
const char* param4) {
DatabaseSync* db = static_cast<DatabaseSync*>(user_data);
CallbackDepthGuard guard(db);
AuthorizerDepthGuard authorizer_guard(db);
Environment* env = db->env();
Isolate* isolate = env->isolate();
HandleScope handle_scope(isolate);
Expand Down Expand Up @@ -2677,12 +2715,20 @@ void StatementSync::Close(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_IF_IN_CALLBACK(env, stmt->db_.get());
stmt->Close();
}

void StatementSync::Dispose(const FunctionCallbackInfo<Value>& args) {
StatementSync* stmt;
ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This());
Environment* env = Environment::GetCurrent(args);
// Disposal is idempotent, so an already-finalized statement is a no-op even
// inside a callback.
if (stmt->IsFinalized()) {
return;
}
THROW_AND_RETURN_IF_IN_CALLBACK(env, stmt->db_.get());
stmt->Close();
}

Expand Down Expand Up @@ -3127,6 +3173,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get());
Isolate* isolate = env->isolate();
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
Expand Down Expand Up @@ -3154,6 +3201,7 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get());
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());

Expand All @@ -3177,6 +3225,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get());
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());

Expand All @@ -3201,6 +3250,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, stmt->db_.get());
int r = stmt->ResetStatement();
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());

Expand Down Expand Up @@ -3483,6 +3533,7 @@ void SQLTagStore::Run(const FunctionCallbackInfo<Value>& args) {

THROW_AND_RETURN_ON_BAD_STATE(
env, !session->database_->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get());

BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);

Expand All @@ -3509,6 +3560,7 @@ void SQLTagStore::Iterate(const FunctionCallbackInfo<Value>& args) {

THROW_AND_RETURN_ON_BAD_STATE(
env, !session->database_->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get());

BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);

Expand Down Expand Up @@ -3537,6 +3589,7 @@ void SQLTagStore::Get(const FunctionCallbackInfo<Value>& args) {

THROW_AND_RETURN_ON_BAD_STATE(
env, !session->database_->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get());

BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);

Expand Down Expand Up @@ -3566,6 +3619,7 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {

THROW_AND_RETURN_ON_BAD_STATE(
env, !session->database_->IsOpen(), "database is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get());

BaseObjectPtr<StatementSync> stmt = PrepareStatement(args);

Expand All @@ -3592,6 +3646,10 @@ void SQLTagStore::All(const FunctionCallbackInfo<Value>& args) {
void SQLTagStore::Clear(const FunctionCallbackInfo<Value>& args) {
SQLTagStore* store;
ASSIGN_OR_RETURN_UNWRAP(&store, args.This());
Environment* env = Environment::GetCurrent(args);
if (store->database_) {
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, store->database_.get());
}
store->sql_tags_.Clear();
}

Expand Down Expand Up @@ -3785,6 +3843,7 @@ void StatementSyncIterator::Next(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, iter->stmt_->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, iter->stmt_->db_.get());
Isolate* isolate = env->isolate();

auto iter_template = getLazyIterTemplate(env);
Expand Down Expand Up @@ -3862,6 +3921,7 @@ void StatementSyncIterator::Return(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, iter->stmt_->IsFinalized(), "statement has been finalized");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, iter->stmt_->db_.get());
Isolate* isolate = env->isolate();

sqlite3_reset(iter->stmt_->statement_);
Expand Down Expand Up @@ -3940,6 +4000,7 @@ void Session::Changeset(const FunctionCallbackInfo<Value>& args) {
env, !session->database_->IsOpen(), "database is not open");
THROW_AND_RETURN_ON_BAD_STATE(
env, session->session_ == nullptr, "session is not open");
THROW_AND_RETURN_IF_IN_AUTHORIZER(env, session->database_.get());

int nChangeset;
void* pChangeset;
Expand Down
21 changes: 21 additions & 0 deletions src/node_sqlite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,13 @@ class DatabaseSync : public BaseObject {
void DecrementCallbackDepth() { --callback_depth_; }
bool IsInCallback() const { return callback_depth_ > 0; }

// SQLite forbids an authorizer callback from doing anything that modifies
// the database connection that invoked it, which includes preparing and
// stepping statements. See https://www.sqlite.org/c3ref/set_authorizer.html.
void IncrementAuthorizerDepth() { ++authorizer_depth_; }
void DecrementAuthorizerDepth() { --authorizer_depth_; }
bool IsInAuthorizerCallback() const { return authorizer_depth_ > 0; }

SET_MEMORY_INFO_NAME(DatabaseSync)
SET_SELF_SIZE(DatabaseSync)

Expand All @@ -247,6 +254,7 @@ class DatabaseSync : public BaseObject {
sqlite3* connection_;
bool ignore_next_sqlite_error_;
int callback_depth_ = 0;
int authorizer_depth_ = 0;

std::set<BackupJob*> backups_;
std::unordered_set<Session*> sessions_;
Expand Down Expand Up @@ -426,6 +434,19 @@ class CallbackDepthGuard {
DatabaseSync* db_;
};

class AuthorizerDepthGuard {
public:
explicit AuthorizerDepthGuard(DatabaseSync* db) : db_(db) {
db_->IncrementAuthorizerDepth();
}
~AuthorizerDepthGuard() { db_->DecrementAuthorizerDepth(); }
AuthorizerDepthGuard(const AuthorizerDepthGuard&) = delete;
AuthorizerDepthGuard& operator=(const AuthorizerDepthGuard&) = delete;

private:
DatabaseSync* db_;
};

class UserDefinedFunction {
public:
UserDefinedFunction(Environment* env,
Expand Down
Loading