diff --git a/internal/batches/executor/executor_test.go b/internal/batches/executor/executor_test.go index 03f25e08a8..b507302f9c 100644 --- 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 index 97e952be71..47b5715887 100644 --- 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 index 71678ff898..89b3cd56c8 100644 --- 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 index 031788ef9a..a21de45903 100644 --- 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 index 14afc7e002..c550cafd6d 100644 --- 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) {