From 7af056226185752ae07fcf00943230a37cd28e49 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 14 Jul 2026 19:01:00 -0500 Subject: [PATCH 1/4] Create the offline event cache with owner-only (0600) permissions The SQLite offline cache buffers pending telemetry events (tenant ids, user identifiers, serialized event payloads) but was created with SQLite's default file permissions (0644 -- world-readable), letting any co-located user on a POSIX system read the buffered event stream or tamper with pending events. Restrict the database file to 0600 immediately after opening it in SQLiteWrapper::open. This runs before WAL is enabled, so the -wal/-journal companion files inherit 0600 from the main database file (SQLite's findCreateFileMode derives their mode from the main db). The chmod is best-effort (a failure, e.g. an in-memory ":memory:" database, does not fail the open) and POSIX-only -- on Windows the Unix mode bits are meaningless (NTFS ACLs govern access). Adds a POSIX unit test asserting the cache and its companions are not group/world accessible. Verified on Linux (umask 022): a bare SQLite db is created 0644; with this change the cache and its -wal/-journal are 0600. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/SQLiteWrapper.hpp | 20 +++++++++++ .../unittests/OfflineStorageTests_SQLite.cpp | 33 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/lib/offline/SQLiteWrapper.hpp b/lib/offline/SQLiteWrapper.hpp index 982b4053b..922b34c26 100644 --- a/lib/offline/SQLiteWrapper.hpp +++ b/lib/offline/SQLiteWrapper.hpp @@ -16,6 +16,11 @@ #include #include +#if !defined(_WIN32) +#include +#include +#endif + namespace MAT_NS_BEGIN { using SQLRecord = std::vector; @@ -300,6 +305,21 @@ namespace MAT_NS_BEGIN { g_sqlite3Proxy->sqlite3_extended_result_codes(m_db, 1); + // SECURITY: the offline cache buffers pending telemetry/audit events + // (tenant ids, user identifiers, serialized event payloads). SQLite creates + // the database file with SQLITE_DEFAULT_FILE_PERMISSIONS -- 0644, i.e. + // world-readable -- so restrict it to owner read/write only (0600). This runs + // before WAL is enabled: SQLite derives the -wal/-journal permissions from the + // main database file (findCreateFileMode), so those companions inherit 0600. + // POSIX only -- on Windows the Unix mode bits are meaningless (access is + // governed by NTFS ACLs). Best-effort: a failure (e.g. an in-memory ":memory:" + // database, or a filesystem that ignores chmod) must not fail the open. +#if !defined(_WIN32) + if (::chmod(filename.c_str(), S_IRUSR | S_IWUSR) != 0) { + LOG_WARN("Could not restrict database file permissions to 0600 (errno %d)", errno); + } +#endif + if (!registerTokenizeFunction()) { shutdown(); return false; diff --git a/tests/unittests/OfflineStorageTests_SQLite.cpp b/tests/unittests/OfflineStorageTests_SQLite.cpp index e90b0a9ae..b8ea948cc 100644 --- a/tests/unittests/OfflineStorageTests_SQLite.cpp +++ b/tests/unittests/OfflineStorageTests_SQLite.cpp @@ -12,6 +12,9 @@ #include "offline/OfflineStorage_SQLite.hpp" #include #include +#if !defined(_WIN32) +#include +#endif #include "NullObjects.hpp" @@ -839,4 +842,34 @@ TEST_F(OfflineStorageTests_SQLite, SqliteDbInstancesAreCounted) shutdownAndRemoveFile(); EXPECT_EQ(offlineStorage->GetDbInstanceCount(), 0); } + +#if !defined(_WIN32) +// SECURITY: the offline cache buffers pending telemetry/audit events, so it must +// not be world-readable. SQLite creates the file 0644 by default; SQLiteWrapper +// tightens it to 0600 after open, and the -wal/-journal companions inherit that +// mode from the main database file. POSIX-only (mode bits are meaningless on +// Windows, where access is governed by NTFS ACLs). +TEST_F(OfflineStorageTests_SQLite, CacheFileCreatedOwnerReadWriteOnly) +{ + initializeStorage(); + + struct stat st; + ASSERT_EQ(0, ::stat(storageFilename.c_str(), &st)) << "cache database file was not created"; + EXPECT_EQ(static_cast(S_IRUSR | S_IWUSR), static_cast(st.st_mode & 0777)) + << "offline cache database must be created 0600, not world-readable"; + + // Any WAL/journal/shm companion that exists must not grant group or other access + // (SQLite derives their permissions from the main database file's mode). + for (const char* suffix : { "-wal", "-journal", "-shm" }) + { + struct stat cst; + const std::string companion = storageFilename + suffix; + if (::stat(companion.c_str(), &cst) == 0) + { + EXPECT_EQ(0, static_cast(cst.st_mode & (S_IRWXG | S_IRWXO))) + << "companion file " << suffix << " must not be group/world accessible"; + } + } +} +#endif #endif From fcb9e935f1bdd2f331e000cd5bd6525f0370d5b2 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 14 Jul 2026 20:43:59 -0500 Subject: [PATCH 2/4] Also tighten pre-existing companion files on open (round 1) Address Copilot review: only the main database was chmod'd, so a cache created by an older SDK (or a companion left behind after a crash) could retain the old world-readable 0644 mode on its -wal/-shm/-journal files. Best-effort chmod any pre-existing companions to 0600 on open, ignoring ENOENT. Add ExistingFilesAreTightenedOnOpen covering the migration path (loosen db + plant a leftover -wal, then verify reopen re-tightens both). Files: - lib/offline/SQLiteWrapper.hpp - tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/SQLiteWrapper.hpp | 17 +++++++--- .../unittests/OfflineStorageTests_SQLite.cpp | 33 +++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/lib/offline/SQLiteWrapper.hpp b/lib/offline/SQLiteWrapper.hpp index 922b34c26..9fce5de82 100644 --- a/lib/offline/SQLiteWrapper.hpp +++ b/lib/offline/SQLiteWrapper.hpp @@ -310,14 +310,23 @@ namespace MAT_NS_BEGIN { // the database file with SQLITE_DEFAULT_FILE_PERMISSIONS -- 0644, i.e. // world-readable -- so restrict it to owner read/write only (0600). This runs // before WAL is enabled: SQLite derives the -wal/-journal permissions from the - // main database file (findCreateFileMode), so those companions inherit 0600. - // POSIX only -- on Windows the Unix mode bits are meaningless (access is - // governed by NTFS ACLs). Best-effort: a failure (e.g. an in-memory ":memory:" - // database, or a filesystem that ignores chmod) must not fail the open. + // main database file (findCreateFileMode), so companions it creates inherit + // 0600. A cache created by an older SDK (before this fix) may already have + // companion files on disk with the old 0644 mode, so tighten any pre-existing + // ones too. POSIX only -- on Windows the Unix mode bits are meaningless (access + // is governed by NTFS ACLs). Best-effort: a failure (e.g. an in-memory + // ":memory:" database, or a filesystem that ignores chmod) must not fail the + // open; a missing companion (ENOENT) is expected and ignored. #if !defined(_WIN32) if (::chmod(filename.c_str(), S_IRUSR | S_IWUSR) != 0) { LOG_WARN("Could not restrict database file permissions to 0600 (errno %d)", errno); } + for (const char* suffix : { "-wal", "-shm", "-journal" }) { + std::string companion = filename + suffix; + if (::chmod(companion.c_str(), S_IRUSR | S_IWUSR) != 0 && errno != ENOENT) { + LOG_WARN("Could not restrict %s file permissions to 0600 (errno %d)", suffix, errno); + } + } #endif if (!registerTokenizeFunction()) { diff --git a/tests/unittests/OfflineStorageTests_SQLite.cpp b/tests/unittests/OfflineStorageTests_SQLite.cpp index b8ea948cc..867ab634d 100644 --- a/tests/unittests/OfflineStorageTests_SQLite.cpp +++ b/tests/unittests/OfflineStorageTests_SQLite.cpp @@ -871,5 +871,38 @@ TEST_F(OfflineStorageTests_SQLite, CacheFileCreatedOwnerReadWriteOnly) } } } + +// A cache written by an older SDK (or left behind after a crash) can have the +// database and companion files already on disk with the old world-readable 0644 +// mode; SQLite only derives 0600 for companions it creates itself. Opening the +// storage must re-tighten both the database and any pre-existing companion. +TEST_F(OfflineStorageTests_SQLite, ExistingFilesAreTightenedOnOpen) +{ + initializeStorage(); + offlineStorage->Shutdown(); + storageInitialized = false; + + // Simulate an old cache: loosen the database and plant a leftover -wal at 0644. + const mode_t loose = S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH; // 0644 + ASSERT_EQ(0, ::chmod(storageFilename.c_str(), loose)); + const std::string wal = storageFilename + "-wal"; + std::ofstream(wal, std::ios::binary); // empty leftover companion + ASSERT_EQ(0, ::chmod(wal.c_str(), loose)); + + // Reopen -- the open path must re-tighten both files. + initializeStorage(); + + struct stat st; + ASSERT_EQ(0, ::stat(storageFilename.c_str(), &st)); + EXPECT_EQ(0, static_cast(st.st_mode & (S_IRWXG | S_IRWXO))) + << "reopened database must be tightened to 0600"; + + struct stat wst; + if (::stat(wal.c_str(), &wst) == 0) + { + EXPECT_EQ(0, static_cast(wst.st_mode & (S_IRWXG | S_IRWXO))) + << "pre-existing -wal companion must be tightened to 0600"; + } +} #endif #endif From be5e2de121e9f58a5e9b1d8d577ff3f04a9bfd95 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 14 Jul 2026 21:28:39 -0500 Subject: [PATCH 3/4] Silence in-memory chmod warning and clean up test companions (round 2) Address Copilot review round 2: - The main-database chmod now ignores ENOENT, so opening an in-memory (":memory:") database -- which LogManagerImpl uses when no tenant token is configured -- no longer logs a spurious permission-tightening warning on every open (there is no file to secure). - ExistingFilesAreTightenedOnOpen now removes the database and its companion files (including the planted -wal) at the end, so they don't leak into other tests that reuse the same storage filename. Files: - lib/offline/SQLiteWrapper.hpp - tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/offline/SQLiteWrapper.hpp | 9 +++++---- tests/unittests/OfflineStorageTests_SQLite.cpp | 10 ++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/offline/SQLiteWrapper.hpp b/lib/offline/SQLiteWrapper.hpp index 9fce5de82..2a5f0d108 100644 --- a/lib/offline/SQLiteWrapper.hpp +++ b/lib/offline/SQLiteWrapper.hpp @@ -314,11 +314,12 @@ namespace MAT_NS_BEGIN { // 0600. A cache created by an older SDK (before this fix) may already have // companion files on disk with the old 0644 mode, so tighten any pre-existing // ones too. POSIX only -- on Windows the Unix mode bits are meaningless (access - // is governed by NTFS ACLs). Best-effort: a failure (e.g. an in-memory - // ":memory:" database, or a filesystem that ignores chmod) must not fail the - // open; a missing companion (ENOENT) is expected and ignored. + // is governed by NTFS ACLs). Best-effort: a failure (e.g. a filesystem that + // ignores chmod) must not fail the open, and a missing file -- ENOENT, e.g. an + // in-memory ":memory:" database, which has no file to secure -- is expected and + // silently ignored. #if !defined(_WIN32) - if (::chmod(filename.c_str(), S_IRUSR | S_IWUSR) != 0) { + if (::chmod(filename.c_str(), S_IRUSR | S_IWUSR) != 0 && errno != ENOENT) { LOG_WARN("Could not restrict database file permissions to 0600 (errno %d)", errno); } for (const char* suffix : { "-wal", "-shm", "-journal" }) { diff --git a/tests/unittests/OfflineStorageTests_SQLite.cpp b/tests/unittests/OfflineStorageTests_SQLite.cpp index 867ab634d..848cb20cb 100644 --- a/tests/unittests/OfflineStorageTests_SQLite.cpp +++ b/tests/unittests/OfflineStorageTests_SQLite.cpp @@ -903,6 +903,16 @@ TEST_F(OfflineStorageTests_SQLite, ExistingFilesAreTightenedOnOpen) EXPECT_EQ(0, static_cast(wst.st_mode & (S_IRWXG | S_IRWXO))) << "pre-existing -wal companion must be tightened to 0600"; } + + // Clean up the database and any companion files (the planted -wal in particular) + // so they don't leak into other tests that reuse the same storage filename. + offlineStorage->Shutdown(); + storageInitialized = false; + for (const char* suffix : { "-wal", "-shm", "-journal" }) + { + ::remove((storageFilename + suffix).c_str()); + } + ::remove(storageFilename.c_str()); } #endif #endif From 39b61dd43df0b78af8526f17c9a65e588b6d7545 Mon Sep 17 00:00:00 2001 From: Bhagirath Mehta Date: Tue, 14 Jul 2026 21:43:51 -0500 Subject: [PATCH 4/4] Remove WAL companion files in shared test teardown (round 3) Address Copilot review round 3: CacheFileCreatedOwnerReadWriteOnly (and any other WAL-mode test) relied on shutdownAndRemoveFile(), which only removed the main database and could leave -wal/-shm/-journal companions behind to pollute the temp dir. Remove the companions in the shared teardown helper so every test is covered, and drop the now-redundant explicit cleanup from ExistingFilesAreTightenedOnOpen. Files: - tests/unittests/OfflineStorageTests_SQLite.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/unittests/OfflineStorageTests_SQLite.cpp | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/unittests/OfflineStorageTests_SQLite.cpp b/tests/unittests/OfflineStorageTests_SQLite.cpp index 848cb20cb..d5aa6808a 100644 --- a/tests/unittests/OfflineStorageTests_SQLite.cpp +++ b/tests/unittests/OfflineStorageTests_SQLite.cpp @@ -92,6 +92,12 @@ struct OfflineStorageTests_SQLite : public Test EXPECT_THAT(fileExists(storageFilename), true); ::remove(storageFilename.c_str()); EXPECT_THAT(fileExists(storageFilename), false); + // WAL mode can leave -wal/-shm/-journal companions behind; remove them too + // so they don't leak into other tests that reuse the same storage filename. + for (const char* suffix : { "-wal", "-shm", "-journal" }) + { + ::remove((storageFilename + suffix).c_str()); + } } } @@ -903,16 +909,6 @@ TEST_F(OfflineStorageTests_SQLite, ExistingFilesAreTightenedOnOpen) EXPECT_EQ(0, static_cast(wst.st_mode & (S_IRWXG | S_IRWXO))) << "pre-existing -wal companion must be tightened to 0600"; } - - // Clean up the database and any companion files (the planted -wal in particular) - // so they don't leak into other tests that reuse the same storage filename. - offlineStorage->Shutdown(); - storageInitialized = false; - for (const char* suffix : { "-wal", "-shm", "-journal" }) - { - ::remove((storageFilename + suffix).c_str()); - } - ::remove(storageFilename.c_str()); } #endif #endif