fix(security): use unpredictable temp file for config writes
Replace the predictable <config>.tmp path with os.CreateTemp to prevent symlink-following attacks on shared hosts. The temp file is created in the config directory with a random name and explicit 0600 permissions.
- commit
- d2e89e6df6dc5cb2e85b72be02d7aa56af072317
- author
- brice · · 2026-03-14 22:19 UTC
- parent
- 7a412ec
- signature
- unsigned
2 files · +56 -6
M internal/usecase/config/service.go +23 -6
| @@ -591,13 +591,30 @@ func (s *Service) writeConfigSurgical(configFile string) error { | |||
| 591 | 591 | return fmt.Errorf("failed to marshal config: %w", err) | |
| 592 | 592 | } | |
| 593 | 593 | ||
| 594 | - | tmpFile := configFile + ".tmp" | |
| 595 | - | if err := os.WriteFile(tmpFile, out, 0600); err != nil { | |
| 596 | - | return fmt.Errorf("failed to write temp config: %w", err) | |
| 594 | + | configDir := filepath.Dir(configFile) | |
| 595 | + | tmpFile, err := os.CreateTemp(configDir, ".gordon-config-*.tmp") | |
| 596 | + | if err != nil { | |
| 597 | + | return fmt.Errorf("failed to create temp config file: %w", err) | |
| 598 | + | } | |
| 599 | + | tmpPath := tmpFile.Name() | |
| 600 | + | ||
| 601 | + | if _, writeErr := tmpFile.Write(out); writeErr != nil { | |
| 602 | + | _ = tmpFile.Close() | |
| 603 | + | _ = os.Remove(tmpPath) | |
| 604 | + | return fmt.Errorf("failed to write temp config: %w", writeErr) | |
| 605 | + | } | |
| 606 | + | if chmodErr := tmpFile.Chmod(0600); chmodErr != nil { | |
| 607 | + | _ = tmpFile.Close() | |
| 608 | + | _ = os.Remove(tmpPath) | |
| 609 | + | return fmt.Errorf("failed to set temp config permissions: %w", chmodErr) | |
| 610 | + | } | |
| 611 | + | if closeErr := tmpFile.Close(); closeErr != nil { | |
| 612 | + | _ = os.Remove(tmpPath) | |
| 613 | + | return fmt.Errorf("failed to close temp config: %w", closeErr) | |
| 597 | 614 | } | |
| 598 | - | if err := os.Rename(tmpFile, configFile); err != nil { | |
| 599 | - | _ = os.Remove(tmpFile) | |
| 600 | - | return fmt.Errorf("failed to rename temp config: %w", err) | |
| 615 | + | if renameErr := os.Rename(tmpPath, configFile); renameErr != nil { | |
| 616 | + | _ = os.Remove(tmpPath) | |
| 617 | + | return fmt.Errorf("failed to rename temp config: %w", renameErr) | |
| 601 | 618 | } | |
| 602 | 619 | ||
| 603 | 620 | return nil | |
| @@ -591,13 +591,30 @@ func (s *Service) writeConfigSurgical(configFile string) error { | |||||
| 591 | return fmt.Errorf("failed to marshal config: %w", err) | 591 | return fmt.Errorf("failed to marshal config: %w", err) | ||
| 592 | } | 592 | } | ||
| 593 | 593 | ||||
| 594 | - | tmpFile := configFile + ".tmp" | 594 | + | configDir := filepath.Dir(configFile) |
| 595 | - | if err := os.WriteFile(tmpFile, out, 0600); err != nil { | 595 | + | tmpFile, err := os.CreateTemp(configDir, ".gordon-config-*.tmp") |
| 596 | - | return fmt.Errorf("failed to write temp config: %w", err) | 596 | + | if err != nil { |
| 597 | + | return fmt.Errorf("failed to create temp config file: %w", err) | |||
| 598 | + | } | |||
| 599 | + | tmpPath := tmpFile.Name() | |||
| 600 | + | ||||
| 601 | + | if _, writeErr := tmpFile.Write(out); writeErr != nil { | |||
| 602 | + | _ = tmpFile.Close() | |||
| 603 | + | _ = os.Remove(tmpPath) | |||
| 604 | + | return fmt.Errorf("failed to write temp config: %w", writeErr) | |||
| 605 | + | } | |||
| 606 | + | if chmodErr := tmpFile.Chmod(0600); chmodErr != nil { | |||
| 607 | + | _ = tmpFile.Close() | |||
| 608 | + | _ = os.Remove(tmpPath) | |||
| 609 | + | return fmt.Errorf("failed to set temp config permissions: %w", chmodErr) | |||
| 610 | + | } | |||
| 611 | + | if closeErr := tmpFile.Close(); closeErr != nil { | |||
| 612 | + | _ = os.Remove(tmpPath) | |||
| 613 | + | return fmt.Errorf("failed to close temp config: %w", closeErr) | |||
| 597 | } | 614 | } | ||
| 598 | - | if err := os.Rename(tmpFile, configFile); err != nil { | 615 | + | if renameErr := os.Rename(tmpPath, configFile); renameErr != nil { |
| 599 | - | _ = os.Remove(tmpFile) | 616 | + | _ = os.Remove(tmpPath) |
| 600 | - | return fmt.Errorf("failed to rename temp config: %w", err) | 617 | + | return fmt.Errorf("failed to rename temp config: %w", renameErr) |
| 601 | } | 618 | } | ||
| 602 | 619 | ||||
| 603 | return nil | 620 | return nil | ||
M internal/usecase/config/service_test.go +33 -0
| @@ -1236,6 +1236,39 @@ func TestService_FindAttachmentTargetsByImage(t *testing.T) { | |||
| 1236 | 1236 | } | |
| 1237 | 1237 | ||
| 1238 | 1238 | func TestSavePreservesAllConfigFields(t *testing.T) { | |
| 1239 | + | t.Run("does not write predictable tmp path", func(t *testing.T) { | |
| 1240 | + | dir := t.TempDir() | |
| 1241 | + | configFile := filepath.Join(dir, "gordon.toml") | |
| 1242 | + | err := os.WriteFile(configFile, []byte("[server]\nport = 8080\n\n[routes]\n\"app.example.com\" = \"myapp:latest\"\n"), 0600) | |
| 1243 | + | require.NoError(t, err) | |
| 1244 | + | ||
| 1245 | + | v := viper.New() | |
| 1246 | + | v.SetConfigFile(configFile) | |
| 1247 | + | err = v.ReadInConfig() | |
| 1248 | + | require.NoError(t, err) | |
| 1249 | + | ||
| 1250 | + | eventBus := mocks.NewMockEventPublisher(t) | |
| 1251 | + | svc := NewService(v, eventBus) | |
| 1252 | + | ctx := testContext() | |
| 1253 | + | err = svc.Load(ctx) | |
| 1254 | + | require.NoError(t, err) | |
| 1255 | + | ||
| 1256 | + | maliciousTarget := filepath.Join(dir, "pwned.txt") | |
| 1257 | + | predictableTmp := configFile + ".tmp" | |
| 1258 | + | err = os.Symlink(maliciousTarget, predictableTmp) | |
| 1259 | + | require.NoError(t, err) | |
| 1260 | + | ||
| 1261 | + | err = svc.Save(ctx) | |
| 1262 | + | require.NoError(t, err) | |
| 1263 | + | ||
| 1264 | + | _, statErr := os.Stat(maliciousTarget) | |
| 1265 | + | assert.True(t, os.IsNotExist(statErr), "symlink target should not exist - temp file should use unpredictable name") | |
| 1266 | + | ||
| 1267 | + | info, err := os.Lstat(predictableTmp) | |
| 1268 | + | require.NoError(t, err) | |
| 1269 | + | assert.NotZero(t, info.Mode()&os.ModeSymlink, "predictable .tmp path should still be the original symlink") | |
| 1270 | + | }) | |
| 1271 | + | ||
| 1239 | 1272 | t.Run("with defaults does not corrupt config", func(t *testing.T) { | |
| 1240 | 1273 | tmpDir := t.TempDir() | |
| 1241 | 1274 | configFile := filepath.Join(tmpDir, "gordon.toml") | |
| @@ -1236,6 +1236,39 @@ func TestService_FindAttachmentTargetsByImage(t *testing.T) { | |||||
| 1236 | } | 1236 | } | ||
| 1237 | 1237 | ||||
| 1238 | func TestSavePreservesAllConfigFields(t *testing.T) { | 1238 | func TestSavePreservesAllConfigFields(t *testing.T) { | ||
| 1239 | + | t.Run("does not write predictable tmp path", func(t *testing.T) { | |||
| 1240 | + | dir := t.TempDir() | |||
| 1241 | + | configFile := filepath.Join(dir, "gordon.toml") | |||
| 1242 | + | err := os.WriteFile(configFile, []byte("[server]\nport = 8080\n\n[routes]\n\"app.example.com\" = \"myapp:latest\"\n"), 0600) | |||
| 1243 | + | require.NoError(t, err) | |||
| 1244 | + | ||||
| 1245 | + | v := viper.New() | |||
| 1246 | + | v.SetConfigFile(configFile) | |||
| 1247 | + | err = v.ReadInConfig() | |||
| 1248 | + | require.NoError(t, err) | |||
| 1249 | + | ||||
| 1250 | + | eventBus := mocks.NewMockEventPublisher(t) | |||
| 1251 | + | svc := NewService(v, eventBus) | |||
| 1252 | + | ctx := testContext() | |||
| 1253 | + | err = svc.Load(ctx) | |||
| 1254 | + | require.NoError(t, err) | |||
| 1255 | + | ||||
| 1256 | + | maliciousTarget := filepath.Join(dir, "pwned.txt") | |||
| 1257 | + | predictableTmp := configFile + ".tmp" | |||
| 1258 | + | err = os.Symlink(maliciousTarget, predictableTmp) | |||
| 1259 | + | require.NoError(t, err) | |||
| 1260 | + | ||||
| 1261 | + | err = svc.Save(ctx) | |||
| 1262 | + | require.NoError(t, err) | |||
| 1263 | + | ||||
| 1264 | + | _, statErr := os.Stat(maliciousTarget) | |||
| 1265 | + | assert.True(t, os.IsNotExist(statErr), "symlink target should not exist - temp file should use unpredictable name") | |||
| 1266 | + | ||||
| 1267 | + | info, err := os.Lstat(predictableTmp) | |||
| 1268 | + | require.NoError(t, err) | |||
| 1269 | + | assert.NotZero(t, info.Mode()&os.ModeSymlink, "predictable .tmp path should still be the original symlink") | |||
| 1270 | + | }) | |||
| 1271 | + | ||||
| 1239 | t.Run("with defaults does not corrupt config", func(t *testing.T) { | 1272 | t.Run("with defaults does not corrupt config", func(t *testing.T) { | ||
| 1240 | tmpDir := t.TempDir() | 1273 | tmpDir := t.TempDir() | ||
| 1241 | configFile := filepath.Join(tmpDir, "gordon.toml") | 1274 | configFile := filepath.Join(tmpDir, "gordon.toml") | ||