From 240231b0679e3be819809b9853e51f73cf3fef19 Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Wed, 8 Apr 2026 01:17:42 +0200 Subject: [PATCH] fix: validate paths in DownloadActionOutputs and symlink targets in DownloadOutputs Two path traversal variants were found in the download path: 1. DownloadActionOutputs passes dir.Path from the ActionResult proto directly to os.RemoveAll via filepath.Join, without validating it through getAbsPath. A malicious remote server could set dir.Path to "../../important_data" to delete arbitrary directories on the client. 2. DownloadOutputs creates symlinks without validating that the symlink target stays within the output directory. A malicious remote server could set SymlinkTarget to "/etc/shadow" or "../../../../etc/passwd" to create symlinks pointing outside the output directory, enabling arbitrary file read/write through symlink following. Both fixes use the same getAbsPath validation introduced in the previous path traversal fix, ensuring all server-controlled paths are validated before use. --- go/pkg/client/cas_download.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/go/pkg/client/cas_download.go b/go/pkg/client/cas_download.go index c584ed8b..4005491b 100644 --- a/go/pkg/client/cas_download.go +++ b/go/pkg/client/cas_download.go @@ -9,6 +9,7 @@ import ( "path/filepath" "sort" "strconv" + "strings" "sync" "time" @@ -147,7 +148,21 @@ func (c *Client) DownloadOutputs(ctx context.Context, outs map[string]*TreeOutpu } } for _, out := range symlinks { - if err := os.Symlink(out.SymlinkTarget, filepath.Join(outDir, out.Path)); err != nil { + symlinkPath, err := getAbsPath(outDir, out.Path) + if err != nil { + return fullStats, err + } + // Validate that the symlink target resolves within outDir to prevent + // symlink-based path traversal (e.g. target = "/etc/shadow"). + target := out.SymlinkTarget + if !filepath.IsAbs(target) { + target = filepath.Join(filepath.Dir(symlinkPath), target) + } + target = filepath.Clean(target) + if target != outDir && !strings.HasPrefix(target, outDir+string(filepath.Separator)) { + return fullStats, fmt.Errorf("symlink target %q resolves to %q which is outside output directory %q", out.SymlinkTarget, target, outDir) + } + if err := os.Symlink(out.SymlinkTarget, symlinkPath); err != nil { return fullStats, err } } @@ -522,7 +537,11 @@ func (c *Client) DownloadActionOutputs(ctx context.Context, resPb *repb.ActionRe } // Remove the existing output directories before downloading. for _, dir := range resPb.OutputDirectories { - if err := os.RemoveAll(filepath.Join(outDir, dir.Path)); err != nil { + absPath, err := getAbsPath(outDir, dir.Path) + if err != nil { + return nil, err + } + if err := os.RemoveAll(absPath); err != nil { return nil, err } }