@@ -15,18 +15,20 @@ import ( |
| 15 | 15 | type recordingRunner struct { |
| 16 | 16 | name string |
| 17 | 17 | args []string |
| 18 | + env []string |
| 18 | 19 | err error |
| 19 | 20 | } |
| 20 | 21 | |
| 21 | | -func (r *recordingRunner) Run(_ context.Context, name string, args []string, _, _ io.Writer) error { |
| 22 | +func (r *recordingRunner) Run(_ context.Context, name string, args []string, env []string, _, _ io.Writer) error { |
| 22 | 23 | r.name = name |
| 23 | 24 | r.args = append([]string{}, args...) |
| 25 | + r.env = append([]string{}, env...) |
| 24 | 26 | return r.err |
| 25 | 27 | } |
| 26 | 28 | |
| 27 | 29 | type loggingRunner struct{} |
| 28 | 30 | |
| 29 | | -func (loggingRunner) Run(_ context.Context, _ string, _ []string, stdout, stderr io.Writer) error { |
| 31 | +func (loggingRunner) Run(_ context.Context, _ string, _ []string, _ []string, stdout, stderr io.Writer) error { |
| 30 | 32 | _, _ = stdout.Write([]byte("hello ")) |
| 31 | 33 | _, _ = stderr.Write([]byte("world\n")) |
| 32 | 34 | return nil |
@@ -34,7 +36,7 @@ func (loggingRunner) Run(_ context.Context, _ string, _ []string, stdout, stderr |
| 34 | 36 | |
| 35 | 37 | type secretLoggingRunner struct{} |
| 36 | 38 | |
| 37 | | -func (secretLoggingRunner) Run(_ context.Context, _ string, _ []string, stdout, _ io.Writer) error { |
| 39 | +func (secretLoggingRunner) Run(_ context.Context, _ string, _ []string, _ []string, stdout, _ io.Writer) error { |
| 38 | 40 | _, _ = stdout.Write([]byte("hun")) |
| 39 | 41 | _, _ = stdout.Write([]byte("ter2\n")) |
| 40 | 42 | return nil |
@@ -80,7 +82,7 @@ func TestDockerExecute_BuildsResourceCappedRunCommand(t *testing.T) { |
| 80 | 82 | "--user", "65534:65534", |
| 81 | 83 | "--workdir=/workspace/subdir", |
| 82 | 84 | "--mount", rec.args[23], |
| 83 | | - "-e", "A=job", "-e", "B=step", |
| 85 | + "--env", "A", "--env", "B", |
| 84 | 86 | "runner-image", "bash", "-c", "echo hi", |
| 85 | 87 | } |
| 86 | 88 | if rec.name != "podman" { |
@@ -92,6 +94,9 @@ func TestDockerExecute_BuildsResourceCappedRunCommand(t *testing.T) { |
| 92 | 94 | if !strings.HasPrefix(rec.args[23], "type=bind,src=") || !strings.HasSuffix(rec.args[23], ",dst=/workspace,rw") { |
| 93 | 95 | t.Fatalf("workspace mount arg: %q", rec.args[23]) |
| 94 | 96 | } |
| 97 | + if wantEnv := []string{"A=job", "B=step"}; !reflect.DeepEqual(rec.env, wantEnv) { |
| 98 | + t.Fatalf("env:\ngot %#v\nwant %#v", rec.env, wantEnv) |
| 99 | + } |
| 95 | 100 | } |
| 96 | 101 | |
| 97 | 102 | func TestDockerExecute_RendersTaintedExpressionsThroughInputEnv(t *testing.T) { |
@@ -121,9 +126,51 @@ func TestDockerExecute_RendersTaintedExpressionsThroughInputEnv(t *testing.T) { |
| 121 | 126 | if got := rec.args[len(rec.args)-1]; got != `echo "${SHITHUB_INPUT_0}"` { |
| 122 | 127 | t.Fatalf("rendered command: %q", got) |
| 123 | 128 | } |
| 124 | | - if !containsArg(rec.args, "SHITHUB_INPUT_0="+malicious) { |
| 129 | + if !containsFlagValue(rec.args, "--env", "SHITHUB_INPUT_0") { |
| 125 | 130 | t.Fatalf("input binding missing from args: %#v", rec.args) |
| 126 | 131 | } |
| 132 | + if containsSubstring(rec.args, malicious) { |
| 133 | + t.Fatalf("tainted input leaked into argv: %#v", rec.args) |
| 134 | + } |
| 135 | + if !containsEnv(rec.env, "SHITHUB_INPUT_0="+malicious) { |
| 136 | + t.Fatalf("input binding missing from process env: %#v", rec.env) |
| 137 | + } |
| 138 | +} |
| 139 | + |
| 140 | +func TestDockerExecute_RendersSecretsThroughEnvWithoutArgvLeak(t *testing.T) { |
| 141 | + t.Parallel() |
| 142 | + rec := &recordingRunner{} |
| 143 | + d := NewDocker(DockerConfig{ |
| 144 | + DefaultImage: "runner-image", |
| 145 | + Network: "bridge", |
| 146 | + Memory: "2g", |
| 147 | + CPUs: "2", |
| 148 | + Runner: rec, |
| 149 | + }) |
| 150 | + const secret = "hunter2" |
| 151 | + if _, err := d.Execute(t.Context(), Job{ |
| 152 | + ID: 1, |
| 153 | + RunID: 2, |
| 154 | + Secrets: map[string]string{"TOKEN": secret}, |
| 155 | + WorkspaceDir: t.TempDir(), |
| 156 | + Steps: []Step{{ |
| 157 | + Run: `printf '%s\n' "${{ secrets.TOKEN }}"`, |
| 158 | + }}, |
| 159 | + }); err != nil { |
| 160 | + t.Fatalf("Execute: %v", err) |
| 161 | + } |
| 162 | + if got := rec.args[len(rec.args)-1]; got != `printf '%s\n' "${SHITHUB_INPUT_0}"` { |
| 163 | + t.Fatalf("rendered command: %q", got) |
| 164 | + } |
| 165 | + if !containsFlagValue(rec.args, "--env", "SHITHUB_INPUT_0") { |
| 166 | + t.Fatalf("secret binding missing from args: %#v", rec.args) |
| 167 | + } |
| 168 | + if containsSubstring(rec.args, secret) { |
| 169 | + t.Fatalf("secret leaked into argv: %#v", rec.args) |
| 170 | + } |
| 171 | + if !containsEnv(rec.env, "SHITHUB_INPUT_0="+secret) { |
| 172 | + t.Fatalf("secret binding missing from process env: %#v", rec.env) |
| 173 | + } |
| 127 | 174 | } |
| 128 | 175 | |
| 129 | 176 | func TestDockerExecute_RootRequiresExplicitPermission(t *testing.T) { |
@@ -135,7 +182,7 @@ func TestDockerExecute_RootRequiresExplicitPermission(t *testing.T) { |
| 135 | 182 | }{ |
| 136 | 183 | {name: "default", permissions: `{}`, wantUser: "65534:65534"}, |
| 137 | 184 | {name: "write-all-does-not-root", permissions: `{"mode":"write-all"}`, wantUser: "65534:65534"}, |
| 138 | | - {name: "explicit-root", permissions: `{"per":{"shithub-runner-root":"write"}}`, wantUser: "0:0"}, |
| 185 | + {name: "explicit-root-disabled-by-default", permissions: `{"per":{"shithub-runner-root":"write"}}`, wantUser: "65534:65534"}, |
| 139 | 186 | } { |
| 140 | 187 | t.Run(tc.name, func(t *testing.T) { |
| 141 | 188 | t.Parallel() |
@@ -162,6 +209,30 @@ func TestDockerExecute_RootRequiresExplicitPermission(t *testing.T) { |
| 162 | 209 | } |
| 163 | 210 | } |
| 164 | 211 | |
| 212 | +func TestDockerExecute_AllowRootEnablesExplicitRootPermission(t *testing.T) { |
| 213 | + t.Parallel() |
| 214 | + rec := &recordingRunner{} |
| 215 | + d := NewDocker(DockerConfig{ |
| 216 | + DefaultImage: "runner-image", |
| 217 | + Network: "bridge", |
| 218 | + Memory: "2g", |
| 219 | + CPUs: "2", |
| 220 | + AllowRoot: true, |
| 221 | + Runner: rec, |
| 222 | + }) |
| 223 | + if _, err := d.Execute(t.Context(), Job{ |
| 224 | + ID: 1, |
| 225 | + Permissions: []byte(`{"per":{"shithub-runner-root":"write"}}`), |
| 226 | + WorkspaceDir: t.TempDir(), |
| 227 | + Steps: []Step{{Run: "id -u"}}, |
| 228 | + }); err != nil { |
| 229 | + t.Fatalf("Execute: %v", err) |
| 230 | + } |
| 231 | + if got := argAfter(rec.args, "--user"); got != "0:0" { |
| 232 | + t.Fatalf("--user: got %q want %q in %#v", got, "0:0", rec.args) |
| 233 | + } |
| 234 | +} |
| 235 | + |
| 165 | 236 | func TestDockerExecute_AddsConfiguredDNSServers(t *testing.T) { |
| 166 | 237 | t.Parallel() |
| 167 | 238 | rec := &recordingRunner{} |
@@ -365,6 +436,33 @@ func containsArg(args []string, want string) bool { |
| 365 | 436 | return false |
| 366 | 437 | } |
| 367 | 438 | |
| 439 | +func containsFlagValue(args []string, flag, value string) bool { |
| 440 | + for i, arg := range args { |
| 441 | + if arg == flag && i+1 < len(args) && args[i+1] == value { |
| 442 | + return true |
| 443 | + } |
| 444 | + } |
| 445 | + return false |
| 446 | +} |
| 447 | + |
| 448 | +func containsSubstring(args []string, substr string) bool { |
| 449 | + for _, arg := range args { |
| 450 | + if strings.Contains(arg, substr) { |
| 451 | + return true |
| 452 | + } |
| 453 | + } |
| 454 | + return false |
| 455 | +} |
| 456 | + |
| 457 | +func containsEnv(env []string, want string) bool { |
| 458 | + for _, item := range env { |
| 459 | + if item == want { |
| 460 | + return true |
| 461 | + } |
| 462 | + } |
| 463 | + return false |
| 464 | +} |
| 465 | + |
| 368 | 466 | func argAfter(args []string, flag string) string { |
| 369 | 467 | for i, arg := range args { |
| 370 | 468 | if arg == flag && i+1 < len(args) { |