From ef5c9f86e725a4c8b08a03181dcc234d0b8b4468 Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Wed, 8 Jul 2026 15:49:45 +0200 Subject: [PATCH 01/13] buildctl: revert trace shutdown timeout This partially reverts the buildctl tracer shutdown change from #6757. The 50ms shutdown context can expire while delegated traces are flushed over a slow but healthy connhelper transport. Restore the previous buildctl shutdown behavior so that this telemetry cleanup path does not fail an otherwise successful command. Keep the daemon-side telemetry shutdown bounds and trace forwarder changes from #6757 intact. Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> (cherry picked from commit b2d2edadd8e01d5037b476b32800ca5c50622dcf) --- cmd/buildctl/common/trace.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/cmd/buildctl/common/trace.go b/cmd/buildctl/common/trace.go index 96691d850..365cbc55d 100644 --- a/cmd/buildctl/common/trace.go +++ b/cmd/buildctl/common/trace.go @@ -3,12 +3,10 @@ package common import ( "context" "os" - "time" "github.com/moby/buildkit/util/appcontext" "github.com/moby/buildkit/util/tracing/delegated" "github.com/moby/buildkit/util/tracing/detect" - "github.com/pkg/errors" "github.com/urfave/cli/v3" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" @@ -16,8 +14,6 @@ import ( "go.opentelemetry.io/otel/trace" ) -const exportTimeout = 50 * time.Millisecond - func AttachAppContext(app *cli.Command) error { baseCtx := appcontext.Context() @@ -79,13 +75,7 @@ func AttachAppContext(app *cli.Command) error { span.End() } - // Set a rather aggressive timeout for shutting down the tracer provider - // to ensure we don't stall on a non-responsive tracing endpoint for too long - // on shutdown. - ctx, cancel := context.WithTimeoutCause(appcontext.Shutdown(), exportTimeout, errors.WithStack(context.DeadlineExceeded)) - defer cancel() - - return tp.Shutdown(ctx) + return tp.Shutdown(context.TODO()) } return nil } From 8d369817a5aea348d477b5e32f040c479a357018 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 30 Jun 2026 17:45:47 -0700 Subject: [PATCH 02/13] solver: fix job value iteration race Snapshot the state's job set under state.mu before walking job values. This matches the lock used by job attach and discard paths and avoids concurrent map iteration when op resolution loads shared job values. Signed-off-by: Tonis Tiigi (cherry picked from commit 9866e2ceb3948940efa678b1dd57f1160eb580d6) --- solver/jobs.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/solver/jobs.go b/solver/jobs.go index 711dc441a..297222b05 100644 --- a/solver/jobs.go +++ b/solver/jobs.go @@ -354,9 +354,14 @@ func (sb *subBuilder) InContext(ctx context.Context, f func(context.Context, Job } func (sb *subBuilder) EachValue(ctx context.Context, key string, fn func(any) error) error { - sb.mu.Lock() - defer sb.mu.Unlock() + sb.state.mu.Lock() + jobs := make([]*Job, 0, len(sb.jobs)) for j := range sb.jobs { + jobs = append(jobs, j) + } + sb.state.mu.Unlock() + + for _, j := range jobs { if err := j.EachValue(ctx, key, fn); err != nil { return err } From 2252f8b96a4eebf0a140359ea6e4d3d62a602abf Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Fri, 3 Jul 2026 10:27:43 +0200 Subject: [PATCH 03/13] ci: remove vagrant dependency workaround Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> (cherry picked from commit 719caf38b76974005eddc6eb0050cc1246ca3706) --- .github/workflows/test-os.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/test-os.yml b/.github/workflows/test-os.yml index e2cdf299f..3ab961a76 100644 --- a/.github/workflows/test-os.yml +++ b/.github/workflows/test-os.yml @@ -198,8 +198,6 @@ jobs: - build env: GOOS: freebsd - # https://github.com/hashicorp/vagrant/issues/13652 - VAGRANT_DISABLE_STRICT_DEPENDENCY_ENFORCEMENT: 1 steps: - name: Checkout From eb63c321e859ed2d8ee966d50ec5c653d841048b Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 14 Jul 2026 11:11:06 -0700 Subject: [PATCH 04/13] source/git: reject option-like refs in git bundle operations Validate the default branch returned by ls-remote before using it, so a malicious remote cannot advertise a HEAD symref such as that would later be reused as an argument to other git commands. Signed-off-by: Tonis Tiigi (cherry picked from commit a838953fe8776d68379d32a7af0776d538215763) --- source/git/bundle.go | 11 ++++-- source/git/source.go | 6 +++- source/git/source_test.go | 73 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/source/git/bundle.go b/source/git/bundle.go index 882980a8a..374008d21 100644 --- a/source/git/bundle.go +++ b/source/git/bundle.go @@ -398,8 +398,8 @@ func (gs *gitSourceHandler) checkoutAsBundle(ctx context.Context, repo *gitRepo, // natural-name ref to pull; update-ref alone places the tip against // the pinned commit under the target name. sharedURL := "file://" + repo.dir - if _, err := tmpGit.Run(ctx, "fetch", sharedURL, commit); err != nil { - return nil, errors.Wrapf(err, "failed to fetch commit %s from shared repo for bundle creation", commit) + if err := fetchCommitForBundle(ctx, tmpGit, sharedURL, commit); err != nil { + return nil, err } if _, err := tmpGit.Run(ctx, "update-ref", targetRef, commit); err != nil { @@ -441,6 +441,13 @@ func (gs *gitSourceHandler) checkoutAsBundle(ctx context.Context, repo *gitRepo, return snap, nil } +func fetchCommitForBundle(ctx context.Context, git *gitutil.GitCLI, sharedURL, commit string) error { + if _, err := git.Run(ctx, "fetch", sharedURL, "--", commit); err != nil { + return errors.Wrapf(err, "failed to fetch commit %s from shared repo for bundle creation", commit) + } + return nil +} + // writeBundleToMount copies the bytes at stagePath into // / via os.OpenRoot, so a symlink at the destination // cannot redirect the write outside checkoutDir. The staging file is expected diff --git a/source/git/source.go b/source/git/source.go index 81d15fc57..4a658535d 100644 --- a/source/git/source.go +++ b/source/git/source.go @@ -1557,7 +1557,11 @@ func getDefaultBranch(ctx context.Context, git *gitutil.GitCLI, remoteURL string if len(ss) == 0 || len(ss[0]) != 2 { return "", errors.Errorf("could not find default branch for repository: %s", urlutil.RedactCredentials(remoteURL)) } - return ss[0][1], nil + ref := ss[0][1] + if err := validateGitRef(ref); err != nil { + return "", err + } + return ref, nil } const ( diff --git a/source/git/source_test.go b/source/git/source_test.go index a7b2ed53a..86aa2581a 100644 --- a/source/git/source_test.go +++ b/source/git/source_test.go @@ -2815,6 +2815,79 @@ func TestBundleStagedRefShape(t *testing.T) { require.Equal(t, headSha, md.Checksum) } +func TestGetDefaultBranchRejectsDashPrefixedRef(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Depends on git shell helpers") + } + + t.Parallel() + ctx := t.Context() + + work := t.TempDir() + runShell(t, work, + "git init -q seed", + "git -C seed -c user.email=test@example.com -c user.name=test commit -q --allow-empty -m seed", + "git clone -q --bare seed mal.git", + ) + + shaCmd := exec.CommandContext(ctx, "git", "-C", filepath.Join(work, "seed"), "rev-parse", "HEAD") //nolint:gosec // test uses t.TempDir-owned path + out, err := shaCmd.Output() + require.NoError(t, err) + sha := strings.TrimSpace(string(out)) + + malDir := filepath.Join(work, "mal.git") + require.NoError(t, os.RemoveAll(filepath.Join(malDir, "refs", "heads"))) + require.NoError(t, os.WriteFile(filepath.Join(malDir, "packed-refs"), + fmt.Appendf(nil, "# pack-refs with: peeled fully-peeled sorted\n%s refs/heads/--upload-pack=/tmp/evil.sh\n", sha), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(malDir, "HEAD"), + []byte("ref: refs/heads/--upload-pack=/tmp/evil.sh\n"), 0600)) + + ref, err := getDefaultBranch(ctx, gitutil.NewGitCLI(), "file://"+malDir) + require.ErrorContains(t, err, `invalid git ref "--upload-pack=/tmp/evil.sh"`) + require.Empty(t, ref) +} + +func TestFetchCommitForBundleUsesOptionTerminator(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Depends on shell script git stub") + } + + t.Parallel() + ctx := t.Context() + + dir := t.TempDir() + capture := filepath.Join(dir, "args") + fakeGit := filepath.Join(dir, "git") + require.NoError(t, os.WriteFile(fakeGit, fmt.Appendf(nil, `#!/bin/sh +for arg do + printf '%%s\n' "$arg" +done > %s +`, strconv.Quote(capture)), 0600)) + require.NoError(t, os.Chmod(fakeGit, 0700)) + + sharedURL := "file:///tmp/shared.git" + commit := "--upload-pack=/tmp/evil.sh" + err := fetchCommitForBundle(ctx, gitutil.NewGitCLI(gitutil.WithGitBinary(fakeGit)), sharedURL, commit) + require.NoError(t, err) + + buf, err := os.ReadFile(capture) + require.NoError(t, err) + args := strings.Split(strings.TrimSpace(string(buf)), "\n") + + fetchIdx := -1 + for i, arg := range args { + if arg == "fetch" { + fetchIdx = i + break + } + } + require.NotEqual(t, -1, fetchIdx, "fake git args: %v", args) + require.GreaterOrEqual(t, len(args), fetchIdx+4, "fake git args: %v", args) + require.Equal(t, sharedURL, args[fetchIdx+1]) + require.Equal(t, "--", args[fetchIdx+2]) + require.Equal(t, commit, args[fetchIdx+3]) +} + func TestDetectBundleSHA256(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Depends on git bundle support exercised via shell helpers") From ed777881d1ecc93324c6ee87d0172e9b4d336e95 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 8 Jul 2026 20:53:03 -0700 Subject: [PATCH 05/13] solver: validate op input indices to avoid panics Malicious or malformed LLB definitions could reference negative or out-of-range op input indices, or omit required diff lower/upper inputs, causing the daemon to panic on a slice index or nil pointer dereference during load and cache-key computation. Signed-off-by: Tonis Tiigi (cherry picked from commit 2177ce16335f943e82458958902f9f21f6993d8d) --- solver/llbsolver/ops/build.go | 2 +- solver/llbsolver/ops/diff.go | 6 ++++ solver/llbsolver/ops/exec.go | 4 +-- solver/llbsolver/ops/file.go | 3 ++ solver/llbsolver/ops/opsutils/validate.go | 3 ++ solver/llbsolver/vertex.go | 41 +++++++++++++---------- 6 files changed, 38 insertions(+), 21 deletions(-) diff --git a/solver/llbsolver/ops/build.go b/solver/llbsolver/ops/build.go index c4631fa91..4545ef9f8 100644 --- a/solver/llbsolver/ops/build.go +++ b/solver/llbsolver/ops/build.go @@ -77,7 +77,7 @@ func (b *BuildOp) Exec(ctx context.Context, job solver.JobContext, inputs []solv } i := int(llbDef.Input) - if i >= len(inputs) { + if i < 0 || i >= len(inputs) { return nil, errors.Errorf("invalid index %v", i) // TODO: this should be validated before } inp := inputs[i] diff --git a/solver/llbsolver/ops/diff.go b/solver/llbsolver/ops/diff.go index 62c50a360..d4e0fe2e3 100644 --- a/solver/llbsolver/ops/diff.go +++ b/solver/llbsolver/ops/diff.go @@ -70,6 +70,9 @@ func (d *diffOp) Exec(ctx context.Context, jobCtx solver.JobContext, inputs []so var lowerRef cache.ImmutableRef if d.op.Lower.Input != int64(pb.Empty) { + if curInput >= len(inputs) { + return nil, errors.Errorf("invalid lower input index %d for diff op with %d inputs", curInput, len(inputs)) + } if lowerInp := inputs[curInput]; lowerInp != nil { wref, ok := lowerInp.Sys().(*worker.WorkerRef) if !ok { @@ -84,6 +87,9 @@ func (d *diffOp) Exec(ctx context.Context, jobCtx solver.JobContext, inputs []so var upperRef cache.ImmutableRef if d.op.Upper.Input != int64(pb.Empty) { + if curInput >= len(inputs) { + return nil, errors.Errorf("invalid upper input index %d for diff op with %d inputs", curInput, len(inputs)) + } if upperInp := inputs[curInput]; upperInp != nil { wref, ok := upperInp.Sys().(*worker.WorkerRef) if !ok { diff --git a/solver/llbsolver/ops/exec.go b/solver/llbsolver/ops/exec.go index 30fcafcf4..0ebed560c 100644 --- a/solver/llbsolver/ops/exec.go +++ b/solver/llbsolver/ops/exec.go @@ -302,7 +302,7 @@ func (e *ExecOp) getMountDeps() ([]dep, error) { if m.Input == int64(pb.Empty) { continue } - if int(m.Input) >= len(deps) { + if m.Input < 0 || int(m.Input) >= len(deps) { return nil, errors.Errorf("invalid mountinput %v", m) } @@ -394,7 +394,7 @@ func (e *ExecOp) Exec(ctx context.Context, jobCtx solver.JobContext, inputs []so if err != nil { execInputs := make([]solver.Result, len(e.op.Mounts)) for i, m := range e.op.Mounts { - if m.Input == -1 { + if m.Input < 0 || int(m.Input) >= len(inputs) { continue } execInputs[i] = inputs[m.Input].Clone() diff --git a/solver/llbsolver/ops/file.go b/solver/llbsolver/ops/file.go index 379e4d03d..50575805c 100644 --- a/solver/llbsolver/ops/file.go +++ b/solver/llbsolver/ops/file.go @@ -149,6 +149,9 @@ func (f *fileOp) CacheMap(ctx context.Context, jobCtx solver.JobContext, index i } for idx, m := range selectors { + if idx < 0 || idx >= len(cm.Deps) { + return nil, false, errors.Errorf("invalid input index %d in file op with %d inputs", idx, len(cm.Deps)) + } if _, ok := invalidSelectors[idx]; ok { continue } diff --git a/solver/llbsolver/ops/opsutils/validate.go b/solver/llbsolver/ops/opsutils/validate.go index b58cf0f69..126d53641 100644 --- a/solver/llbsolver/ops/opsutils/validate.go +++ b/solver/llbsolver/ops/opsutils/validate.go @@ -59,6 +59,9 @@ func Validate(op *pb.Op) error { if op.Diff == nil { return errors.Errorf("invalid nil diff op") } + if op.Diff.Lower == nil || op.Diff.Upper == nil { + return errors.Errorf("invalid diff op with nil lower or upper input") + } case *pb.Op_Passthrough: if op.Passthrough == nil { return errors.Errorf("invalid nil passthrough op") diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go index 04f784229..8bcd76739 100644 --- a/solver/llbsolver/vertex.go +++ b/solver/llbsolver/vertex.go @@ -482,25 +482,16 @@ func llbOpName(pbOp *pb.Op, load func(string) (solver.Vertex, error)) (string, e } return "merge " + fmt.Sprintf("(%s)", strings.Join(subnames, ", ")), nil case *pb.Op_Diff: - var lowerName string - if op.Diff.Lower.Input == -1 { - lowerName = "scratch" - } else { - lowerVtx, err := load(pbOp.Inputs[op.Diff.Lower.Input].Digest) - if err != nil { - return "", err - } - lowerName = fmt.Sprintf("(%s)", lowerVtx.Name()) + if op.Diff.Lower == nil || op.Diff.Upper == nil { + return "", errors.Errorf("invalid diff op with nil lower or upper input") } - var upperName string - if op.Diff.Upper.Input == -1 { - upperName = "scratch" - } else { - upperVtx, err := load(pbOp.Inputs[op.Diff.Upper.Input].Digest) - if err != nil { - return "", err - } - upperName = fmt.Sprintf("(%s)", upperVtx.Name()) + lowerName, err := diffSideName(pbOp, op.Diff.Lower.Input, load) + if err != nil { + return "", err + } + upperName, err := diffSideName(pbOp, op.Diff.Upper.Input, load) + if err != nil { + return "", err } return "diff " + lowerName + " -> " + upperName, nil case *pb.Op_Passthrough: @@ -510,6 +501,20 @@ func llbOpName(pbOp *pb.Op, load func(string) (solver.Vertex, error)) (string, e } } +func diffSideName(pbOp *pb.Op, input int64, load func(string) (solver.Vertex, error)) (string, error) { + if input == int64(pb.Empty) { + return "scratch", nil + } + if input < 0 || int(input) >= len(pbOp.Inputs) { + return "", errors.Errorf("invalid diff input index %d", input) + } + vtx, err := load(pbOp.Inputs[input].Digest) + if err != nil { + return "", err + } + return fmt.Sprintf("(%s)", vtx.Name()), nil +} + func fileOpName(actions []*pb.FileAction) string { names := make([]string, 0, len(actions)) for _, action := range actions { From 1659c301df5de609df00add51bef45f8f41a3aca Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 8 Jul 2026 21:01:27 -0700 Subject: [PATCH 06/13] solver: add tests for op input index validation Signed-off-by: Tonis Tiigi (cherry picked from commit c862447344f2dac3a45164239011231ec426c56a) --- solver/llbsolver/diff_validation_test.go | 55 +++++++++++ .../llbsolver/ops/opindex_validation_test.go | 99 +++++++++++++++++++ .../llbsolver/ops/opsutils/validate_test.go | 14 +++ 3 files changed, 168 insertions(+) create mode 100644 solver/llbsolver/diff_validation_test.go create mode 100644 solver/llbsolver/ops/opindex_validation_test.go create mode 100644 solver/llbsolver/ops/opsutils/validate_test.go diff --git a/solver/llbsolver/diff_validation_test.go b/solver/llbsolver/diff_validation_test.go new file mode 100644 index 000000000..9bb0590ab --- /dev/null +++ b/solver/llbsolver/diff_validation_test.go @@ -0,0 +1,55 @@ +package llbsolver + +import ( + "testing" + + "github.com/moby/buildkit/solver" + "github.com/moby/buildkit/solver/pb" + digest "github.com/opencontainers/go-digest" + "github.com/stretchr/testify/require" +) + +func noopLoad(string) (solver.Vertex, error) { return nil, nil } + +// llbOpName previously panicked on malicious diff ops (nil lower/upper input, +// out-of-range input index). It must now return an error instead. + +func TestDiffNilLowerName(t *testing.T) { + pbOp := &pb.Op{ + Op: &pb.Op_Diff{Diff: &pb.DiffOp{Lower: nil, Upper: &pb.UpperDiffInput{Input: -1}}}, + Inputs: []*pb.Input{}, + } + require.NotPanics(t, func() { + _, err := llbOpName(pbOp, noopLoad) + require.Error(t, err) + }) +} + +func TestDiffOOBInputName(t *testing.T) { + pbOp := &pb.Op{ + Op: &pb.Op_Diff{Diff: &pb.DiffOp{ + Lower: &pb.LowerDiffInput{Input: 5}, + Upper: &pb.UpperDiffInput{Input: -1}, + }}, + Inputs: []*pb.Input{{Digest: string(digest.FromBytes([]byte("x")))}}, + } + require.NotPanics(t, func() { + _, err := llbOpName(pbOp, noopLoad) + require.Error(t, err) + }) +} + +func TestDiffInputWithoutInputsName(t *testing.T) { + // input index 0 referenced but no inputs declared. + pbOp := &pb.Op{ + Op: &pb.Op_Diff{Diff: &pb.DiffOp{ + Lower: &pb.LowerDiffInput{Input: 0}, + Upper: &pb.UpperDiffInput{Input: -1}, + }}, + Inputs: []*pb.Input{}, + } + require.NotPanics(t, func() { + _, err := llbOpName(pbOp, noopLoad) + require.Error(t, err) + }) +} diff --git a/solver/llbsolver/ops/opindex_validation_test.go b/solver/llbsolver/ops/opindex_validation_test.go new file mode 100644 index 000000000..3311de25e --- /dev/null +++ b/solver/llbsolver/ops/opindex_validation_test.go @@ -0,0 +1,99 @@ +package ops + +import ( + "testing" + + "github.com/moby/buildkit/solver/pb" + "github.com/stretchr/testify/require" +) + +// These exercise malicious op input indices that previously panicked the +// daemon (out-of-range / negative slice indexing). They must now return an +// error instead of panicking. + +func TestExecMountNegativeInput(t *testing.T) { + op := &ExecOp{ + op: &pb.ExecOp{ + Meta: &pb.Meta{Args: []string{"x"}}, + Mounts: []*pb.Mount{{Dest: "/", Input: -2}}, + }, + numInputs: 1, + } + require.NotPanics(t, func() { + _, _, err := op.CacheMap(t.Context(), testJobContext(t), 1) + require.Error(t, err) + }) +} + +func TestFileCopySecondaryNegativeInput(t *testing.T) { + fo := &pb.FileOp{ + Actions: []*pb.FileAction{ + { + Input: 0, + SecondaryInput: -2, + Output: 0, + Action: &pb.FileAction_Copy{ + Copy: &pb.FileActionCopy{Src: "/src", Dest: "/dest"}, + }, + }, + }, + } + f := &fileOp{op: fo, numInputs: 1} + require.NotPanics(t, func() { + _, _, err := f.CacheMap(t.Context(), testJobContext(t), 1) + require.Error(t, err) + }) +} + +func TestFileOwnerUnboundedInput(t *testing.T) { + fo := &pb.FileOp{ + Actions: []*pb.FileAction{ + { + Input: 0, + SecondaryInput: -1, + Output: 0, + Action: &pb.FileAction_Mkdir{ + Mkdir: &pb.FileActionMkDir{ + Path: "/foo", + Mode: 0700, + Owner: &pb.ChownOpt{ + User: &pb.UserOpt{User: &pb.UserOpt_ByName{ByName: &pb.NamedUserOpt{Input: 1000000}}}, + }, + }, + }, + }, + }, + } + f := &fileOp{op: fo, numInputs: 1} + require.NotPanics(t, func() { + _, _, err := f.CacheMap(t.Context(), testJobContext(t), 1) + require.Error(t, err) + }) +} + +func TestFileOwnerInputWithoutInputs(t *testing.T) { + // numInputs == 0 edge: owner references input 0 but there are no inputs. + fo := &pb.FileOp{ + Actions: []*pb.FileAction{ + { + Input: -1, + SecondaryInput: -1, + Output: 0, + Action: &pb.FileAction_Mkdir{ + Mkdir: &pb.FileActionMkDir{ + Path: "/foo", + Mode: 0700, + Owner: &pb.ChownOpt{ + User: &pb.UserOpt{User: &pb.UserOpt_ByName{ByName: &pb.NamedUserOpt{Input: 0}}}, + }, + }, + }, + }, + }, + } + f := &fileOp{op: fo, numInputs: 0} + require.NotPanics(t, func() { + _, _, err := f.CacheMap(t.Context(), testJobContext(t), 1) + require.Error(t, err) + }) +} diff --git a/solver/llbsolver/ops/opsutils/validate_test.go b/solver/llbsolver/ops/opsutils/validate_test.go new file mode 100644 index 000000000..56e59794a --- /dev/null +++ b/solver/llbsolver/ops/opsutils/validate_test.go @@ -0,0 +1,14 @@ +package opsutils + +import ( + "testing" + + "github.com/moby/buildkit/solver/pb" + "github.com/stretchr/testify/require" +) + +func TestValidateDiffNilInputs(t *testing.T) { + require.Error(t, Validate(&pb.Op{Op: &pb.Op_Diff{Diff: &pb.DiffOp{Lower: nil, Upper: &pb.UpperDiffInput{Input: -1}}}})) + require.Error(t, Validate(&pb.Op{Op: &pb.Op_Diff{Diff: &pb.DiffOp{Lower: &pb.LowerDiffInput{Input: -1}, Upper: nil}}})) + require.NoError(t, Validate(&pb.Op{Op: &pb.Op_Diff{Diff: &pb.DiffOp{Lower: &pb.LowerDiffInput{Input: -1}, Upper: &pb.UpperDiffInput{Input: -1}}}})) +} From 5aefc4a61f0345cecf83ac26789ecb7c71833eb9 Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Wed, 15 Jul 2026 14:22:31 +0200 Subject: [PATCH 07/13] solver: reject negative LLB input indexes Reject negative output indexes on LLB input edges while decoding the definition. This prevents malformed client-provided graphs from carrying a negative solver edge index into execution. Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> (cherry picked from commit c3de4fd617413d6e4b4ecfcd7842a15ef5e2b66d) --- solver/llbsolver/vertex.go | 5 +++ solver/llbsolver/vertex_test.go | 55 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/solver/llbsolver/vertex.go b/solver/llbsolver/vertex.go index 8bcd76739..61dd3e3c4 100644 --- a/solver/llbsolver/vertex.go +++ b/solver/llbsolver/vertex.go @@ -367,6 +367,11 @@ func loadLLB(ctx context.Context, def *pb.Definition, polEngine SourcePolicyEval if err := pbop.Unmarshal(dt); err != nil { return solver.Edge{}, errors.Wrap(err, "failed to parse llb proto op") } + for i, input := range pbop.Inputs { + if input.Index < 0 { + return solver.Edge{}, errors.Errorf("invalid input %d output index %d", i, input.Index) + } + } dgst := digest.FromBytes(dt) if pbop.GetSource() != nil { sources[dgst] = struct{}{} diff --git a/solver/llbsolver/vertex_test.go b/solver/llbsolver/vertex_test.go index 9a7f61b5c..10f31b77d 100644 --- a/solver/llbsolver/vertex_test.go +++ b/solver/llbsolver/vertex_test.go @@ -199,6 +199,30 @@ func TestBridgeUsesDefaultProxyNetwork(t *testing.T) { require.True(t, br.proxyNetwork) } +func TestLoadRejectsNegativeInputIndex(t *testing.T) { + for _, tt := range []struct { + name string + execInputIndex int64 + rootInputIndex int64 + }{ + { + name: "vertex input", + execInputIndex: -1, + rootInputIndex: 0, + }, + { + name: "root input", + execInputIndex: 0, + rootInputIndex: -1, + }, + } { + t.Run(tt.name, func(t *testing.T) { + _, err := Load(t.Context(), inputIndexTestDefinition(t, tt.execInputIndex, tt.rootInputIndex), nil) + require.ErrorContains(t, err, "invalid input 0 output index -1") + }) + } +} + func proxyNetworkTestDefinition(t *testing.T, opts ...func(*pb.ExecOp)) *pb.Definition { t.Helper() source := &pb.Op{ @@ -233,6 +257,37 @@ func proxyNetworkTestDefinition(t *testing.T, opts ...func(*pb.ExecOp)) *pb.Defi return &pb.Definition{Def: [][]byte{sourceBytes, execBytes, rootBytes}} } +func inputIndexTestDefinition(t *testing.T, execInputIndex, rootInputIndex int64) *pb.Definition { + t.Helper() + source := &pb.Op{ + Op: &pb.Op_Source{ + Source: &pb.SourceOp{Identifier: "local://context"}, + }, + } + sourceDigest, sourceBytes := marshalTestOp(t, source) + + exec := &pb.Op{ + Inputs: []*pb.Input{{Digest: string(sourceDigest), Index: execInputIndex}}, + Op: &pb.Op_Exec{ + Exec: &pb.ExecOp{ + Meta: &pb.Meta{Args: []string{"true"}}, + Mounts: []*pb.Mount{{ + Input: 0, + Dest: pb.RootMount, + }}, + }, + }, + } + execDigest, execBytes := marshalTestOp(t, exec) + + root := &pb.Op{ + Inputs: []*pb.Input{{Digest: string(execDigest), Index: rootInputIndex}}, + } + _, rootBytes := marshalTestOp(t, root) + + return &pb.Definition{Def: [][]byte{sourceBytes, execBytes, rootBytes}} +} + func marshalTestOp(t *testing.T, op *pb.Op) (digest.Digest, []byte) { t.Helper() dt, err := op.Marshal() From 3916124aa4f6aacd1c8cadde4880d6d62c1cedd0 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 8 Jul 2026 16:38:14 -0700 Subject: [PATCH 08/13] fileop: contain rm parent traversal Anchor rm paths before splitting them so parent traversal is normalized relative to the fileop root before the final path component is appended. Add regression coverage for parent traversal attempts and preserve removal of terminal symlinks without following them. Signed-off-by: Tonis Tiigi (cherry picked from commit cc8b70d02fb890880e79abd44b9a0ad8b7e15d5b) --- solver/llbsolver/file/backend.go | 4 +-- solver/llbsolver/file/backend_test.go | 43 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/solver/llbsolver/file/backend.go b/solver/llbsolver/file/backend.go index 3e380d53b..be431a4f7 100644 --- a/solver/llbsolver/file/backend.go +++ b/solver/llbsolver/file/backend.go @@ -162,12 +162,12 @@ func rm(d string, action *pb.FileActionRm) (err error) { } func rmPath(root, src string, allowNotFound bool) error { - src = filepath.Clean(src) + src = filepath.Join("/", src) dir, base := filepath.Split(src) if base == "" { return errors.New("rmPath: invalid empty path") } - dir, err := fs.RootPath(root, filepath.Join("/", dir)) + dir, err := fs.RootPath(root, dir) if err != nil { return errors.WithStack(err) } diff --git a/solver/llbsolver/file/backend_test.go b/solver/llbsolver/file/backend_test.go index 41bda56d9..3c9d798ed 100644 --- a/solver/llbsolver/file/backend_test.go +++ b/solver/llbsolver/file/backend_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "testing" + "github.com/moby/buildkit/solver/pb" "github.com/pkg/errors" "github.com/stretchr/testify/require" ) @@ -35,3 +36,45 @@ func TestRmPathFileExists(t *testing.T) { require.True(t, os.IsNotExist(err)) } + +func TestRmParentTraversalDoesNotEscapeRoot(t *testing.T) { + // Backslash variants are separators on Windows (real traversal there) and + // ordinary filename characters on Linux (inert, but still must not escape). + for _, p := range []string{ + "..", "../..", "a/../..", "../victim", + "..\\..", "a\\..\\..", "..\\victim", + } { + t.Run(p, func(t *testing.T) { + parent := t.TempDir() + root := filepath.Join(parent, "root") + victim := filepath.Join(parent, "victim") + + require.NoError(t, os.Mkdir(root, 0o755)) + require.NoError(t, os.Mkdir(victim, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(victim, "data"), []byte("data"), 0o644)) + + require.Error(t, rm(root, &pb.FileActionRm{Path: p})) + + _, err := os.Stat(victim) + require.NoError(t, err, "rm escaped root and deleted sibling %q", victim) + }) + } +} + +func TestRmPathRemovesSymlinkItself(t *testing.T) { + root := t.TempDir() + + target := filepath.Join(root, "target") + link := filepath.Join(root, "link") + + require.NoError(t, os.WriteFile(target, []byte("target"), 0o644)) + require.NoError(t, os.Symlink("target", link)) + + require.NoError(t, rmPath(root, "link", false)) + + _, err := os.Lstat(link) + require.True(t, os.IsNotExist(err)) + + _, err = os.Stat(target) + require.NoError(t, err) +} From a9155a456b9b37ea6603d7a97f19839f813023da Mon Sep 17 00:00:00 2001 From: Dawei Wei Date: Mon, 29 Jun 2026 14:22:05 -0700 Subject: [PATCH 09/13] executor/oci: confine WCOW cache mount source within cache root On Windows, RUN --mount=type=cache,source= resolved the source subpath with fs.RootPath, which does not follow Windows reparse points. A junction placed inside the cache root could therefore point to a path outside the cache, and the resolved source was mounted into the build container, exposing host files outside the intended cache subdirectory. Resolve the real path of both the cache root and the selected source via GetFinalPathNameByHandle (which follows junctions and symlinks) and reject any source that resolves outside the cache root. Add Windows unit tests for the resolver and end-to-end regression tests for the junction escape (Windows) and the symlink escape (Linux). Signed-off-by: Dawei Wei (cherry picked from commit 70370e18de3cc7545fcf7f986612a28a803f59d2) --- executor/oci/spec_windows.go | 67 ++++++++++ executor/oci/spec_windows_test.go | 131 +++++++++++++++++++ frontend/dockerfile/dockerfile_mount_test.go | 55 ++++++++ 3 files changed, 253 insertions(+) create mode 100644 executor/oci/spec_windows_test.go diff --git a/executor/oci/spec_windows.go b/executor/oci/spec_windows.go index 398777c92..33ab0c9f5 100644 --- a/executor/oci/spec_windows.go +++ b/executor/oci/spec_windows.go @@ -18,6 +18,7 @@ import ( "github.com/moby/sys/user" specs "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" + "golang.org/x/sys/windows" ) const ( @@ -111,10 +112,76 @@ func sub(m mount.Mount, subPath string) (mount.Mount, func() error, error) { if err != nil { return mount.Mount{}, nil, err } + if err := verifySubpathWithinRoot(m.Source, src, subPath); err != nil { + return mount.Mount{}, nil, err + } m.Source = src return m, func() error { return nil }, nil } +func verifySubpathWithinRoot(root, src, subPath string) error { + realRoot, err := resolveFinalPath(root) + if err != nil { + return errors.Wrapf(err, "resolving mount root %q", root) + } + realSrc, err := resolveFinalPath(src) + if err != nil { + return errors.Wrapf(err, "resolving mount source subpath %q", subPath) + } + if !pathWithinRoot(realRoot, realSrc) { + return errors.Errorf("mount source subpath %q resolves to %q which escapes the mount root %q", subPath, realSrc, realRoot) + } + return nil +} + +// resolveFinalPath returns the real path of p with all reparse points followed. +func resolveFinalPath(p string) (string, error) { + pathPtr, err := windows.UTF16PtrFromString(p) + if err != nil { + return "", err + } + // FILE_FLAG_BACKUP_SEMANTICS allows opening a directory; omitting + // FILE_FLAG_OPEN_REPARSE_POINT makes GetFinalPathNameByHandle follow links. + h, err := windows.CreateFile( + pathPtr, + 0, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS, + 0, + ) + if err != nil { + return "", err + } + defer windows.CloseHandle(h) + + // flags 0 == FILE_NAME_NORMALIZED | VOLUME_NAME_DOS. + n := uint32(windows.MAX_PATH) + for { + buf := make([]uint16, n) + got, err := windows.GetFinalPathNameByHandle(h, &buf[0], n, 0) + if err != nil { + return "", err + } + if got <= n { + return windows.UTF16ToString(buf[:got]), nil + } + n = got + } +} + +// pathWithinRoot reports whether p is root or a descendant of root. Inputs are +// already-resolved real paths; filepath.Rel handles case-insensitivity and +// cross-volume paths on Windows. +func pathWithinRoot(root, p string) bool { + rel, err := filepath.Rel(root, p) + if err != nil { + return false + } + return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) +} + func generateLinuxResourceOpts(res *pb.LinuxResources) ([]oci.SpecOpts, error) { if res == nil { return nil, nil diff --git a/executor/oci/spec_windows_test.go b/executor/oci/spec_windows_test.go new file mode 100644 index 000000000..8275590d3 --- /dev/null +++ b/executor/oci/spec_windows_test.go @@ -0,0 +1,131 @@ +//go:build windows + +package oci + +import ( + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/containerd/containerd/v2/core/mount" + "github.com/stretchr/testify/require" +) + +// mklinkJunction creates a Windows directory junction (mklink /J). +func mklinkJunction(t *testing.T, link, target string) { + t.Helper() + out, err := exec.CommandContext(t.Context(), "cmd", "/c", "mklink", "/J", link, target).CombinedOutput() + require.NoErrorf(t, err, "mklink /J %q %q failed: %s", link, target, out) +} + +// mklinkSymlinkDir creates a Windows directory symlink (mklink /D). It may need +// Developer Mode or SeCreateSymbolicLinkPrivilege; callers should skip on error. +func mklinkSymlinkDir(t *testing.T, link, target string) error { + t.Helper() + out, err := exec.CommandContext(t.Context(), "cmd", "/c", "mklink", "/D", link, target).CombinedOutput() + if err != nil { + t.Logf("mklink /D %q %q failed (symlink creation may be unprivileged): %s", link, target, out) + } + return err +} + +// TestSubResolvesBenignSubdir checks that a normal cache subdirectory resolves +// to a path inside the cache root. +func TestSubResolvesBenignSubdir(t *testing.T) { + cacheRoot := t.TempDir() + sel := filepath.Join(cacheRoot, "sel") + require.NoError(t, os.MkdirAll(sel, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(sel, "benign.txt"), []byte("benign"), 0600)) + + m, cleanup, err := sub(mount.Mount{Source: cacheRoot}, "sel") + require.NoError(t, err) + if cleanup != nil { + defer cleanup() + } + + require.Equal(t, sel, m.Source, "benign subdir must resolve to the cache subdirectory") +} + +// TestSubRejectsJunctionEscape is the core regression test: a cache subdir +// replaced by a junction pointing outside the cache root must be rejected. +func TestSubRejectsJunctionEscape(t *testing.T) { + outside := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(outside, "marker.txt"), []byte("OUTSIDE"), 0600)) + + cacheRoot := t.TempDir() + sel := filepath.Join(cacheRoot, "sel") + mklinkJunction(t, sel, outside) + + m, cleanup, err := sub(mount.Mount{Source: cacheRoot}, "sel") + if cleanup != nil { + defer cleanup() + } + require.Error(t, err, "sub() must reject a junction that escapes the cache root") + require.NotEqual(t, outside, m.Source, "resolved source must not be the outside host path") +} + +// TestSubRejectsSymlinkEscape verifies the same protection for a directory +// symbolic link (mklink /D) pointing outside the cache root. +func TestSubRejectsSymlinkEscape(t *testing.T) { + outside := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(outside, "marker.txt"), []byte("OUTSIDE"), 0600)) + + cacheRoot := t.TempDir() + sel := filepath.Join(cacheRoot, "sel") + if err := mklinkSymlinkDir(t, sel, outside); err != nil { + t.Skipf("could not create directory symlink (insufficient privilege?): %v", err) + } + + m, cleanup, err := sub(mount.Mount{Source: cacheRoot}, "sel") + if cleanup != nil { + defer cleanup() + } + require.Error(t, err, "sub() must reject a symlink that escapes the cache root") + require.NotEqual(t, outside, m.Source, "resolved source must not be the outside host path") +} + +// TestSubRejectsNestedJunctionEscape checks a junction in a non-terminal path +// component (source "mid/leaf" where "mid" escapes the root) is also rejected. +func TestSubRejectsNestedJunctionEscape(t *testing.T) { + outside := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(outside, "leaf"), 0700)) + require.NoError(t, os.WriteFile(filepath.Join(outside, "leaf", "marker.txt"), []byte("OUTSIDE"), 0600)) + + cacheRoot := t.TempDir() + mid := filepath.Join(cacheRoot, "mid") + mklinkJunction(t, mid, outside) + + m, cleanup, err := sub(mount.Mount{Source: cacheRoot}, "mid/leaf") + if cleanup != nil { + defer cleanup() + } + require.Error(t, err, "sub() must reject a path that traverses a junction outside the cache root") + require.NotEqual(t, filepath.Join(outside, "leaf"), m.Source, "resolved source must not escape the cache root") +} + +func TestPathWithinRoot(t *testing.T) { + cases := []struct { + name string + root string + p string + want bool + }{ + {"equal", `C:\cache`, `C:\cache`, true}, + {"equal trailing sep on p", `C:\cache`, `C:\cache\`, true}, + {"equal trailing sep on root", `C:\cache\`, `C:\cache`, true}, + {"direct child", `C:\cache`, `C:\cache\sel`, true}, + {"nested child", `C:\cache`, `C:\cache\a\b`, true}, + {"case-insensitive child", `C:\Cache`, `c:\cache\sel`, true}, + {"child name starting with dotdot", `C:\cache`, `C:\cache\..foo`, true}, + {"exact parent", `C:\cache`, `C:\`, false}, + {"sibling prefix confusion", `C:\cache`, `C:\cache-evil`, false}, + {"unrelated escape", `C:\cache`, `C:\Windows`, false}, + {"different volume", `C:\cache`, `D:\cache\sel`, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, pathWithinRoot(tc.root, tc.p)) + }) + } +} diff --git a/frontend/dockerfile/dockerfile_mount_test.go b/frontend/dockerfile/dockerfile_mount_test.go index bab8ba789..fafa05f9e 100644 --- a/frontend/dockerfile/dockerfile_mount_test.go +++ b/frontend/dockerfile/dockerfile_mount_test.go @@ -29,6 +29,7 @@ var mountTests = integration.TestFuncs( testMountDuplicate, testCacheMountUser, testCacheMountParallel, + testCacheMountSourceEscape, ) func init() { @@ -699,3 +700,57 @@ COPY --from=b2 /License.txt p2 require.NoError(t, err) } } + +func testCacheMountSourceEscape(t *testing.T, sb integration.Sandbox) { + f := getFrontend(t, sb) + + dockerfile := []byte(integration.UnixOrWindows( + ` +FROM busybox AS init +RUN --mount=type=cache,id=sourceescape,target=/cache mkdir -p /cache/sel && echo benign > /cache/sel/benign.txt + +FROM init AS link +RUN --mount=type=cache,id=sourceescape,target=/cache rm -rf /cache/sel && ln -s /etc /cache/sel + +FROM link AS victim +RUN --mount=type=cache,id=sourceescape,source=sel,target=/victim cat /victim/hostname + +FROM scratch +COPY --from=victim /etc/hostname /hostname +`, + `# escape=`+"`"+` +FROM nanoserver AS init +USER ContainerAdministrator +RUN --mount=type=cache,id=sourceescape,target=C:\cache cmd /S /C "if not exist C:\cache\sel mkdir C:\cache\sel & echo benign> C:\cache\sel\benign.txt" + +FROM init AS link +RUN --mount=type=cache,id=sourceescape,target=C:\cache cmd /S /C "rmdir C:\cache\sel /S /Q & mklink /J C:\cache\sel C:\Windows" + +FROM link AS victim +RUN --mount=type=cache,id=sourceescape,source=sel,target=C:\victim cmd /S /C "dir C:\victim" + +FROM nanoserver AS final +COPY --from=victim C:\License.txt C:\License.txt +`, + )) + + dir := integration.Tmpdir( + t, + fstest.CreateFile("Dockerfile", dockerfile, 0600), + ) + + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + FrontendAttrs: map[string]string{ + "no-cache": "", + }, + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameContext: dir, + }, + }, nil) + require.Error(t, err, "build must fail when a cache source= resolves through a link outside the cache root") +} From 473663bad08913e3d8602534b6583e555715a03d Mon Sep 17 00:00:00 2001 From: Dawei Wei Date: Sun, 5 Jul 2026 16:20:12 -0700 Subject: [PATCH 10/13] executor/oci: pin WCOW cache mount source across mount realization The Windows cache mount is realized later by HCS from the resolved host path, so verification alone leaves a check-to-use window in which the verified entry could be swapped for a junction escaping the cache root (concurrent cache access). Hold the verified source open with GENERIC_READ and a share mode that omits FILE_SHARE_DELETE until the mount is released, so the entry cannot be renamed or deleted (and thus swapped) during that window. GENERIC_READ rather than DELETE is used so a concurrent read/traverse open by the mount stack still succeeds; validated against a real HCS worker (benign cache mounts still mount and the escape is still rejected). Add a unit test asserting the source cannot be renamed while pinned and can be renamed after release. Signed-off-by: Dawei Wei (cherry picked from commit 39a7b40489a036134a93502ea3d84d71dca072ea) --- executor/oci/spec_windows.go | 31 ++++++++++++++++++++++++++++++- executor/oci/spec_windows_test.go | 17 +++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/executor/oci/spec_windows.go b/executor/oci/spec_windows.go index 33ab0c9f5..4c4acafab 100644 --- a/executor/oci/spec_windows.go +++ b/executor/oci/spec_windows.go @@ -115,8 +115,37 @@ func sub(m mount.Mount, subPath string) (mount.Mount, func() error, error) { if err := verifySubpathWithinRoot(m.Source, src, subPath); err != nil { return mount.Mount{}, nil, err } + + release, err := pinPathForMount(src) + if err != nil { + return mount.Mount{}, nil, errors.Wrapf(err, "pinning mount source subpath %q", subPath) + } m.Source = src - return m, func() error { return nil }, nil + return m, release, nil +} + +// pinPathForMount holds src open (omitting FILE_SHARE_DELETE) so the entry +// cannot be renamed or swapped for an escaping junction before the later HCS +// mount. GENERIC_READ is used rather than DELETE so a concurrent read/traverse +// open still succeeds. The returned func releases the handle. +func pinPathForMount(src string) (func() error, error) { + pathPtr, err := windows.UTF16PtrFromString(src) + if err != nil { + return nil, err + } + h, err := windows.CreateFile( + pathPtr, + windows.GENERIC_READ, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return nil, err + } + return func() error { return windows.CloseHandle(h) }, nil } func verifySubpathWithinRoot(root, src, subPath string) error { diff --git a/executor/oci/spec_windows_test.go b/executor/oci/spec_windows_test.go index 8275590d3..0eeaca709 100644 --- a/executor/oci/spec_windows_test.go +++ b/executor/oci/spec_windows_test.go @@ -104,6 +104,23 @@ func TestSubRejectsNestedJunctionEscape(t *testing.T) { require.NotEqual(t, filepath.Join(outside, "leaf"), m.Source, "resolved source must not escape the cache root") } +// TestSubPinsSourceAgainstSwap verifies that while sub() holds the mount source, +// the entry cannot be renamed/swapped, and that releasing the cleanup lifts it. +func TestSubPinsSourceAgainstSwap(t *testing.T) { + cacheRoot := t.TempDir() + sel := filepath.Join(cacheRoot, "sel") + require.NoError(t, os.MkdirAll(sel, 0700)) + + _, cleanup, err := sub(mount.Mount{Source: cacheRoot}, "sel") + require.NoError(t, err) + require.NotNil(t, cleanup) + + require.Error(t, os.Rename(sel, sel+".swap"), "pinned source must not be renamable during the mount window") + + require.NoError(t, cleanup()) + require.NoError(t, os.Rename(sel, sel+".swap"), "source must be renamable after the pin is released") +} + func TestPathWithinRoot(t *testing.T) { cases := []struct { name string From 44c0ed6b1c3a8676653efe64c3a6e30f2bef22ec Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 15 Jul 2026 00:18:33 -0700 Subject: [PATCH 11/13] executor/oci: pin resolved WCOW cache source Resolve the selected cache subpath through the same handle used to pin it, then verify that resolved target is still within the cache root before passing it as the mount source. This closes the check-to-use gap where containment verification and pinning could observe different path objects. It also avoids handing HCS the original path containing attacker-controlled reparse points. Open the pinned directory with delete access while omitting delete sharing, so the selected resolved directory cannot be renamed, deleted, or swapped before HCS consumes the mount source. Keep write sharing enabled so writable cache contents can still be modified while the handle is alive. Keep the HCS-facing source in the normal DOS path form when possible, so this does not introduce a new \\?\ path format requirement for WCOW mount realization. Signed-off-by: Tonis Tiigi (cherry picked from commit d854e00d473958b6e164f2e821eab902245705ad) --- executor/oci/spec_windows.go | 67 ++++++++++++++++-------- executor/oci/spec_windows_test.go | 86 ++++++++++++++++++++++++++++++- 2 files changed, 131 insertions(+), 22 deletions(-) diff --git a/executor/oci/spec_windows.go b/executor/oci/spec_windows.go index 4c4acafab..ad7d23ad8 100644 --- a/executor/oci/spec_windows.go +++ b/executor/oci/spec_windows.go @@ -112,51 +112,59 @@ func sub(m mount.Mount, subPath string) (mount.Mount, func() error, error) { if err != nil { return mount.Mount{}, nil, err } - if err := verifySubpathWithinRoot(m.Source, src, subPath); err != nil { + + realSrc, release, err := resolveAndPinPath(src) + if err != nil { + return mount.Mount{}, nil, errors.Wrapf(err, "resolving and pinning mount source subpath %q", subPath) + } + if err := verifySubpathWithinRoot(m.Source, realSrc, subPath); err != nil { + _ = release() return mount.Mount{}, nil, err } - release, err := pinPathForMount(src) - if err != nil { - return mount.Mount{}, nil, errors.Wrapf(err, "pinning mount source subpath %q", subPath) - } - m.Source = src + // Use the path of the object represented by the pinned handle. HCS can then + // realize the mount without traversing attacker-controlled reparse points in + // the original source path again. + m.Source = mountSourcePath(realSrc) return m, release, nil } -// pinPathForMount holds src open (omitting FILE_SHARE_DELETE) so the entry -// cannot be renamed or swapped for an escaping junction before the later HCS -// mount. GENERIC_READ is used rather than DELETE so a concurrent read/traverse -// open still succeeds. The returned func releases the handle. -func pinPathForMount(src string) (func() error, error) { +// resolveAndPinPath opens src while following reparse points and returns the +// normalized path of the object represented by that handle. Omitting delete +// sharing prevents the resolved directory from being renamed or deleted before +// HCS realizes the mount. Write sharing is still allowed so a writable cache +// mount can modify contents while the handle is alive. +func resolveAndPinPath(src string) (string, func() error, error) { pathPtr, err := windows.UTF16PtrFromString(src) if err != nil { - return nil, err + return "", nil, err } h, err := windows.CreateFile( pathPtr, - windows.GENERIC_READ, + windows.DELETE, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, nil, windows.OPEN_EXISTING, - windows.FILE_FLAG_BACKUP_SEMANTICS|windows.FILE_FLAG_OPEN_REPARSE_POINT, + windows.FILE_FLAG_BACKUP_SEMANTICS, 0, ) if err != nil { - return nil, err + return "", nil, err } - return func() error { return windows.CloseHandle(h) }, nil + + realSrc, err := finalPathNameByHandle(h) + if err != nil { + _ = windows.CloseHandle(h) + return "", nil, err + } + return realSrc, func() error { return windows.CloseHandle(h) }, nil } -func verifySubpathWithinRoot(root, src, subPath string) error { +func verifySubpathWithinRoot(root, realSrc, subPath string) error { realRoot, err := resolveFinalPath(root) if err != nil { return errors.Wrapf(err, "resolving mount root %q", root) } - realSrc, err := resolveFinalPath(src) - if err != nil { - return errors.Wrapf(err, "resolving mount source subpath %q", subPath) - } if !pathWithinRoot(realRoot, realSrc) { return errors.Errorf("mount source subpath %q resolves to %q which escapes the mount root %q", subPath, realSrc, realRoot) } @@ -184,7 +192,10 @@ func resolveFinalPath(p string) (string, error) { return "", err } defer windows.CloseHandle(h) + return finalPathNameByHandle(h) +} +func finalPathNameByHandle(h windows.Handle) (string, error) { // flags 0 == FILE_NAME_NORMALIZED | VOLUME_NAME_DOS. n := uint32(windows.MAX_PATH) for { @@ -200,6 +211,20 @@ func resolveFinalPath(p string) (string, error) { } } +func mountSourcePath(p string) string { + if strings.HasPrefix(p, `\\?\UNC\`) { + return `\\` + p[len(`\\?\UNC\`):] + } + if len(p) >= len(`\\?\C:\`) && p[:4] == `\\?\` && isDriveLetter(p[4]) && p[5:7] == `:\` { + return p[4:] + } + return p +} + +func isDriveLetter(c byte) bool { + return ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') +} + // pathWithinRoot reports whether p is root or a descendant of root. Inputs are // already-resolved real paths; filepath.Rel handles case-insensitivity and // cross-volume paths on Windows. diff --git a/executor/oci/spec_windows_test.go b/executor/oci/spec_windows_test.go index 0eeaca709..c7f213259 100644 --- a/executor/oci/spec_windows_test.go +++ b/executor/oci/spec_windows_test.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/containerd/containerd/v2/core/mount" @@ -44,7 +45,12 @@ func TestSubResolvesBenignSubdir(t *testing.T) { defer cleanup() } - require.Equal(t, sel, m.Source, "benign subdir must resolve to the cache subdirectory") + realSel, err := resolveFinalPath(sel) + require.NoError(t, err) + realMountSource, err := resolveFinalPath(m.Source) + require.NoError(t, err) + require.Equal(t, realSel, realMountSource, "benign subdir must resolve to the cache subdirectory") + require.False(t, strings.HasPrefix(m.Source, `\\?\`), "mount source should use the plain DOS path form when available") } // TestSubRejectsJunctionEscape is the core regression test: a cache subdir @@ -121,6 +127,84 @@ func TestSubPinsSourceAgainstSwap(t *testing.T) { require.NoError(t, os.Rename(sel, sel+".swap"), "source must be renamable after the pin is released") } +// TestSubPinAllowsChildChanges verifies that the restrictive share mode applies +// to the selected directory object without making the mounted cache contents +// read-only. +func TestSubPinAllowsChildChanges(t *testing.T) { + cacheRoot := t.TempDir() + sel := filepath.Join(cacheRoot, "sel") + require.NoError(t, os.MkdirAll(sel, 0700)) + + _, cleanup, err := sub(mount.Mount{Source: cacheRoot}, "sel") + require.NoError(t, err) + require.NotNil(t, cleanup) + defer cleanup() + + child := filepath.Join(sel, "child") + require.NoError(t, os.WriteFile(child, []byte("first"), 0600)) + require.NoError(t, os.WriteFile(child, []byte("second"), 0600)) + + renamed := filepath.Join(sel, "renamed") + require.NoError(t, os.Rename(child, renamed)) + require.NoError(t, os.Remove(renamed)) + + nested := filepath.Join(sel, "nested") + require.NoError(t, os.Mkdir(nested, 0700)) + require.NoError(t, os.Remove(nested)) +} + +// TestSubUsesPinnedResolvedPath verifies that HCS receives the resolved target +// path rather than the original path containing an attacker-controlled +// junction. Replacing the original junction therefore cannot redirect the +// later mount. +func TestSubUsesPinnedResolvedPath(t *testing.T) { + cacheRoot := t.TempDir() + inside := filepath.Join(cacheRoot, "inside") + require.NoError(t, os.MkdirAll(inside, 0700)) + + sel := filepath.Join(cacheRoot, "sel") + mklinkJunction(t, sel, inside) + + m, cleanup, err := sub(mount.Mount{Source: cacheRoot}, "sel") + require.NoError(t, err) + require.NotNil(t, cleanup) + defer cleanup() + + realInside, err := resolveFinalPath(inside) + require.NoError(t, err) + realMountSource, err := resolveFinalPath(m.Source) + require.NoError(t, err) + require.Equal(t, realInside, realMountSource) + require.False(t, strings.HasPrefix(m.Source, `\\?\`), "mount source should use the plain DOS path form when available") + + require.NoError(t, os.Remove(sel), "the original junction is not the pinned source object") + outside := t.TempDir() + mklinkJunction(t, sel, outside) + + realMountSource, err = resolveFinalPath(m.Source) + require.NoError(t, err) + require.Equal(t, realInside, realMountSource, "replacing the original junction must not redirect the mount source") +} + +func TestMountSourcePath(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"drive letter", `\\?\C:\cache\sel`, `C:\cache\sel`}, + {"unc", `\\?\UNC\server\share\cache\sel`, `\\server\share\cache\sel`}, + {"volume guid", `\\?\Volume{11111111-1111-1111-1111-111111111111}\cache\sel`, `\\?\Volume{11111111-1111-1111-1111-111111111111}\cache\sel`}, + {"plain drive letter", `C:\cache\sel`, `C:\cache\sel`}, + {"similar prefix", `\\x\C:\cache\sel`, `\\x\C:\cache\sel`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, mountSourcePath(tc.in)) + }) + } +} + func TestPathWithinRoot(t *testing.T) { cases := []struct { name string From 0669f4b974dce27cc74282e8c5121ba0d5f62c70 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 15 Jul 2026 23:43:07 -0700 Subject: [PATCH 12/13] executor/oci: avoid delete access for WCOW pin Open the resolved mount source with GENERIC_READ instead of DELETE. The nonzero desired access keeps the share-mode pin effective, while omitting FILE_SHARE_DELETE still prevents the source from being renamed or deleted before HCS realizes the mount. Avoid requiring delete permission on legitimate mount sources, which caused access denied failures for WCOW bind mounts. Keep write sharing enabled so writable cache contents continue to work normally. Signed-off-by: Tonis Tiigi (cherry picked from commit 2523c023ecac816c8131ea08b37f9b84145d5acc) --- executor/oci/spec_windows.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/executor/oci/spec_windows.go b/executor/oci/spec_windows.go index ad7d23ad8..3eefb124f 100644 --- a/executor/oci/spec_windows.go +++ b/executor/oci/spec_windows.go @@ -130,10 +130,11 @@ func sub(m mount.Mount, subPath string) (mount.Mount, func() error, error) { } // resolveAndPinPath opens src while following reparse points and returns the -// normalized path of the object represented by that handle. Omitting delete -// sharing prevents the resolved directory from being renamed or deleted before -// HCS realizes the mount. Write sharing is still allowed so a writable cache -// mount can modify contents while the handle is alive. +// normalized path of the object represented by that handle. GENERIC_READ makes +// the handle participate in share-access checks without requiring delete access +// to the source. Omitting delete sharing prevents the resolved directory from +// being renamed or deleted before HCS realizes the mount. Write sharing is still +// allowed so a writable cache mount can modify contents while the handle is alive. func resolveAndPinPath(src string) (string, func() error, error) { pathPtr, err := windows.UTF16PtrFromString(src) if err != nil { @@ -141,7 +142,7 @@ func resolveAndPinPath(src string) (string, func() error, error) { } h, err := windows.CreateFile( pathPtr, - windows.DELETE, + windows.GENERIC_READ, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, nil, windows.OPEN_EXISTING, From 1a3268174d8ffd8a1264a0560bcaff9b8b4c60fc Mon Sep 17 00:00:00 2001 From: CrazyMax <1951866+crazy-max@users.noreply.github.com> Date: Thu, 16 Jul 2026 13:59:11 +0200 Subject: [PATCH 13/13] vendor: update github.com/tonistiigi/fsutil to 30cd4fc5d911 Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com> --- go.mod | 2 +- go.sum | 4 ++-- vendor/github.com/tonistiigi/fsutil/validator.go | 2 +- vendor/modules.txt | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 2dd01e74f..857078f61 100644 --- a/go.mod +++ b/go.mod @@ -78,7 +78,7 @@ require ( github.com/spdx/tools-golang v0.5.7 github.com/stretchr/testify v1.11.1 github.com/tonistiigi/dchapes-mode v0.0.0-20250318174251-73d941a28323 - github.com/tonistiigi/fsutil v0.0.0-20260609091201-0257b3308df4 + github.com/tonistiigi/fsutil v0.0.0-20260716115106-30cd4fc5d911 // buildkit-v0.31 github.com/tonistiigi/go-actions-cache v0.0.0-20260120203934-54bc28c26fd2 github.com/tonistiigi/go-archvariant v1.0.0 github.com/tonistiigi/go-csvvalue v0.0.0-20240814133006-030d3b2625d0 diff --git a/go.sum b/go.sum index 5cfe3957d..b1443645b 100644 --- a/go.sum +++ b/go.sum @@ -584,8 +584,8 @@ github.com/titanous/rocacheck v0.0.0-20171023193734-afe73141d399 h1:e/5i7d4oYZ+C github.com/titanous/rocacheck v0.0.0-20171023193734-afe73141d399/go.mod h1:LdwHTNJT99C5fTAzDz0ud328OgXz+gierycbcIx2fRs= github.com/tonistiigi/dchapes-mode v0.0.0-20250318174251-73d941a28323 h1:r0p7fK56l8WPequOaR3i9LBqfPtEdXIQbUTzT55iqT4= github.com/tonistiigi/dchapes-mode v0.0.0-20250318174251-73d941a28323/go.mod h1:3Iuxbr0P7D3zUzBMAZB+ois3h/et0shEz0qApgHYGpY= -github.com/tonistiigi/fsutil v0.0.0-20260609091201-0257b3308df4 h1:tJkv/edHw9FXVtbHxc6cpqDttiCLNzhqI1W40fcnxIY= -github.com/tonistiigi/fsutil v0.0.0-20260609091201-0257b3308df4/go.mod h1:K5zrLch9UaSGNiek5XHZeqZUf1zPWJHqDfLIcnpquQ4= +github.com/tonistiigi/fsutil v0.0.0-20260716115106-30cd4fc5d911 h1:xJZz1fhsRSrGTzQ6wvh1gX6d5jQaYjbIuGXT2s0AWuM= +github.com/tonistiigi/fsutil v0.0.0-20260716115106-30cd4fc5d911/go.mod h1:K5zrLch9UaSGNiek5XHZeqZUf1zPWJHqDfLIcnpquQ4= github.com/tonistiigi/go-actions-cache v0.0.0-20260120203934-54bc28c26fd2 h1:5p6hffZeB25G4rhBc3HU6x1aIlyDELfib+/Omq+ZfQA= github.com/tonistiigi/go-actions-cache v0.0.0-20260120203934-54bc28c26fd2/go.mod h1:cD0SB2270BYw6HYKriFn4H6NRLhGj6ytf48YTpsm8LY= github.com/tonistiigi/go-archvariant v1.0.0 h1:5LC1eDWiBNflnTF1prCiX09yfNHIxDC/aukdhCdTyb0= diff --git a/vendor/github.com/tonistiigi/fsutil/validator.go b/vendor/github.com/tonistiigi/fsutil/validator.go index 44d80cdf0..773ffcdb7 100644 --- a/vendor/github.com/tonistiigi/fsutil/validator.go +++ b/vendor/github.com/tonistiigi/fsutil/validator.go @@ -38,7 +38,7 @@ func (v *Validator) HandleChange(kind ChangeKind, p string, fi os.FileInfo, err if dir == "." { dir = "" } - if dir == ".." || strings.HasPrefix(p, filepath.FromSlash("../")) { + if p == ".." || dir == ".." || strings.HasPrefix(p, filepath.FromSlash("../")) { return errors.WithStack(&os.PathError{Path: p, Err: syscall.EINVAL, Op: "escape check"}) } diff --git a/vendor/modules.txt b/vendor/modules.txt index 5d41a4cc7..d72e077b7 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1038,7 +1038,7 @@ github.com/theupdateframework/go-tuf/v2/metadata/updater # github.com/tonistiigi/dchapes-mode v0.0.0-20250318174251-73d941a28323 ## explicit; go 1.21 github.com/tonistiigi/dchapes-mode -# github.com/tonistiigi/fsutil v0.0.0-20260609091201-0257b3308df4 +# github.com/tonistiigi/fsutil v0.0.0-20260716115106-30cd4fc5d911 ## explicit; go 1.25.0 github.com/tonistiigi/fsutil github.com/tonistiigi/fsutil/copy