fix(executor): resolve every archive symlink through the other links before accepting the tree
- commit
- 547d930b9a31312675aa5b1c49588aa441e0e26e
- author
- brice · · 2026-10-14 03:00 UTC
- parent
- 0a7462c
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +139 -2
M internal/executor/actions_extract.go +93 -2
| @@ -9,8 +9,10 @@ import ( | |||
| 9 | 9 | "errors" | |
| 10 | 10 | "fmt" | |
| 11 | 11 | "io" | |
| 12 | + | "io/fs" | |
| 12 | 13 | "os" | |
| 13 | 14 | "path" | |
| 15 | + | "path/filepath" | |
| 14 | 16 | "strings" | |
| 15 | 17 | ) | |
| 16 | 18 | ||
| @@ -78,7 +80,7 @@ func extractTarball(ctx context.Context, r io.Reader, dir string, owner *[2]int) | |||
| 78 | 80 | } | |
| 79 | 81 | h, err := tr.Next() | |
| 80 | 82 | if errors.Is(err, io.EOF) { | |
| 81 | - | return nil | |
| 83 | + | return verifyLinks(ctx, root, dir) | |
| 82 | 84 | } | |
| 83 | 85 | if err != nil { | |
| 84 | 86 | var ea errArchive | |
| @@ -208,7 +210,10 @@ func extractEntry(root *os.Root, tr *tar.Reader, h *tar.Header, owner *[2]int, t | |||
| 208 | 210 | } | |
| 209 | 211 | return chown() | |
| 210 | 212 | case tar.TypeSymlink: | |
| 211 | - | if !linkInside(name, h.Linkname, false) { | |
| 213 | + | // Only the shape is checked here: where the link leads depends on the links | |
| 214 | + | // that come before and after it, so verifyLinks resolves every symlink of | |
| 215 | + | // the finished tree. | |
| 216 | + | if h.Linkname == "" || strings.HasPrefix(h.Linkname, "/") || strings.ContainsRune(h.Linkname, 0) { | |
| 212 | 217 | return errArchive(fmt.Sprintf("the archive has a link that points outside of it: %q -> %q", name, h.Linkname)) | |
| 213 | 218 | } | |
| 214 | 219 | _ = root.Remove(name) | |
| @@ -226,3 +231,89 @@ func extractEntry(root *os.Root, tr *tar.Reader, h *tar.Header, owner *[2]int, t | |||
| 226 | 231 | } | |
| 227 | 232 | return nil // devices, fifos: skipped | |
| 228 | 233 | } | |
| 234 | + | ||
| 235 | + | // maxLinkHops bounds the symbolic links followed to resolve one link (Linux | |
| 236 | + | // stops at 40). | |
| 237 | + | const maxLinkHops = 40 | |
| 238 | + | ||
| 239 | + | // verifyLinks resolves every symbolic link of the extracted tree component by | |
| 240 | + | // component, following the links met on the way, and rejects the archive when | |
| 241 | + | // one leads out of the tree or loops. A lexical check is not enough: with | |
| 242 | + | // "a -> ." a link "link -> a/../outside" looks like it stays in the tree, and | |
| 243 | + | // leaves it. A link to something that does not exist is allowed (git archives | |
| 244 | + | // have such links); the rest of its target is still resolved. | |
| 245 | + | func verifyLinks(ctx context.Context, root *os.Root, dir string) error { | |
| 246 | + | return filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error { | |
| 247 | + | if err != nil { | |
| 248 | + | return err | |
| 249 | + | } | |
| 250 | + | if err := ctx.Err(); err != nil { | |
| 251 | + | return err | |
| 252 | + | } | |
| 253 | + | if d.Type()&fs.ModeSymlink == 0 { | |
| 254 | + | return nil | |
| 255 | + | } | |
| 256 | + | rel, err := filepath.Rel(dir, p) | |
| 257 | + | if err != nil { | |
| 258 | + | return err | |
| 259 | + | } | |
| 260 | + | target, err := root.Readlink(filepath.ToSlash(rel)) | |
| 261 | + | if err != nil { | |
| 262 | + | return err | |
| 263 | + | } | |
| 264 | + | if err := resolveInside(root, path.Dir(filepath.ToSlash(rel)), target); err != nil { | |
| 265 | + | return errArchive(fmt.Sprintf("the archive has a link that points outside of it: %q -> %q (%v)", filepath.ToSlash(rel), target, err)) | |
| 266 | + | } | |
| 267 | + | return nil | |
| 268 | + | }) | |
| 269 | + | } | |
| 270 | + | ||
| 271 | + | // resolveInside resolves target, relative to the directory dirRel of the tree | |
| 272 | + | // (slash separated, "." for the root), the way the kernel would, and fails when | |
| 273 | + | // the path leaves the root, is absolute, or needs more than maxLinkHops links. | |
| 274 | + | func resolveInside(root *os.Root, dirRel, target string) error { | |
| 275 | + | var cur []string // resolved directory components, none of them a link | |
| 276 | + | if dirRel != "." && dirRel != "" { | |
| 277 | + | cur = strings.Split(dirRel, "/") | |
| 278 | + | } | |
| 279 | + | pending := strings.Split(target, "/") | |
| 280 | + | hops := 0 | |
| 281 | + | for len(pending) > 0 { | |
| 282 | + | c := pending[0] | |
| 283 | + | pending = pending[1:] | |
| 284 | + | switch c { | |
| 285 | + | case "", ".": | |
| 286 | + | continue | |
| 287 | + | case "..": | |
| 288 | + | if len(cur) == 0 { | |
| 289 | + | return errors.New("leaves the tree") | |
| 290 | + | } | |
| 291 | + | cur = cur[:len(cur)-1] | |
| 292 | + | continue | |
| 293 | + | } | |
| 294 | + | p := strings.Join(append(append([]string(nil), cur...), c), "/") | |
| 295 | + | fi, err := root.Lstat(p) | |
| 296 | + | switch { | |
| 297 | + | case errors.Is(err, os.ErrNotExist): | |
| 298 | + | cur = append(cur, c) | |
| 299 | + | continue | |
| 300 | + | case err != nil: | |
| 301 | + | return err | |
| 302 | + | case fi.Mode()&fs.ModeSymlink == 0: | |
| 303 | + | cur = append(cur, c) | |
| 304 | + | continue | |
| 305 | + | } | |
| 306 | + | if hops++; hops > maxLinkHops { | |
| 307 | + | return errors.New("too many levels of links") | |
| 308 | + | } | |
| 309 | + | next, err := root.Readlink(p) | |
| 310 | + | if err != nil { | |
| 311 | + | return err | |
| 312 | + | } | |
| 313 | + | if strings.HasPrefix(next, "/") || next == "" { | |
| 314 | + | return errors.New("an absolute or empty link") | |
| 315 | + | } | |
| 316 | + | pending = append(strings.Split(next, "/"), pending...) | |
| 317 | + | } | |
| 318 | + | return nil | |
| 319 | + | } | |
| @@ -9,8 +9,10 @@ import ( | |||||
| 9 | "errors" | 9 | "errors" | ||
| 10 | "fmt" | 10 | "fmt" | ||
| 11 | "io" | 11 | "io" | ||
| 12 | + | "io/fs" | |||
| 12 | "os" | 13 | "os" | ||
| 13 | "path" | 14 | "path" | ||
| 15 | + | "path/filepath" | |||
| 14 | "strings" | 16 | "strings" | ||
| 15 | ) | 17 | ) | ||
| 16 | 18 | ||||
| @@ -78,7 +80,7 @@ func extractTarball(ctx context.Context, r io.Reader, dir string, owner *[2]int) | |||||
| 78 | } | 80 | } | ||
| 79 | h, err := tr.Next() | 81 | h, err := tr.Next() | ||
| 80 | if errors.Is(err, io.EOF) { | 82 | if errors.Is(err, io.EOF) { | ||
| 81 | - | return nil | 83 | + | return verifyLinks(ctx, root, dir) |
| 82 | } | 84 | } | ||
| 83 | if err != nil { | 85 | if err != nil { | ||
| 84 | var ea errArchive | 86 | var ea errArchive | ||
| @@ -208,7 +210,10 @@ func extractEntry(root *os.Root, tr *tar.Reader, h *tar.Header, owner *[2]int, t | |||||
| 208 | } | 210 | } | ||
| 209 | return chown() | 211 | return chown() | ||
| 210 | case tar.TypeSymlink: | 212 | case tar.TypeSymlink: | ||
| 211 | - | if !linkInside(name, h.Linkname, false) { | 213 | + | // Only the shape is checked here: where the link leads depends on the links |
| 214 | + | // that come before and after it, so verifyLinks resolves every symlink of | |||
| 215 | + | // the finished tree. | |||
| 216 | + | if h.Linkname == "" || strings.HasPrefix(h.Linkname, "/") || strings.ContainsRune(h.Linkname, 0) { | |||
| 212 | return errArchive(fmt.Sprintf("the archive has a link that points outside of it: %q -> %q", name, h.Linkname)) | 217 | return errArchive(fmt.Sprintf("the archive has a link that points outside of it: %q -> %q", name, h.Linkname)) | ||
| 213 | } | 218 | } | ||
| 214 | _ = root.Remove(name) | 219 | _ = root.Remove(name) | ||
| @@ -226,3 +231,89 @@ func extractEntry(root *os.Root, tr *tar.Reader, h *tar.Header, owner *[2]int, t | |||||
| 226 | } | 231 | } | ||
| 227 | return nil // devices, fifos: skipped | 232 | return nil // devices, fifos: skipped | ||
| 228 | } | 233 | } | ||
| 234 | + | ||||
| 235 | + | // maxLinkHops bounds the symbolic links followed to resolve one link (Linux | |||
| 236 | + | // stops at 40). | |||
| 237 | + | const maxLinkHops = 40 | |||
| 238 | + | ||||
| 239 | + | // verifyLinks resolves every symbolic link of the extracted tree component by | |||
| 240 | + | // component, following the links met on the way, and rejects the archive when | |||
| 241 | + | // one leads out of the tree or loops. A lexical check is not enough: with | |||
| 242 | + | // "a -> ." a link "link -> a/../outside" looks like it stays in the tree, and | |||
| 243 | + | // leaves it. A link to something that does not exist is allowed (git archives | |||
| 244 | + | // have such links); the rest of its target is still resolved. | |||
| 245 | + | func verifyLinks(ctx context.Context, root *os.Root, dir string) error { | |||
| 246 | + | return filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error { | |||
| 247 | + | if err != nil { | |||
| 248 | + | return err | |||
| 249 | + | } | |||
| 250 | + | if err := ctx.Err(); err != nil { | |||
| 251 | + | return err | |||
| 252 | + | } | |||
| 253 | + | if d.Type()&fs.ModeSymlink == 0 { | |||
| 254 | + | return nil | |||
| 255 | + | } | |||
| 256 | + | rel, err := filepath.Rel(dir, p) | |||
| 257 | + | if err != nil { | |||
| 258 | + | return err | |||
| 259 | + | } | |||
| 260 | + | target, err := root.Readlink(filepath.ToSlash(rel)) | |||
| 261 | + | if err != nil { | |||
| 262 | + | return err | |||
| 263 | + | } | |||
| 264 | + | if err := resolveInside(root, path.Dir(filepath.ToSlash(rel)), target); err != nil { | |||
| 265 | + | return errArchive(fmt.Sprintf("the archive has a link that points outside of it: %q -> %q (%v)", filepath.ToSlash(rel), target, err)) | |||
| 266 | + | } | |||
| 267 | + | return nil | |||
| 268 | + | }) | |||
| 269 | + | } | |||
| 270 | + | ||||
| 271 | + | // resolveInside resolves target, relative to the directory dirRel of the tree | |||
| 272 | + | // (slash separated, "." for the root), the way the kernel would, and fails when | |||
| 273 | + | // the path leaves the root, is absolute, or needs more than maxLinkHops links. | |||
| 274 | + | func resolveInside(root *os.Root, dirRel, target string) error { | |||
| 275 | + | var cur []string // resolved directory components, none of them a link | |||
| 276 | + | if dirRel != "." && dirRel != "" { | |||
| 277 | + | cur = strings.Split(dirRel, "/") | |||
| 278 | + | } | |||
| 279 | + | pending := strings.Split(target, "/") | |||
| 280 | + | hops := 0 | |||
| 281 | + | for len(pending) > 0 { | |||
| 282 | + | c := pending[0] | |||
| 283 | + | pending = pending[1:] | |||
| 284 | + | switch c { | |||
| 285 | + | case "", ".": | |||
| 286 | + | continue | |||
| 287 | + | case "..": | |||
| 288 | + | if len(cur) == 0 { | |||
| 289 | + | return errors.New("leaves the tree") | |||
| 290 | + | } | |||
| 291 | + | cur = cur[:len(cur)-1] | |||
| 292 | + | continue | |||
| 293 | + | } | |||
| 294 | + | p := strings.Join(append(append([]string(nil), cur...), c), "/") | |||
| 295 | + | fi, err := root.Lstat(p) | |||
| 296 | + | switch { | |||
| 297 | + | case errors.Is(err, os.ErrNotExist): | |||
| 298 | + | cur = append(cur, c) | |||
| 299 | + | continue | |||
| 300 | + | case err != nil: | |||
| 301 | + | return err | |||
| 302 | + | case fi.Mode()&fs.ModeSymlink == 0: | |||
| 303 | + | cur = append(cur, c) | |||
| 304 | + | continue | |||
| 305 | + | } | |||
| 306 | + | if hops++; hops > maxLinkHops { | |||
| 307 | + | return errors.New("too many levels of links") | |||
| 308 | + | } | |||
| 309 | + | next, err := root.Readlink(p) | |||
| 310 | + | if err != nil { | |||
| 311 | + | return err | |||
| 312 | + | } | |||
| 313 | + | if strings.HasPrefix(next, "/") || next == "" { | |||
| 314 | + | return errors.New("an absolute or empty link") | |||
| 315 | + | } | |||
| 316 | + | pending = append(strings.Split(next, "/"), pending...) | |||
| 317 | + | } | |||
| 318 | + | return nil | |||
| 319 | + | } | |||
M internal/executor/actions_extract_test.go +46 -0
| @@ -209,3 +209,49 @@ func TestExtractTarballCancelled(t *testing.T) { | |||
| 209 | 209 | t.Fatalf("%v", err) | |
| 210 | 210 | } | |
| 211 | 211 | } | |
| 212 | + | ||
| 213 | + | // GO-007: where a link leads depends on the other links. "a -> ." makes | |
| 214 | + | // "link -> a/../outside" leave the tree, which a lexical check accepts. | |
| 215 | + | func TestExtractTarballLinkChainsAreResolved(t *testing.T) { | |
| 216 | + | sym := func(name, to string) tent { return tent{name: "top/" + name, typ: tar.TypeSymlink, link: to} } | |
| 217 | + | for name, tc := range map[string]struct { | |
| 218 | + | ents []tent | |
| 219 | + | ok bool | |
| 220 | + | }{ | |
| 221 | + | "two links escape": {[]tent{sym("a", "."), sym("link", "a/../outside")}, false}, | |
| 222 | + | "two links, reverse order": {[]tent{sym("link", "a/../outside"), sym("a", ".")}, false}, // forward reference | |
| 223 | + | "three hops": {[]tent{sym("a", "b"), sym("b", "."), sym("link", "a/../../x")}, false}, | |
| 224 | + | "through a directory link": {[]tent{{name: "top/d/e/f", body: "x"}, sym("d/up", "../.."), sym("link", "d/up/..")}, false}, | |
| 225 | + | "absolute inside a hop": {[]tent{sym("a", "/etc"), sym("link", "a/passwd")}, false}, | |
| 226 | + | "loop": {[]tent{sym("a", "b"), sym("b", "a"), sym("link", "a/x")}, false}, | |
| 227 | + | "long chain": {chain(45), false}, | |
| 228 | + | "stays inside": {[]tent{{name: "top/d/f", body: "1"}, sym("a", "."), sym("link", "a/d/f"), sym("up", "d/..")}, true}, | |
| 229 | + | "dangling is fine": {[]tent{sym("link", "nowhere/at/all")}, true}, | |
| 230 | + | "forward ref inside": {[]tent{sym("link", "a/b"), sym("a", "d"), {name: "top/d/b", body: "1"}}, true}, | |
| 231 | + | "dotdot after real dir": {[]tent{{name: "top/d/f", body: "1"}, sym("link", "d/../d/f")}, true}, | |
| 232 | + | } { | |
| 233 | + | t.Run(name, func(t *testing.T) { | |
| 234 | + | parent := t.TempDir() | |
| 235 | + | dir := filepath.Join(parent, "stage") | |
| 236 | + | _ = os.Mkdir(dir, 0o755) | |
| 237 | + | err := extractTarball(context.Background(), tarball(t, tc.ents...), dir, nil) | |
| 238 | + | if (err == nil) != tc.ok { | |
| 239 | + | t.Fatalf("accepted=%v, want %v (%v)", err == nil, tc.ok, err) | |
| 240 | + | } | |
| 241 | + | }) | |
| 242 | + | } | |
| 243 | + | } | |
| 244 | + | ||
| 245 | + | // chain makes n links, each one to the next, the last to ".": more hops than | |
| 246 | + | // the kernel follows. | |
| 247 | + | func chain(n int) []tent { | |
| 248 | + | var out []tent | |
| 249 | + | for i := 0; i < n; i++ { | |
| 250 | + | to := "l" + string(rune('A'+(i+1)%26)) + string(rune('a'+(i+1)/26)) | |
| 251 | + | if i == n-1 { | |
| 252 | + | to = "." | |
| 253 | + | } | |
| 254 | + | out = append(out, tent{name: "top/l" + string(rune('A'+i%26)) + string(rune('a'+i/26)), typ: tar.TypeSymlink, link: to}) | |
| 255 | + | } | |
| 256 | + | return append(out, tent{name: "top/start", typ: tar.TypeSymlink, link: "lAa/x"}) | |
| 257 | + | } | |
| @@ -209,3 +209,49 @@ func TestExtractTarballCancelled(t *testing.T) { | |||||
| 209 | t.Fatalf("%v", err) | 209 | t.Fatalf("%v", err) | ||
| 210 | } | 210 | } | ||
| 211 | } | 211 | } | ||
| 212 | + | ||||
| 213 | + | // GO-007: where a link leads depends on the other links. "a -> ." makes | |||
| 214 | + | // "link -> a/../outside" leave the tree, which a lexical check accepts. | |||
| 215 | + | func TestExtractTarballLinkChainsAreResolved(t *testing.T) { | |||
| 216 | + | sym := func(name, to string) tent { return tent{name: "top/" + name, typ: tar.TypeSymlink, link: to} } | |||
| 217 | + | for name, tc := range map[string]struct { | |||
| 218 | + | ents []tent | |||
| 219 | + | ok bool | |||
| 220 | + | }{ | |||
| 221 | + | "two links escape": {[]tent{sym("a", "."), sym("link", "a/../outside")}, false}, | |||
| 222 | + | "two links, reverse order": {[]tent{sym("link", "a/../outside"), sym("a", ".")}, false}, // forward reference | |||
| 223 | + | "three hops": {[]tent{sym("a", "b"), sym("b", "."), sym("link", "a/../../x")}, false}, | |||
| 224 | + | "through a directory link": {[]tent{{name: "top/d/e/f", body: "x"}, sym("d/up", "../.."), sym("link", "d/up/..")}, false}, | |||
| 225 | + | "absolute inside a hop": {[]tent{sym("a", "/etc"), sym("link", "a/passwd")}, false}, | |||
| 226 | + | "loop": {[]tent{sym("a", "b"), sym("b", "a"), sym("link", "a/x")}, false}, | |||
| 227 | + | "long chain": {chain(45), false}, | |||
| 228 | + | "stays inside": {[]tent{{name: "top/d/f", body: "1"}, sym("a", "."), sym("link", "a/d/f"), sym("up", "d/..")}, true}, | |||
| 229 | + | "dangling is fine": {[]tent{sym("link", "nowhere/at/all")}, true}, | |||
| 230 | + | "forward ref inside": {[]tent{sym("link", "a/b"), sym("a", "d"), {name: "top/d/b", body: "1"}}, true}, | |||
| 231 | + | "dotdot after real dir": {[]tent{{name: "top/d/f", body: "1"}, sym("link", "d/../d/f")}, true}, | |||
| 232 | + | } { | |||
| 233 | + | t.Run(name, func(t *testing.T) { | |||
| 234 | + | parent := t.TempDir() | |||
| 235 | + | dir := filepath.Join(parent, "stage") | |||
| 236 | + | _ = os.Mkdir(dir, 0o755) | |||
| 237 | + | err := extractTarball(context.Background(), tarball(t, tc.ents...), dir, nil) | |||
| 238 | + | if (err == nil) != tc.ok { | |||
| 239 | + | t.Fatalf("accepted=%v, want %v (%v)", err == nil, tc.ok, err) | |||
| 240 | + | } | |||
| 241 | + | }) | |||
| 242 | + | } | |||
| 243 | + | } | |||
| 244 | + | ||||
| 245 | + | // chain makes n links, each one to the next, the last to ".": more hops than | |||
| 246 | + | // the kernel follows. | |||
| 247 | + | func chain(n int) []tent { | |||
| 248 | + | var out []tent | |||
| 249 | + | for i := 0; i < n; i++ { | |||
| 250 | + | to := "l" + string(rune('A'+(i+1)%26)) + string(rune('a'+(i+1)/26)) | |||
| 251 | + | if i == n-1 { | |||
| 252 | + | to = "." | |||
| 253 | + | } | |||
| 254 | + | out = append(out, tent{name: "top/l" + string(rune('A'+i%26)) + string(rune('a'+i/26)), typ: tar.TypeSymlink, link: to}) | |||
| 255 | + | } | |||
| 256 | + | return append(out, tent{name: "top/start", typ: tar.TypeSymlink, link: "lAa/x"}) | |||
| 257 | + | } | |||