From 0a079277ef1184703658171f234b9468c1ad8183 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Tue, 28 Apr 2026 15:20:46 -0300 Subject: [PATCH] imagepull: reject symlink ancestors during OCI flatten MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit safeJoin previously did textual cleaning + dest-prefix check only. That's enough to catch `../escape`, but not the symlink-ancestor attack: a malicious OCI layer plants `etc -> /tmp/probe`, a later layer writes/deletes/hardlinks against `etc/anything`, and the kernel silently dereferences the symlink so the operation lands at `/tmp/probe/anything` on the host. The daemon runs flatten as the owner UID, so anywhere that UID can write becomes a write target; anywhere it can delete (e.g. its own home) becomes a delete target. Whiteouts and hardlinks make this worse — a whiteout for `etc/.wh.victim` would `RemoveAll` the host file `/tmp/probe/victim`, and a TypeLink would expose host files inside the extracted rootfs. safeJoin now Lstat-walks every intermediate component of the joined path against the already-extracted tree, refusing if any ancestor is a symlink. Walking is race-free against the extraction loop because we process tar entries serially. Leaf components stay caller-owned (TypeSymlink writes legitimately want a symlink leaf; TypeReg RemoveAll's any prior leaf before opening; etc.). Three new tests pin the protection: write through a symlinked ancestor, whiteout through a symlinked ancestor, and hardlink target through a symlinked ancestor — each must fail and leave the host probe path untouched. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/imagepull/flatten.go | 47 +++++++++++++- internal/imagepull/imagepull_test.go | 97 ++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 1 deletion(-) diff --git a/internal/imagepull/flatten.go b/internal/imagepull/flatten.go index 3aad45c..002366a 100644 --- a/internal/imagepull/flatten.go +++ b/internal/imagepull/flatten.go @@ -267,12 +267,57 @@ func applyEntry(tr *tar.Reader, hdr *tar.Header, dest string, meta *Metadata) er } } -// safeJoin returns dest+rel after verifying the result lies under dest. +// safeJoin returns dest+rel after verifying: +// +// 1. The cleaned result lies textually under dest (catches "../escape"). +// 2. No INTERMEDIATE component of the result is a symlink (catches the +// OCI extraction-escape attack: a layer plants `etc -> /etc`, then a +// later layer writes `etc/passwd` — without this walk the kernel +// would dereference the symlink and the operation would land at +// /etc/passwd on the host, not at /etc/passwd). +// +// The leaf component is intentionally NOT Lstat'd here: it may legitimately +// be a symlink (TypeSymlink entries), a missing file (TypeReg about to be +// created), or an existing entry that the caller will RemoveAll before +// re-creating. Leaf type is the caller's contract. +// +// Walking against the already-extracted tree is race-free in practice: +// the only mutator is this same extraction loop, and we're processing +// entries serially. func safeJoin(dest, rel string) (string, error) { joined := filepath.Join(dest, rel) if joined != dest && !strings.HasPrefix(joined, dest+string(filepath.Separator)) { return "", fmt.Errorf("unsafe path: %q escapes %q", rel, dest) } + if joined == dest { + return joined, nil + } + suffix := strings.TrimPrefix(joined, dest+string(filepath.Separator)) + segs := strings.Split(suffix, string(filepath.Separator)) + cur := dest + for i, seg := range segs { + if seg == "" { + continue + } + cur = filepath.Join(cur, seg) + if i == len(segs)-1 { + break + } + info, err := os.Lstat(cur) + if err != nil { + if os.IsNotExist(err) { + // Ancestor not yet materialised. Once an extraction + // op creates it (via this same routed code), it can't + // be a symlink — TypeSymlink writes go through this + // validator too. + return joined, nil + } + return "", err + } + if info.Mode()&os.ModeSymlink != 0 { + return "", fmt.Errorf("unsafe path: ancestor %q of %q is a symlink", cur, rel) + } + } return joined, nil } diff --git a/internal/imagepull/imagepull_test.go b/internal/imagepull/imagepull_test.go index d7dfd9f..5ac33fc 100644 --- a/internal/imagepull/imagepull_test.go +++ b/internal/imagepull/imagepull_test.go @@ -327,6 +327,103 @@ func TestFlattenRejectsRelativeSymlinkEscape(t *testing.T) { } } +// TestFlattenRejectsWriteThroughSymlinkAncestor exercises the OCI +// extraction-escape attack: layer 1 plants `etc -> /tmp` (a directory +// the daemon can write to), layer 2 writes `etc/probe`. Without the +// ancestor walk in safeJoin the write would land at /tmp/probe on the +// host. With it, the second layer's write is refused. +func TestFlattenRejectsWriteThroughSymlinkAncestor(t *testing.T) { + host := startRegistry(t) + probeDir := t.TempDir() // a path the daemon user can write to + ref := pushImage(t, host, "banger/test", "sym-ancestor", + makeLayer(t, []tarMember{ + {name: "etc", symlink: true, link: probeDir}, + }), + makeLayer(t, []tarMember{ + {name: "etc/probe", body: []byte("escaped")}, + }), + ) + pulled, err := Pull(context.Background(), ref, t.TempDir()) + if err != nil { + t.Fatalf("Pull: %v", err) + } + dest := t.TempDir() + _, err = Flatten(context.Background(), pulled, dest) + if err == nil || !strings.Contains(err.Error(), "symlink") { + t.Fatalf("Flatten: err=%v, want symlink-ancestor rejection", err) + } + // The escape file must NOT have been written outside dest. + if _, statErr := os.Stat(filepath.Join(probeDir, "probe")); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("escape file at %s should not exist; got %v", filepath.Join(probeDir, "probe"), statErr) + } +} + +// TestFlattenRejectsWhiteoutThroughSymlinkAncestor pins the same +// guarantee for the whiteout path: a symlinked ancestor must not let +// the extractor RemoveAll on a host file outside dest. +func TestFlattenRejectsWhiteoutThroughSymlinkAncestor(t *testing.T) { + host := startRegistry(t) + probeDir := t.TempDir() + probeFile := filepath.Join(probeDir, "victim") + if err := os.WriteFile(probeFile, []byte("preserved"), 0o644); err != nil { + t.Fatalf("write probe: %v", err) + } + ref := pushImage(t, host, "banger/test", "wh-sym-ancestor", + makeLayer(t, []tarMember{ + {name: "etc", symlink: true, link: probeDir}, + }), + makeLayer(t, []tarMember{ + {name: "etc/.wh.victim"}, + }), + ) + pulled, err := Pull(context.Background(), ref, t.TempDir()) + if err != nil { + t.Fatalf("Pull: %v", err) + } + dest := t.TempDir() + _, err = Flatten(context.Background(), pulled, dest) + if err == nil || !strings.Contains(err.Error(), "symlink") { + t.Fatalf("Flatten: err=%v, want symlink-ancestor rejection on whiteout", err) + } + if _, statErr := os.Stat(probeFile); statErr != nil { + t.Fatalf("probe file %s removed via whiteout escape: %v", probeFile, statErr) + } +} + +// TestFlattenRejectsHardlinkTargetThroughSymlinkAncestor covers the +// hardlink-target validator: a symlinked ancestor on the link source +// must not let `os.Link` resolve through it and hard-link a host file +// (e.g. /etc/passwd) into the extraction tree. +func TestFlattenRejectsHardlinkTargetThroughSymlinkAncestor(t *testing.T) { + host := startRegistry(t) + probeDir := t.TempDir() + probeFile := filepath.Join(probeDir, "secret") + if err := os.WriteFile(probeFile, []byte("hands off"), 0o644); err != nil { + t.Fatalf("write probe: %v", err) + } + ref := pushImage(t, host, "banger/test", "ln-sym-ancestor", + makeLayer(t, []tarMember{ + {name: "etc", symlink: true, link: probeDir}, + }), + makeLayer(t, []tarMember{ + {name: "leaked", hardlink: true, link: "etc/secret"}, + }), + ) + pulled, err := Pull(context.Background(), ref, t.TempDir()) + if err != nil { + t.Fatalf("Pull: %v", err) + } + dest := t.TempDir() + _, err = Flatten(context.Background(), pulled, dest) + if err == nil || !strings.Contains(err.Error(), "symlink") { + t.Fatalf("Flatten: err=%v, want symlink-ancestor rejection on hardlink target", err) + } + // dest must not contain a hardlink to the host secret. + if _, statErr := os.Lstat(filepath.Join(dest, "leaked")); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("hardlink leaked file should not exist in dest; got %v", statErr) + } +} + func TestFlattenTarRejectsDebugFSHostilePath(t *testing.T) { tarData := buildTar(t, []tarMember{ {name: "etc/bad\tname", body: []byte("bad")},