[v0.13.0-beta1-m.1] backport: security: path traversal and git option injection fixes

.github/workflows/.test.yml

@@ -89,7 +89,7 @@ jobs:
             });
       -
         name: Build
-        uses: docker/bake-action@v4
+        uses: docker/bake-action@v5
         with:
           targets: integration-tests-base
           set: |
@@ -145,7 +145,7 @@ jobs:
           buildkitd-flags: --debug
       -
         name: Build test image
-        uses: docker/bake-action@v4
+        uses: docker/bake-action@v5
         with:
           targets: integration-tests
           set: |
@@ -186,7 +186,7 @@ jobs:
       -
         name: Upload test reports
         if: always()
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: test-reports
           path: ./bin/testreports

.github/workflows/buildkit.yml

@@ -129,7 +129,7 @@ jobs:
           CACHE_TO: type=gha,scope=binaries-${{ env.PLATFORM_PAIR }}
       -
         name: Upload artifacts
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: buildkit
           path: ${{ env.DESTDIR }}/*
@@ -190,7 +190,7 @@ jobs:
     steps:
       -
         name: Download artifacts
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: buildkit
           path: ${{ env.DESTDIR }}

.github/workflows/dockerd.yml

@@ -63,7 +63,7 @@ jobs:
           wget -qO- "https://download.docker.com/linux/static/stable/x86_64/docker-${{ env.DOCKER_VERSION }}.tgz" | tar xvz --strip 1
       -
         name: Upload dockerd
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: dockerd
           path: /tmp/moby/dockerd
@@ -109,7 +109,7 @@ jobs:
           buildkitd-flags: --debug
       -
         name: Download dockerd
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: dockerd
           path: ./build/

.github/workflows/test-os.yml

@@ -81,7 +81,7 @@ jobs:
       -
         name: Upload test reports
         if: always()
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: test-reports
           path: ./bin/testreports
@@ -101,14 +101,14 @@ jobs:
         uses: docker/setup-buildx-action@v3
       -
         name: Build
-        uses: docker/bake-action@v4
+        uses: docker/bake-action@v5
         with:
           targets: binaries
           set: |
             *.platform=freebsd/amd64
       -
         name: Upload artifacts
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: buildkit-freebsd-amd64
           path: ${{ env.DESTDIR }}/*
@@ -128,7 +128,7 @@ jobs:
         uses: actions/checkout@v4
       -
         name: Download artifacts
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: buildkit-freebsd-amd64
           path: ${{ env.DESTDIR }}

.github/workflows/validate.yml

@@ -55,6 +55,6 @@ jobs:
           buildkitd-flags: --debug
       -
         name: Validate
-        uses: docker/bake-action@v4
+        uses: docker/bake-action@v5
         with:
           targets: ${{ matrix.target }}

executor/containerdexecutor/executor.go

@@ -94,6 +94,9 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M
 	if id == "" {
 		id = identity.NewID()
 	}
+	if err := executor.ValidContainerID(id); err != nil {
+		return nil, err
+	}
 
 	startedOnce := sync.Once{}
 	done := make(chan error, 1)

executor/containerid.go

@@ -0,0 +1,18 @@
+package executor
+
+import "github.com/pkg/errors"
+
+// ValidContainerID validates that id is non-empty and contains only ASCII letters and digits.
+func ValidContainerID(id string) error {
+	if id == "" {
+		return errors.New("container id must not be empty")
+	}
+	for i := 0; i < len(id); i++ {
+		ch := id[i]
+		if (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') {
+			continue
+		}
+		return errors.Errorf("invalid container id %q: only letters and numbers are allowed", id)
+	}
+	return nil
+}

executor/containerid_test.go

@@ -0,0 +1,34 @@
+package executor
+
+import "testing"
+
+func TestValidContainerID(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		id      string
+		wantErr bool
+	}{
+		{name: "lowercase", id: "abc123", wantErr: false},
+		{name: "uppercase", id: "AbC123", wantErr: false},
+		{name: "empty", id: "", wantErr: true},
+		{name: "dash", id: "abc-123", wantErr: true},
+		{name: "slash", id: "abc/123", wantErr: true},
+		{name: "underscore", id: "abc_123", wantErr: true},
+	}
+
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			err := ValidContainerID(tc.id)
+			if tc.wantErr && err == nil {
+				t.Fatalf("expected an error for id %q", tc.id)
+			}
+			if !tc.wantErr && err != nil {
+				t.Fatalf("expected no error for id %q, got %v", tc.id, err)
+			}
+		})
+	}
+}

executor/runcexecutor/executor.go

@@ -148,6 +148,13 @@ func New(opt Opt, networkProviders map[pb.NetMode]network.Provider) (executor.Ex
 func (w *runcExecutor) Run(ctx context.Context, id string, root executor.Mount, mounts []executor.Mount, process executor.ProcessInfo, started chan<- struct{}) (rec resourcestypes.Recorder, err error) {
 	meta := process.Meta
 
+	if id == "" {
+		id = identity.NewID()
+	}
+	if err := executor.ValidContainerID(id); err != nil {
+		return nil, err
+	}
+
 	startedOnce := sync.Once{}
 	done := make(chan error, 1)
 	w.mu.Lock()
@@ -211,9 +218,6 @@ func (w *runcExecutor) Run(ctx context.Context, id string, root executor.Mount,
 		defer release()
 	}
 
-	if id == "" {
-		id = identity.NewID()
-	}
 	bundle := filepath.Join(w.root, id)
 
 	if err := os.Mkdir(bundle, 0o711); err != nil {

go.mod

@@ -26,6 +26,7 @@ require (
 	github.com/containerd/stargz-snapshotter/estargz v0.14.3
 	github.com/containerd/typeurl/v2 v2.1.1
 	github.com/coreos/go-systemd/v22 v22.5.0
+	github.com/cyphar/filepath-securejoin v0.2.4
 	github.com/distribution/reference v0.5.0
 	github.com/docker/cli v24.0.5+incompatible
 	github.com/docker/docker v24.0.0-rc.2.0.20230905130451-032797ea4bcb+incompatible // master (v25.0.0-dev)

go.sum

@@ -410,6 +410,8 @@ github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46t
 github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7DoTY=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/cyphar/filepath-securejoin v0.2.2/go.mod h1:FpkQEhXnPnOthhzymB7CGsFk2G9VLXONKD9G7QGMM+4=
+github.com/cyphar/filepath-securejoin v0.2.4 h1:Ugdm7cg7i6ZK6x3xDF1oEu1nfkyfH53EtKeQYTC3kyg=
+github.com/cyphar/filepath-securejoin v0.2.4/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4=
 github.com/d2g/dhcp4 v0.0.0-20170904100407-a1d1b6c41b1c/go.mod h1:Ct2BUK8SB0YC1SMSibvLzxjeJLnrYEVLULFNiHY9YfQ=
 github.com/d2g/dhcp4client v1.0.0/go.mod h1:j0hNfjhrt2SxUOw55nL0ATM/z4Yt3t2Kd1mW34z5W5s=
 github.com/d2g/dhcp4server v0.0.0-20181031114812-7d4a0a7f59a5/go.mod h1:Eo87+Kg/IX2hfWJfwxMzLyuSZyxSoAug2nGa1G2QAi8=

source/git/gitsource_unix_test.go

@@ -0,0 +1,35 @@
+//go:build !windows
+// +build !windows
+
+package git
+
+import (
+	"os"
+	"path/filepath"
+	"sort"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestReaddirnames verifies that readdirnames correctly lists directory entries
+// from an O_PATH fd returned by openSubdirSafe.
+func TestReaddirnames(t *testing.T) {
+	t.Parallel()
+	root := t.TempDir()
+
+	require.NoError(t, os.MkdirAll(filepath.Join(root, "sub"), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(root, "sub", "a.txt"), []byte("a"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(root, "sub", "b.txt"), []byte("b"), 0o644))
+	require.NoError(t, os.MkdirAll(filepath.Join(root, "sub", "child"), 0o755))
+
+	f, err := openSubdirSafe(root, "sub")
+	require.NoError(t, err)
+	defer f.Close()
+
+	names, err := readdirnames(f)
+	require.NoError(t, err)
+
+	sort.Strings(names)
+	require.Equal(t, []string{"a.txt", "b.txt", "child"}, names)
+}

source/git/identifier.go

@@ -9,6 +9,7 @@ import (
 	srctypes "github.com/moby/buildkit/source/types"
 	"github.com/moby/buildkit/util/gitutil"
 	"github.com/moby/buildkit/util/sshutil"
+	"github.com/pkg/errors"
 )
 
 type GitIdentifier struct {
@@ -36,12 +37,22 @@ func NewGitIdentifier(remoteURL string) (*GitIdentifier, error) {
 		repo.Ref = u.Fragment.Ref
 		repo.Subdir = u.Fragment.Subdir
 	}
-	if sd := path.Clean(repo.Subdir); sd == "/" || sd == "." {
-		repo.Subdir = ""
+	repo.Subdir = strings.TrimPrefix(path.Join("/", repo.Subdir), "/")
+	if err := validateGitRef(repo.Ref); err != nil {
+		return nil, err
 	}
 	return &repo, nil
 }
 
+// validateGitRef rejects refs that start with '-', which could be
+// interpreted as option flags by git commands.
+func validateGitRef(ref string) error {
+	if strings.HasPrefix(ref, "-") {
+		return errors.Errorf("invalid git ref %q", ref)
+	}
+	return nil
+}
+
 func (GitIdentifier) Scheme() string {
 	return srctypes.GitScheme
 }

source/git/identifier_test.go

@@ -48,14 +48,14 @@ func TestNewGitIdentifier(t *testing.T) {
 			expected: GitIdentifier{
 				Remote: "git://github.com/user/repo.git",
 				Ref:    "mybranch",
-				Subdir: "mydir/mysubdir/",
+				Subdir: "mydir/mysubdir",
 			},
 		},
 		{
 			url: "git://github.com/user/repo.git#:mydir/mysubdir/",
 			expected: GitIdentifier{
 				Remote: "git://github.com/user/repo.git",
-				Subdir: "mydir/mysubdir/",
+				Subdir: "mydir/mysubdir",
 			},
 		},
 		{
@@ -69,7 +69,7 @@ func TestNewGitIdentifier(t *testing.T) {
 			expected: GitIdentifier{
 				Remote: "https://github.com/user/repo.git",
 				Ref:    "mybranch",
-				Subdir: "mydir/mysubdir/",
+				Subdir: "mydir/mysubdir",
 			},
 		},
 		{
@@ -83,7 +83,7 @@ func TestNewGitIdentifier(t *testing.T) {
 			expected: GitIdentifier{
 				Remote: "git@github.com:user/repo.git",
 				Ref:    "mybranch",
-				Subdir: "mydir/mysubdir/",
+				Subdir: "mydir/mysubdir",
 			},
 		},
 		{
@@ -97,15 +97,78 @@ func TestNewGitIdentifier(t *testing.T) {
 			expected: GitIdentifier{
 				Remote: "ssh://github.com/user/repo.git",
 				Ref:    "mybranch",
-				Subdir: "mydir/mysubdir/",
+				Subdir: "mydir/mysubdir",
 			},
 		},
 		{
 			url: "ssh://foo%40barcorp.com@github.com/user/repo.git#mybranch:mydir/mysubdir/",
 			expected: GitIdentifier{
 				Remote: "ssh://foo%40barcorp.com@github.com/user/repo.git",
 				Ref:    "mybranch",
-				Subdir: "mydir/mysubdir/",
+				Subdir: "mydir/mysubdir",
+			},
+		},
+		{
+			url: "https://github.com/user/repo.git#main:../../escape",
+			expected: GitIdentifier{
+				Remote: "https://github.com/user/repo.git",
+				Ref:    "main",
+				Subdir: "escape",
+			},
+		},
+		{
+			url: "https://github.com/user/repo.git#main:dir/../../escape",
+			expected: GitIdentifier{
+				Remote: "https://github.com/user/repo.git",
+				Ref:    "main",
+				Subdir: "escape",
+			},
+		},
+		{
+			url: "https://github.com/user/repo.git#main:/absolute/path",
+			expected: GitIdentifier{
+				Remote: "https://github.com/user/repo.git",
+				Ref:    "main",
+				Subdir: "absolute/path",
+			},
+		},
+		{
+			url: "https://github.com/user/repo.git#main:../",
+			expected: GitIdentifier{
+				Remote: "https://github.com/user/repo.git",
+				Ref:    "main",
+			},
+		},
+		{
+			url: "ssh://github.com/user/repo.git#main:../../escape",
+			expected: GitIdentifier{
+				Remote: "ssh://github.com/user/repo.git",
+				Ref:    "main",
+				Subdir: "escape",
+			},
+		},
+		{
+			url: "ssh://github.com/user/repo.git#main:/absolute/path",
+			expected: GitIdentifier{
+				Remote: "ssh://github.com/user/repo.git",
+				Ref:    "main",
+				Subdir: "absolute/path",
+			},
+		},
+		{
+			url: "git@github.com:user/repo.git#main:../../escape",
+			expected: GitIdentifier{
+				Remote: "git@github.com:user/repo.git",
+				Ref:    "main",
+				Subdir: "escape",
+			},
+		},
+		{
+			url: "git@github.com:user/repo.git#main:/absolute/path",
+			expected: GitIdentifier{
+				Remote: "git@github.com:user/repo.git",
+				Ref:    "main",
+				Subdir: "absolute/path",
 			},
 		},
 	}