CVE-2020-26273: Improper Neutralization of Special Elements used in a Command ('Command Injection') (CWE-77)
value: osquery is a SQL powered operating system instrumentation, monitoring, and analytics framework. In osquery before version 4.6.0, by using sqlite's ATTACH verb, someone with administrative access to osquery can cause reads and writes to arbitrary sqlite databases on disk. This _does_ allow arbitrary files to be created, but they will be sqlite databases. It does not appear to allow existing non-sqlite files to be overwritten. This has been patched in osquery 4.6.0. There are several mitigating factors and possible workarounds. In some deployments, the people with access to these interfaces may be considered administrators. In some deployments, configuration is managed by a central tool. This tool can filter for the `ATTACH` keyword. osquery can be run as non-root user. Because this also limits the desired access levels, this requires deployment specific testing and configuration.
CWE description: The product constructs all or part of a command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended command when it is sent to a downstream component.
CVSS v3 base score: 5.2
Published: 2020-12-16T02:15Z
Repository: https://github.com/osquery/osquery
Language: C++
Files changed: osquery/sql/sqlite_util.cpp, osquery/sql/sqlite_util.h, osquery/sql/tests/sqlite_util_tests.cpp, osquery/sql/virtual_sqlite_table.cpp
Commit date: 2020-12-14 15:41:57 -0500
Commit message: 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
Diff: diff --git a/osquery/sql/sqlite_util.cpp b/osquery/sql/sqlite_util.cpp
index eb0dc95d1..0f1700d08 100644
--- a/osquery/sql/sqlite_util.cpp
+++ b/osquery/sql/sqlite_util.cpp
@@ -14,10 +14,11 @@
 
 #include <osquery/utils/conversions/castvariant.h>
 
 #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>
 
 #include <osquery/utils/conversions/split.h>
@@ -268,10 +269,27 @@ SQLiteDBInstance::SQLiteDBInstance(sqlite3*& db, Mutex& mtx)
     VLOG(1) << "DBManager contention: opening transient SQLite database";
     init();
   }
 }
 
+// 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);
 
   std::string settings;
   for (const auto& setting : kMemoryDBSettings) {
@@ -288,10 +306,16 @@ static inline void openOptimized(sqlite3*& db) {
   registerOperationExtensions(db);
 #endif
   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() {
   primary_ = false;
   openOptimized(db_);
@@ -456,10 +480,11 @@ SQLiteDBInstanceRef SQLiteDBManager::getConnection(bool primary) {
   // Create a 'database connection' for the managed database instance.
   auto instance = std::make_shared<SQLiteDBInstance>(self.db_, self.mutex_);
   if (!instance->isPrimary()) {
     attachVirtualTables(instance);
   }
+
   return instance;
 }
 
 SQLiteDBManager::~SQLiteDBManager() {
   connection_ = nullptr;
diff --git a/osquery/sql/sqlite_util.h b/osquery/sql/sqlite_util.h
index 91b967eab..fac3662ac 100644
--- a/osquery/sql/sqlite_util.h
+++ b/osquery/sql/sqlite_util.h
@@ -27,10 +27,51 @@
 
 #define SQLITE_SOFT_HEAP_LIMIT (5 * 1024 * 1024)
 
 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;
 
 /**
  * @brief An RAII wrapper around an `sqlite3` object.
  *
diff --git a/osquery/sql/tests/sqlite_util_tests.cpp b/osquery/sql/tests/sqlite_util_tests.cpp
index 2408082a8..9c9f13a83 100644
--- a/osquery/sql/tests/sqlite_util_tests.cpp
+++ b/osquery/sql/tests/sqlite_util_tests.cpp
@@ -34,13 +34,14 @@ 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)"};
 
   for (auto q : queries) {
     sqlite3_exec(dbc->db(), q.c_str(), nullptr, nullptr, &err);
@@ -432,11 +433,11 @@ TEST_F(SQLiteUtilTests, test_column_type_determination) {
                     TypeMap({{"seconds", INTEGER_TYPE}}));
   // Correctly recognizing values from columns declared double as double, even
   // 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}}));
 }
 
@@ -448,6 +449,19 @@ TEST_F(SQLiteUtilTests, test_enable) {
   // Fake_table is explicitely in enabled_tables and
   // disable_tables, it should be disabled.
   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
diff --git a/osquery/sql/virtual_sqlite_table.cpp b/osquery/sql/virtual_sqlite_table.cpp
index f83c27d21..2dc0691fc 100644
--- a/osquery/sql/virtual_sqlite_table.cpp
+++ b/osquery/sql/virtual_sqlite_table.cpp
@@ -94,10 +94,18 @@ Status genTableRowsForSqliteTable(const fs::path& sqlite_db,
       sqlite3_close(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) {
     sqlite3_close(db);
     VLOG(1) << "ATC table: Could not prepare database at path: " << sqlite_db;

Vulnerable code:       "CREATE TABLE test_table (username varchar(30) primary key, age int)",
      "CREATE TABLE test_types_table (username varchar(30) primary key, age "
Fixed code: #include <osquery/core/shutdown.h>
// 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;
}


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

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,
};


      "CREATE TABLE test_table (username varchar(30), age int)",
      "CREATE TABLE test_types_table (username varchar(30), age "
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);
}

  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);
  }