diff --git a/internal/batches/workspace/bind_workspace.go b/internal/batches/workspace/bind_workspace.go index c76a30ba60..3068ec2240 100644 --- a/internal/batches/workspace/bind_workspace.go +++ b/internal/batches/workspace/bind_workspace.go @@ -190,6 +190,15 @@ func unzip(ctx context.Context, zipFile, dest string) error { } defer r.Close() + for _, f := range r.File { + // ZIP paths use forward slashes on every platform. Reject backslashes + // rather than letting Windows reinterpret a filename from a Unix + // repository as a directory hierarchy. + if strings.ContainsRune(f.Name, '\\') { + return fmt.Errorf("%q: illegal file path", f.Name) + } + } + outputBase := filepath.Clean(dest) + string(os.PathSeparator) for _, f := range r.File { diff --git a/internal/batches/workspace/bind_workspace_test.go b/internal/batches/workspace/bind_workspace_test.go index c120eb0c11..80ae000c03 100644 --- a/internal/batches/workspace/bind_workspace_test.go +++ b/internal/batches/workspace/bind_workspace_test.go @@ -166,6 +166,54 @@ func TestUnzipRejectsGitMetadata(t *testing.T) { } } +func TestUnzipRejectsUnsafeArchivePaths(t *testing.T) { + tests := []string{ + `.git\config`, + `hooks\pre-commit`, + } + + for _, name := range tests { + t.Run(name, func(t *testing.T) { + archivePath := zipUpFiles(t, t.TempDir(), map[string]string{name: "malicious"}) + dest := t.TempDir() + + if err := unzip(context.Background(), archivePath, dest); err == nil { + t.Fatal("expected unsafe archive path to be rejected") + } + + entries, err := os.ReadDir(dest) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("archive was partially extracted: %v", entries) + } + }) + } +} + +func TestUnzipAllowsSafeControlPaths(t *testing.T) { + files := map[string]string{ + ".git_config": "config", + "hooks_pre-commit": "hook", + } + archivePath := zipUpFiles(t, t.TempDir(), files) + dest := t.TempDir() + + if err := unzip(context.Background(), archivePath, dest); err != nil { + t.Fatal(err) + } + for name, want := range files { + have, err := os.ReadFile(filepath.Join(dest, name)) + if err != nil { + t.Fatal(err) + } + if string(have) != want { + t.Errorf("%s: got %q, want %q", name, have, want) + } + } +} + func TestDockerBindWorkspace_ApplyDiff(t *testing.T) { // Create a zip file for all the other tests to use. fakeFilesTmpDir := t.TempDir() diff --git a/internal/batches/workspace/git.go b/internal/batches/workspace/git.go index 20c20d1b07..8f59a282b6 100644 --- a/internal/batches/workspace/git.go +++ b/internal/batches/workspace/git.go @@ -9,6 +9,9 @@ import ( ) func runGitCmd(ctx context.Context, dir string, args ...string) ([]byte, error) { + // Repository contents are untrusted. Keep hooks disabled even if a command + // encounters an attacker-controlled local Git configuration. + args = append([]string{"-c", "core.hooksPath=/dev/null"}, args...) cmd := exec.CommandContext(ctx, "git", args...) cmd.Env = []string{ // Don't use the system wide git config. diff --git a/internal/batches/workspace/git_test.go b/internal/batches/workspace/git_test.go new file mode 100644 index 0000000000..f4cd887ca1 --- /dev/null +++ b/internal/batches/workspace/git_test.go @@ -0,0 +1,46 @@ +package workspace + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +func TestRunGitCmdDisablesHooks(t *testing.T) { + dir := t.TempDir() + ctx := context.Background() + + if _, err := runGitCmd(ctx, dir, "init", "--quiet"); err != nil { + t.Fatal(err) + } + config, err := os.OpenFile(filepath.Join(dir, ".git", "config"), os.O_APPEND|os.O_WRONLY, 0) + if err != nil { + t.Fatal(err) + } + if _, err := config.WriteString("[core]\n\thooksPath = hooks\n"); err != nil { + t.Fatal(err) + } + if err := config.Close(); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(dir, "hooks"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "hooks", "pre-commit"), []byte("#!/bin/sh\necho hook-ran > hook-ran\n"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("test\n"), 0644); err != nil { + t.Fatal(err) + } + + if _, err := runGitCmd(ctx, dir, "add", "README.md"); err != nil { + t.Fatal(err) + } + if _, err := runGitCmd(ctx, dir, "commit", "--quiet", "-m", "test"); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(dir, "hook-ran")); !os.IsNotExist(err) { + t.Fatalf("pre-commit hook ran: %v", err) + } +}