diff --git a/docs/release-control/v6/internal/subsystems/registry.json b/docs/release-control/v6/internal/subsystems/registry.json index 21b7041e8..06ff9fd04 100644 --- a/docs/release-control/v6/internal/subsystems/registry.json +++ b/docs/release-control/v6/internal/subsystems/registry.json @@ -4240,6 +4240,8 @@ "internal/api/security.go", "internal/api/security_tokens.go", "internal/api/system_settings.go", + "internal/cloudcp/auth/magiclink.go", + "internal/cloudcp/auth/magiclink_store.go", "internal/crypto/crypto.go", "internal/securityutil/secure_storage_dir.go", "internal/telemetry/telemetry.go", @@ -4261,6 +4263,7 @@ "internal/api/security_tokens_owner_binding_test.go", "internal/api/security_tokens_test.go", "internal/api/system_settings_telemetry_test.go", + "internal/cloudcp/auth/magiclink_test.go", "internal/telemetry/telemetry_test.go", "scripts/tests/test_telemetry_adoption_report.py" ], @@ -4350,12 +4353,15 @@ "label": "storage directory hardening proof", "match_prefixes": [], "match_files": [ + "internal/cloudcp/auth/magiclink.go", + "internal/cloudcp/auth/magiclink_store.go", "internal/crypto/crypto.go", "internal/securityutil/secure_storage_dir.go" ], "allow_same_subsystem_tests": false, "test_prefixes": [], "exact_files": [ + "internal/cloudcp/auth/magiclink_test.go", "internal/crypto/crypto_test.go", "internal/securityutil/secure_storage_dir_test.go" ] diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index 396798a87..118e031f6 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -51,7 +51,9 @@ visibility, and privacy controls to operators. 25. `internal/api/router_routes_auth_security.go` 26. `internal/crypto/crypto.go` 27. `internal/securityutil/secure_storage_dir.go` -28. `scripts/telemetry_adoption_report.py` +28. `internal/cloudcp/auth/magiclink.go` +29. `internal/cloudcp/auth/magiclink_store.go` +30. `scripts/telemetry_adoption_report.py` ## Shared Boundaries @@ -75,7 +77,7 @@ visibility, and privacy controls to operators. 4. Change security/auth/token transport behavior through the shared `frontend-modern/src/api/security.ts`, `frontend-modern/src/components/Settings/APITokenManager.tsx`, `frontend-modern/src/components/Settings/apiTokenManagerModel.ts`, `frontend-modern/src/components/Settings/useAPITokenManagerState.ts`, `internal/api/security.go`, `internal/api/security_tokens.go`, and `internal/api/system_settings.go` boundary. 5. Change security/privacy settings presentation through the shared `frontend-modern/src/components/Settings/GeneralSettingsPanel.tsx`, `frontend-modern/src/components/Settings/SecurityAuthPanel.tsx`, `frontend-modern/src/components/Settings/SecurityOverviewPanel.tsx`, `frontend-modern/src/components/Settings/QuickSecuritySetup.tsx`, `frontend-modern/src/components/Settings/SecurityPostureSummary.tsx`, `frontend-modern/src/components/Settings/SSOProviderTypeIcon.tsx`, `frontend-modern/src/utils/securityAuthPresentation.ts`, `frontend-modern/src/utils/securityScorePresentation.ts`, `frontend-modern/src/utils/auditLogPresentation.ts`, and `frontend-modern/src/utils/auditWebhookPresentation.ts` boundary. 6. Change operator-facing telemetry/adoption reporting through `scripts/telemetry_adoption_report.py` together with the privacy disclosure whenever release-identity interpretation changes. -7. Change data-at-rest encryption-key storage-root hardening semantics through `internal/crypto/crypto.go` and `internal/securityutil/secure_storage_dir.go` together so writable-but-not-owned runtime storage mounts stay supported without weakening file-level secrecy. +7. Change data-at-rest encryption-key or control-plane magic-link HMAC key and storage-root hardening semantics through `internal/crypto/crypto.go`, `internal/cloudcp/auth/magiclink.go`, `internal/cloudcp/auth/magiclink_store.go`, and `internal/securityutil/secure_storage_dir.go` together so writable-but-not-owned runtime storage mounts stay supported without weakening file-level secrecy. ## Forbidden Paths @@ -90,7 +92,7 @@ visibility, and privacy controls to operators. 3. Keep shared frontend settings proof routing aligned whenever security/privacy presentation changes. 4. Keep the checked-in telemetry adoption report aligned with the same release-identity rules used by the runtime telemetry payload. 5. Update this contract whenever a new canonical security, token, auth, or privacy surface becomes part of the governed trust boundary. -6. Keep the shared storage-directory hardening helper and the crypto manager aligned whenever runtime data-root ownership assumptions change. +6. Keep the shared storage-directory and secure storage-file hardening helper aligned with the crypto manager plus control-plane magic-link key and store handling whenever runtime data-root ownership assumptions change. ## Current State @@ -105,6 +107,14 @@ adoption baselines. `scripts/telemetry_adoption_report.py` must emit windowed 24h, 72h, and 7d latest-install snapshots that split published versions from unpublished or development builds, so RC adoption reads stop depending on ad hoc SQL or one-off local helper scripts. +That same storage hardening boundary now also owns secure regular-file +handling for secret-bearing local trust material and the control-plane +magic-link storage root. `internal/crypto/crypto.go`, +`internal/cloudcp/auth/magiclink.go`, and +`internal/cloudcp/auth/magiclink_store.go` must route encryption keys, +magic-link HMAC keys, and the magic-link SQLite store path through the shared +secure storage helpers so symlink, oversize, and non-regular file paths fail +closed instead of slipping past directory-only hardening. Security-facing settings remain intentionally shared with `frontend-primitives` because shell framing and presentation consistency still belong there, but the diff --git a/internal/cloudcp/auth/magiclink.go b/internal/cloudcp/auth/magiclink.go index 1d46f9662..0b645410f 100644 --- a/internal/cloudcp/auth/magiclink.go +++ b/internal/cloudcp/auth/magiclink.go @@ -5,22 +5,24 @@ import ( "crypto/rand" "crypto/sha256" "encoding/base64" + "errors" "fmt" "io" "net/url" "os" - "path/filepath" "strings" "time" + "github.com/rcourtman/pulse-go-rewrite/internal/securityutil" "github.com/rs/zerolog/log" ) const ( - magicLinkTTL = 15 * time.Minute - magicLinkPrefix = "ml1_" - hmacKeyFile = ".cp_magic_link_key" - hmacKeySize = 32 + magicLinkTTL = 15 * time.Minute + magicLinkPrefix = "ml1_" + hmacKeyFile = ".cp_magic_link_key" + hmacKeySize = 32 + maxHMACKeyFileSize = 64 ) type MagicLinkTarget string @@ -50,16 +52,25 @@ type Service struct { // NewService creates a Service backed by a SQLite store in cpDataDir. // It loads (or generates) an HMAC key from {cpDataDir}/.cp_magic_link_key. func NewService(cpDataDir string) (*Service, error) { - if err := ensureOwnerOnlyDir(cpDataDir); err != nil { + normalizedDir, err := securityutil.NormalizeStorageDir(cpDataDir) + if err != nil { + return nil, fmt.Errorf("cp data dir is required: %w", err) + } + if err := ensureOwnerOnlyDir(normalizedDir); err != nil { return nil, fmt.Errorf("ensure cp data dir: %w", err) } - key, err := loadOrGenerateKey(filepath.Join(cpDataDir, hmacKeyFile)) + keyPath, err := securityutil.JoinStorageLeaf(normalizedDir, hmacKeyFile) + if err != nil { + return nil, fmt.Errorf("resolve magic link hmac key path: %w", err) + } + + key, err := loadOrGenerateKey(keyPath) if err != nil { return nil, fmt.Errorf("magic link hmac key: %w", err) } - store, err := NewStore(cpDataDir) + store, err := NewStore(normalizedDir) if err != nil { return nil, fmt.Errorf("magic link store: %w", err) } @@ -178,11 +189,17 @@ func (s *Service) Close() { } func loadOrGenerateKey(path string) ([]byte, error) { - data, err := os.ReadFile(path) - if err == nil && len(data) >= hmacKeySize { - return data[:hmacKeySize], nil + data, err := securityutil.ReadSecureStorageFile(path, maxHMACKeyFileSize) + if err == nil { + if len(data) != hmacKeySize { + return nil, fmt.Errorf("invalid key file %s: expected %d bytes, got %d", path, hmacKeySize, len(data)) + } + return append([]byte(nil), data...), nil } if err != nil && !os.IsNotExist(err) { + if errors.Is(err, securityutil.ErrUnsafeStorageFile) { + return nil, fmt.Errorf("unsafe key file %s: %w", path, err) + } return nil, fmt.Errorf("read key file %s: %w", path, err) } @@ -190,7 +207,7 @@ func loadOrGenerateKey(path string) ([]byte, error) { if _, err := io.ReadFull(rand.Reader, key); err != nil { return nil, fmt.Errorf("generate key: %w", err) } - if err := os.WriteFile(path, key, 0o600); err != nil { + if err := securityutil.WriteSecureStorageFile(path, key, privateDirPerm, 0o600); err != nil { return nil, fmt.Errorf("write key file %s: %w", path, err) } log.Info().Str("path", path).Msg("Generated new magic link HMAC key") diff --git a/internal/cloudcp/auth/magiclink_store.go b/internal/cloudcp/auth/magiclink_store.go index d038cb186..600c216a8 100644 --- a/internal/cloudcp/auth/magiclink_store.go +++ b/internal/cloudcp/auth/magiclink_store.go @@ -6,12 +6,11 @@ import ( "errors" "fmt" "net/url" - "os" - "path/filepath" "strings" "sync" "time" + "github.com/rcourtman/pulse-go-rewrite/internal/securityutil" "github.com/rs/zerolog/log" _ "modernc.org/sqlite" ) @@ -26,10 +25,7 @@ const storeCleanupInterval = 5 * time.Minute const privateDirPerm = 0o700 func ensureOwnerOnlyDir(dir string) error { - if err := os.MkdirAll(dir, privateDirPerm); err != nil { - return err - } - return os.Chmod(dir, privateDirPerm) + return securityutil.EnsureSecureStorageDir(dir, privateDirPerm) } // TokenRecord holds the data associated with a stored magic link token. @@ -51,15 +47,18 @@ type Store struct { // NewStore opens (or creates) the magic link token database in dir. func NewStore(dir string) (*Store, error) { - dir = filepath.Clean(dir) - if strings.TrimSpace(dir) == "" { - return nil, fmt.Errorf("dir is required") + normalizedDir, err := securityutil.NormalizeStorageDir(dir) + if err != nil { + return nil, fmt.Errorf("dir is required: %w", err) } - if err := ensureOwnerOnlyDir(dir); err != nil { + if err := ensureOwnerOnlyDir(normalizedDir); err != nil { return nil, fmt.Errorf("create magic link store dir: %w", err) } - dbPath := filepath.Join(dir, "cp_magic_links.db") + dbPath, err := securityutil.JoinStorageLeaf(normalizedDir, "cp_magic_links.db") + if err != nil { + return nil, fmt.Errorf("resolve magic link db path: %w", err) + } dsn := dbPath + "?" + url.Values{ "_pragma": []string{ "busy_timeout(30000)", diff --git a/internal/cloudcp/auth/magiclink_test.go b/internal/cloudcp/auth/magiclink_test.go index eb2fdaf4d..4bc4e5fd4 100644 --- a/internal/cloudcp/auth/magiclink_test.go +++ b/internal/cloudcp/auth/magiclink_test.go @@ -225,6 +225,54 @@ func TestKeyPersistence(t *testing.T) { } } +func TestKeyPersistence_SecuresKeyFilePermissions(t *testing.T) { + dir := t.TempDir() + + svc, err := NewService(dir) + if err != nil { + t.Fatalf("NewService: %v", err) + } + svc.Close() + + info, err := os.Stat(filepath.Join(dir, hmacKeyFile)) + if err != nil { + t.Fatalf("stat key file: %v", err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("key file perms = %o, want %o", got, 0o600) + } +} + +func TestNewService_RejectsShortExistingKeyFile(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, hmacKeyFile), make([]byte, hmacKeySize-1), 0o600); err != nil { + t.Fatalf("write short key file: %v", err) + } + + _, err := NewService(dir) + if err == nil || !strings.Contains(err.Error(), "expected 32 bytes") { + t.Fatalf("expected invalid key length error, got %v", err) + } +} + +func TestNewService_RejectsSymlinkedKeyFile(t *testing.T) { + dir := t.TempDir() + realKeyPath := filepath.Join(dir, "real.key") + if err := os.WriteFile(realKeyPath, make([]byte, hmacKeySize), 0o600); err != nil { + t.Fatalf("write real key: %v", err) + } + + keyPath := filepath.Join(dir, hmacKeyFile) + if err := os.Symlink(realKeyPath, keyPath); err != nil { + t.Skipf("symlink not supported on this platform: %v", err) + } + + _, err := NewService(dir) + if err == nil || !strings.Contains(err.Error(), "unsafe key file") { + t.Fatalf("expected unsafe key file error, got %v", err) + } +} + func TestNewService_SecuresDataDirPermissions(t *testing.T) { dir := filepath.Join(t.TempDir(), "cp-data") diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index d3365e560..5c4d60569 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -59,35 +59,14 @@ func decodeEncryptionKey(data []byte) ([]byte, error) { return decoded[:n], nil } -func validateEncryptionKeyFile(path string, info os.FileInfo) error { - if info.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("%w: refusing symlink key path %q", errUnsafeKeyPath, path) - } - if !info.Mode().IsRegular() { - return fmt.Errorf("%w: non-regular key path %q", errInvalidKeyMaterial, path) - } - if info.Size() > maxEncryptionKeyFileSize { - return fmt.Errorf("%w: key file %q is too large (%d bytes)", errUnsafeKeyPath, path, info.Size()) - } - return nil -} - func loadKeyFromFile(path string) ([]byte, error) { - info, err := os.Lstat(path) + data, err := securityutil.ReadSecureStorageFile(path, maxEncryptionKeyFileSize) if err != nil { + if errors.Is(err, securityutil.ErrUnsafeStorageFile) { + return nil, fmt.Errorf("%w: %v", errUnsafeKeyPath, err) + } return nil, err } - if err := validateEncryptionKeyFile(path, info); err != nil { - return nil, err - } - - data, err := os.ReadFile(path) - if err != nil { - return nil, err - } - if len(data) > maxEncryptionKeyFileSize { - return nil, fmt.Errorf("%w: key file %q exceeded size limit while reading", errUnsafeKeyPath, path) - } return decodeEncryptionKey(data) } @@ -100,43 +79,8 @@ func writeKeyFile(path string, key []byte) error { return fmt.Errorf("refusing to write invalid key length %d", len(key)) } - dir := filepath.Dir(path) - if err := ensureOwnerOnlyDir(dir); err != nil { - return err - } - - tmpFile, err := os.CreateTemp(dir, ".encryption.key.*.tmp") - if err != nil { - return err - } - tmpPath := tmpFile.Name() - cleanup := true - defer func() { - if cleanup { - _ = os.Remove(tmpPath) - } - }() - - if err := tmpFile.Chmod(encryptionKeyFilePerm); err != nil { - _ = tmpFile.Close() - return err - } - encoded := base64.StdEncoding.EncodeToString(key) - if _, err := tmpFile.WriteString(encoded); err != nil { - _ = tmpFile.Close() - return err - } - if err := tmpFile.Close(); err != nil { - return err - } - - if err := os.Rename(tmpPath, path); err != nil { - return err - } - cleanup = false - - return os.Chmod(path, encryptionKeyFilePerm) + return securityutil.WriteSecureStorageFile(path, []byte(encoded), encryptionKeyDirPerm, encryptionKeyFilePerm) } // CryptoManager handles encryption/decryption of sensitive data @@ -279,61 +223,48 @@ func getOrCreateKeyAt(dataDir string) ([]byte, error) { Str("oldKeyPath", oldKeyPath). Msg("checking for legacy encryption key migration") - if data, err := os.ReadFile(oldKeyPath); err == nil { - decoded := make([]byte, base64.StdEncoding.DecodedLen(len(data))) - n, decodeErr := base64.StdEncoding.Decode(decoded, data) - if decodeErr != nil { + if key, err := loadKeyFromFile(oldKeyPath); err == nil { + // Migrate key to new location + if err := writeKeyFile(keyPath, key); err != nil { + // Migration failed, but we can still use the old key log.Warn(). - Err(decodeErr). - Str("path", oldKeyPath). - Msg("Failed to decode legacy encryption key during migration check") - } else if n != 32 { - log.Warn(). - Int("decodedBytes", n). - Str("path", oldKeyPath). - Msg("Legacy encryption key has invalid length during migration check") - } else { - key := decoded[:n] - // Migrate key to new location - if err := writeKeyFile(keyPath, key); err != nil { - // Migration failed, but we can still use the old key - log.Warn(). - Err(err). - Str("from", oldKeyPath). - Str("to", keyPath). - Msg("Failed to migrate encryption key, using old location") - return key, nil - } - log.Info(). + Err(err). Str("from", oldKeyPath). Str("to", keyPath). - Msg("migrated encryption key to data directory") - - // CRITICAL: This is the ONLY place in the codebase that deletes the encryption key! - // BUG FIX: Disabling key deletion to prevent key loss. - // Keeping both copies is safe - the old key at /etc/pulse will just be unused. - log.Info(). - Str("oldKeyPath", oldKeyPath). - Str("newKeyPath", keyPath). - Str("dataDir", resolvedDataDir). - Msg("Key migration complete - PRESERVING old key at original location for safety") - - // DISABLED: Key deletion was causing mysterious key loss bugs. - // The old key is now preserved. This is safe because: - // 1. We just successfully wrote the key to the new location - // 2. Future reads will use the new location (checked first) - // 3. Keeping the backup prevents data loss if something goes wrong - // - // if err := os.Remove(oldKeyPath); err != nil { - // log.Debug().Err(err).Msg("Could not remove old encryption key (may lack permissions)") - // } else { - // log.Error(). - // Str("deletedPath", oldKeyPath). - // Msg("CRITICAL: ENCRYPTION KEY HAS BEEN DELETED") - // } + Msg("Failed to migrate encryption key, using old location") return key, nil } - } else if !os.IsNotExist(err) { + log.Info(). + Str("from", oldKeyPath). + Str("to", keyPath). + Msg("migrated encryption key to data directory") + + // CRITICAL: This is the ONLY place in the codebase that deletes the encryption key! + // BUG FIX: Disabling key deletion to prevent key loss. + // Keeping both copies is safe - the old key at /etc/pulse will just be unused. + log.Info(). + Str("oldKeyPath", oldKeyPath). + Str("newKeyPath", keyPath). + Str("dataDir", resolvedDataDir). + Msg("Key migration complete - PRESERVING old key at original location for safety") + + // DISABLED: Key deletion was causing mysterious key loss bugs. + // The old key is now preserved. This is safe because: + // 1. We just successfully wrote the key to the new location + // 2. Future reads will use the new location (checked first) + // 3. Keeping the backup prevents data loss if something goes wrong + // + // if err := os.Remove(oldKeyPath); err != nil { + // log.Debug().Err(err).Msg("Could not remove old encryption key (may lack permissions)") + // } else { + // log.Error(). + // Str("deletedPath", oldKeyPath). + // Msg("CRITICAL: ENCRYPTION KEY HAS BEEN DELETED") + // } + return key, nil + } else if errors.Is(err, errUnsafeKeyPath) { + return nil, fmt.Errorf("unsafe legacy encryption key path %q: %w", oldKeyPath, err) + } else if !os.IsNotExist(err) && !errors.Is(err, errInvalidKeyMaterial) { log.Warn(). Err(err). Str("path", oldKeyPath). diff --git a/internal/crypto/crypto_test.go b/internal/crypto/crypto_test.go index 4174d5c5d..1b8feecc6 100644 --- a/internal/crypto/crypto_test.go +++ b/internal/crypto/crypto_test.go @@ -586,6 +586,46 @@ func TestGetOrCreateKeyAt_MigrateSuccess(t *testing.T) { } } +func TestGetOrCreateKeyAt_RejectsSymlinkedLegacyKeyPath(t *testing.T) { + legacyDir := t.TempDir() + legacyPath := filepath.Join(legacyDir, ".encryption.key") + withLegacyKeyPath(t, legacyPath) + + realKeyPath := filepath.Join(legacyDir, "real-encryption.key") + oldKey := make([]byte, 32) + for i := range oldKey { + oldKey[i] = byte(i) + } + encoded := base64.StdEncoding.EncodeToString(oldKey) + if err := os.WriteFile(realKeyPath, []byte(encoded), 0o600); err != nil { + t.Fatalf("write real key: %v", err) + } + if err := os.Symlink(realKeyPath, legacyPath); err != nil { + t.Skipf("symlink not supported on this platform: %v", err) + } + + _, err := getOrCreateKeyAt(t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "unsafe legacy encryption key path") { + t.Fatalf("expected unsafe legacy key path error, got %v", err) + } +} + +func TestGetOrCreateKeyAt_RejectsOversizedLegacyKeyFile(t *testing.T) { + legacyDir := t.TempDir() + legacyPath := filepath.Join(legacyDir, ".encryption.key") + withLegacyKeyPath(t, legacyPath) + + oversized := bytes.Repeat([]byte("A"), maxEncryptionKeyFileSize+1) + if err := os.WriteFile(legacyPath, oversized, 0o600); err != nil { + t.Fatalf("write oversized legacy key: %v", err) + } + + _, err := getOrCreateKeyAt(t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "unsafe legacy encryption key path") { + t.Fatalf("expected unsafe legacy key path error, got %v", err) + } +} + func TestGetOrCreateKeyAt_IgnoresRelativeLegacyOverride(t *testing.T) { legacyDir := t.TempDir() legacyPath := filepath.Join(legacyDir, ".encryption.key") @@ -647,7 +687,7 @@ func TestGetOrCreateKeyAt_MigrateMkdirError(t *testing.T) { } } -func TestGetOrCreateKeyAt_MigrateWriteError(t *testing.T) { +func TestGetOrCreateKeyAt_RejectsUnsafePrimaryKeyPathBeforeMigration(t *testing.T) { legacyDir := t.TempDir() legacyPath := filepath.Join(legacyDir, ".encryption.key") withLegacyKeyPath(t, legacyPath) @@ -667,12 +707,9 @@ func TestGetOrCreateKeyAt_MigrateWriteError(t *testing.T) { t.Fatalf("Failed to create key path dir: %v", err) } - key, err := getOrCreateKeyAt(newDir) - if err != nil { - t.Fatalf("getOrCreateKeyAt() error: %v", err) - } - if !bytes.Equal(key, oldKey) { - t.Fatalf("expected legacy key on write error") + _, err := getOrCreateKeyAt(newDir) + if err == nil || !strings.Contains(err.Error(), "unsafe encryption key path") { + t.Fatalf("expected unsafe encryption key path error, got %v", err) } } diff --git a/internal/securityutil/secure_storage_dir.go b/internal/securityutil/secure_storage_dir.go index 81774069c..fa017aa3c 100644 --- a/internal/securityutil/secure_storage_dir.go +++ b/internal/securityutil/secure_storage_dir.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "syscall" ) @@ -11,8 +12,16 @@ var ( secureStorageDirMkdirAllFn = os.MkdirAll secureStorageDirChmodFn = os.Chmod secureStorageDirLstatFn = os.Lstat + secureStorageFileLstatFn = os.Lstat + secureStorageFileReadFn = os.ReadFile + secureStorageFileTempFn = os.CreateTemp + secureStorageFileRenameFn = os.Rename + secureStorageFileRemoveFn = os.Remove + secureStorageFileChmodFn = os.Chmod ) +var ErrUnsafeStorageFile = errors.New("unsafe storage file") + // EnsureSecureStorageDir creates or hardens a storage directory to the desired // permissions when possible. For pre-mounted runtime storage roots such as // Kubernetes volume mounts, the running process may be able to write inside the @@ -50,3 +59,75 @@ func isStorageDirPermissionError(err error) bool { errors.Is(err, syscall.EPERM) || errors.Is(err, syscall.EACCES) } + +// ReadSecureStorageFile reads a regular, non-symlink storage file while +// enforcing a caller-provided size ceiling both before and after the read. +func ReadSecureStorageFile(path string, maxSize int64) ([]byte, error) { + if maxSize <= 0 { + return nil, fmt.Errorf("max storage file size must be positive") + } + + info, err := secureStorageFileLstatFn(path) + if err != nil { + return nil, err + } + if info.Mode()&os.ModeSymlink != 0 { + return nil, fmt.Errorf("%w: refusing symlink file path %q", ErrUnsafeStorageFile, path) + } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("%w: non-regular file path %q", ErrUnsafeStorageFile, path) + } + if info.Size() > maxSize { + return nil, fmt.Errorf("%w: file %q is too large (%d bytes)", ErrUnsafeStorageFile, path, info.Size()) + } + + data, err := secureStorageFileReadFn(path) + if err != nil { + return nil, err + } + if int64(len(data)) > maxSize { + return nil, fmt.Errorf("%w: file %q exceeded size limit while reading", ErrUnsafeStorageFile, path) + } + + return data, nil +} + +// WriteSecureStorageFile writes a file via a temp file + rename inside an +// already validated storage directory so file creation stays owner-only and +// does not follow pre-existing symlinks at the destination path. +func WriteSecureStorageFile(path string, data []byte, dirPerm, filePerm os.FileMode) error { + dir := filepath.Dir(path) + if err := EnsureSecureStorageDir(dir, dirPerm); err != nil { + return err + } + + tmpFile, err := secureStorageFileTempFn(dir, "."+filepath.Base(path)+".*.tmp") + if err != nil { + return err + } + tmpPath := tmpFile.Name() + cleanup := true + defer func() { + if cleanup { + _ = secureStorageFileRemoveFn(tmpPath) + } + }() + + if err := tmpFile.Chmod(filePerm); err != nil { + _ = tmpFile.Close() + return err + } + if _, err := tmpFile.Write(data); err != nil { + _ = tmpFile.Close() + return err + } + if err := tmpFile.Close(); err != nil { + return err + } + if err := secureStorageFileRenameFn(tmpPath, path); err != nil { + return err + } + cleanup = false + + return secureStorageFileChmodFn(path, filePerm) +} diff --git a/internal/securityutil/secure_storage_dir_test.go b/internal/securityutil/secure_storage_dir_test.go index 83472bc1e..be6cdfc18 100644 --- a/internal/securityutil/secure_storage_dir_test.go +++ b/internal/securityutil/secure_storage_dir_test.go @@ -1,17 +1,20 @@ package securityutil import ( + "errors" "os" + "path/filepath" "testing" "time" ) type stubFileInfo struct { mode os.FileMode + size int64 } func (s stubFileInfo) Name() string { return "stub" } -func (s stubFileInfo) Size() int64 { return 0 } +func (s stubFileInfo) Size() int64 { return s.size } func (s stubFileInfo) Mode() os.FileMode { return s.mode } func (s stubFileInfo) ModTime() time.Time { return time.Time{} } func (s stubFileInfo) IsDir() bool { return s.mode.IsDir() } @@ -21,6 +24,12 @@ func resetSecureStorageDirFns() { secureStorageDirMkdirAllFn = os.MkdirAll secureStorageDirChmodFn = os.Chmod secureStorageDirLstatFn = os.Lstat + secureStorageFileLstatFn = os.Lstat + secureStorageFileReadFn = os.ReadFile + secureStorageFileTempFn = os.CreateTemp + secureStorageFileRenameFn = os.Rename + secureStorageFileRemoveFn = os.Remove + secureStorageFileChmodFn = os.Chmod } func TestEnsureSecureStorageDir_CreatesOwnerOnlyDirectory(t *testing.T) { @@ -94,3 +103,57 @@ func TestEnsureSecureStorageDir_RejectsNonDirectoryPaths(t *testing.T) { t.Fatal("expected non-directory path error") } } + +func TestReadSecureStorageFile_RejectsSymlinkPaths(t *testing.T) { + t.Cleanup(resetSecureStorageDirFns) + + secureStorageFileLstatFn = func(string) (os.FileInfo, error) { + return stubFileInfo{mode: os.ModeSymlink}, nil + } + + _, err := ReadSecureStorageFile("/data/key", 32) + if !errors.Is(err, ErrUnsafeStorageFile) { + t.Fatalf("expected ErrUnsafeStorageFile, got %v", err) + } +} + +func TestReadSecureStorageFile_RejectsOversizedFiles(t *testing.T) { + t.Cleanup(resetSecureStorageDirFns) + + secureStorageFileLstatFn = func(string) (os.FileInfo, error) { + return stubFileInfo{mode: 0o600, size: 33}, nil + } + + _, err := ReadSecureStorageFile("/data/key", 32) + if !errors.Is(err, ErrUnsafeStorageFile) { + t.Fatalf("expected ErrUnsafeStorageFile, got %v", err) + } +} + +func TestWriteSecureStorageFile_CreatesOwnerOnlyFile(t *testing.T) { + t.Cleanup(resetSecureStorageDirFns) + + root := t.TempDir() + target := filepath.Join(root, "secure", "key.bin") + want := []byte("secret") + + if err := WriteSecureStorageFile(target, want, 0o700, 0o600); err != nil { + t.Fatalf("WriteSecureStorageFile() error: %v", err) + } + + data, err := os.ReadFile(target) + if err != nil { + t.Fatalf("read target: %v", err) + } + if string(data) != string(want) { + t.Fatalf("contents = %q, want %q", data, want) + } + + info, err := os.Stat(target) + if err != nil { + t.Fatalf("stat target: %v", err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("permissions = %o, want 600", got) + } +} diff --git a/scripts/release_control/subsystem_lookup_test.py b/scripts/release_control/subsystem_lookup_test.py index fb94f3d14..2a7e709cb 100644 --- a/scripts/release_control/subsystem_lookup_test.py +++ b/scripts/release_control/subsystem_lookup_test.py @@ -4428,9 +4428,11 @@ class SubsystemLookupTest(unittest.TestCase): ["scripts/release_control/subsystem_lookup_test.py"], ) - def test_lookup_paths_assigns_crypto_storage_hardening_to_security_privacy(self) -> None: + def test_lookup_paths_assigns_secure_storage_hardening_to_security_privacy(self) -> None: result = lookup_paths( [ + "internal/cloudcp/auth/magiclink.go", + "internal/cloudcp/auth/magiclink_store.go", "internal/crypto/crypto.go", "internal/securityutil/secure_storage_dir.go", ] @@ -4459,6 +4461,7 @@ class SubsystemLookupTest(unittest.TestCase): self.assertEqual( match["verification_requirement"]["exact_files"], [ + "internal/cloudcp/auth/magiclink_test.go", "internal/crypto/crypto_test.go", "internal/securityutil/secure_storage_dir_test.go", ],