security: use os.Root API for file path logic (#1279)

cbrnrd · web-flow · commit 5e4aec26c4e6 · 2026-03-27T18:19:22.000Z
diff --git a/internal/batches/executor/executor_test.go b/internal/batches/executor/executor_test.go
@@ -50,7 +50,7 @@ func TestExecutor_Integration(t *testing.T) {
 
 	// create a temp directory with a simple shell file
 	tempDir := t.TempDir()
-	mountScript := fmt.Sprintf("%s/sample.sh", tempDir)
+	mountScript := filepath.Join(tempDir, "sample.sh")
 	err := os.WriteFile(mountScript, []byte(`echo -e "foobar\n" >> README.md`), 0777)
 	require.NoError(t, err)
 
@@ -80,7 +80,8 @@ func TestExecutor_Integration(t *testing.T) {
 
 		wantCacheCount int
 
-		failFast bool
+		failFast         bool
+		workingDirectory string
 	}{
 		{
 			name: "success",
@@ -362,7 +363,7 @@ func TestExecutor_Integration(t *testing.T) {
 			steps: []batcheslib.Step{
 				{
 					Run:   mountScript,
-					Mount: []batcheslib.Mount{{Path: mountScript, Mountpoint: mountScript}},
+					Mount: []batcheslib.Mount{{Path: "sample.sh", Mountpoint: mountScript}},
 				},
 			},
 			tasks: []*Task{
@@ -373,8 +374,9 @@ func TestExecutor_Integration(t *testing.T) {
 					rootPath: []string{"README.md"},
 				},
 			},
-			wantFinished:   1,
-			wantCacheCount: 1,
+			wantFinished:     1,
+			wantCacheCount:   1,
+			workingDirectory: tempDir,
 		},
 	}
 
@@ -426,10 +428,11 @@ func TestExecutor_Integration(t *testing.T) {
 				Logger:              mock.LogNoOpManager{},
 				EnsureImage:         imageMapEnsurer(images),
 
-				TempDir:     testTempDir,
-				Parallelism: runtime.GOMAXPROCS(parallelism),
-				Timeout:     tc.executorTimeout,
-				FailFast:    tc.failFast,
+				TempDir:          testTempDir,
+				Parallelism:      runtime.GOMAXPROCS(parallelism),
+				Timeout:          tc.executorTimeout,
+				FailFast:         tc.failFast,
+				WorkingDirectory: tc.workingDirectory,
 			}
 
 			if opts.Timeout == 0 {
diff --git a/internal/batches/executor/run_steps.go b/internal/batches/executor/run_steps.go
@@ -610,26 +610,38 @@ func createCidFile(ctx context.Context, tempDir string, repoSlug string) (string
 }
 
 func getAbsoluteMountPath(batchSpecDir string, mountPath string) (string, error) {
+	// Use OpenRoot to prevent path traversal and symlink attacks via mount paths
+	root, err := os.OpenRoot(batchSpecDir)
+	if err != nil {
+		return "", errors.Wrap(err, "opening batch spec directory")
+	}
+	defer root.Close()
+
 	p := mountPath
-	if !filepath.IsAbs(p) {
-		// Try to build the absolute path since Docker will only mount absolute paths
-		p = filepath.Join(batchSpecDir, p)
+	if filepath.IsAbs(p) {
+		rel, err := filepath.Rel(batchSpecDir, p)
+		if err != nil || strings.HasPrefix(rel, "..") {
+			return "", errors.Newf("mount path %s is not in the same directory or subdirectory as the batch spec", mountPath)
+		}
+		p = rel
 	}
-	pathInfo, err := os.Stat(p)
+
+	pathInfo, err := root.Stat(p)
 	if os.IsNotExist(err) {
-		return "", errors.Newf("mount path %s does not exist", p)
+		return "", errors.Newf("mount path %s does not exist", mountPath)
 	} else if err != nil {
-		return "", errors.Wrap(err, "mount path validation")
-	}
-	if !strings.HasPrefix(p, batchSpecDir) {
 		return "", errors.Newf("mount path %s is not in the same directory or subdirectory as the batch spec", mountPath)
 	}
+
+	// Build the absolute path for Docker bind mount.
+	absPath := filepath.Join(batchSpecDir, p)
+
 	// Mounting a directory on Docker must end with the separator. So, append the file separator to make
 	// users' lives easier.
-	if pathInfo.IsDir() && !strings.HasSuffix(p, string(filepath.Separator)) {
-		p += string(filepath.Separator)
+	if pathInfo.IsDir() && !strings.HasSuffix(absPath, string(filepath.Separator)) {
+		absPath += string(filepath.Separator)
 	}
-	return p, nil
+	return absPath, nil
 }
 
 type stepFailedErr struct {
diff --git a/internal/batches/service/remote.go b/internal/batches/service/remote.go
@@ -129,31 +129,43 @@ func (svc *Service) UploadBatchSpecWorkspaceFiles(ctx context.Context, workingDi
 }
 
 func getFilePaths(workingDir, filePath string) ([]string, error) {
+	root, err := os.OpenRoot(workingDir)
+	if err != nil {
+		return nil, errors.Wrap(err, "opening working directory")
+	}
+	defer root.Close()
+	return getFilePathsInRoot(root, filePath)
+}
+
+func getFilePathsInRoot(root *os.Root, filePath string) ([]string, error) {
+	// Clean the path to resolve any ".." segments before accessing the root.
+	filePath = filepath.Clean(filePath)
+
 	var filePaths []string
-	actualFilePath := filepath.Join(workingDir, filePath)
-	info, err := os.Stat(actualFilePath)
+	info, err := root.Stat(filePath)
 	if err != nil {
 		return nil, err
 	}
 
 	if info.IsDir() {
-		dir, err := os.ReadDir(actualFilePath)
+		dir, err := root.Open(filePath)
+		if err != nil {
+			return nil, err
+		}
+		entries, err := dir.ReadDir(-1)
+		dir.Close()
 		if err != nil {
 			return nil, err
 		}
-		for _, dirEntry := range dir {
-			paths, err := getFilePaths(workingDir, filepath.Join(filePath, dirEntry.Name()))
+		for _, dirEntry := range entries {
+			paths, err := getFilePathsInRoot(root, filepath.Join(filePath, dirEntry.Name()))
 			if err != nil {
 				return nil, err
 			}
 			filePaths = append(filePaths, paths...)
 		}
 	} else {
-		relPath, err := filepath.Rel(workingDir, actualFilePath)
-		if err != nil {
-			return nil, err
-		}
-		filePaths = append(filePaths, relPath)
+		filePaths = append(filePaths, filePath)
 	}
 	return filePaths, nil
 }
@@ -201,7 +213,13 @@ func (svc *Service) uploadFile(ctx context.Context, workingDir, filePath, batchS
 const maxFileSize = 10 << 20 // 10MB
 
 func createFormFile(w *multipart.Writer, workingDir string, mountPath string) error {
-	f, err := os.Open(filepath.Join(workingDir, mountPath))
+	root, err := os.OpenRoot(workingDir)
+	if err != nil {
+		return errors.Wrap(err, "opening working directory")
+	}
+	defer root.Close()
+
+	f, err := root.Open(mountPath)
 	if err != nil {
 		return err
 	}
diff --git a/internal/batches/service/service.go b/internal/batches/service/service.go
@@ -486,19 +486,39 @@ func (svc *Service) ParseBatchSpec(dir string, data []byte) (*batcheslib.BatchSp
 }
 
 func validateMount(batchSpecDir string, spec *batcheslib.BatchSpec) error {
+	// Check if any step has mounts before opening the root directory.
+	hasMounts := false
+	for _, step := range spec.Steps {
+		if len(step.Mount) > 0 {
+			hasMounts = true
+			break
+		}
+	}
+	if !hasMounts {
+		return nil
+	}
+
+	root, err := os.OpenRoot(batchSpecDir)
+	if err != nil {
+		return errors.Wrap(err, "opening batch spec directory")
+	}
+	defer root.Close()
+
 	for i, step := range spec.Steps {
 		for _, mount := range step.Mount {
-			if !filepath.IsAbs(mount.Path) {
-				// Try to build the absolute path since Docker will only mount absolute paths
-				mount.Path = filepath.Join(batchSpecDir, mount.Path)
+			mountPath := mount.Path
+			if filepath.IsAbs(mountPath) {
+				// Convert absolute paths to relative paths within the root.
+				rel, err := filepath.Rel(batchSpecDir, mountPath)
+				if err != nil || strings.HasPrefix(rel, "..") {
+					return errors.Newf("step %d mount path is not in the same directory or subdirectory as the batch spec", i+1)
+				}
+				mountPath = rel
 			}
-			_, err := os.Stat(mount.Path)
+			_, err := root.Stat(mountPath)
 			if os.IsNotExist(err) {
 				return errors.Newf("step %d mount path %s does not exist", i+1, mount.Path)
 			} else if err != nil {
-				return errors.Wrapf(err, "step %d mount path validation", i+1)
-			}
-			if !strings.HasPrefix(mount.Path, batchSpecDir) {
 				return errors.Newf("step %d mount path is not in the same directory or subdirectory as the batch spec", i+1)
 			}
 		}
diff --git a/internal/batches/service/service_test.go b/internal/batches/service/service_test.go
@@ -230,6 +230,17 @@ func TestService_ParseBatchSpec(t *testing.T) {
 	_, err = os.Create(filepath.Join(tempDir, "another.sh"))
 	require.NoError(t, err)
 
+	// Create a sibling directory whose name shares a prefix with tempDir (e.g. /tmp/specdir-123 vs /tmp/specdir-123-outside)
+	prefixBypassDir := tempDir + "-outside"
+	require.NoError(t, os.MkdirAll(prefixBypassDir, 0o755))
+	t.Cleanup(func() { os.RemoveAll(prefixBypassDir) })
+	secretFile := filepath.Join(prefixBypassDir, "secret.txt")
+	require.NoError(t, os.WriteFile(secretFile, []byte("SECRET"), 0o644))
+
+	// Create a symlink inside tempDir that points outside it
+	symlinkPath := filepath.Join(tempDir, "leak")
+	require.NoError(t, os.Symlink(secretFile, symlinkPath))
+
 	tests := []struct {
 		name         string
 		batchSpecDir string
@@ -541,6 +552,69 @@ changesetTemplate:
 `, tempOutsideDir),
 			expectedErr: errors.New("handling mount: step 1 mount path is not in the same directory or subdirectory as the batch spec"),
 		},
+		{
+			name:         "mount path prefix confusion bypass",
+			batchSpecDir: tempDir,
+			rawSpec: fmt.Sprintf(`
+name: test-spec
+description: A test spec
+steps:
+  - run: cat /tmp/mounted
+    container: alpine:3
+    mount:
+      - path: %s
+        mountpoint: /tmp/mounted
+changesetTemplate:
+  title: Test Mount
+  body: Test a mounted path
+  branch: test
+  commit:
+    message: Test
+`, secretFile),
+			expectedErr: errors.New("handling mount: step 1 mount path is not in the same directory or subdirectory as the batch spec"),
+		},
+		{
+			name:         "mount path symlink escape",
+			batchSpecDir: tempDir,
+			rawSpec: `
+name: test-spec
+description: A test spec
+steps:
+  - run: cat /tmp/mounted
+    container: alpine:3
+    mount:
+      - path: ./leak
+        mountpoint: /tmp/mounted
+changesetTemplate:
+  title: Test Mount
+  body: Test a mounted path
+  branch: test
+  commit:
+    message: Test
+`,
+			expectedErr: errors.New("handling mount: step 1 mount path is not in the same directory or subdirectory as the batch spec"),
+		},
+		{
+			name:         "mount path dot-dot traversal",
+			batchSpecDir: tempDir,
+			rawSpec: `
+name: test-spec
+description: A test spec
+steps:
+  - run: cat /tmp/mounted
+    container: alpine:3
+    mount:
+      - path: ../../../etc/passwd
+        mountpoint: /tmp/mounted
+changesetTemplate:
+  title: Test Mount
+  body: Test a mounted path
+  branch: test
+  commit:
+    message: Test
+`,
+			expectedErr: errors.New("handling mount: step 1 mount path is not in the same directory or subdirectory as the batch spec"),
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
