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")},