From 748f712411eaea22adf6cca90dddef8c672732eb Mon Sep 17 00:00:00 2001 From: Adrien Langou Date: Wed, 13 May 2026 16:19:25 +0200 Subject: [PATCH] fix(server): restrict SQLite database file permissions to 0o600 Signed-off-by: Adrien Langou --- architecture/gateway.md | 6 + .../src/persistence/sqlite.rs | 47 ++++- .../openshell-server/src/persistence/tests.rs | 177 ++++++++++++++++++ 3 files changed, 228 insertions(+), 2 deletions(-) diff --git a/architecture/gateway.md b/architecture/gateway.md index 383430503..68832d0cf 100644 --- a/architecture/gateway.md +++ b/architecture/gateway.md @@ -104,6 +104,12 @@ This keeps the gateway data model portable across storage backends and leaves room for future stores that can provide the same object, label, version, and scope semantics. +The SQLite adapter tightens the on-disk database file to mode `0o600` on every +connect so that provider API keys, SSH session tokens, and sandbox metadata are +not readable by other local users on shared hosts. The same restriction is +reapplied to the `-wal` and `-shm` sidecars (created by SQLite's +default WAL journal mode), which mirror the same sensitive contents. + Persisted state includes sandboxes, providers, SSH sessions, policy revisions, settings, inference configuration, and deployment records. diff --git a/crates/openshell-server/src/persistence/sqlite.rs b/crates/openshell-server/src/persistence/sqlite.rs index fafb07597..b66e5dabe 100644 --- a/crates/openshell-server/src/persistence/sqlite.rs +++ b/crates/openshell-server/src/persistence/sqlite.rs @@ -2,15 +2,17 @@ // SPDX-License-Identifier: Apache-2.0 use super::{ - DraftChunkRecord, ObjectRecord, PersistenceResult, PolicyRecord, current_time_ms, map_db_error, - map_migrate_error, + DraftChunkRecord, ObjectRecord, PersistenceError, PersistenceResult, PolicyRecord, + current_time_ms, map_db_error, map_migrate_error, }; use crate::policy_store::{ draft_chunk_payload_from_record, draft_chunk_record_from_parts, policy_payload_from_record, policy_record_from_parts, }; +use openshell_core::paths::set_file_owner_only; use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; use sqlx::{Row, SqlitePool}; +use std::path::{Path, PathBuf}; use std::str::FromStr; static SQLITE_MIGRATOR: sqlx::migrate::Migrator = sqlx::migrate!("./migrations/sqlite"); @@ -40,11 +42,20 @@ impl SqliteStore { pool_options = pool_options.idle_timeout(None).max_lifetime(None); } + // Capture the on-disk path before `connect_with` consumes the options + // so we can restrict the permissions after the database is connected. + let db_path = (!is_in_memory).then(|| options.get_filename().to_path_buf()); + let pool = pool_options .connect_with(options) .await .map_err(|e| map_db_error(&e))?; + // Tighten the permissions of the database file to owner-only access (0o600). + if let Some(path) = db_path { + restrict_db_file_permissions(&path)?; + } + Ok(Self { pool }) } @@ -616,6 +627,38 @@ fn draft_chunk_dedup_key(chunk: &DraftChunkRecord) -> String { format!("{}|{}|{}", chunk.host, chunk.port, chunk.binary) } +/// Restrict the on-disk `SQLite` database file (and its WAL/SHM sidecars, +/// when present) to owner-only read/write (`0o600`). +/// +/// In WAL mode, `SQLite` keeps two sidecars next to +/// the main database file: `-wal` (uncommitted page log) +/// and `-shm` (shared memory index). They mirror the same sensitive data +/// as the main file, so they get the same `0o600` treatment whenever they exist on disk. +/// +/// Delegates to `set_file_owner_only`, which is a no-op on non-Unix platforms. +pub(super) fn restrict_db_file_permissions(path: &Path) -> PersistenceResult<()> { + set_file_owner_only(path).map_err(|err| PersistenceError::Database(err.to_string()))?; + + for sidecar in sqlite_sidecar_paths(path) { + if sidecar.exists() { + set_file_owner_only(&sidecar) + .map_err(|err| PersistenceError::Database(err.to_string()))?; + } + } + Ok(()) +} + +/// Compute the WAL/SHM sidecar paths `SQLite` derives from a main database file +/// (e.g. `foo.db` -> [`foo.db-wal`, `foo.db-shm`]). +pub(super) fn sqlite_sidecar_paths(path: &Path) -> [PathBuf; 2] { + let with_suffix = |suffix: &str| -> PathBuf { + let mut buf = path.as_os_str().to_os_string(); + buf.push(suffix); + PathBuf::from(buf) + }; + [with_suffix("-wal"), with_suffix("-shm")] +} + fn row_to_object_record(row: sqlx::sqlite::SqliteRow) -> ObjectRecord { ObjectRecord { object_type: row.get("object_type"), diff --git a/crates/openshell-server/src/persistence/tests.rs b/crates/openshell-server/src/persistence/tests.rs index bef95d4b6..09549ad29 100644 --- a/crates/openshell-server/src/persistence/tests.rs +++ b/crates/openshell-server/src/persistence/tests.rs @@ -34,6 +34,183 @@ async fn sqlite_connect_runs_embedded_migrations() { assert!(records.is_empty()); } +#[cfg(unix)] +#[tokio::test] +async fn sqlite_connect_restricts_db_file_permissions() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().expect("tempdir"); + let db_path = tmp.path().join("openshell.db"); + let url = format!("sqlite:{}?mode=rwc", db_path.display()); + + let _store = Store::connect(&url).await.expect("connect to sqlite"); + + let mode = std::fs::metadata(&db_path) + .expect("db file exists") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "expected 0600, got {mode:04o}"); +} + +#[cfg(unix)] +#[tokio::test] +async fn sqlite_connect_tightens_existing_db_file_permissions() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().expect("tempdir"); + let db_path = tmp.path().join("openshell.db"); + let url = format!("sqlite:{}?mode=rwc", db_path.display()); + + // First connect creates the file; close the pool by dropping the store. + { + let _store = Store::connect(&url).await.expect("initial connect"); + } + + // Simulate a pre-existing database left with permissive permissions + // (e.g., from an older gateway version that lacked this hardening). + std::fs::set_permissions(&db_path, std::fs::Permissions::from_mode(0o644)) + .expect("loosen permissions"); + + let _store = Store::connect(&url).await.expect("reconnect to sqlite"); + + let mode = std::fs::metadata(&db_path) + .expect("db file exists") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "expected 0600, got {mode:04o}"); +} + +// The next three tests cover `restrict_db_file_permissions` against the +// WAL/SHM sidecars at increasing levels of fidelity: +// +// 1. `_tightens_main_and_wal_and_shm_files`: synthetic empty files, proves +// the chmod loop walks all three paths. +// 2. `_skips_missing_sidecars`: proves the `exists()` guard, which is the +// actual production path today (sqlx 0.8 doesn't default to WAL and +// doesn't accept `journal_mode` as a URL parameter). +// 3. `_handles_real_sqlite_wal_files`: opens a real sqlx pool with +// `SqliteJournalMode::Wal` via the builder API so SQLite materializes +// real `-wal` and `-shm` files, then checks the helper tightens them. + +#[cfg(unix)] +#[test] +fn restrict_db_file_permissions_tightens_main_and_wal_and_shm_files() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().expect("tempdir"); + let db_path = tmp.path().join("openshell.db"); + let [wal_path, shm_path] = super::sqlite::sqlite_sidecar_paths(&db_path); + + // Simulate a SQLite database in WAL mode whose three files were left + // world-readable (older gateway version, or non-zero umask at creation). + for path in [&db_path, &wal_path, &shm_path] { + std::fs::write(path, b"").expect("create file"); + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o644)).expect("set 0o644"); + } + + super::sqlite::restrict_db_file_permissions(&db_path).expect("restrict permissions"); + + for path in [&db_path, &wal_path, &shm_path] { + let mode = std::fs::metadata(path) + .expect("file exists") + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, + 0o600, + "expected 0600 on {}, got {mode:04o}", + path.display() + ); + } +} + +#[cfg(unix)] +#[test] +fn restrict_db_file_permissions_skips_missing_sidecars() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().expect("tempdir"); + let db_path = tmp.path().join("openshell.db"); + let [wal_path, shm_path] = super::sqlite::sqlite_sidecar_paths(&db_path); + + // Only the main DB file exists (non-WAL journal mode, or pre-write WAL). + std::fs::write(&db_path, b"").expect("create file"); + std::fs::set_permissions(&db_path, std::fs::Permissions::from_mode(0o644)).expect("set 0o644"); + + super::sqlite::restrict_db_file_permissions(&db_path).expect("restrict permissions"); + + assert!(!wal_path.exists(), "WAL sidecar should not be created"); + assert!(!shm_path.exists(), "SHM sidecar should not be created"); + + let mode = std::fs::metadata(&db_path) + .expect("db file exists") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "expected 0600, got {mode:04o}"); +} + +#[cfg(unix)] +#[tokio::test] +async fn restrict_db_file_permissions_handles_real_sqlite_wal_files() { + use sqlx::sqlite::{SqliteConnectOptions, SqliteJournalMode, SqlitePoolOptions}; + use std::os::unix::fs::PermissionsExt; + use std::str::FromStr; + + let tmp = tempfile::tempdir().expect("tempdir"); + let db_path = tmp.path().join("openshell.db"); + let url = format!("sqlite:{}", db_path.display()); + + // sqlx does not parse `journal_mode` from the connection URL — callers + // must opt into WAL via the builder API. + let options = SqliteConnectOptions::from_str(&url) + .expect("parse url") + .create_if_missing(true) + .journal_mode(SqliteJournalMode::Wal); + + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect_with(options) + .await + .expect("connect with WAL"); + + // Force a write so SQLite definitely materializes a non-empty WAL on disk. + sqlx::query("CREATE TABLE _hardening_probe (x INTEGER)") + .execute(&pool) + .await + .expect("write"); + + let [wal_path, shm_path] = super::sqlite::sqlite_sidecar_paths(&db_path); + assert!(wal_path.exists(), "WAL should exist after write"); + assert!(shm_path.exists(), "SHM should exist after WAL write"); + + // Loosen permissions on every file to simulate what an older gateway + // version (or a non-zero default umask) would have left behind. + for path in [&db_path, &wal_path, &shm_path] { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o644)) + .expect("loosen permissions"); + } + + super::sqlite::restrict_db_file_permissions(&db_path).expect("restrict permissions"); + + for path in [&db_path, &wal_path, &shm_path] { + let mode = std::fs::metadata(path) + .expect("metadata") + .permissions() + .mode() + & 0o777; + assert_eq!( + mode, + 0o600, + "expected 0600 on {}, got {mode:04o}", + path.display() + ); + } +} + #[tokio::test] async fn sqlite_updates_timestamp() { let store = Store::connect("sqlite::memory:?cache=shared")