fix(executor): write the step script through the RUNNER_TEMP root, exclusively, instead of an absolute path (GO-002)
- commit
- abd0a011481a6edf1c9f214a2bc70b3056d32bdd
- author
- brice · · 2026-10-12 20:10 UTC
- parent
- f9fc798
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +56 -22
M internal/executor/exec_step.go +1 -2
| @@ -135,9 +135,8 @@ func (j *job) printHeader(s *stepRun, in scriptIn) (ShellPlan, error) { | |||
| 135 | 135 | func (j *job) scriptCommand(plan ShellPlan, in scriptIn) (dir string, argv []string, err error) { | |
| 136 | 136 | j.mu.Lock() | |
| 137 | 137 | workspace := j.state.GitHub("workspace") | |
| 138 | - | temp := j.state.Layout().Temp | |
| 139 | 138 | j.mu.Unlock() | |
| 140 | - | sc, err := WriteScript(temp, plan, in.script) | |
| 139 | + | sc, err := WriteScript(j.tempRoot, plan, in.script) | |
| 141 | 140 | if err != nil { | |
| 142 | 141 | return "", nil, err | |
| 143 | 142 | } | |
| @@ -135,9 +135,8 @@ func (j *job) printHeader(s *stepRun, in scriptIn) (ShellPlan, error) { | |||||
| 135 | func (j *job) scriptCommand(plan ShellPlan, in scriptIn) (dir string, argv []string, err error) { | 135 | func (j *job) scriptCommand(plan ShellPlan, in scriptIn) (dir string, argv []string, err error) { | ||
| 136 | j.mu.Lock() | 136 | j.mu.Lock() | ||
| 137 | workspace := j.state.GitHub("workspace") | 137 | workspace := j.state.GitHub("workspace") | ||
| 138 | - | temp := j.state.Layout().Temp | |||
| 139 | j.mu.Unlock() | 138 | j.mu.Unlock() | ||
| 140 | - | sc, err := WriteScript(temp, plan, in.script) | 139 | + | sc, err := WriteScript(j.tempRoot, plan, in.script) |
| 141 | if err != nil { | 140 | if err != nil { | ||
| 142 | return "", nil, err | 141 | return "", nil, err | ||
| 143 | } | 142 | } | ||
M internal/executor/shell.go +10 -17
| @@ -7,7 +7,6 @@ import ( | |||
| 7 | 7 | "os" | |
| 8 | 8 | "path/filepath" | |
| 9 | 9 | "strings" | |
| 10 | - | "syscall" | |
| 11 | 10 | ||
| 12 | 11 | "github.com/google/uuid" | |
| 13 | 12 | ) | |
| @@ -285,27 +284,21 @@ type Script struct { | |||
| 285 | 284 | Argv []string | |
| 286 | 285 | } | |
| 287 | 286 | ||
| 288 | - | // WriteScript writes the script of a step to tempDir under a random name with | |
| 289 | - | // the extension of the shell, and builds the command line | |
| 290 | - | // (ScriptHandler.RunAsync :247-285). The file has no BOM and is not | |
| 291 | - | // executable: the shell reads it. | |
| 292 | - | func WriteScript(tempDir string, p ShellPlan, contents string) (Script, error) { | |
| 293 | - | path := filepath.Join(tempDir, uuid.NewString()+p.Extension()) | |
| 287 | + | // WriteScript writes the script of a step to the directory of root (RUNNER_TEMP) | |
| 288 | + | // under a random name with the extension of the shell, and builds the command | |
| 289 | + | // line (ScriptHandler.RunAsync :247-285). The file has no BOM and is not | |
| 290 | + | // executable: the shell reads it. The directory belongs to the user of the steps, | |
| 291 | + | // so the file is created through root, exclusively and without following links | |
| 292 | + | // (see createExcl). | |
| 293 | + | func WriteScript(root *os.Root, p ShellPlan, contents string) (Script, error) { | |
| 294 | + | name := uuid.NewString() + p.Extension() | |
| 295 | + | path := filepath.Join(root.Name(), name) | |
| 294 | 296 | args, err := FormatArguments(p.ArgFormat, path) | |
| 295 | 297 | if err != nil { | |
| 296 | 298 | return Script{}, err | |
| 297 | 299 | } | |
| 298 | 300 | contents = FixUpScriptContents(p.Command, contents) | |
| 299 | - | // The name is random and the file is created exclusively without following | |
| 300 | - | // links: the directory belongs to the user of the steps. | |
| 301 | - | fh, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL|syscall.O_NOFOLLOW, 0o644) | |
| 302 | - | if err == nil { | |
| 303 | - | _, err = fh.WriteString(contents) | |
| 304 | - | if cerr := fh.Close(); err == nil { | |
| 305 | - | err = cerr | |
| 306 | - | } | |
| 307 | - | } | |
| 308 | - | if err != nil { | |
| 301 | + | if err := createExcl(root, name, []byte(contents), 0o644, nil); err != nil { | |
| 309 | 302 | return Script{}, fmt.Errorf("write script: %w", err) | |
| 310 | 303 | } | |
| 311 | 304 | return Script{Path: path, Argv: append([]string{p.CommandPath}, ParseArguments(args)...)}, nil | |
| @@ -7,7 +7,6 @@ import ( | |||||
| 7 | "os" | 7 | "os" | ||
| 8 | "path/filepath" | 8 | "path/filepath" | ||
| 9 | "strings" | 9 | "strings" | ||
| 10 | - | "syscall" | |||
| 11 | 10 | ||||
| 12 | "github.com/google/uuid" | 11 | "github.com/google/uuid" | ||
| 13 | ) | 12 | ) | ||
| @@ -285,27 +284,21 @@ type Script struct { | |||||
| 285 | Argv []string | 284 | Argv []string | ||
| 286 | } | 285 | } | ||
| 287 | 286 | ||||
| 288 | - | // WriteScript writes the script of a step to tempDir under a random name with | 287 | + | // WriteScript writes the script of a step to the directory of root (RUNNER_TEMP) |
| 289 | - | // the extension of the shell, and builds the command line | 288 | + | // under a random name with the extension of the shell, and builds the command |
| 290 | - | // (ScriptHandler.RunAsync :247-285). The file has no BOM and is not | 289 | + | // line (ScriptHandler.RunAsync :247-285). The file has no BOM and is not |
| 291 | - | // executable: the shell reads it. | 290 | + | // executable: the shell reads it. The directory belongs to the user of the steps, |
| 292 | - | func WriteScript(tempDir string, p ShellPlan, contents string) (Script, error) { | 291 | + | // so the file is created through root, exclusively and without following links |
| 293 | - | path := filepath.Join(tempDir, uuid.NewString()+p.Extension()) | 292 | + | // (see createExcl). |
| 293 | + | func WriteScript(root *os.Root, p ShellPlan, contents string) (Script, error) { | |||
| 294 | + | name := uuid.NewString() + p.Extension() | |||
| 295 | + | path := filepath.Join(root.Name(), name) | |||
| 294 | args, err := FormatArguments(p.ArgFormat, path) | 296 | args, err := FormatArguments(p.ArgFormat, path) | ||
| 295 | if err != nil { | 297 | if err != nil { | ||
| 296 | return Script{}, err | 298 | return Script{}, err | ||
| 297 | } | 299 | } | ||
| 298 | contents = FixUpScriptContents(p.Command, contents) | 300 | contents = FixUpScriptContents(p.Command, contents) | ||
| 299 | - | // The name is random and the file is created exclusively without following | 301 | + | if err := createExcl(root, name, []byte(contents), 0o644, nil); err != nil { |
| 300 | - | // links: the directory belongs to the user of the steps. | |||
| 301 | - | fh, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL|syscall.O_NOFOLLOW, 0o644) | |||
| 302 | - | if err == nil { | |||
| 303 | - | _, err = fh.WriteString(contents) | |||
| 304 | - | if cerr := fh.Close(); err == nil { | |||
| 305 | - | err = cerr | |||
| 306 | - | } | |||
| 307 | - | } | |||
| 308 | - | if err != nil { | |||
| 309 | return Script{}, fmt.Errorf("write script: %w", err) | 302 | return Script{}, fmt.Errorf("write script: %w", err) | ||
| 310 | } | 303 | } | ||
| 311 | return Script{Path: path, Argv: append([]string{p.CommandPath}, ParseArguments(args)...)}, nil | 304 | return Script{Path: path, Argv: append([]string{p.CommandPath}, ParseArguments(args)...)}, nil | ||
M internal/executor/shell_test.go +45 -3
| @@ -182,6 +182,11 @@ func TestParseArguments(t *testing.T) { | |||
| 182 | 182 | ||
| 183 | 183 | func TestWriteScript(t *testing.T) { | |
| 184 | 184 | dir := t.TempDir() | |
| 185 | + | root, err := os.OpenRoot(dir) | |
| 186 | + | if err != nil { | |
| 187 | + | t.Fatal(err) | |
| 188 | + | } | |
| 189 | + | defer func() { _ = root.Close() }() | |
| 185 | 190 | bin := fakeBin(t, "bash", "sh", "python") | |
| 186 | 191 | l := Lookup{Path: bin} | |
| 187 | 192 | for _, tc := range []struct { | |
| @@ -196,7 +201,7 @@ func TestWriteScript(t *testing.T) { | |||
| 196 | 201 | if err != nil { | |
| 197 | 202 | t.Fatal(err) | |
| 198 | 203 | } | |
| 199 | - | s, err := WriteScript(dir, plan, "echo hi\n") | |
| 204 | + | s, err := WriteScript(root, plan, "echo hi\n") | |
| 200 | 205 | if err != nil { | |
| 201 | 206 | t.Fatal(err) | |
| 202 | 207 | } | |
| @@ -214,12 +219,12 @@ func TestWriteScript(t *testing.T) { | |||
| 214 | 219 | } | |
| 215 | 220 | // A custom shell without an extension in the table: no extension. | |
| 216 | 221 | plan, _ := PlanShell("python {0}", l) | |
| 217 | - | if s, _ := WriteScript(dir, plan, "x"); filepath.Ext(s.Path) != ".py" { | |
| 222 | + | if s, _ := WriteScript(root, plan, "x"); filepath.Ext(s.Path) != ".py" { | |
| 218 | 223 | t.Fatalf("python {0}: %s", s.Path) | |
| 219 | 224 | } | |
| 220 | 225 | bin2 := fakeBin(t, "perl") | |
| 221 | 226 | plan, _ = PlanShell("perl {0}", Lookup{Path: bin2}) | |
| 222 | - | if s, _ := WriteScript(dir, plan, "x"); filepath.Ext(s.Path) != "" { | |
| 227 | + | if s, _ := WriteScript(root, plan, "x"); filepath.Ext(s.Path) != "" { | |
| 223 | 228 | t.Fatalf("perl: %s", s.Path) | |
| 224 | 229 | } | |
| 225 | 230 | } | |
| @@ -237,3 +242,40 @@ func TestResolveWorkingDirectory(t *testing.T) { | |||
| 237 | 242 | } | |
| 238 | 243 | } | |
| 239 | 244 | } | |
| 245 | + | ||
| 246 | + | // The script goes through the os.Root of RUNNER_TEMP: a directory swapped for a | |
| 247 | + | // link to elsewhere after the root was opened does not redirect it (GO-002). | |
| 248 | + | func TestWriteScriptStaysInTheOpenedDirectory(t *testing.T) { | |
| 249 | + | parent := t.TempDir() | |
| 250 | + | dir := filepath.Join(parent, "tmp") | |
| 251 | + | if err := os.Mkdir(dir, 0o755); err != nil { | |
| 252 | + | t.Fatal(err) | |
| 253 | + | } | |
| 254 | + | root, err := os.OpenRoot(dir) | |
| 255 | + | if err != nil { | |
| 256 | + | t.Fatal(err) | |
| 257 | + | } | |
| 258 | + | defer func() { _ = root.Close() }() | |
| 259 | + | outside := filepath.Join(parent, "outside") | |
| 260 | + | if err := os.Mkdir(outside, 0o755); err != nil { | |
| 261 | + | t.Fatal(err) | |
| 262 | + | } | |
| 263 | + | // The step swaps the directory for a link to another one. | |
| 264 | + | if err := os.Rename(dir, dir+".moved"); err != nil { | |
| 265 | + | t.Fatal(err) | |
| 266 | + | } | |
| 267 | + | if err := os.Symlink(outside, dir); err != nil { | |
| 268 | + | t.Fatal(err) | |
| 269 | + | } | |
| 270 | + | bin := fakeBin(t, "bash", "sh") | |
| 271 | + | plan, _ := PlanShell("", Lookup{Path: bin}) | |
| 272 | + | if _, err := WriteScript(root, plan, "echo hi\n"); err != nil { | |
| 273 | + | t.Fatal(err) | |
| 274 | + | } | |
| 275 | + | if ents, _ := os.ReadDir(outside); len(ents) != 0 { | |
| 276 | + | t.Fatalf("the script was written through the swapped link: %v", ents) | |
| 277 | + | } | |
| 278 | + | if ents, _ := os.ReadDir(dir + ".moved"); len(ents) != 1 { | |
| 279 | + | t.Fatalf("the script is not in the directory that root was opened on: %v", ents) | |
| 280 | + | } | |
| 281 | + | } | |
| @@ -182,6 +182,11 @@ func TestParseArguments(t *testing.T) { | |||||
| 182 | 182 | ||||
| 183 | func TestWriteScript(t *testing.T) { | 183 | func TestWriteScript(t *testing.T) { | ||
| 184 | dir := t.TempDir() | 184 | dir := t.TempDir() | ||
| 185 | + | root, err := os.OpenRoot(dir) | |||
| 186 | + | if err != nil { | |||
| 187 | + | t.Fatal(err) | |||
| 188 | + | } | |||
| 189 | + | defer func() { _ = root.Close() }() | |||
| 185 | bin := fakeBin(t, "bash", "sh", "python") | 190 | bin := fakeBin(t, "bash", "sh", "python") | ||
| 186 | l := Lookup{Path: bin} | 191 | l := Lookup{Path: bin} | ||
| 187 | for _, tc := range []struct { | 192 | for _, tc := range []struct { | ||
| @@ -196,7 +201,7 @@ func TestWriteScript(t *testing.T) { | |||||
| 196 | if err != nil { | 201 | if err != nil { | ||
| 197 | t.Fatal(err) | 202 | t.Fatal(err) | ||
| 198 | } | 203 | } | ||
| 199 | - | s, err := WriteScript(dir, plan, "echo hi\n") | 204 | + | s, err := WriteScript(root, plan, "echo hi\n") |
| 200 | if err != nil { | 205 | if err != nil { | ||
| 201 | t.Fatal(err) | 206 | t.Fatal(err) | ||
| 202 | } | 207 | } | ||
| @@ -214,12 +219,12 @@ func TestWriteScript(t *testing.T) { | |||||
| 214 | } | 219 | } | ||
| 215 | // A custom shell without an extension in the table: no extension. | 220 | // A custom shell without an extension in the table: no extension. | ||
| 216 | plan, _ := PlanShell("python {0}", l) | 221 | plan, _ := PlanShell("python {0}", l) | ||
| 217 | - | if s, _ := WriteScript(dir, plan, "x"); filepath.Ext(s.Path) != ".py" { | 222 | + | if s, _ := WriteScript(root, plan, "x"); filepath.Ext(s.Path) != ".py" { |
| 218 | t.Fatalf("python {0}: %s", s.Path) | 223 | t.Fatalf("python {0}: %s", s.Path) | ||
| 219 | } | 224 | } | ||
| 220 | bin2 := fakeBin(t, "perl") | 225 | bin2 := fakeBin(t, "perl") | ||
| 221 | plan, _ = PlanShell("perl {0}", Lookup{Path: bin2}) | 226 | plan, _ = PlanShell("perl {0}", Lookup{Path: bin2}) | ||
| 222 | - | if s, _ := WriteScript(dir, plan, "x"); filepath.Ext(s.Path) != "" { | 227 | + | if s, _ := WriteScript(root, plan, "x"); filepath.Ext(s.Path) != "" { |
| 223 | t.Fatalf("perl: %s", s.Path) | 228 | t.Fatalf("perl: %s", s.Path) | ||
| 224 | } | 229 | } | ||
| 225 | } | 230 | } | ||
| @@ -237,3 +242,40 @@ func TestResolveWorkingDirectory(t *testing.T) { | |||||
| 237 | } | 242 | } | ||
| 238 | } | 243 | } | ||
| 239 | } | 244 | } | ||
| 245 | + | ||||
| 246 | + | // The script goes through the os.Root of RUNNER_TEMP: a directory swapped for a | |||
| 247 | + | // link to elsewhere after the root was opened does not redirect it (GO-002). | |||
| 248 | + | func TestWriteScriptStaysInTheOpenedDirectory(t *testing.T) { | |||
| 249 | + | parent := t.TempDir() | |||
| 250 | + | dir := filepath.Join(parent, "tmp") | |||
| 251 | + | if err := os.Mkdir(dir, 0o755); err != nil { | |||
| 252 | + | t.Fatal(err) | |||
| 253 | + | } | |||
| 254 | + | root, err := os.OpenRoot(dir) | |||
| 255 | + | if err != nil { | |||
| 256 | + | t.Fatal(err) | |||
| 257 | + | } | |||
| 258 | + | defer func() { _ = root.Close() }() | |||
| 259 | + | outside := filepath.Join(parent, "outside") | |||
| 260 | + | if err := os.Mkdir(outside, 0o755); err != nil { | |||
| 261 | + | t.Fatal(err) | |||
| 262 | + | } | |||
| 263 | + | // The step swaps the directory for a link to another one. | |||
| 264 | + | if err := os.Rename(dir, dir+".moved"); err != nil { | |||
| 265 | + | t.Fatal(err) | |||
| 266 | + | } | |||
| 267 | + | if err := os.Symlink(outside, dir); err != nil { | |||
| 268 | + | t.Fatal(err) | |||
| 269 | + | } | |||
| 270 | + | bin := fakeBin(t, "bash", "sh") | |||
| 271 | + | plan, _ := PlanShell("", Lookup{Path: bin}) | |||
| 272 | + | if _, err := WriteScript(root, plan, "echo hi\n"); err != nil { | |||
| 273 | + | t.Fatal(err) | |||
| 274 | + | } | |||
| 275 | + | if ents, _ := os.ReadDir(outside); len(ents) != 0 { | |||
| 276 | + | t.Fatalf("the script was written through the swapped link: %v", ents) | |||
| 277 | + | } | |||
| 278 | + | if ents, _ := os.ReadDir(dir + ".moved"); len(ents) != 1 { | |||
| 279 | + | t.Fatalf("the script is not in the directory that root was opened on: %v", ents) | |||
| 280 | + | } | |||
| 281 | + | } | |||