Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-4g56-2482-x7q8
* Proposed fix for attach tables vulnerability

* Add authorizer to ATC tables and cleanups

- Add unit test for authorizer function
  • Loading branch information
zwass committed Dec 14, 2020
1 parent dcfbd89 commit c3f9a3d
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 2 deletions.
25 changes: 25 additions & 0 deletions osquery/sql/sqlite_util.cpp
Expand Up @@ -16,6 +16,7 @@

#include <osquery/core/core.h>
#include <osquery/core/flags.h>
#include <osquery/core/shutdown.h>
#include <osquery/logger/logger.h>
#include <osquery/registry/registry_factory.h>
#include <osquery/sql/sql.h>
Expand Down Expand Up @@ -270,6 +271,23 @@ SQLiteDBInstance::SQLiteDBInstance(sqlite3*& db, Mutex& mtx)
}
}

// This function is called by SQLite when a statement is prepared and we use
// it to allowlist specific actions.
int sqliteAuthorizer(void* userData,
int code,
const char* arg3,
const char* arg4,
const char* arg5,
const char* arg6) {
if (kAllowedSQLiteActionCodes.count(code) > 0) {
return SQLITE_OK;
}
LOG(ERROR) << "Authorizer denied action " << code << " "
<< (arg3 ? arg3 : "null") << " " << (arg4 ? arg4 : "null") << " "
<< (arg5 ? arg5 : "null") << " " << (arg6 ? arg6 : "null");
return SQLITE_DENY;
}

static inline void openOptimized(sqlite3*& db) {
sqlite3_open(":memory:", &db);

Expand All @@ -290,6 +308,12 @@ static inline void openOptimized(sqlite3*& db) {
registerFilesystemExtensions(db);
registerHashingExtensions(db);
registerEncodingExtensions(db);

auto rc = sqlite3_set_authorizer(db, &sqliteAuthorizer, nullptr);
if (rc != SQLITE_OK) {
LOG(ERROR) << "Failed to set sqlite authorizer: " << sqlite3_errmsg(db);
requestShutdown(rc);
}
}

void SQLiteDBInstance::init() {
Expand Down Expand Up @@ -458,6 +482,7 @@ SQLiteDBInstanceRef SQLiteDBManager::getConnection(bool primary) {
if (!instance->isPrimary()) {
attachVirtualTables(instance);
}

return instance;
}

Expand Down
41 changes: 41 additions & 0 deletions osquery/sql/sqlite_util.h
Expand Up @@ -29,6 +29,47 @@

namespace osquery {

int sqliteAuthorizer(void* userData,
int code,
const char* arg3,
const char* arg4,
const char* arg5,
const char* arg6);

// Allowlist of SQLite actions. Any action not in this set is denied. See
// possible values in https://sqlite.org/c3ref/c_alter_table.html.
// ** Never allow SQLITE_ATTACH ** as it can be used to write arbitrary files.
const std::unordered_set<int> kAllowedSQLiteActionCodes = {
// Enable basic functionality
SQLITE_READ,
SQLITE_SELECT,

// Some extensions implement writeable tables
SQLITE_INSERT,
SQLITE_UPDATE,
SQLITE_DELETE,

// Allow virtual tables to be attached
SQLITE_CREATE_VTABLE,
SQLITE_DROP_VTABLE,

// Users may sometimes want to create tables and views
SQLITE_CREATE_VIEW,
SQLITE_DROP_VIEW,
SQLITE_CREATE_TABLE,
SQLITE_DROP_TABLE,
SQLITE_CREATE_TEMP_TABLE,
SQLITE_DROP_TEMP_TABLE,
SQLITE_CREATE_TEMP_VIEW,
SQLITE_DROP_TEMP_VIEW,

// Required to allow calling functions in SQL
SQLITE_FUNCTION,

// Required for recursive queries
SQLITE_RECURSIVE,
};

class SQLiteDBManager;

/**
Expand Down
18 changes: 16 additions & 2 deletions osquery/sql/tests/sqlite_util_tests.cpp
Expand Up @@ -36,9 +36,10 @@ class SQLiteUtilTests : public testing::Test {

std::shared_ptr<SQLiteDBInstance> getTestDBC() {
auto dbc = SQLiteDBManager::getUnique();

char* err = nullptr;
std::vector<std::string> queries = {
"CREATE TABLE test_table (username varchar(30) primary key, age int)",
"CREATE TABLE test_table (username varchar(30), age int)",
"INSERT INTO test_table VALUES (\"mike\", 23)",
"INSERT INTO test_table VALUES (\"matt\", 24)"};

Expand Down Expand Up @@ -434,7 +435,7 @@ TEST_F(SQLiteUtilTests, test_column_type_determination) {
// if they happen to have integer value. And also test multi-statement
// queries.
testTypesExpected(
"CREATE TABLE test_types_table (username varchar(30) primary key, age "
"CREATE TABLE test_types_table (username varchar(30), age "
"double);INSERT INTO test_types_table VALUES (\"mike\", 23); SELECT age "
"from test_types_table",
TypeMap({{"age", DOUBLE_TYPE}}));
Expand All @@ -450,4 +451,17 @@ TEST_F(SQLiteUtilTests, test_enable) {
ASSERT_TRUE(SQLiteDBManager::isDisabled("fake_table"));
}

TEST_F(SQLiteUtilTests, test_sqlite_authorizer) {
auto rc = sqliteAuthorizer(
nullptr, SQLITE_ATTACH, nullptr, nullptr, nullptr, nullptr);
EXPECT_EQ(SQLITE_DENY, rc);

rc = sqliteAuthorizer(nullptr, 534, nullptr, nullptr, nullptr, nullptr);
EXPECT_EQ(SQLITE_DENY, rc);

rc = sqliteAuthorizer(
nullptr, SQLITE_SELECT, nullptr, nullptr, nullptr, nullptr);
EXPECT_EQ(SQLITE_OK, rc);
}

} // namespace osquery
8 changes: 8 additions & 0 deletions osquery/sql/virtual_sqlite_table.cpp
Expand Up @@ -96,6 +96,14 @@ Status genTableRowsForSqliteTable(const fs::path& sqlite_db,
return Status(1, "Could not open database");
}

rc = sqlite3_set_authorizer(db, &sqliteAuthorizer, nullptr);
if (rc != SQLITE_OK) {
sqlite3_close(db);
auto errMsg =
std::string("Failed to set sqlite authorizer: ") + sqlite3_errmsg(db);
return Status(1, errMsg);
}

sqlite3_stmt* stmt = nullptr;
rc = sqlite3_prepare_v2(db, sqlite_query.c_str(), -1, &stmt, nullptr);
if (rc != SQLITE_OK) {
Expand Down

0 comments on commit c3f9a3d

Please sign in to comment.