diff --git a/internal/imagepull/flatten.go b/internal/imagepull/flatten.go index 7404ca9..ee7ff74 100644 --- a/internal/imagepull/flatten.go +++ b/internal/imagepull/flatten.go @@ -132,16 +132,18 @@ func applyEntry(tr *tar.Reader, hdr *tar.Header, dest string) error { if err := os.MkdirAll(filepath.Dir(abs), 0o755); err != nil { return err } - // Resolve the link target relative to the link's parent and - // require that it stays inside dest. Absolute targets that - // resolve outside dest are also rejected. - resolved := hdr.Linkname - if !filepath.IsAbs(resolved) { - resolved = filepath.Join(filepath.Dir(abs), resolved) - } - resolved = filepath.Clean(resolved) - if resolved != dest && !strings.HasPrefix(resolved, dest+string(filepath.Separator)) { - return fmt.Errorf("unsafe symlink in layer: %q -> %q", hdr.Name, hdr.Linkname) + // Container layers commonly use absolute symlink targets like + // "/usr/bin/mawk" — these are interpreted relative to the + // rootfs (`/` inside the eventual VM), so they're rooted at + // dest by construction and need no escape check. + // Relative targets, however, can escape with "../"s and must + // be checked against dest at write time (we never follow them + // during extraction, but a future caller might). + if !filepath.IsAbs(hdr.Linkname) { + resolved := filepath.Clean(filepath.Join(filepath.Dir(abs), hdr.Linkname)) + if resolved != dest && !strings.HasPrefix(resolved, dest+string(filepath.Separator)) { + return fmt.Errorf("unsafe symlink in layer: %q -> %q", hdr.Name, hdr.Linkname) + } } if err := os.RemoveAll(abs); err != nil && !errors.Is(err, os.ErrNotExist) { return err diff --git a/internal/imagepull/imagepull_test.go b/internal/imagepull/imagepull_test.go index 2532fdc..6525dc1 100644 --- a/internal/imagepull/imagepull_test.go +++ b/internal/imagepull/imagepull_test.go @@ -237,11 +237,43 @@ func TestFlattenRejectsPathTraversal(t *testing.T) { } } -func TestFlattenRejectsUnsafeSymlink(t *testing.T) { +func TestFlattenAcceptsAbsoluteSymlink(t *testing.T) { + // Container layers regularly contain absolute symlinks like + // /usr/bin/mawk — they're interpreted relative to the rootfs at + // boot time, not against the host filesystem. They must extract + // cleanly. host := startRegistry(t) - ref := pushImage(t, host, "banger/test", "evil-sym", + ref := pushImage(t, host, "banger/test", "abs-sym", makeLayer(t, []tarMember{ - {name: "evil", symlink: true, link: "/etc/passwd"}, // absolute target outside dest + {name: "etc/alternatives/awk", symlink: true, link: "/usr/bin/mawk"}, + }), + ) + pulled, err := Pull(context.Background(), ref, t.TempDir()) + if err != nil { + t.Fatalf("Pull: %v", err) + } + dest := t.TempDir() + if err := Flatten(context.Background(), pulled, dest); err != nil { + t.Fatalf("Flatten: %v", err) + } + link := filepath.Join(dest, "etc/alternatives/awk") + target, err := os.Readlink(link) + if err != nil { + t.Fatalf("readlink: %v", err) + } + if target != "/usr/bin/mawk" { + t.Errorf("link target = %q, want /usr/bin/mawk", target) + } +} + +func TestFlattenRejectsRelativeSymlinkEscape(t *testing.T) { + // Relative symlinks with .. must still be rejected: the resolved + // path can escape dest at the host level even if the in-VM + // resolution would be safe. + host := startRegistry(t) + ref := pushImage(t, host, "banger/test", "rel-escape", + makeLayer(t, []tarMember{ + {name: "etc/evil", symlink: true, link: "../../../../etc/passwd"}, }), ) pulled, err := Pull(context.Background(), ref, t.TempDir()) @@ -250,7 +282,7 @@ func TestFlattenRejectsUnsafeSymlink(t *testing.T) { } err = Flatten(context.Background(), pulled, t.TempDir()) if err == nil || !strings.Contains(err.Error(), "unsafe symlink") { - t.Fatalf("Flatten unsafe symlink: err=%v", err) + t.Fatalf("Flatten relative escape: err=%v", err) } } diff --git a/internal/kernelcat/fetch.go b/internal/kernelcat/fetch.go index 91eec81..415d050 100644 --- a/internal/kernelcat/fetch.go +++ b/internal/kernelcat/fetch.go @@ -167,14 +167,15 @@ func extractTar(r io.Reader, target string) error { if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { return err } - link := hdr.Linkname - resolved := link - if !filepath.IsAbs(link) { - resolved = filepath.Join(filepath.Dir(dst), link) - } - resolved = filepath.Clean(resolved) - if resolved != absTarget && !strings.HasPrefix(resolved, absTarget+string(filepath.Separator)) { - return fmt.Errorf("unsafe symlink in tarball: %q -> %q", hdr.Name, hdr.Linkname) + // Absolute targets are interpreted at runtime against the + // eventual rootfs (`/` inside the VM), so they're rooted + // inside absTarget by construction. Only relative targets + // need an escape check at write time. + if !filepath.IsAbs(hdr.Linkname) { + resolved := filepath.Clean(filepath.Join(filepath.Dir(dst), hdr.Linkname)) + if resolved != absTarget && !strings.HasPrefix(resolved, absTarget+string(filepath.Separator)) { + return fmt.Errorf("unsafe symlink in tarball: %q -> %q", hdr.Name, hdr.Linkname) + } } if err := os.Symlink(hdr.Linkname, dst); err != nil { return err