From 0ca3f085b614021e635b2be821c492ef6d494e49 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:14:58 +0300 Subject: [PATCH 1/2] fix(storage): create config.db owner-only and secure database backups config.db stores OAuth access/refresh tokens and DCR client secrets as plain JSON, but was created with mode 0644 and backed up with 0644. - Open the database 0600. - Tighten an existing group/world-readable database on open, clearing only the 0o077 bits so a stricter owner mode is never widened. This is best-effort: failures are logged at debug level and never block startup. - Stage Backup in an owner-only temp file next to the destination and rename it into place. bbolt's Tx.CopyFile opens with O_CREATE|O_TRUNC, so its mode argument is ignored for an existing destination and the database bytes would land in whatever permissions a stale backup carried. Co-Authored-By: Claude Opus 5 --- internal/storage/bbolt.go | 86 ++++++++++++- internal/storage/bbolt_file_mode_test.go | 153 +++++++++++++++++++++++ 2 files changed, 233 insertions(+), 6 deletions(-) create mode 100644 internal/storage/bbolt_file_mode_test.go diff --git a/internal/storage/bbolt.go b/internal/storage/bbolt.go index d23806906..6f0b48ca7 100644 --- a/internal/storage/bbolt.go +++ b/internal/storage/bbolt.go @@ -39,8 +39,10 @@ type BoltDB struct { func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) { dbPath := filepath.Join(dataDir, "config.db") - // Try to open with timeout, if it fails, immediately return database locked error - db, err := bbolt.Open(dbPath, 0644, &bbolt.Options{ + // Try to open with timeout, if it fails, immediately return database locked error. + // The database holds OAuth tokens and DCR client secrets, so it is created + // owner-only (0600). + db, err := bbolt.Open(dbPath, 0600, &bbolt.Options{ Timeout: 10 * time.Second, }) if err != nil { @@ -59,6 +61,10 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) { return nil, fmt.Errorf("failed to open bolt database: %w", err) } + // The mode passed to bbolt.Open only applies when the file is created, so + // databases created by older builds keep their 0644 mode. Tighten them here. + tightenFilePermissions(dbPath, logger) + boltDB := &BoltDB{ db: db, logger: logger, @@ -73,6 +79,31 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) { return boltDB, nil } +// tightenFilePermissions clears group and other permission bits from path, +// leaving owner bits untouched so a deliberately stricter mode is never widened. +// It is best-effort: any failure is logged at debug level and ignored, because a +// mode that cannot be tightened (read-only mount, exotic filesystem, Windows) +// must never prevent the proxy from starting. +func tightenFilePermissions(path string, logger *zap.SugaredLogger) { + info, err := os.Stat(path) + if err != nil { + logger.Debugf("Could not stat %s to check file permissions: %v", path, err) + return + } + + perm := info.Mode().Perm() + if perm&0o077 == 0 { + return + } + + if err := os.Chmod(path, perm&^0o077); err != nil { + logger.Debugf("Could not tighten permissions on %s: %v", path, err) + return + } + + logger.Debugf("Tightened permissions on %s from %#o to %#o", path, perm, perm&^0o077) +} + // Close closes the database func (b *BoltDB) Close() error { return b.db.Close() @@ -663,11 +694,54 @@ func (b *BoltDB) DeleteServerPromptApprovals(serverName string) error { // Generic operations -// Backup creates a backup of the database +// Backup creates a backup of the database. +// The copy carries the same secrets as the live database and the destination is +// caller-chosen (potentially outside the 0700 data directory), so it is written +// owner-only. func (b *BoltDB) Backup(destPath string) error { - return b.db.View(func(tx *bbolt.Tx) error { - return tx.CopyFile(destPath, 0644) - }) + // bbolt's Tx.CopyFile opens the destination with O_CREATE|O_TRUNC, so its + // mode argument is ignored when the file already exists: writing straight to + // destPath would pour the database into whatever permissions a stale backup + // happened to carry, and a chmod afterwards comes too late. Stage the copy in + // an owner-only temporary file (os.CreateTemp creates it 0600) next to the + // destination, then rename it into place - the rename keeps the temp file's + // inode and so carries the 0600 mode with it (atomically on POSIX; Go makes + // no atomicity promise on Windows). + tmpFile, err := os.CreateTemp(filepath.Dir(destPath), ".config.db.backup-*") + if err != nil { + return fmt.Errorf("failed to create temporary backup file: %w", err) + } + + tmpPath := tmpFile.Name() + renamed := false + defer func() { + tmpFile.Close() //nolint:errcheck // best-effort cleanup; Close below is the checked one + if !renamed { + os.Remove(tmpPath) //nolint:errcheck // best-effort cleanup + } + }() + + if err := b.db.View(func(tx *bbolt.Tx) error { + _, writeErr := tx.WriteTo(tmpFile) + return writeErr + }); err != nil { + return fmt.Errorf("failed to write backup: %w", err) + } + + if err := tmpFile.Sync(); err != nil { + return fmt.Errorf("failed to flush backup: %w", err) + } + + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("failed to close backup: %w", err) + } + + if err := os.Rename(tmpPath, destPath); err != nil { + return fmt.Errorf("failed to move backup into place: %w", err) + } + renamed = true + + return nil } // Stats returns database statistics diff --git a/internal/storage/bbolt_file_mode_test.go b/internal/storage/bbolt_file_mode_test.go new file mode 100644 index 000000000..d026b00ef --- /dev/null +++ b/internal/storage/bbolt_file_mode_test.go @@ -0,0 +1,153 @@ +package storage + +import ( + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/stretchr/testify/require" + "go.etcd.io/bbolt" + "go.uber.org/zap" +) + +// skipOnWindows guards the permission assertions below: os.Chmod on Windows only +// toggles the read-only bit and os.Stat reports 0666, so POSIX mode bits are +// meaningless there. CI runs the unit tests on Windows too. +func skipOnWindows(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("POSIX file mode bits are not meaningful on Windows") + } +} + +// TestNewBoltDBCreatesDatabaseOwnerOnly pins that a freshly created config.db is +// owner-only: it holds OAuth access/refresh tokens and DCR client secrets. +func TestNewBoltDBCreatesDatabaseOwnerOnly(t *testing.T) { + skipOnWindows(t) + + dir := t.TempDir() + db, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + defer db.Close() + + info, err := os.Stat(filepath.Join(dir, "config.db")) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "a freshly created config.db must not be group/world readable") +} + +// TestNewBoltDBTightensWorldReadableDatabaseOnOpen covers existing installs: +// bbolt.Open's mode argument applies only at creation, so an already-created +// 0644 database must be chmod-migrated on open. +func TestNewBoltDBTightensWorldReadableDatabaseOnOpen(t *testing.T) { + skipOnWindows(t) + + dir := t.TempDir() + dbPath := filepath.Join(dir, "config.db") + + db, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db.Close()) + + // Simulate a database created by an older build. + require.NoError(t, os.Chmod(dbPath, 0o644)) + + db2, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db2.Close()) + + info, err := os.Stat(dbPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "an existing world-readable config.db must be tightened on open") +} + +// TestNewBoltDBPreservesOwnerBitsWhenTightening makes sure the migration only +// clears group/other bits instead of assigning 0600 outright, so it can never +// widen a mode the owner chose. +func TestNewBoltDBPreservesOwnerBitsWhenTightening(t *testing.T) { + skipOnWindows(t) + + dir := t.TempDir() + dbPath := filepath.Join(dir, "config.db") + + db, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db.Close()) + + require.NoError(t, os.Chmod(dbPath, 0o744)) + + db2, err := NewBoltDB(dir, zap.NewNop().Sugar()) + require.NoError(t, err) + require.NoError(t, db2.Close()) + + info, err := os.Stat(dbPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o700), info.Mode().Perm(), + "only group/other bits should be cleared; owner bits must be preserved") +} + +// TestBackupWritesOwnerOnlyCopy pins the backup copy: the destination is an +// operator-chosen path that may sit outside the 0700 data directory, where a +// 0644 copy really would be world-readable. +func TestBackupWritesOwnerOnlyCopy(t *testing.T) { + skipOnWindows(t) + + db, err := NewBoltDB(t.TempDir(), zap.NewNop().Sugar()) + require.NoError(t, err) + defer db.Close() + + backupPath := filepath.Join(t.TempDir(), "backup.db") + require.NoError(t, db.Backup(backupPath)) + + info, err := os.Stat(backupPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "database backups must not be group/world readable") + + // The copy must still be a usable database, not just a well-permissioned file. + restored, err := bbolt.Open(backupPath, 0600, &bbolt.Options{ + ReadOnly: true, + Timeout: 5 * time.Second, + }) + require.NoError(t, err) + require.NoError(t, restored.Close()) +} + +// TestBackupTightensExistingDestination covers overwriting an earlier backup: +// bbolt's CopyFile opens the destination with O_CREATE|O_TRUNC, so its mode +// argument applies only when the destination does not already exist. The backup +// must not be written into a pre-existing world-readable file at all, so it is +// staged in an owner-only temporary file and renamed into place - which means +// the old destination inode is replaced rather than truncated and rewritten. +func TestBackupTightensExistingDestination(t *testing.T) { + skipOnWindows(t) + + db, err := NewBoltDB(t.TempDir(), zap.NewNop().Sugar()) + require.NoError(t, err) + defer db.Close() + + destDir := t.TempDir() + backupPath := filepath.Join(destDir, "backup.db") + // A backup left behind by an older build. + require.NoError(t, os.WriteFile(backupPath, []byte("stale"), 0o644)) + + before, err := os.Stat(backupPath) + require.NoError(t, err) + + require.NoError(t, db.Backup(backupPath)) + + after, err := os.Stat(backupPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o600), after.Mode().Perm(), + "overwriting an existing backup must also tighten its permissions") + require.False(t, os.SameFile(before, after), + "database bytes must never be written into the pre-existing, possibly world-readable file; "+ + "stage in an owner-only temp file and rename over the destination") + + entries, err := os.ReadDir(destDir) + require.NoError(t, err) + require.Len(t, entries, 1, "backup must not leave temporary files behind: %v", entries) +} From f6235335ac11de9f351b5cdb20798d176d07b00e Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 23 Sep 2026 06:26:29 +0300 Subject: [PATCH 2/2] fix(storage): log SEC-03 permission-tightening failures at Warn tightenFilePermissions logged both its failure branches (stat and chmod) at Debug, which sits below the project's default zap level (Info). On a legacy 0644 config.db where chmod cannot succeed (read-only mount, macOS uchg/schg flag, SELinux/AppArmor denial), mcpproxy started normally and the DB - which holds OAuth tokens and DCR client secrets - stayed silently world-readable with no operator-visible signal that the migration didn't take effect. Not failing startup is still correct; the log level wasn't. Bump the two failure branches to Warn and the one-time successful-migration message to Info so this is visible in default-level logs, per second-lens review of PR #1342. Co-Authored-By: Claude Opus 5 --- internal/storage/bbolt.go | 15 ++++--- internal/storage/bbolt_file_mode_test.go | 51 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/internal/storage/bbolt.go b/internal/storage/bbolt.go index 6f0b48ca7..0ee7fa182 100644 --- a/internal/storage/bbolt.go +++ b/internal/storage/bbolt.go @@ -81,13 +81,16 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) { // tightenFilePermissions clears group and other permission bits from path, // leaving owner bits untouched so a deliberately stricter mode is never widened. -// It is best-effort: any failure is logged at debug level and ignored, because a -// mode that cannot be tightened (read-only mount, exotic filesystem, Windows) -// must never prevent the proxy from starting. +// It is best-effort and never prevents the proxy from starting: a mode that +// cannot be tightened (read-only mount, exotic filesystem, Windows) is logged +// at warn level (the default log level is info) rather than blocking startup, +// because the file holds OAuth tokens and DCR client secrets and a failure +// here means it silently stays group/world readable - that must be visible +// to an operator without enabling debug logging. func tightenFilePermissions(path string, logger *zap.SugaredLogger) { info, err := os.Stat(path) if err != nil { - logger.Debugf("Could not stat %s to check file permissions: %v", path, err) + logger.Warnf("Could not stat %s to check file permissions: %v", path, err) return } @@ -97,11 +100,11 @@ func tightenFilePermissions(path string, logger *zap.SugaredLogger) { } if err := os.Chmod(path, perm&^0o077); err != nil { - logger.Debugf("Could not tighten permissions on %s: %v", path, err) + logger.Warnf("Could not tighten permissions on %s: %v", path, err) return } - logger.Debugf("Tightened permissions on %s from %#o to %#o", path, perm, perm&^0o077) + logger.Infof("Tightened permissions on %s from %#o to %#o", path, perm, perm&^0o077) } // Close closes the database diff --git a/internal/storage/bbolt_file_mode_test.go b/internal/storage/bbolt_file_mode_test.go index d026b00ef..f7dd33e03 100644 --- a/internal/storage/bbolt_file_mode_test.go +++ b/internal/storage/bbolt_file_mode_test.go @@ -2,6 +2,7 @@ package storage import ( "os" + "os/exec" "path/filepath" "runtime" "testing" @@ -10,6 +11,8 @@ import ( "github.com/stretchr/testify/require" "go.etcd.io/bbolt" "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" ) // skipOnWindows guards the permission assertions below: os.Chmod on Windows only @@ -151,3 +154,51 @@ func TestBackupTightensExistingDestination(t *testing.T) { require.NoError(t, err) require.Len(t, entries, 1, "backup must not leave temporary files behind: %v", entries) } + +// TestTightenFilePermissionsLogsStatFailureAtWarn covers SEC-03 follow-up +// review: the database holds OAuth tokens and DCR client secrets, so an +// operator running at the project's default log level (Info) must be able to +// see that the permission-tightening migration did not run, instead of the +// failure disappearing into Debug output nobody has enabled. +func TestTightenFilePermissionsLogsStatFailureAtWarn(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + logger := zap.New(core).Sugar() + + tightenFilePermissions(filepath.Join(t.TempDir(), "does-not-exist.db"), logger) + + entries := logs.FilterMessageSnippet("Could not stat").All() + require.Len(t, entries, 1, "a stat failure must be logged") + require.Equal(t, zapcore.WarnLevel, entries[0].Level, + "a stat failure must be visible at the project's default (Info) log level") +} + +// TestTightenFilePermissionsLogsChmodFailureAtWarn reproduces one of the +// review's named real-world causes (macOS uchg/schg flag) for a chmod that +// cannot succeed even though the process owns the file: darwin's immutable +// flag makes chmod fail with EPERM regardless of ownership. +func TestTightenFilePermissionsLogsChmodFailureAtWarn(t *testing.T) { + skipOnWindows(t) + if runtime.GOOS != "darwin" { + t.Skip("uses chflags uchg to force an owner-proof chmod failure; darwin-only") + } + + dir := t.TempDir() + path := filepath.Join(dir, "config.db") + require.NoError(t, os.WriteFile(path, []byte("x"), 0o644)) + + require.NoError(t, exec.Command("chflags", "uchg", path).Run()) + defer func() { + _ = exec.Command("chflags", "nouchg", path).Run() + }() + + core, logs := observer.New(zapcore.DebugLevel) + logger := zap.New(core).Sugar() + + tightenFilePermissions(path, logger) + + entries := logs.FilterMessageSnippet("Could not tighten permissions").All() + require.Len(t, entries, 1, "a chmod failure must be logged") + require.Equal(t, zapcore.WarnLevel, entries[0].Level, + "a chmod failure must be visible at the project's default (Info) log level, "+ + "since it means the DB is left silently world-readable") +}