Prevent downloaded files escape from outDir (#634)

Add additional file path check to make sure all the downloaded files will be under the outDir.

Any attempt of escaping the outDir will fail the download.

This is a breaking change though, note that the TestDownloadActionOutputs test has been updated to avoid break.

Bug: b/460031498
Test: Unit test updated.

Author: ywmei-brt1

go/pkg/client/cas_download.go

@@ -85,7 +85,10 @@ func (c *Client) DownloadOutputs(ctx context.Context, outs map[string]*TreeOutpu
 	downloads := make(map[digest.Digest]*TreeOutput)
 	fullStats := &MovedBytesMetadata{}
 	for _, out := range outs {
-		path := filepath.Join(outDir, out.Path)
+		path, err := getAbsPath(outDir, out.Path)
+		if err != nil {
+			return fullStats, err
+		}
 		if out.IsEmptyDirectory {
 			if err := os.MkdirAll(path, c.DirMode); err != nil {
 				return fullStats, err
@@ -784,12 +787,13 @@ func (c *Client) downloadBatch(ctx context.Context, batch []digest.Digest, reqs
 			if r.output.IsExecutable {
 				perm = c.ExecutableMode
 			}
+			path, err := getAbsPath(r.outDir, r.output.Path)
+			if err == nil {
+				err = os.WriteFile(path, bi.Data, perm)
+			}
 			// bytesMoved will be zero for error cases.
 			// We only report it to the first client to prevent double accounting.
-			r.wait <- &downloadResponse{
-				stats: stats,
-				err:   os.WriteFile(filepath.Join(r.outDir, r.output.Path), bi.Data, perm),
-			}
+			r.wait <- &downloadResponse{stats, err}
 			if i == 0 {
 				// Prevent races by not writing to the original stats.
 				newStats := &MovedBytesMetadata{}
@@ -816,7 +820,10 @@ func (c *Client) downloadSingle(ctx context.Context, dg digest.Digest, reqs map[
 	}
 	r := rs[0]
 	rs = rs[1:]
-	path := filepath.Join(r.outDir, r.output.Path)
+	path, err := getAbsPath(r.outDir, r.output.Path)
+	if err != nil {
+		return err
+	}
 	contextmd.Infof(ctx, log.Level(3), "Downloading single file with digest %s to %s", r.output.Digest, path)
 	stats, err := c.ReadBlobToFile(ctx, r.output.Digest, path)
 	if err != nil {
@@ -833,6 +840,9 @@ func (c *Client) downloadSingle(ctx context.Context, dg digest.Digest, reqs map[
 		if cp.output.IsExecutable {
 			perm = c.ExecutableMode
 		}
+		if _, err := getAbsPath(cp.outDir, cp.output.Path); err != nil {
+			return err
+		}
 		if err := copyFile(r.outDir, cp.outDir, r.output.Path, cp.output.Path, perm); err != nil {
 			return err
 		}
@@ -905,7 +915,11 @@ func (c *Client) downloadNonUnified(ctx context.Context, outDir string, outputs
 					if out.IsExecutable {
 						perm = c.ExecutableMode
 					}
-					if err := os.WriteFile(filepath.Join(outDir, out.Path), bi.Data, perm); err != nil {
+					path, err := getAbsPath(outDir, out.Path)
+					if err != nil {
+						return err
+					}
+					if err := os.WriteFile(path, bi.Data, perm); err != nil {
 						return err
 					}
 					statsMu.Lock()
@@ -918,7 +932,10 @@ func (c *Client) downloadNonUnified(ctx context.Context, outDir string, outputs
 				}
 			} else {
 				out := outputs[batch[0]]
-				path := filepath.Join(outDir, out.Path)
+				path, err := getAbsPath(outDir, out.Path)
+				if err != nil {
+					return err
+				}
 				contextmd.Infof(ctx, log.Level(3), "Downloading single file with digest %s to %s", out.Digest, path)
 				stats, err := c.ReadBlobToFile(ctx, out.Digest, path)
 				if err != nil {

go/pkg/client/cas_test.go

@@ -1106,7 +1106,7 @@ func TestDownloadActionOutputs(t *testing.T) {
 	treeADigest := fake.Put(treeABlob)
 	ar := &repb.ActionResult{
 		OutputFiles: []*repb.OutputFile{
-			&repb.OutputFile{Path: "../foo", Digest: fooDigest.ToProto()}},
+			&repb.OutputFile{Path: "foo/../foo", Digest: fooDigest.ToProto()}},
 		OutputFileSymlinks: []*repb.OutputSymlink{
 			&repb.OutputSymlink{Path: "x/bar", Target: "../dir/a/bar"}},
 		OutputDirectorySymlinks: []*repb.OutputSymlink{
@@ -1172,7 +1172,7 @@ func TestDownloadActionOutputs(t *testing.T) {
 			contents: []byte("bar"),
 		},
 		{
-			path:       "foo",
+			path:       "wd/foo",
 			contents:   []byte("foo"),
 			fileDigest: &fooDigest,
 		},
@@ -2050,3 +2050,149 @@ func TestBatchDownloadBlobsBrokenCompression(t *testing.T) {
 		})
 	}
 }
+
+type escapedPathTestEnv struct {
+	ctx           context.Context
+	c             *client.Client
+	cache         filemetadata.Cache
+	outDir        string
+	escapedDigest digest.Digest
+	safeDigest    digest.Digest
+	escapedPath   string
+	safePath      string
+}
+
+func createEscapedPathTestEnv(t *testing.T) escapedPathTestEnv {
+	t.Helper()
+	ctx := context.Background()
+	e, cleanup := fakes.NewTestEnv(t)
+	t.Cleanup(cleanup)
+	fake := e.Server.CAS
+	c := e.Client.GrpcClient
+	cache := filemetadata.NewSingleFlightCache()
+
+	escapedContent := []byte("escaped vulnerability test")
+	escapedDigest := fake.Put(escapedContent)
+	escapedPath := "../escaped_file.txt"
+
+	safeContent := []byte("safe content")
+	safeDigest := fake.Put(safeContent)
+	safePath := "safe_file.txt"
+
+	outDir := filepath.Join(t.TempDir(), "real_out_dir")
+	if err := os.MkdirAll(outDir, 0755); err != nil {
+		t.Fatalf("Failed to create outDir: %v", err)
+	}
+
+	// Verify that the hardcoded escapedPath indeed attempts to escape.
+	targetPath := filepath.Clean(filepath.Join(outDir, escapedPath))
+	if strings.HasPrefix(targetPath, outDir) {
+		t.Fatalf("Failed to construct an escaped path: %s is still within %s", targetPath, outDir)
+	}
+
+	return escapedPathTestEnv{
+		ctx:           ctx,
+		c:             c,
+		cache:         cache,
+		outDir:        outDir,
+		escapedDigest: escapedDigest,
+		safeDigest:    safeDigest,
+		escapedPath:   escapedPath,
+		safePath:      safePath,
+	}
+}
+
+func TestEscapeDownloadOutputs(t *testing.T) {
+	t.Parallel()
+	env := createEscapedPathTestEnv(t)
+
+	outputs := map[string]*client.TreeOutput{
+		env.escapedPath: {
+			Digest: env.escapedDigest,
+			Path:   env.escapedPath,
+		},
+	}
+
+	if _, err := env.c.DownloadOutputs(env.ctx, outputs, env.outDir, env.cache); err == nil {
+		t.Errorf("DownloadOutputs didn't fail when output path %v try to escape from outDir %v", env.escapedPath, env.outDir)
+	}
+}
+
+func TestEscapeDownloadFiles_Single(t *testing.T) {
+	t.Parallel()
+	env := createEscapedPathTestEnv(t)
+
+	outputs := map[digest.Digest]*client.TreeOutput{
+		env.escapedDigest: {
+			Digest: env.escapedDigest,
+			Path:   env.escapedPath,
+		},
+	}
+
+	if _, err := env.c.DownloadFiles(env.ctx, env.outDir, outputs); err == nil {
+		t.Errorf("DownloadFiles didn't fail when output path try to escape from outDir %v", env.outDir)
+	}
+}
+
+func TestEscapeDownloadFiles_Batch(t *testing.T) {
+	t.Parallel()
+	env := createEscapedPathTestEnv(t)
+
+	outputs := map[digest.Digest]*client.TreeOutput{
+		env.escapedDigest: {
+			Digest: env.escapedDigest,
+			Path:   env.escapedPath,
+		},
+		env.safeDigest: {
+			Digest: env.safeDigest,
+			Path:   env.safePath,
+		},
+	}
+	client.UseBatchOps(true).Apply(env.c)
+	client.MaxBatchDigests(2).Apply(env.c)
+	env.c.RunBackgroundTasks(env.ctx)
+
+	if _, err := env.c.DownloadFiles(env.ctx, env.outDir, outputs); err == nil {
+		t.Errorf("DownloadFiles didn't fail when output path try to escape from outDir %v", env.outDir)
+	}
+}
+
+func TestEscapeDownloadNonUnified_Single(t *testing.T) {
+	t.Parallel()
+	env := createEscapedPathTestEnv(t)
+
+	outputs := map[digest.Digest]*client.TreeOutput{
+		env.escapedDigest: {
+			Digest: env.escapedDigest,
+			Path:   env.escapedPath,
+		},
+	}
+	client.UnifiedDownloads(false).Apply(env.c)
+	if _, err := env.c.DownloadFiles(env.ctx, env.outDir, outputs); err == nil {
+		t.Errorf("DownloadFiles didn't fail when output path try to escape from outDir %v", env.outDir)
+	}
+}
+
+func TestEscapeDownloadNonUnified_Batch(t *testing.T) {
+	t.Parallel()
+	env := createEscapedPathTestEnv(t)
+
+	outputs := map[digest.Digest]*client.TreeOutput{
+		env.escapedDigest: {
+			Digest: env.escapedDigest,
+			Path:   env.escapedPath,
+		},
+		env.safeDigest: {
+			Digest: env.safeDigest,
+			Path:   env.safePath,
+		},
+	}
+	client.UnifiedDownloads(false).Apply(env.c)
+	client.UseBatchOps(true).Apply(env.c)
+	client.MaxBatchDigests(2).Apply(env.c)
+	env.c.RunBackgroundTasks(env.ctx)
+
+	if _, err := env.c.DownloadFiles(env.ctx, env.outDir, outputs); err == nil {
+		t.Errorf("DownloadFiles didn't fail when output path try to escape from outDir %v", env.outDir)
+	}
+}

go/pkg/client/tree.go

@@ -130,6 +130,18 @@ func getRelPath(base, path string) (string, error) {
 	return rel, nil
 }
 
+func getAbsPath(base, relPath string) (string, error) {
+	base = filepath.Clean(base)
+	if filepath.IsAbs(relPath) {
+		return "", fmt.Errorf("input path %q must be relative to the base directory", relPath)
+	}
+	res := filepath.Clean(filepath.Join(base, relPath))
+	if !strings.HasPrefix(res, base) {
+		return "", fmt.Errorf("path %v is not under %v", relPath, base)
+	}
+	return res, nil
+}
+
 // getTargetRelPath returns two versions of targetPath, the first is relative to execRoot
 // and the second is relative to the directory of symlinkRelPath.
 // symlinkRelPath must be relative to execRoot.