fix(snapshot): harden rooted filesystem lifecycle
- commit
- b649528a80174eabd724eb9ffeffaee1fb06d3a9
- author
- brice · · 2026-07-22 09:51 UTC
- parent
- d3bf1f1
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +139 -21
M internal/adapters/snapshot/atomic.go +7 -8
| @@ -69,7 +69,7 @@ func (r *Repository) ensurePrivateDirectoryPhase(dir, phase string) (err error) | |||
| 69 | 69 | if err != nil { | |
| 70 | 70 | return err | |
| 71 | 71 | } | |
| 72 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 72 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 73 | 73 | created := false | |
| 74 | 74 | mkdirErr := root.Mkdir(rel, 0o700) | |
| 75 | 75 | if mkdirErr == nil { | |
| @@ -85,8 +85,7 @@ func (r *Repository) ensurePrivateDirectoryPhase(dir, phase string) (err error) | |||
| 85 | 85 | if !fi.IsDir() { | |
| 86 | 86 | return fmt.Errorf("snapshot directory is not a directory") | |
| 87 | 87 | } | |
| 88 | - | st, ok := fi.Sys().(*syscall.Stat_t) | |
| 89 | - | if !ok || int(st.Uid) != os.Geteuid() || fi.Mode().Perm() != 0o700 { | |
| 88 | + | if !privateDirectory(fi) { | |
| 90 | 89 | return fmt.Errorf("snapshot directory is not private") | |
| 91 | 90 | } | |
| 92 | 91 | if created { | |
| @@ -250,7 +249,7 @@ func (r *Repository) installImmutable(oldPath, newPath string) (err error) { | |||
| 250 | 249 | if err != nil { | |
| 251 | 250 | return err | |
| 252 | 251 | } | |
| 253 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 252 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 254 | 253 | return root.Link(oldRel, newRel) | |
| 255 | 254 | } | |
| 256 | 255 | func (r *Repository) rename(oldPath, newPath string) (err error) { | |
| @@ -271,7 +270,7 @@ func (r *Repository) rename(oldPath, newPath string) (err error) { | |||
| 271 | 270 | if err != nil { | |
| 272 | 271 | return err | |
| 273 | 272 | } | |
| 274 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 273 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 275 | 274 | return root.Rename(oldRel, newRel) | |
| 276 | 275 | } | |
| 277 | 276 | func (r *Repository) remove(path string) (err error) { | |
| @@ -288,7 +287,7 @@ func (r *Repository) remove(path string) (err error) { | |||
| 288 | 287 | if err != nil { | |
| 289 | 288 | return err | |
| 290 | 289 | } | |
| 291 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 290 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 292 | 291 | return root.Remove(rel) | |
| 293 | 292 | } | |
| 294 | 293 | func (r *Repository) syncDirectory(dir string) error { | |
| @@ -328,7 +327,7 @@ func (r *Repository) readBounded(path string) (data []byte, err error) { | |||
| 328 | 327 | if err != nil { | |
| 329 | 328 | return nil, err | |
| 330 | 329 | } | |
| 331 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 330 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 332 | 331 | return r.readBoundedRoot(root, path) | |
| 333 | 332 | } | |
| 334 | 333 | ||
| @@ -348,7 +347,7 @@ func (r *Repository) readBoundedRoot(root *os.Root, path string) ([]byte, error) | |||
| 348 | 347 | } | |
| 349 | 348 | ||
| 350 | 349 | // readBounded remains for focused descriptor-hardening tests. Repository code | |
| 351 | - | // must use r.readBounded so intermediate components are pinned and no-follow. | |
| 350 | + | // must use r.readBounded so every path stays confined to the pinned private root. | |
| 352 | 351 | func readBounded(path string) ([]byte, error) { | |
| 353 | 352 | fd, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) | |
| 354 | 353 | if err != nil { | |
| @@ -69,7 +69,7 @@ func (r *Repository) ensurePrivateDirectoryPhase(dir, phase string) (err error) | |||||
| 69 | if err != nil { | 69 | if err != nil { | ||
| 70 | return err | 70 | return err | ||
| 71 | } | 71 | } | ||
| 72 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 72 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() |
| 73 | created := false | 73 | created := false | ||
| 74 | mkdirErr := root.Mkdir(rel, 0o700) | 74 | mkdirErr := root.Mkdir(rel, 0o700) | ||
| 75 | if mkdirErr == nil { | 75 | if mkdirErr == nil { | ||
| @@ -85,8 +85,7 @@ func (r *Repository) ensurePrivateDirectoryPhase(dir, phase string) (err error) | |||||
| 85 | if !fi.IsDir() { | 85 | if !fi.IsDir() { | ||
| 86 | return fmt.Errorf("snapshot directory is not a directory") | 86 | return fmt.Errorf("snapshot directory is not a directory") | ||
| 87 | } | 87 | } | ||
| 88 | - | st, ok := fi.Sys().(*syscall.Stat_t) | 88 | + | if !privateDirectory(fi) { |
| 89 | - | if !ok || int(st.Uid) != os.Geteuid() || fi.Mode().Perm() != 0o700 { | |||
| 90 | return fmt.Errorf("snapshot directory is not private") | 89 | return fmt.Errorf("snapshot directory is not private") | ||
| 91 | } | 90 | } | ||
| 92 | if created { | 91 | if created { | ||
| @@ -250,7 +249,7 @@ func (r *Repository) installImmutable(oldPath, newPath string) (err error) { | |||||
| 250 | if err != nil { | 249 | if err != nil { | ||
| 251 | return err | 250 | return err | ||
| 252 | } | 251 | } | ||
| 253 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 252 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() |
| 254 | return root.Link(oldRel, newRel) | 253 | return root.Link(oldRel, newRel) | ||
| 255 | } | 254 | } | ||
| 256 | func (r *Repository) rename(oldPath, newPath string) (err error) { | 255 | func (r *Repository) rename(oldPath, newPath string) (err error) { | ||
| @@ -271,7 +270,7 @@ func (r *Repository) rename(oldPath, newPath string) (err error) { | |||||
| 271 | if err != nil { | 270 | if err != nil { | ||
| 272 | return err | 271 | return err | ||
| 273 | } | 272 | } | ||
| 274 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 273 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() |
| 275 | return root.Rename(oldRel, newRel) | 274 | return root.Rename(oldRel, newRel) | ||
| 276 | } | 275 | } | ||
| 277 | func (r *Repository) remove(path string) (err error) { | 276 | func (r *Repository) remove(path string) (err error) { | ||
| @@ -288,7 +287,7 @@ func (r *Repository) remove(path string) (err error) { | |||||
| 288 | if err != nil { | 287 | if err != nil { | ||
| 289 | return err | 288 | return err | ||
| 290 | } | 289 | } | ||
| 291 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 290 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() |
| 292 | return root.Remove(rel) | 291 | return root.Remove(rel) | ||
| 293 | } | 292 | } | ||
| 294 | func (r *Repository) syncDirectory(dir string) error { | 293 | func (r *Repository) syncDirectory(dir string) error { | ||
| @@ -328,7 +327,7 @@ func (r *Repository) readBounded(path string) (data []byte, err error) { | |||||
| 328 | if err != nil { | 327 | if err != nil { | ||
| 329 | return nil, err | 328 | return nil, err | ||
| 330 | } | 329 | } | ||
| 331 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 330 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() |
| 332 | return r.readBoundedRoot(root, path) | 331 | return r.readBoundedRoot(root, path) | ||
| 333 | } | 332 | } | ||
| 334 | 333 | ||||
| @@ -348,7 +347,7 @@ func (r *Repository) readBoundedRoot(root *os.Root, path string) ([]byte, error) | |||||
| 348 | } | 347 | } | ||
| 349 | 348 | ||||
| 350 | // readBounded remains for focused descriptor-hardening tests. Repository code | 349 | // readBounded remains for focused descriptor-hardening tests. Repository code | ||
| 351 | - | // must use r.readBounded so intermediate components are pinned and no-follow. | 350 | + | // must use r.readBounded so every path stays confined to the pinned private root. |
| 352 | func readBounded(path string) ([]byte, error) { | 351 | func readBounded(path string) ([]byte, error) { | ||
| 353 | fd, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) | 352 | fd, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_CLOEXEC|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) | ||
| 354 | if err != nil { | 353 | if err != nil { | ||
M internal/adapters/snapshot/path_security_test.go +91 -2
| @@ -54,6 +54,97 @@ func TestRepositoryRejectsSymlinkedRoot(t *testing.T) { | |||
| 54 | 54 | } | |
| 55 | 55 | } | |
| 56 | 56 | ||
| 57 | + | func TestOpenRootRejectsUnsafePinnedRoot(t *testing.T) { | |
| 58 | + | dir := privateDir(t) | |
| 59 | + | if err := os.Mkdir(dir, 0o700); err != nil { | |
| 60 | + | t.Fatal(err) | |
| 61 | + | } | |
| 62 | + | if err := os.Chmod(dir, 0o755); err != nil { | |
| 63 | + | t.Fatal(err) | |
| 64 | + | } | |
| 65 | + | if _, err := NewRepository(dir).openRoot(); err == nil || !strings.Contains(err.Error(), "not private") { | |
| 66 | + | t.Fatalf("openRoot error = %v, want private-directory rejection", err) | |
| 67 | + | } | |
| 68 | + | } | |
| 69 | + | ||
| 70 | + | func TestOpenRootDetectsReplacementAfterOpen(t *testing.T) { | |
| 71 | + | dir := privateDir(t) | |
| 72 | + | if err := os.Mkdir(dir, 0o700); err != nil { | |
| 73 | + | t.Fatal(err) | |
| 74 | + | } | |
| 75 | + | old := dir + "-old" | |
| 76 | + | replacementMarker := filepath.Join(dir, "replacement-marker") | |
| 77 | + | repo := NewRepository(dir) | |
| 78 | + | repo.hooks.afterOpenRoot = func() { | |
| 79 | + | repo.hooks.afterOpenRoot = nil | |
| 80 | + | if err := os.Rename(dir, old); err != nil { | |
| 81 | + | t.Fatal(err) | |
| 82 | + | } | |
| 83 | + | if err := os.Mkdir(dir, 0o700); err != nil { | |
| 84 | + | t.Fatal(err) | |
| 85 | + | } | |
| 86 | + | if err := os.WriteFile(replacementMarker, []byte("unchanged"), 0o600); err != nil { | |
| 87 | + | t.Fatal(err) | |
| 88 | + | } | |
| 89 | + | } | |
| 90 | + | if _, err := repo.openRoot(); !errors.Is(err, syscall.ESTALE) { | |
| 91 | + | t.Fatalf("openRoot error = %v, want ESTALE", err) | |
| 92 | + | } | |
| 93 | + | if got, err := os.ReadFile(replacementMarker); err != nil || string(got) != "unchanged" { | |
| 94 | + | t.Fatalf("replacement marker = %q, %v", got, err) | |
| 95 | + | } | |
| 96 | + | if _, err := os.Stat(filepath.Join(old, "replacement-marker")); !errors.Is(err, os.ErrNotExist) { | |
| 97 | + | t.Fatalf("pinned tree was operated on: %v", err) | |
| 98 | + | } | |
| 99 | + | } | |
| 100 | + | ||
| 101 | + | func TestOpenDirectoryClosesChildWhenRootCloseFails(t *testing.T) { | |
| 102 | + | repo := NewRepository(privateDir(t)) | |
| 103 | + | if err := os.Mkdir(repo.dir, 0o700); err != nil { | |
| 104 | + | t.Fatal(err) | |
| 105 | + | } | |
| 106 | + | child := filepath.Join(repo.dir, "child") | |
| 107 | + | if err := os.Mkdir(child, 0o700); err != nil { | |
| 108 | + | t.Fatal(err) | |
| 109 | + | } | |
| 110 | + | closeErr := errors.New("injected root close") | |
| 111 | + | repo.hooks.closeRoot = func() error { return closeErr } | |
| 112 | + | before := openDescriptorCount(t) | |
| 113 | + | for range 20 { | |
| 114 | + | file, err := repo.openDirectory(child) | |
| 115 | + | if file != nil || !errors.Is(err, closeErr) { | |
| 116 | + | t.Fatalf("openDirectory = (%v, %v), want nil and close error", file, err) | |
| 117 | + | } | |
| 118 | + | } | |
| 119 | + | if got := openDescriptorCount(t); got > before+4 { | |
| 120 | + | t.Fatalf("open descriptors after failed openDirectory = %d, before = %d", got, before) | |
| 121 | + | } | |
| 122 | + | } | |
| 123 | + | ||
| 124 | + | func TestRootCloseErrorsJoinAndCloseRealRoot(t *testing.T) { | |
| 125 | + | repo := NewRepository(privateDir(t)) | |
| 126 | + | if err := os.Mkdir(repo.dir, 0o700); err != nil { | |
| 127 | + | t.Fatal(err) | |
| 128 | + | } | |
| 129 | + | closeErr := errors.New("injected root close") | |
| 130 | + | repo.hooks.closeRoot = func() error { return closeErr } | |
| 131 | + | root, err := repo.openRoot() | |
| 132 | + | if err != nil { | |
| 133 | + | t.Fatal(err) | |
| 134 | + | } | |
| 135 | + | if err := repo.closeRoot(root); !errors.Is(err, closeErr) { | |
| 136 | + | t.Fatalf("closeRoot error = %v, want injected error", err) | |
| 137 | + | } | |
| 138 | + | if _, err := root.Stat("."); err == nil { | |
| 139 | + | t.Fatal("real root remained open after injected close error") | |
| 140 | + | } | |
| 141 | + | ||
| 142 | + | _, err = repo.stat(filepath.Join(repo.dir, "missing")) | |
| 143 | + | if !errors.Is(err, closeErr) || !errors.Is(err, os.ErrNotExist) { | |
| 144 | + | t.Fatalf("stat error = %v, want primary and close errors", err) | |
| 145 | + | } | |
| 146 | + | } | |
| 147 | + | ||
| 57 | 148 | func TestRepositoryRejectsSymlinkedGenerationAndObjectShards(t *testing.T) { | |
| 58 | 149 | for _, target := range []string{repositoryGenerations, repositoryObjectsDir} { | |
| 59 | 150 | t.Run(target, func(t *testing.T) { | |
| @@ -78,7 +169,6 @@ func TestRepositoryRejectsSymlinkedGenerationAndObjectShards(t *testing.T) { | |||
| 78 | 169 | if _, err := repo.Load(context.Background(), publication.Name); err == nil { | |
| 79 | 170 | t.Fatal("Load succeeded through symlinked repository component") | |
| 80 | 171 | } | |
| 81 | - | // Maintenance must reject the replacement rather than following it. | |
| 82 | 172 | if err := repo.Maintain(context.Background()); err == nil { | |
| 83 | 173 | t.Fatal("Maintain succeeded through symlinked repository component") | |
| 84 | 174 | } | |
| @@ -103,7 +193,6 @@ func TestFinalSymlinksRejectedByRootOperations(t *testing.T) { | |||
| 103 | 193 | if err := os.Symlink(target, link); err != nil { | |
| 104 | 194 | t.Fatal(err) | |
| 105 | 195 | } | |
| 106 | - | ||
| 107 | 196 | if _, err := repo.openDirectory(link); !errors.Is(err, syscall.ELOOP) || strings.Contains(err.Error(), repo.dir) { | |
| 108 | 197 | t.Fatalf("openDirectory error = %v, want sanitized ELOOP", err) | |
| 109 | 198 | } | |
| @@ -54,6 +54,97 @@ func TestRepositoryRejectsSymlinkedRoot(t *testing.T) { | |||||
| 54 | } | 54 | } | ||
| 55 | } | 55 | } | ||
| 56 | 56 | ||||
| 57 | + | func TestOpenRootRejectsUnsafePinnedRoot(t *testing.T) { | |||
| 58 | + | dir := privateDir(t) | |||
| 59 | + | if err := os.Mkdir(dir, 0o700); err != nil { | |||
| 60 | + | t.Fatal(err) | |||
| 61 | + | } | |||
| 62 | + | if err := os.Chmod(dir, 0o755); err != nil { | |||
| 63 | + | t.Fatal(err) | |||
| 64 | + | } | |||
| 65 | + | if _, err := NewRepository(dir).openRoot(); err == nil || !strings.Contains(err.Error(), "not private") { | |||
| 66 | + | t.Fatalf("openRoot error = %v, want private-directory rejection", err) | |||
| 67 | + | } | |||
| 68 | + | } | |||
| 69 | + | ||||
| 70 | + | func TestOpenRootDetectsReplacementAfterOpen(t *testing.T) { | |||
| 71 | + | dir := privateDir(t) | |||
| 72 | + | if err := os.Mkdir(dir, 0o700); err != nil { | |||
| 73 | + | t.Fatal(err) | |||
| 74 | + | } | |||
| 75 | + | old := dir + "-old" | |||
| 76 | + | replacementMarker := filepath.Join(dir, "replacement-marker") | |||
| 77 | + | repo := NewRepository(dir) | |||
| 78 | + | repo.hooks.afterOpenRoot = func() { | |||
| 79 | + | repo.hooks.afterOpenRoot = nil | |||
| 80 | + | if err := os.Rename(dir, old); err != nil { | |||
| 81 | + | t.Fatal(err) | |||
| 82 | + | } | |||
| 83 | + | if err := os.Mkdir(dir, 0o700); err != nil { | |||
| 84 | + | t.Fatal(err) | |||
| 85 | + | } | |||
| 86 | + | if err := os.WriteFile(replacementMarker, []byte("unchanged"), 0o600); err != nil { | |||
| 87 | + | t.Fatal(err) | |||
| 88 | + | } | |||
| 89 | + | } | |||
| 90 | + | if _, err := repo.openRoot(); !errors.Is(err, syscall.ESTALE) { | |||
| 91 | + | t.Fatalf("openRoot error = %v, want ESTALE", err) | |||
| 92 | + | } | |||
| 93 | + | if got, err := os.ReadFile(replacementMarker); err != nil || string(got) != "unchanged" { | |||
| 94 | + | t.Fatalf("replacement marker = %q, %v", got, err) | |||
| 95 | + | } | |||
| 96 | + | if _, err := os.Stat(filepath.Join(old, "replacement-marker")); !errors.Is(err, os.ErrNotExist) { | |||
| 97 | + | t.Fatalf("pinned tree was operated on: %v", err) | |||
| 98 | + | } | |||
| 99 | + | } | |||
| 100 | + | ||||
| 101 | + | func TestOpenDirectoryClosesChildWhenRootCloseFails(t *testing.T) { | |||
| 102 | + | repo := NewRepository(privateDir(t)) | |||
| 103 | + | if err := os.Mkdir(repo.dir, 0o700); err != nil { | |||
| 104 | + | t.Fatal(err) | |||
| 105 | + | } | |||
| 106 | + | child := filepath.Join(repo.dir, "child") | |||
| 107 | + | if err := os.Mkdir(child, 0o700); err != nil { | |||
| 108 | + | t.Fatal(err) | |||
| 109 | + | } | |||
| 110 | + | closeErr := errors.New("injected root close") | |||
| 111 | + | repo.hooks.closeRoot = func() error { return closeErr } | |||
| 112 | + | before := openDescriptorCount(t) | |||
| 113 | + | for range 20 { | |||
| 114 | + | file, err := repo.openDirectory(child) | |||
| 115 | + | if file != nil || !errors.Is(err, closeErr) { | |||
| 116 | + | t.Fatalf("openDirectory = (%v, %v), want nil and close error", file, err) | |||
| 117 | + | } | |||
| 118 | + | } | |||
| 119 | + | if got := openDescriptorCount(t); got > before+4 { | |||
| 120 | + | t.Fatalf("open descriptors after failed openDirectory = %d, before = %d", got, before) | |||
| 121 | + | } | |||
| 122 | + | } | |||
| 123 | + | ||||
| 124 | + | func TestRootCloseErrorsJoinAndCloseRealRoot(t *testing.T) { | |||
| 125 | + | repo := NewRepository(privateDir(t)) | |||
| 126 | + | if err := os.Mkdir(repo.dir, 0o700); err != nil { | |||
| 127 | + | t.Fatal(err) | |||
| 128 | + | } | |||
| 129 | + | closeErr := errors.New("injected root close") | |||
| 130 | + | repo.hooks.closeRoot = func() error { return closeErr } | |||
| 131 | + | root, err := repo.openRoot() | |||
| 132 | + | if err != nil { | |||
| 133 | + | t.Fatal(err) | |||
| 134 | + | } | |||
| 135 | + | if err := repo.closeRoot(root); !errors.Is(err, closeErr) { | |||
| 136 | + | t.Fatalf("closeRoot error = %v, want injected error", err) | |||
| 137 | + | } | |||
| 138 | + | if _, err := root.Stat("."); err == nil { | |||
| 139 | + | t.Fatal("real root remained open after injected close error") | |||
| 140 | + | } | |||
| 141 | + | ||||
| 142 | + | _, err = repo.stat(filepath.Join(repo.dir, "missing")) | |||
| 143 | + | if !errors.Is(err, closeErr) || !errors.Is(err, os.ErrNotExist) { | |||
| 144 | + | t.Fatalf("stat error = %v, want primary and close errors", err) | |||
| 145 | + | } | |||
| 146 | + | } | |||
| 147 | + | ||||
| 57 | func TestRepositoryRejectsSymlinkedGenerationAndObjectShards(t *testing.T) { | 148 | func TestRepositoryRejectsSymlinkedGenerationAndObjectShards(t *testing.T) { | ||
| 58 | for _, target := range []string{repositoryGenerations, repositoryObjectsDir} { | 149 | for _, target := range []string{repositoryGenerations, repositoryObjectsDir} { | ||
| 59 | t.Run(target, func(t *testing.T) { | 150 | t.Run(target, func(t *testing.T) { | ||
| @@ -78,7 +169,6 @@ func TestRepositoryRejectsSymlinkedGenerationAndObjectShards(t *testing.T) { | |||||
| 78 | if _, err := repo.Load(context.Background(), publication.Name); err == nil { | 169 | if _, err := repo.Load(context.Background(), publication.Name); err == nil { | ||
| 79 | t.Fatal("Load succeeded through symlinked repository component") | 170 | t.Fatal("Load succeeded through symlinked repository component") | ||
| 80 | } | 171 | } | ||
| 81 | - | // Maintenance must reject the replacement rather than following it. | |||
| 82 | if err := repo.Maintain(context.Background()); err == nil { | 172 | if err := repo.Maintain(context.Background()); err == nil { | ||
| 83 | t.Fatal("Maintain succeeded through symlinked repository component") | 173 | t.Fatal("Maintain succeeded through symlinked repository component") | ||
| 84 | } | 174 | } | ||
| @@ -103,7 +193,6 @@ func TestFinalSymlinksRejectedByRootOperations(t *testing.T) { | |||||
| 103 | if err := os.Symlink(target, link); err != nil { | 193 | if err := os.Symlink(target, link); err != nil { | ||
| 104 | t.Fatal(err) | 194 | t.Fatal(err) | ||
| 105 | } | 195 | } | ||
| 106 | - | ||||
| 107 | if _, err := repo.openDirectory(link); !errors.Is(err, syscall.ELOOP) || strings.Contains(err.Error(), repo.dir) { | 196 | if _, err := repo.openDirectory(link); !errors.Is(err, syscall.ELOOP) || strings.Contains(err.Error(), repo.dir) { | ||
| 108 | t.Fatalf("openDirectory error = %v, want sanitized ELOOP", err) | 197 | t.Fatalf("openDirectory error = %v, want sanitized ELOOP", err) | ||
| 109 | } | 198 | } | ||
M internal/adapters/snapshot/publish.go +1 -1
| @@ -153,7 +153,7 @@ func (r *Repository) unchangedPublication(key string, publication ports.Snapshot | |||
| 153 | 153 | if err != nil { | |
| 154 | 154 | return false, err | |
| 155 | 155 | } | |
| 156 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 156 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 157 | 157 | ||
| 158 | 158 | generation, _, err := r.readHeadWithRoot(root, key) | |
| 159 | 159 | if err != nil || generation != publication.Generation { | |
| @@ -153,7 +153,7 @@ func (r *Repository) unchangedPublication(key string, publication ports.Snapshot | |||||
| 153 | if err != nil { | 153 | if err != nil { | ||
| 154 | return false, err | 154 | return false, err | ||
| 155 | } | 155 | } | ||
| 156 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 156 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() |
| 157 | 157 | ||||
| 158 | generation, _, err := r.readHeadWithRoot(root, key) | 158 | generation, _, err := r.readHeadWithRoot(root, key) | ||
| 159 | if err != nil || generation != publication.Generation { | 159 | if err != nil || generation != publication.Generation { | ||
M internal/adapters/snapshot/repository.go +5 -1
| @@ -71,7 +71,11 @@ type repositoryHooks struct { | |||
| 71 | 71 | rename func(string) error | |
| 72 | 72 | syncDirectory func(string) error | |
| 73 | 73 | remove func(string) error | |
| 74 | - | openMaintenanceDirectory func(string) (maintenanceDirectory, error) | |
| 74 | + | // afterOpenRoot and closeRoot make descriptor race and close-error paths | |
| 75 | + | // deterministic in package tests. closeRoot never replaces the real close. | |
| 76 | + | afterOpenRoot func() | |
| 77 | + | closeRoot func() error | |
| 78 | + | openMaintenanceDirectory func(string) (maintenanceDirectory, error) | |
| 75 | 79 | // beforeMaintenanceWork observes each budgeted quarantine filesystem step. | |
| 76 | 80 | // It is test-only instrumentation for hostile traversal bounds. | |
| 77 | 81 | beforeMaintenanceWork func(string) | |
| @@ -71,7 +71,11 @@ type repositoryHooks struct { | |||||
| 71 | rename func(string) error | 71 | rename func(string) error | ||
| 72 | syncDirectory func(string) error | 72 | syncDirectory func(string) error | ||
| 73 | remove func(string) error | 73 | remove func(string) error | ||
| 74 | - | openMaintenanceDirectory func(string) (maintenanceDirectory, error) | 74 | + | // afterOpenRoot and closeRoot make descriptor race and close-error paths |
| 75 | + | // deterministic in package tests. closeRoot never replaces the real close. | |||
| 76 | + | afterOpenRoot func() | |||
| 77 | + | closeRoot func() error | |||
| 78 | + | openMaintenanceDirectory func(string) (maintenanceDirectory, error) | |||
| 75 | // beforeMaintenanceWork observes each budgeted quarantine filesystem step. | 79 | // beforeMaintenanceWork observes each budgeted quarantine filesystem step. | ||
| 76 | // It is test-only instrumentation for hostile traversal bounds. | 80 | // It is test-only instrumentation for hostile traversal bounds. | ||
| 77 | beforeMaintenanceWork func(string) | 81 | beforeMaintenanceWork func(string) | ||
M internal/adapters/snapshot/safe_path.go +35 -9
| @@ -18,25 +18,44 @@ func (r *Repository) openRoot() (*os.Root, error) { | |||
| 18 | 18 | if err != nil { | |
| 19 | 19 | return nil, err | |
| 20 | 20 | } | |
| 21 | + | if hook := r.hooks.afterOpenRoot; hook != nil { | |
| 22 | + | hook() | |
| 23 | + | } | |
| 21 | 24 | current, err := os.Lstat(r.dir) | |
| 22 | 25 | if err != nil { | |
| 23 | - | return nil, closeRootOnError(root, err) | |
| 26 | + | return nil, r.closeRootOnError(root, err) | |
| 24 | 27 | } | |
| 25 | 28 | if !current.IsDir() || current.Mode()&os.ModeSymlink != 0 { | |
| 26 | - | return nil, closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ELOOP}) | |
| 29 | + | return nil, r.closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ELOOP}) | |
| 27 | 30 | } | |
| 28 | 31 | pinned, err := root.Stat(".") | |
| 29 | 32 | if err != nil { | |
| 30 | - | return nil, closeRootOnError(root, err) | |
| 33 | + | return nil, r.closeRootOnError(root, err) | |
| 31 | 34 | } | |
| 32 | 35 | if !os.SameFile(current, pinned) { | |
| 33 | - | return nil, closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ESTALE}) | |
| 36 | + | return nil, r.closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ESTALE}) | |
| 37 | + | } | |
| 38 | + | if !privateDirectory(pinned) { | |
| 39 | + | return nil, r.closeRootOnError(root, fmt.Errorf("snapshot directory is not private")) | |
| 34 | 40 | } | |
| 35 | 41 | return root, nil | |
| 36 | 42 | } | |
| 37 | 43 | ||
| 38 | - | func closeRootOnError(root *os.Root, err error) error { | |
| 39 | - | return errors.Join(err, root.Close()) | |
| 44 | + | func privateDirectory(fi os.FileInfo) bool { | |
| 45 | + | st, ok := fi.Sys().(*syscall.Stat_t) | |
| 46 | + | return ok && fi.IsDir() && int(st.Uid) == os.Geteuid() && fi.Mode().Perm() == 0o700 | |
| 47 | + | } | |
| 48 | + | ||
| 49 | + | func (r *Repository) closeRoot(root *os.Root) error { | |
| 50 | + | var injected error | |
| 51 | + | if hook := r.hooks.closeRoot; hook != nil { | |
| 52 | + | injected = hook() | |
| 53 | + | } | |
| 54 | + | return errors.Join(injected, root.Close()) | |
| 55 | + | } | |
| 56 | + | ||
| 57 | + | func (r *Repository) closeRootOnError(root *os.Root, err error) error { | |
| 58 | + | return errors.Join(err, r.closeRoot(root)) | |
| 40 | 59 | } | |
| 41 | 60 | ||
| 42 | 61 | // rejectFinalSymlink preserves the pre-os.Root guarantee that the final | |
| @@ -62,7 +81,14 @@ func (r *Repository) openDirectory(path string) (file *os.File, err error) { | |||
| 62 | 81 | if err != nil { | |
| 63 | 82 | return nil, err | |
| 64 | 83 | } | |
| 65 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 84 | + | defer func() { | |
| 85 | + | closeErr := r.closeRoot(root) | |
| 86 | + | if closeErr != nil && file != nil { | |
| 87 | + | joinCloseError(&err, "close snapshot directory after root close failure", file.Close()) | |
| 88 | + | file = nil | |
| 89 | + | } | |
| 90 | + | joinCloseError(&err, "close snapshot root", closeErr) | |
| 91 | + | }() | |
| 66 | 92 | if err := rejectFinalSymlink(root, rel); err != nil { | |
| 67 | 93 | return nil, err | |
| 68 | 94 | } | |
| @@ -93,7 +119,7 @@ func (r *Repository) stat(path string) (fi os.FileInfo, err error) { | |||
| 93 | 119 | if err != nil { | |
| 94 | 120 | return nil, err | |
| 95 | 121 | } | |
| 96 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | |
| 122 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 97 | 123 | return root.Lstat(rel) | |
| 98 | 124 | } | |
| 99 | 125 | ||
| @@ -109,7 +135,7 @@ func (r *Repository) createTempAt(dir string) (temp *os.File, err error) { | |||
| 109 | 135 | return nil, err | |
| 110 | 136 | } | |
| 111 | 137 | defer func() { | |
| 112 | - | joinCloseError(&err, "close snapshot root", root.Close()) | |
| 138 | + | joinCloseError(&err, "close snapshot root", r.closeRoot(root)) | |
| 113 | 139 | if err != nil && temp != nil { | |
| 114 | 140 | joinCloseError(&err, "close snapshot temporary file after root close failure", temp.Close()) | |
| 115 | 141 | temp = nil | |
| @@ -18,25 +18,44 @@ func (r *Repository) openRoot() (*os.Root, error) { | |||||
| 18 | if err != nil { | 18 | if err != nil { | ||
| 19 | return nil, err | 19 | return nil, err | ||
| 20 | } | 20 | } | ||
| 21 | + | if hook := r.hooks.afterOpenRoot; hook != nil { | |||
| 22 | + | hook() | |||
| 23 | + | } | |||
| 21 | current, err := os.Lstat(r.dir) | 24 | current, err := os.Lstat(r.dir) | ||
| 22 | if err != nil { | 25 | if err != nil { | ||
| 23 | - | return nil, closeRootOnError(root, err) | 26 | + | return nil, r.closeRootOnError(root, err) |
| 24 | } | 27 | } | ||
| 25 | if !current.IsDir() || current.Mode()&os.ModeSymlink != 0 { | 28 | if !current.IsDir() || current.Mode()&os.ModeSymlink != 0 { | ||
| 26 | - | return nil, closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ELOOP}) | 29 | + | return nil, r.closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ELOOP}) |
| 27 | } | 30 | } | ||
| 28 | pinned, err := root.Stat(".") | 31 | pinned, err := root.Stat(".") | ||
| 29 | if err != nil { | 32 | if err != nil { | ||
| 30 | - | return nil, closeRootOnError(root, err) | 33 | + | return nil, r.closeRootOnError(root, err) |
| 31 | } | 34 | } | ||
| 32 | if !os.SameFile(current, pinned) { | 35 | if !os.SameFile(current, pinned) { | ||
| 33 | - | return nil, closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ESTALE}) | 36 | + | return nil, r.closeRootOnError(root, &os.PathError{Op: "open", Path: r.dir, Err: syscall.ESTALE}) |
| 37 | + | } | |||
| 38 | + | if !privateDirectory(pinned) { | |||
| 39 | + | return nil, r.closeRootOnError(root, fmt.Errorf("snapshot directory is not private")) | |||
| 34 | } | 40 | } | ||
| 35 | return root, nil | 41 | return root, nil | ||
| 36 | } | 42 | } | ||
| 37 | 43 | ||||
| 38 | - | func closeRootOnError(root *os.Root, err error) error { | 44 | + | func privateDirectory(fi os.FileInfo) bool { |
| 39 | - | return errors.Join(err, root.Close()) | 45 | + | st, ok := fi.Sys().(*syscall.Stat_t) |
| 46 | + | return ok && fi.IsDir() && int(st.Uid) == os.Geteuid() && fi.Mode().Perm() == 0o700 | |||
| 47 | + | } | |||
| 48 | + | ||||
| 49 | + | func (r *Repository) closeRoot(root *os.Root) error { | |||
| 50 | + | var injected error | |||
| 51 | + | if hook := r.hooks.closeRoot; hook != nil { | |||
| 52 | + | injected = hook() | |||
| 53 | + | } | |||
| 54 | + | return errors.Join(injected, root.Close()) | |||
| 55 | + | } | |||
| 56 | + | ||||
| 57 | + | func (r *Repository) closeRootOnError(root *os.Root, err error) error { | |||
| 58 | + | return errors.Join(err, r.closeRoot(root)) | |||
| 40 | } | 59 | } | ||
| 41 | 60 | ||||
| 42 | // rejectFinalSymlink preserves the pre-os.Root guarantee that the final | 61 | // rejectFinalSymlink preserves the pre-os.Root guarantee that the final | ||
| @@ -62,7 +81,14 @@ func (r *Repository) openDirectory(path string) (file *os.File, err error) { | |||||
| 62 | if err != nil { | 81 | if err != nil { | ||
| 63 | return nil, err | 82 | return nil, err | ||
| 64 | } | 83 | } | ||
| 65 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 84 | + | defer func() { |
| 85 | + | closeErr := r.closeRoot(root) | |||
| 86 | + | if closeErr != nil && file != nil { | |||
| 87 | + | joinCloseError(&err, "close snapshot directory after root close failure", file.Close()) | |||
| 88 | + | file = nil | |||
| 89 | + | } | |||
| 90 | + | joinCloseError(&err, "close snapshot root", closeErr) | |||
| 91 | + | }() | |||
| 66 | if err := rejectFinalSymlink(root, rel); err != nil { | 92 | if err := rejectFinalSymlink(root, rel); err != nil { | ||
| 67 | return nil, err | 93 | return nil, err | ||
| 68 | } | 94 | } | ||
| @@ -93,7 +119,7 @@ func (r *Repository) stat(path string) (fi os.FileInfo, err error) { | |||||
| 93 | if err != nil { | 119 | if err != nil { | ||
| 94 | return nil, err | 120 | return nil, err | ||
| 95 | } | 121 | } | ||
| 96 | - | defer func() { joinCloseError(&err, "close snapshot root", root.Close()) }() | 122 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() |
| 97 | return root.Lstat(rel) | 123 | return root.Lstat(rel) | ||
| 98 | } | 124 | } | ||
| 99 | 125 | ||||
| @@ -109,7 +135,7 @@ func (r *Repository) createTempAt(dir string) (temp *os.File, err error) { | |||||
| 109 | return nil, err | 135 | return nil, err | ||
| 110 | } | 136 | } | ||
| 111 | defer func() { | 137 | defer func() { | ||
| 112 | - | joinCloseError(&err, "close snapshot root", root.Close()) | 138 | + | joinCloseError(&err, "close snapshot root", r.closeRoot(root)) |
| 113 | if err != nil && temp != nil { | 139 | if err != nil && temp != nil { | ||
| 114 | joinCloseError(&err, "close snapshot temporary file after root close failure", temp.Close()) | 140 | joinCloseError(&err, "close snapshot temporary file after root close failure", temp.Close()) | ||
| 115 | temp = nil | 141 | temp = nil | ||