diff --git a/.github/workflows/.test.yml b/.github/workflows/.test.yml index e99b5ca8ba04..81b7a1c7d85e 100644 --- a/.github/workflows/.test.yml +++ b/.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 diff --git a/.github/workflows/buildkit.yml b/.github/workflows/buildkit.yml index 3984b98d6c93..3e2132090082 100644 --- a/.github/workflows/buildkit.yml +++ b/.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 }} diff --git a/.github/workflows/dockerd.yml b/.github/workflows/dockerd.yml index 4234b2254826..9cdd9f817818 100644 --- a/.github/workflows/dockerd.yml +++ b/.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/ diff --git a/.github/workflows/test-os.yml b/.github/workflows/test-os.yml index 9daa23d57e39..babb4700c8e7 100644 --- a/.github/workflows/test-os.yml +++ b/.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 }} diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 922cb08af219..a33c56daec47 100644 --- a/.github/workflows/validate.yml +++ b/.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 }} diff --git a/executor/containerdexecutor/executor.go b/executor/containerdexecutor/executor.go index 5c1b77778167..6f4126630d81 100644 --- a/executor/containerdexecutor/executor.go +++ b/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) diff --git a/executor/containerid.go b/executor/containerid.go new file mode 100644 index 000000000000..421c8ad4eba6 --- /dev/null +++ b/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 +} diff --git a/executor/containerid_test.go b/executor/containerid_test.go new file mode 100644 index 000000000000..f84a7d976ffa --- /dev/null +++ b/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) + } + }) + } +} diff --git a/executor/runcexecutor/executor.go b/executor/runcexecutor/executor.go index e804ee850b28..048e26e3688e 100644 --- a/executor/runcexecutor/executor.go +++ b/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 { diff --git a/go.mod b/go.mod index b784581903af..8fd8140f0eab 100644 --- a/go.mod +++ b/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) diff --git a/go.sum b/go.sum index 0077a348ff07..f86c8e54bea3 100644 --- a/go.sum +++ b/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= diff --git a/source/git/gitsource_unix_test.go b/source/git/gitsource_unix_test.go new file mode 100644 index 000000000000..df0d65405d75 --- /dev/null +++ b/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) +} diff --git a/source/git/identifier.go b/source/git/identifier.go index 78ac6fb3b3f0..59d8744f4027 100644 --- a/source/git/identifier.go +++ b/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 } diff --git a/source/git/identifier_test.go b/source/git/identifier_test.go index 6f9bc7e5575a..7de6670a5ef9 100644 --- a/source/git/identifier_test.go +++ b/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,7 +97,7 @@ func TestNewGitIdentifier(t *testing.T) { expected: GitIdentifier{ Remote: "ssh://github.com/user/repo.git", Ref: "mybranch", - Subdir: "mydir/mysubdir/", + Subdir: "mydir/mysubdir", }, }, { @@ -105,7 +105,70 @@ func TestNewGitIdentifier(t *testing.T) { 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", }, }, } diff --git a/source/git/source.go b/source/git/source.go index 4ba0b2e09132..6d2873d54a5a 100644 --- a/source/git/source.go +++ b/source/git/source.go @@ -365,7 +365,7 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index // TODO: should we assume that remote tag is immutable? add a timer? - buf, err := git.Run(ctx, "ls-remote", "origin", ref) + buf, err := git.Run(ctx, "ls-remote", "--", "origin", ref) if err != nil { return "", "", nil, false, errors.Wrapf(err, "failed to fetch remote %s", urlutil.RedactCredentials(remote)) } @@ -435,7 +435,7 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out doFetch := true if isCommitSHA(ref) { // skip fetch if commit already exists - if _, err := git.Run(ctx, "cat-file", "-e", ref+"^{commit}"); err == nil { + if _, err := git.Run(ctx, "cat-file", "-e", "--", ref+"^{commit}"); err == nil { doFetch = false } } @@ -454,7 +454,7 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out } args = append(args, "origin") if !isCommitSHA(ref) { - args = append(args, "--force", ref+":tags/"+ref) + args = append(args, "--force", "--", ref+":tags/"+ref) // local refs are needed so they would be advertised on next fetches. Force is used // in case the ref is a branch and it now points to a different commit sha // TODO: is there a better way to do this? @@ -494,7 +494,7 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out } }() - subdir := path.Clean(gs.src.Subdir) + subdir := path.Join("/", gs.src.Subdir) if subdir == "/" { subdir = "." } @@ -535,7 +535,7 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out } else { pullref += ":" + pullref } - _, err = checkoutGit.Run(ctx, "fetch", "-u", "--depth=1", "origin", pullref) + _, err = checkoutGit.Run(ctx, "fetch", "-u", "--depth=1", "--", "origin", pullref) if err != nil { return nil, err } @@ -569,7 +569,8 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out return nil, errors.Wrapf(err, "failed to checkout remote %s", urlutil.RedactCredentials(gs.src.Remote)) } if subdir != "." { - d, err := os.Open(filepath.Join(cd, subdir)) + subdir = filepath.FromSlash(subdir) + d, err := openSubdirSafe(cd, subdir) if err != nil { return nil, errors.Wrapf(err, "failed to open subdir %v", subdir) } @@ -578,7 +579,7 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out d.Close() } }() - names, err := d.Readdirnames(0) + names, err := readdirnames(d) if err != nil { return nil, err } diff --git a/source/git/source_test.go b/source/git/source_test.go index 6712340a25d5..2da46d344f15 100644 --- a/source/git/source_test.go +++ b/source/git/source_test.go @@ -694,3 +694,42 @@ func logProgressStreams(ctx context.Context, t *testing.T) context.Context { }() return ctx } + +// TestValidateDirsOnly verifies that subdir path traversal and symlink escapes +// are blocked (CVE-2026-33748). +func TestValidateDirsOnly(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink test not applicable on windows") + } + t.Parallel() + root := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(root, "real", "nested"), 0o755)) + require.NoError(t, os.Symlink("/tmp", filepath.Join(root, "escape"))) + require.NoError(t, os.WriteFile(filepath.Join(root, "file.txt"), []byte("x"), 0o644)) + + tests := []struct { + subpath string + wantErr bool + }{ + {subpath: "real", wantErr: false}, + {subpath: "real/nested", wantErr: false}, + {subpath: "/real/nested", wantErr: false}, + {subpath: "escape", wantErr: true}, + {subpath: "file.txt", wantErr: true}, + {subpath: "nonexistent", wantErr: true}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.subpath, func(t *testing.T) { + t.Parallel() + f, err := openSubdirSafe(root, tt.subpath) + if tt.wantErr { + require.Error(t, err, "subpath %q should be rejected", tt.subpath) + } else { + require.NoError(t, err, "subpath %q should be accepted", tt.subpath) + f.Close() + } + }) + } +} diff --git a/source/git/source_unix.go b/source/git/source_unix.go index d6e15395b98e..04c3625a2099 100644 --- a/source/git/source_unix.go +++ b/source/git/source_unix.go @@ -5,14 +5,62 @@ package git import ( "context" + "os" "os/exec" + "path/filepath" "runtime" + "strings" "syscall" "time" + "github.com/pkg/errors" "golang.org/x/sys/unix" ) +// openSubdirSafe opens a subdirectory safely inside root. +// Linux/Unix version uses openat+O_NOFOLLOW per component to eliminate TOCTOU races. +// Returns a *os.File for the subdirectory. +func openSubdirSafe(root string, subpath string) (*os.File, error) { + dirfd, err := unix.Open(root, unix.O_DIRECTORY|unix.O_PATH, 0) + if err != nil { + return nil, errors.Wrapf(err, "failed to open root %q", root) + } + + rel := filepath.Clean(subpath) + rel = strings.TrimPrefix(rel, string(filepath.Separator)) + if rel == "" || rel == "." { + return os.NewFile(uintptr(dirfd), root), nil + } + + for _, part := range strings.Split(rel, string(filepath.Separator)) { + nextfd, err := unix.Openat( + dirfd, + part, + unix.O_NOFOLLOW|unix.O_DIRECTORY|unix.O_PATH, + 0, + ) + unix.Close(dirfd) + if err != nil { + return nil, errors.Wrapf(err, "subpath %q: failed to open component %q", subpath, part) + } + dirfd = nextfd + } + + return os.NewFile(uintptr(dirfd), filepath.Join(root, subpath)), nil +} + +// readdirnames returns the names of entries in the directory represented by f. +// On Unix, f may be an O_PATH fd; a readable fd is opened via Openat before reading. +func readdirnames(f *os.File) ([]string, error) { + readfd, err := unix.Openat(int(f.Fd()), ".", unix.O_RDONLY|unix.O_DIRECTORY, 0) + if err != nil { + return nil, errors.Wrap(err, "failed to open directory for reading") + } + d := os.NewFile(uintptr(readfd), f.Name()) + defer d.Close() + return d.Readdirnames(0) +} + func runWithStandardUmask(ctx context.Context, cmd *exec.Cmd) error { errCh := make(chan error) diff --git a/source/git/source_windows.go b/source/git/source_windows.go index 8c8a1d3dcf26..92754d2b5c95 100644 --- a/source/git/source_windows.go +++ b/source/git/source_windows.go @@ -5,9 +5,40 @@ package git import ( "context" + "os" "os/exec" + "path/filepath" + "strings" + + "github.com/pkg/errors" ) +// openSubdirSafe on Windows falls back to os.Lstat-based validation since +// unix.Openat is not available. +func openSubdirSafe(root string, subpath string) (*os.File, error) { + rel := filepath.Clean(subpath) + rel = strings.TrimPrefix(rel, string(filepath.Separator)) + if rel != "" && rel != "." { + p := "" + for _, part := range strings.Split(rel, string(filepath.Separator)) { + p = filepath.Join(p, part) + fi, err := os.Lstat(filepath.Join(root, p)) + if err != nil { + return nil, errors.Wrapf(err, "failed to lstat %q", p) + } + if !fi.IsDir() { + return nil, errors.Errorf("git subpath %q contains non-directory %q", subpath, p) + } + } + } + return os.Open(filepath.Join(root, subpath)) +} + +// readdirnames on Windows uses a normal readable fd returned by openSubdirSafe. +func readdirnames(f *os.File) ([]string, error) { + return f.Readdirnames(0) +} + func runWithStandardUmask(ctx context.Context, cmd *exec.Cmd) error { if err := cmd.Start(); err != nil { return err diff --git a/source/http/source.go b/source/http/source.go index 7c026c4ce17b..cda468cce086 100644 --- a/source/http/source.go +++ b/source/http/source.go @@ -15,7 +15,9 @@ import ( "strconv" "strings" "time" + "unicode" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/docker/docker/pkg/idtools" "github.com/moby/buildkit/cache" "github.com/moby/buildkit/session" @@ -330,7 +332,11 @@ func (hs *httpSourceHandler) save(ctx context.Context, resp *http.Response, s se if hs.src.Perm != 0 { perm = hs.src.Perm } - fp := filepath.Join(dir, getFileName(hs.src.URL, hs.src.Filename, resp)) + name := getFileName(hs.src.URL, hs.src.Filename, resp) + fp, err := securejoin.SecureJoin(dir, name) + if err != nil { + return nil, "", err + } f, err := os.OpenFile(fp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.FileMode(perm)) if err != nil { @@ -457,16 +463,30 @@ func (hs *httpSourceHandler) Snapshot(ctx context.Context, g session.Group) (cac return ref, nil } +func safeFileName(s string) string { + defaultName := "download" + name := filepath.Base(filepath.FromSlash(strings.TrimSpace(s))) + if name == "" || name == "." || name == ".." { + return defaultName + } + for _, r := range name { + if r == 0 || unicode.IsControl(r) { + return defaultName + } + } + return name +} + func getFileName(urlStr, manualFilename string, resp *http.Response) string { if manualFilename != "" { - return manualFilename + return safeFileName(manualFilename) } if resp != nil { if contentDisposition := resp.Header.Get("Content-Disposition"); contentDisposition != "" { if _, params, err := mime.ParseMediaType(contentDisposition); err == nil { if params["filename"] != "" && !strings.HasSuffix(params["filename"], "/") { if filename := filepath.Base(filepath.FromSlash(params["filename"])); filename != "" { - return filename + return safeFileName(filename) } } } @@ -475,10 +495,10 @@ func getFileName(urlStr, manualFilename string, resp *http.Response) string { u, err := url.Parse(urlStr) if err == nil { if base := path.Base(u.Path); base != "." && base != "/" { - return base + return safeFileName(base) } } - return "download" + return safeFileName("") } func searchHTTPURLDigest(ctx context.Context, store cache.MetadataStore, dgst digest.Digest) ([]cacheRefMetadata, error) { diff --git a/source/http/source_test.go b/source/http/source_test.go index 8b8e429b6b1e..0a063a5cc7ab 100644 --- a/source/http/source_test.go +++ b/source/http/source_test.go @@ -381,3 +381,48 @@ func newHTTPSource(t *testing.T) (source.Source, error) { CacheAccessor: cm, }) } + +func TestSafeFileName(t *testing.T) { + t.Parallel() + + type testCase struct { + name string + in string + want string + } + + tests := []testCase{ + {name: "simple", in: "foo", want: "foo"}, + {name: "simple_ext", in: "foo.txt", want: "foo.txt"}, + {name: "unicode_cjk", in: "資料.txt", want: "資料.txt"}, + {name: "unicode_cyrillic", in: "тест-файл", want: "тест-файл"}, + {name: "spaces_allowed", in: "name with spaces.txt", want: "name with spaces.txt"}, + {name: "trim_outer_whitespace", in: " foo.txt ", want: "foo.txt"}, + {name: "unix_path", in: "a/b/c.txt", want: "c.txt"}, + {name: "empty", in: "", want: "download"}, + {name: "dot", in: ".", want: "download"}, + {name: "dot_dot", in: "..", want: "download"}, + {name: "traversal_unix", in: "../", want: "download"}, + {name: "nul_byte", in: "a\x00b", want: "download"}, + {name: "control", in: "a\nb", want: "download"}, + } + if runtime.GOOS == "windows" { + tests = append(tests, + testCase{name: "windows_traversal", in: "..\\", want: "download"}, + testCase{name: "windows_path_basename", in: "a\\b\\c.txt", want: "c.txt"}, + ) + } else { + tests = append(tests, + testCase{name: "windows_traversal_literal", in: "..\\", want: "..\\"}, + testCase{name: "windows_path_literal", in: "a\\b\\c.txt", want: "a\\b\\c.txt"}, + ) + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, tt.want, safeFileName(tt.in)) + }) + } +} diff --git a/vendor/github.com/cyphar/filepath-securejoin/LICENSE b/vendor/github.com/cyphar/filepath-securejoin/LICENSE new file mode 100644 index 000000000000..bec842f294f7 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/LICENSE @@ -0,0 +1,28 @@ +Copyright (C) 2014-2015 Docker Inc & Go Authors. All rights reserved. +Copyright (C) 2017 SUSE LLC. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/vendor/github.com/cyphar/filepath-securejoin/README.md b/vendor/github.com/cyphar/filepath-securejoin/README.md new file mode 100644 index 000000000000..4eca0f235502 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/README.md @@ -0,0 +1,79 @@ +## `filepath-securejoin` ## + +[![Build Status](https://github.com/cyphar/filepath-securejoin/actions/workflows/ci.yml/badge.svg)](https://github.com/cyphar/filepath-securejoin/actions/workflows/ci.yml) + +An implementation of `SecureJoin`, a [candidate for inclusion in the Go +standard library][go#20126]. The purpose of this function is to be a "secure" +alternative to `filepath.Join`, and in particular it provides certain +guarantees that are not provided by `filepath.Join`. + +> **NOTE**: This code is *only* safe if you are not at risk of other processes +> modifying path components after you've used `SecureJoin`. If it is possible +> for a malicious process to modify path components of the resolved path, then +> you will be vulnerable to some fairly trivial TOCTOU race conditions. [There +> are some Linux kernel patches I'm working on which might allow for a better +> solution.][lwn-obeneath] +> +> In addition, with a slightly modified API it might be possible to use +> `O_PATH` and verify that the opened path is actually the resolved one -- but +> I have not done that yet. I might add it in the future as a helper function +> to help users verify the path (we can't just return `/proc/self/fd/` +> because that doesn't always work transparently for all users). + +This is the function prototype: + +```go +func SecureJoin(root, unsafePath string) (string, error) +``` + +This library **guarantees** the following: + +* If no error is set, the resulting string **must** be a child path of + `root` and will not contain any symlink path components (they will all be + expanded). + +* When expanding symlinks, all symlink path components **must** be resolved + relative to the provided root. In particular, this can be considered a + userspace implementation of how `chroot(2)` operates on file paths. Note that + these symlinks will **not** be expanded lexically (`filepath.Clean` is not + called on the input before processing). + +* Non-existent path components are unaffected by `SecureJoin` (similar to + `filepath.EvalSymlinks`'s semantics). + +* The returned path will always be `filepath.Clean`ed and thus not contain any + `..` components. + +A (trivial) implementation of this function on GNU/Linux systems could be done +with the following (note that this requires root privileges and is far more +opaque than the implementation in this library, and also requires that +`readlink` is inside the `root` path): + +```go +package securejoin + +import ( + "os/exec" + "path/filepath" +) + +func SecureJoin(root, unsafePath string) (string, error) { + unsafePath = string(filepath.Separator) + unsafePath + cmd := exec.Command("chroot", root, + "readlink", "--canonicalize-missing", "--no-newline", unsafePath) + output, err := cmd.CombinedOutput() + if err != nil { + return "", err + } + expanded := string(output) + return filepath.Join(root, expanded), nil +} +``` + +[lwn-obeneath]: https://lwn.net/Articles/767547/ +[go#20126]: https://github.com/golang/go/issues/20126 + +### License ### + +The license of this project is the same as Go, which is a BSD 3-clause license +available in the `LICENSE` file. diff --git a/vendor/github.com/cyphar/filepath-securejoin/VERSION b/vendor/github.com/cyphar/filepath-securejoin/VERSION new file mode 100644 index 000000000000..abd410582dea --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/VERSION @@ -0,0 +1 @@ +0.2.4 diff --git a/vendor/github.com/cyphar/filepath-securejoin/join.go b/vendor/github.com/cyphar/filepath-securejoin/join.go new file mode 100644 index 000000000000..aa32b85fb84c --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/join.go @@ -0,0 +1,125 @@ +// Copyright (C) 2014-2015 Docker Inc & Go Authors. All rights reserved. +// Copyright (C) 2017 SUSE LLC. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package securejoin is an implementation of the hopefully-soon-to-be-included +// SecureJoin helper that is meant to be part of the "path/filepath" package. +// The purpose of this project is to provide a PoC implementation to make the +// SecureJoin proposal (https://github.com/golang/go/issues/20126) more +// tangible. +package securejoin + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "strings" + "syscall" +) + +// IsNotExist tells you if err is an error that implies that either the path +// accessed does not exist (or path components don't exist). This is +// effectively a more broad version of os.IsNotExist. +func IsNotExist(err error) bool { + // Check that it's not actually an ENOTDIR, which in some cases is a more + // convoluted case of ENOENT (usually involving weird paths). + return errors.Is(err, os.ErrNotExist) || errors.Is(err, syscall.ENOTDIR) || errors.Is(err, syscall.ENOENT) +} + +// SecureJoinVFS joins the two given path components (similar to Join) except +// that the returned path is guaranteed to be scoped inside the provided root +// path (when evaluated). Any symbolic links in the path are evaluated with the +// given root treated as the root of the filesystem, similar to a chroot. The +// filesystem state is evaluated through the given VFS interface (if nil, the +// standard os.* family of functions are used). +// +// Note that the guarantees provided by this function only apply if the path +// components in the returned string are not modified (in other words are not +// replaced with symlinks on the filesystem) after this function has returned. +// Such a symlink race is necessarily out-of-scope of SecureJoin. +// +// Volume names in unsafePath are always discarded, regardless if they are +// provided via direct input or when evaluating symlinks. Therefore: +// +// "C:\Temp" + "D:\path\to\file.txt" results in "C:\Temp\path\to\file.txt" +func SecureJoinVFS(root, unsafePath string, vfs VFS) (string, error) { + // Use the os.* VFS implementation if none was specified. + if vfs == nil { + vfs = osVFS{} + } + + unsafePath = filepath.FromSlash(unsafePath) + var path bytes.Buffer + n := 0 + for unsafePath != "" { + if n > 255 { + return "", &os.PathError{Op: "SecureJoin", Path: root + string(filepath.Separator) + unsafePath, Err: syscall.ELOOP} + } + + if v := filepath.VolumeName(unsafePath); v != "" { + unsafePath = unsafePath[len(v):] + } + + // Next path component, p. + i := strings.IndexRune(unsafePath, filepath.Separator) + var p string + if i == -1 { + p, unsafePath = unsafePath, "" + } else { + p, unsafePath = unsafePath[:i], unsafePath[i+1:] + } + + // Create a cleaned path, using the lexical semantics of /../a, to + // create a "scoped" path component which can safely be joined to fullP + // for evaluation. At this point, path.String() doesn't contain any + // symlink components. + cleanP := filepath.Clean(string(filepath.Separator) + path.String() + p) + if cleanP == string(filepath.Separator) { + path.Reset() + continue + } + fullP := filepath.Clean(root + cleanP) + + // Figure out whether the path is a symlink. + fi, err := vfs.Lstat(fullP) + if err != nil && !IsNotExist(err) { + return "", err + } + // Treat non-existent path components the same as non-symlinks (we + // can't do any better here). + if IsNotExist(err) || fi.Mode()&os.ModeSymlink == 0 { + path.WriteString(p) + path.WriteRune(filepath.Separator) + continue + } + + // Only increment when we actually dereference a link. + n++ + + // It's a symlink, expand it by prepending it to the yet-unparsed path. + dest, err := vfs.Readlink(fullP) + if err != nil { + return "", err + } + // Absolute symlinks reset any work we've already done. + if filepath.IsAbs(dest) { + path.Reset() + } + unsafePath = dest + string(filepath.Separator) + unsafePath + } + + // We have to clean path.String() here because it may contain '..' + // components that are entirely lexical, but would be misleading otherwise. + // And finally do a final clean to ensure that root is also lexically + // clean. + fullP := filepath.Clean(string(filepath.Separator) + path.String()) + return filepath.Clean(root + fullP), nil +} + +// SecureJoin is a wrapper around SecureJoinVFS that just uses the os.* library +// of functions as the VFS. If in doubt, use this function over SecureJoinVFS. +func SecureJoin(root, unsafePath string) (string, error) { + return SecureJoinVFS(root, unsafePath, nil) +} diff --git a/vendor/github.com/cyphar/filepath-securejoin/vfs.go b/vendor/github.com/cyphar/filepath-securejoin/vfs.go new file mode 100644 index 000000000000..a82a5eae11eb --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/vfs.go @@ -0,0 +1,41 @@ +// Copyright (C) 2017 SUSE LLC. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package securejoin + +import "os" + +// In future this should be moved into a separate package, because now there +// are several projects (umoci and go-mtree) that are using this sort of +// interface. + +// VFS is the minimal interface necessary to use SecureJoinVFS. A nil VFS is +// equivalent to using the standard os.* family of functions. This is mainly +// used for the purposes of mock testing, but also can be used to otherwise use +// SecureJoin with VFS-like system. +type VFS interface { + // Lstat returns a FileInfo describing the named file. If the file is a + // symbolic link, the returned FileInfo describes the symbolic link. Lstat + // makes no attempt to follow the link. These semantics are identical to + // os.Lstat. + Lstat(name string) (os.FileInfo, error) + + // Readlink returns the destination of the named symbolic link. These + // semantics are identical to os.Readlink. + Readlink(name string) (string, error) +} + +// osVFS is the "nil" VFS, in that it just passes everything through to the os +// module. +type osVFS struct{} + +// Lstat returns a FileInfo describing the named file. If the file is a +// symbolic link, the returned FileInfo describes the symbolic link. Lstat +// makes no attempt to follow the link. These semantics are identical to +// os.Lstat. +func (o osVFS) Lstat(name string) (os.FileInfo, error) { return os.Lstat(name) } + +// Readlink returns the destination of the named symbolic link. These +// semantics are identical to os.Readlink. +func (o osVFS) Readlink(name string) (string, error) { return os.Readlink(name) } diff --git a/vendor/modules.txt b/vendor/modules.txt index b511cb64ce82..7108e34ec8d1 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -425,6 +425,9 @@ github.com/coreos/go-systemd/v22/daemon # github.com/cpuguy83/go-md2man/v2 v2.0.2 ## explicit; go 1.11 github.com/cpuguy83/go-md2man/v2/md2man +# github.com/cyphar/filepath-securejoin v0.2.4 +## explicit; go 1.13 +github.com/cyphar/filepath-securejoin # github.com/davecgh/go-spew v1.1.1 ## explicit github.com/davecgh/go-spew/spew