fix(executor): never follow links in the directory of the steps: file commands, event.json and scripts go through an os.Root with O_NOFOLLOW
- commit
- 8c002656827abaacc08c770ccc06a2ab71524ac7
- author
- brice · · 2026-10-12 19:24 UTC
- parent
- 366ab07
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
6 files · +340 -56
M internal/executor/exec_step.go +28 -9
| @@ -5,7 +5,6 @@ package executor | |||
| 5 | 5 | import ( | |
| 6 | 6 | "errors" | |
| 7 | 7 | "fmt" | |
| 8 | - | "os" | |
| 9 | 8 | "path/filepath" | |
| 10 | 9 | "strings" | |
| 11 | 10 | ) | |
| @@ -36,14 +35,8 @@ func (j *job) execute(s *stepRun, stepCancel <-chan struct{}) (result stepResult | |||
| 36 | 35 | j.state.SetGitHub("action_repository", "") | |
| 37 | 36 | j.state.SetGitHub("action_ref", "") | |
| 38 | 37 | if ev := j.state.EventJSON(); ev != "" { | |
| 39 | - | dir := filepath.Join(j.state.Layout().Temp, "_github_workflow") | |
| 40 | - | if err := os.MkdirAll(dir, 0o755); err == nil { | |
| 41 | - | j.own(dir) | |
| 42 | - | p := filepath.Join(dir, "event.json") | |
| 43 | - | if err := os.WriteFile(p, []byte(ev), 0o644); err == nil { | |
| 44 | - | j.own(p) | |
| 45 | - | j.state.SetGitHub("event_path", p) | |
| 46 | - | } | |
| 38 | + | if p, err := j.writeEvent(ev); err == nil { | |
| 39 | + | j.state.SetGitHub("event_path", p) | |
| 47 | 40 | } | |
| 48 | 41 | } | |
| 49 | 42 | } | |
| @@ -183,3 +176,29 @@ func (j *job) execute(s *stepRun, stepCancel <-chan struct{}) (result stepResult | |||
| 183 | 176 | } | |
| 184 | 177 | return resSucceeded | |
| 185 | 178 | } | |
| 179 | + | ||
| 180 | + | // writeEvent writes the webhook payload to RUNNER_TEMP/_github_workflow/event.json | |
| 181 | + | // (ExecutionContext.WriteWebhookPayload) and returns its path. The directory | |
| 182 | + | // is the user's: the file is replaced, never followed (see FileCommands). | |
| 183 | + | func (j *job) writeEvent(payload string) (string, error) { | |
| 184 | + | const dir, name = "_github_workflow", "event.json" | |
| 185 | + | if err := j.tempRoot.MkdirAll(dir, 0o755); err != nil { | |
| 186 | + | return "", err | |
| 187 | + | } | |
| 188 | + | if c := j.cfg.Credential; c != nil { | |
| 189 | + | _ = j.tempRoot.Lchown(dir, int(c.Uid), int(c.Gid)) | |
| 190 | + | } | |
| 191 | + | sub, err := j.tempRoot.OpenRoot(dir) | |
| 192 | + | if err != nil { | |
| 193 | + | return "", err | |
| 194 | + | } | |
| 195 | + | defer func() { _ = sub.Close() }() | |
| 196 | + | var owner *[2]int | |
| 197 | + | if c := j.cfg.Credential; c != nil { | |
| 198 | + | owner = &[2]int{int(c.Uid), int(c.Gid)} | |
| 199 | + | } | |
| 200 | + | if err := createExcl(sub, name, []byte(payload), 0o644, owner); err != nil { | |
| 201 | + | return "", err | |
| 202 | + | } | |
| 203 | + | return filepath.Join(j.state.Layout().Temp, dir, name), nil | |
| 204 | + | } | |
| @@ -5,7 +5,6 @@ package executor | |||||
| 5 | import ( | 5 | import ( | ||
| 6 | "errors" | 6 | "errors" | ||
| 7 | "fmt" | 7 | "fmt" | ||
| 8 | - | "os" | |||
| 9 | "path/filepath" | 8 | "path/filepath" | ||
| 10 | "strings" | 9 | "strings" | ||
| 11 | ) | 10 | ) | ||
| @@ -36,14 +35,8 @@ func (j *job) execute(s *stepRun, stepCancel <-chan struct{}) (result stepResult | |||||
| 36 | j.state.SetGitHub("action_repository", "") | 35 | j.state.SetGitHub("action_repository", "") | ||
| 37 | j.state.SetGitHub("action_ref", "") | 36 | j.state.SetGitHub("action_ref", "") | ||
| 38 | if ev := j.state.EventJSON(); ev != "" { | 37 | if ev := j.state.EventJSON(); ev != "" { | ||
| 39 | - | dir := filepath.Join(j.state.Layout().Temp, "_github_workflow") | 38 | + | if p, err := j.writeEvent(ev); err == nil { |
| 40 | - | if err := os.MkdirAll(dir, 0o755); err == nil { | 39 | + | j.state.SetGitHub("event_path", p) |
| 41 | - | j.own(dir) | |||
| 42 | - | p := filepath.Join(dir, "event.json") | |||
| 43 | - | if err := os.WriteFile(p, []byte(ev), 0o644); err == nil { | |||
| 44 | - | j.own(p) | |||
| 45 | - | j.state.SetGitHub("event_path", p) | |||
| 46 | - | } | |||
| 47 | } | 40 | } | ||
| 48 | } | 41 | } | ||
| 49 | } | 42 | } | ||
| @@ -183,3 +176,29 @@ func (j *job) execute(s *stepRun, stepCancel <-chan struct{}) (result stepResult | |||||
| 183 | } | 176 | } | ||
| 184 | return resSucceeded | 177 | return resSucceeded | ||
| 185 | } | 178 | } | ||
| 179 | + | ||||
| 180 | + | // writeEvent writes the webhook payload to RUNNER_TEMP/_github_workflow/event.json | |||
| 181 | + | // (ExecutionContext.WriteWebhookPayload) and returns its path. The directory | |||
| 182 | + | // is the user's: the file is replaced, never followed (see FileCommands). | |||
| 183 | + | func (j *job) writeEvent(payload string) (string, error) { | |||
| 184 | + | const dir, name = "_github_workflow", "event.json" | |||
| 185 | + | if err := j.tempRoot.MkdirAll(dir, 0o755); err != nil { | |||
| 186 | + | return "", err | |||
| 187 | + | } | |||
| 188 | + | if c := j.cfg.Credential; c != nil { | |||
| 189 | + | _ = j.tempRoot.Lchown(dir, int(c.Uid), int(c.Gid)) | |||
| 190 | + | } | |||
| 191 | + | sub, err := j.tempRoot.OpenRoot(dir) | |||
| 192 | + | if err != nil { | |||
| 193 | + | return "", err | |||
| 194 | + | } | |||
| 195 | + | defer func() { _ = sub.Close() }() | |||
| 196 | + | var owner *[2]int | |||
| 197 | + | if c := j.cfg.Credential; c != nil { | |||
| 198 | + | owner = &[2]int{int(c.Uid), int(c.Gid)} | |||
| 199 | + | } | |||
| 200 | + | if err := createExcl(sub, name, []byte(payload), 0o644, owner); err != nil { | |||
| 201 | + | return "", err | |||
| 202 | + | } | |||
| 203 | + | return filepath.Join(j.state.Layout().Temp, dir, name), nil | |||
| 204 | + | } | |||
M internal/executor/filecmd.go +126 -39
| @@ -9,11 +9,14 @@ package executor | |||
| 9 | 9 | ||
| 10 | 10 | import ( | |
| 11 | 11 | "bytes" | |
| 12 | + | "errors" | |
| 12 | 13 | "fmt" | |
| 14 | + | "io" | |
| 13 | 15 | "os" | |
| 14 | 16 | "path/filepath" | |
| 15 | 17 | "slices" | |
| 16 | 18 | "strings" | |
| 19 | + | "syscall" | |
| 17 | 20 | "unicode/utf8" | |
| 18 | 21 | ||
| 19 | 22 | "github.com/google/uuid" | |
| @@ -90,23 +93,43 @@ type Sink interface { | |||
| 90 | 93 | } | |
| 91 | 94 | ||
| 92 | 95 | // FileCommands manages the per-step files, like FileCommandManager. | |
| 96 | + | // | |
| 97 | + | // The directory belongs to the user of the steps, and the executor reads the | |
| 98 | + | // files as root after each step, so every access goes through an os.Root opened | |
| 99 | + | // before the first step ran: replacing the directory or a file by a symbolic | |
| 100 | + | // link cannot make the executor read or write anything outside it. Files are | |
| 101 | + | // opened with O_NOFOLLOW and must be regular files. | |
| 93 | 102 | type FileCommands struct { | |
| 94 | 103 | dir string | |
| 104 | + | root *os.Root | |
| 95 | 105 | suffix string | |
| 96 | - | // Own, when set, gives a file or directory to the user of the steps. | |
| 97 | - | Own func(path string) | |
| 106 | + | // owner, when set, is the user the files are given to. | |
| 107 | + | owner *[2]int | |
| 98 | 108 | } | |
| 99 | 109 | ||
| 100 | 110 | // NewFileCommands uses dir (RUNNER_TEMP/_runner_file_commands), creating it. | |
| 111 | + | // Call Close when the job is over. | |
| 101 | 112 | func NewFileCommands(dir string) (*FileCommands, error) { | |
| 102 | 113 | if err := os.MkdirAll(dir, 0o755); err != nil { | |
| 103 | 114 | return nil, err | |
| 104 | 115 | } | |
| 105 | - | return &FileCommands{dir: dir}, nil | |
| 116 | + | root, err := os.OpenRoot(dir) | |
| 117 | + | if err != nil { | |
| 118 | + | return nil, err | |
| 119 | + | } | |
| 120 | + | return &FileCommands{dir: dir, root: root}, nil | |
| 106 | 121 | } | |
| 107 | 122 | ||
| 123 | + | // Close releases the directory. | |
| 124 | + | func (f *FileCommands) Close() error { return f.root.Close() } | |
| 125 | + | ||
| 126 | + | // SetOwner gives the files of the next steps to uid:gid. | |
| 127 | + | func (f *FileCommands) SetOwner(uid, gid int) { f.owner = &[2]int{uid, gid} } | |
| 128 | + | ||
| 129 | + | func (f *FileCommands) name(c fileCommand, suffix string) string { return c.FilePrefix + suffix } | |
| 130 | + | ||
| 108 | 131 | func (f *FileCommands) path(c fileCommand, suffix string) string { | |
| 109 | - | return filepath.Join(f.dir, c.FilePrefix+suffix) | |
| 132 | + | return filepath.Join(f.dir, f.name(c, suffix)) | |
| 110 | 133 | } | |
| 111 | 134 | ||
| 112 | 135 | // Init creates fresh, empty files for the next step (after deleting the ones | |
| @@ -118,19 +141,12 @@ func (f *FileCommands) Init() (map[string]string, error) { | |||
| 118 | 141 | env := make(map[string]string, len(fileCommands)) | |
| 119 | 142 | for _, c := range fileCommands { | |
| 120 | 143 | if old != "" { | |
| 121 | - | _ = os.Remove(f.path(c, old)) | |
| 144 | + | _ = f.root.Remove(f.name(c, old)) | |
| 122 | 145 | } | |
| 123 | - | p := f.path(c, f.suffix) | |
| 124 | - | _ = os.Remove(p) | |
| 125 | - | fh, err := os.Create(p) | |
| 126 | - | if err != nil { | |
| 146 | + | if err := createExcl(f.root, f.name(c, f.suffix), nil, 0o644, f.owner); err != nil { | |
| 127 | 147 | return nil, err | |
| 128 | 148 | } | |
| 129 | - | _ = fh.Close() | |
| 130 | - | if f.Own != nil { | |
| 131 | - | f.Own(p) | |
| 132 | - | } | |
| 133 | - | env[envVarName(c.ContextName)] = p | |
| 149 | + | env[envVarName(c.ContextName)] = f.path(c, f.suffix) | |
| 134 | 150 | } | |
| 135 | 151 | return env, nil | |
| 136 | 152 | } | |
| @@ -139,19 +155,19 @@ func (f *FileCommands) Init() (map[string]string, error) { | |||
| 139 | 155 | // A command that fails does not stop the others (ProcessFiles, :75). | |
| 140 | 156 | func (f *FileCommands) Process(sink Sink) { | |
| 141 | 157 | for _, c := range fileCommands { | |
| 142 | - | p := f.path(c, f.suffix) | |
| 158 | + | p := f.name(c, f.suffix) | |
| 143 | 159 | var err error | |
| 144 | 160 | switch c.ContextName { | |
| 145 | 161 | case cmdPath: | |
| 146 | - | err = processAddPath(sink, p) | |
| 162 | + | err = processAddPath(sink, f.root, p) | |
| 147 | 163 | case cmdEnv: | |
| 148 | - | err = processSetEnv(sink, p) | |
| 164 | + | err = processSetEnv(sink, f.root, p) | |
| 149 | 165 | case cmdStepSummary: | |
| 150 | - | err = processStepSummary(sink, p) | |
| 166 | + | err = processStepSummary(sink, f.root, p) | |
| 151 | 167 | case cmdState: | |
| 152 | - | err = processPairs(sink, p, sink.SaveState, func(k, v string) { sink.Debug(fmt.Sprintf("Save intra-action state %s = %s", k, v)) }) | |
| 168 | + | err = processPairs(sink, f.root, p, sink.SaveState, func(k, v string) { sink.Debug(fmt.Sprintf("Save intra-action state %s = %s", k, v)) }) | |
| 153 | 169 | case cmdOutput: | |
| 154 | - | err = processPairs(sink, p, sink.SetOutput, func(k, v string) { sink.Debug(fmt.Sprintf("Set output %s = %s", k, v)) }) | |
| 170 | + | err = processPairs(sink, f.root, p, sink.SetOutput, func(k, v string) { sink.Debug(fmt.Sprintf("Set output %s = %s", k, v)) }) | |
| 155 | 171 | } | |
| 156 | 172 | if err != nil { | |
| 157 | 173 | sink.Error(fmt.Sprintf("Unable to process file command '%s' successfully.", c.ContextName)) | |
| @@ -161,26 +177,86 @@ func (f *FileCommands) Process(sink Sink) { | |||
| 161 | 177 | } | |
| 162 | 178 | } | |
| 163 | 179 | ||
| 180 | + | // errNotRegular is the error of a file command file that was replaced by | |
| 181 | + | // something else (a symbolic link, a FIFO, a directory). | |
| 182 | + | var errNotRegular = ghErrorf("The file is not a regular file") | |
| 183 | + | ||
| 184 | + | // openRegular opens name in root for reading without following a symbolic link | |
| 185 | + | // and without blocking on a FIFO, and checks that it is a regular file. | |
| 186 | + | func openRegular(root *os.Root, name string) (*os.File, error) { | |
| 187 | + | fh, err := root.OpenFile(name, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) | |
| 188 | + | if err != nil { | |
| 189 | + | if errors.Is(err, syscall.ELOOP) || errors.Is(err, syscall.EMLINK) { | |
| 190 | + | return nil, errNotRegular | |
| 191 | + | } | |
| 192 | + | return nil, err | |
| 193 | + | } | |
| 194 | + | st, err := fh.Stat() | |
| 195 | + | if err != nil { | |
| 196 | + | _ = fh.Close() | |
| 197 | + | return nil, err | |
| 198 | + | } | |
| 199 | + | if !st.Mode().IsRegular() { | |
| 200 | + | _ = fh.Close() | |
| 201 | + | return nil, errNotRegular | |
| 202 | + | } | |
| 203 | + | return fh, nil | |
| 204 | + | } | |
| 205 | + | ||
| 164 | 206 | // readText is File.ReadAllText: a UTF-8 BOM is dropped and invalid bytes | |
| 165 | - | // become U+FFFD. A missing file is reported as ok=false (the callers debug-log | |
| 166 | - | // it; AddPath and StepSummary skip silently). | |
| 167 | - | func readText(path string) (text string, ok bool) { | |
| 168 | - | b, err := os.ReadFile(path) | |
| 207 | + | // become U+FFFD. A missing file gives ok=false and no error (the callers | |
| 208 | + | // debug-log it; AddPath and StepSummary skip silently); a file that is not a | |
| 209 | + | // regular file is an error. | |
| 210 | + | func readText(root *os.Root, name string) (text string, ok bool, err error) { | |
| 211 | + | fh, err := openRegular(root, name) | |
| 212 | + | if errors.Is(err, os.ErrNotExist) { | |
| 213 | + | return "", false, nil | |
| 214 | + | } | |
| 215 | + | if err != nil { | |
| 216 | + | return "", false, err | |
| 217 | + | } | |
| 218 | + | defer func() { _ = fh.Close() }() | |
| 219 | + | b, err := io.ReadAll(fh) | |
| 169 | 220 | if err != nil { | |
| 170 | - | return "", false | |
| 221 | + | return "", false, err | |
| 171 | 222 | } | |
| 172 | 223 | b = bytes.TrimPrefix(b, []byte("\xef\xbb\xbf")) | |
| 173 | 224 | if !utf8.Valid(b) { | |
| 174 | 225 | b = bytes.ToValidUTF8(b, []byte("\ufffd")) | |
| 175 | 226 | } | |
| 176 | - | return string(b), true | |
| 227 | + | return string(b), true, nil | |
| 228 | + | } | |
| 229 | + | ||
| 230 | + | // createExcl creates name in root with the content, failing if it exists (an | |
| 231 | + | // existing entry, a symbolic link included, is removed first, never followed), | |
| 232 | + | // and gives it to owner when set. The creation uses O_EXCL|O_NOFOLLOW, so | |
| 233 | + | // something planted between the removal and the creation makes it fail. | |
| 234 | + | func createExcl(root *os.Root, name string, content []byte, perm os.FileMode, owner *[2]int) error { | |
| 235 | + | if err := root.Remove(name); err != nil && !errors.Is(err, os.ErrNotExist) { | |
| 236 | + | return err | |
| 237 | + | } | |
| 238 | + | fh, err := root.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL|syscall.O_NOFOLLOW, perm) | |
| 239 | + | if err != nil { | |
| 240 | + | return err | |
| 241 | + | } | |
| 242 | + | if _, err := fh.Write(content); err != nil { | |
| 243 | + | _ = fh.Close() | |
| 244 | + | return err | |
| 245 | + | } | |
| 246 | + | if err := fh.Close(); err != nil { | |
| 247 | + | return err | |
| 248 | + | } | |
| 249 | + | if owner != nil { | |
| 250 | + | return root.Lchown(name, owner[0], owner[1]) | |
| 251 | + | } | |
| 252 | + | return nil | |
| 177 | 253 | } | |
| 178 | 254 | ||
| 179 | 255 | // processAddPath is AddPathFileCommand.ProcessCommand (FileCommandManager.cs:134). | |
| 180 | - | func processAddPath(sink Sink, path string) error { | |
| 181 | - | text, ok := readText(path) | |
| 182 | - | if !ok { | |
| 183 | - | return nil | |
| 256 | + | func processAddPath(sink Sink, root *os.Root, name string) error { | |
| 257 | + | text, ok, err := readText(root, name) | |
| 258 | + | if err != nil || !ok { | |
| 259 | + | return err | |
| 184 | 260 | } | |
| 185 | 261 | // File.ReadAllLines splits on \r\n, \n and \r. | |
| 186 | 262 | for _, line := range splitLines(text) { | |
| @@ -213,8 +289,8 @@ func splitLines(s string) []string { | |||
| 213 | 289 | ||
| 214 | 290 | // processSetEnv is SetEnvFileCommand.ProcessCommand (:167). Pairs read before | |
| 215 | 291 | // a format error are applied, like the lazy enumeration of the original. | |
| 216 | - | func processSetEnv(sink Sink, path string) error { | |
| 217 | - | pairs, err := readEnvFile(sink, path) | |
| 292 | + | func processSetEnv(sink Sink, root *os.Root, name string) error { | |
| 293 | + | pairs, err := readEnvFile(sink, root, name) | |
| 218 | 294 | for _, p := range pairs { | |
| 219 | 295 | if blocked := blockedEnvName(p.Key); blocked != "" { | |
| 220 | 296 | sink.Error(fmt.Sprintf("Can't store %s output parameter using '$GITHUB_ENV' command.", blocked)) | |
| @@ -236,8 +312,8 @@ func blockedEnvName(name string) string { | |||
| 236 | 312 | } | |
| 237 | 313 | ||
| 238 | 314 | // processPairs serves GITHUB_STATE and GITHUB_OUTPUT. | |
| 239 | - | func processPairs(sink Sink, path string, apply func(k, v string), debug func(k, v string)) error { | |
| 240 | - | pairs, err := readEnvFile(sink, path) | |
| 315 | + | func processPairs(sink Sink, root *os.Root, name string, apply func(k, v string), debug func(k, v string)) error { | |
| 316 | + | pairs, err := readEnvFile(sink, root, name) | |
| 241 | 317 | for _, p := range pairs { | |
| 242 | 318 | apply(p.Key, p.Value) | |
| 243 | 319 | debug(p.Key, p.Value) | |
| @@ -245,10 +321,13 @@ func processPairs(sink Sink, path string, apply func(k, v string), debug func(k, | |||
| 245 | 321 | return err | |
| 246 | 322 | } | |
| 247 | 323 | ||
| 248 | - | func readEnvFile(sink Sink, path string) ([]Pair, error) { | |
| 249 | - | text, ok := readText(path) | |
| 324 | + | func readEnvFile(sink Sink, root *os.Root, name string) ([]Pair, error) { | |
| 325 | + | text, ok, err := readText(root, name) | |
| 326 | + | if err != nil { | |
| 327 | + | return nil, err | |
| 328 | + | } | |
| 250 | 329 | if !ok { | |
| 251 | - | sink.Debug(fmt.Sprintf("File does not exist '%s'", path)) | |
| 330 | + | sink.Debug(fmt.Sprintf("File does not exist '%s'", filepath.Join(root.Name(), name))) | |
| 252 | 331 | return nil, nil | |
| 253 | 332 | } | |
| 254 | 333 | return ParseEnvFile(text) | |
| @@ -257,8 +336,16 @@ func readEnvFile(sink Sink, path string) ([]Pair, error) { | |||
| 257 | 336 | // processStepSummary is CreateStepSummaryCommand.ProcessCommand (:230) for | |
| 258 | 337 | // CI-1: the file is only checked (an empty file is ignored, a file above 1 MiB | |
| 259 | 338 | // is an error); ermite does not store summaries yet. | |
| 260 | - | func processStepSummary(sink Sink, path string) error { | |
| 261 | - | st, err := os.Stat(path) | |
| 339 | + | func processStepSummary(sink Sink, root *os.Root, name string) error { | |
| 340 | + | fh, err := openRegular(root, name) | |
| 341 | + | if errors.Is(err, os.ErrNotExist) { | |
| 342 | + | return nil | |
| 343 | + | } | |
| 344 | + | if err != nil { | |
| 345 | + | return err | |
| 346 | + | } | |
| 347 | + | st, err := fh.Stat() | |
| 348 | + | _ = fh.Close() | |
| 262 | 349 | if err != nil || st.Size() == 0 { | |
| 263 | 350 | return nil | |
| 264 | 351 | } | |
| @@ -9,11 +9,14 @@ package executor | |||||
| 9 | 9 | ||||
| 10 | import ( | 10 | import ( | ||
| 11 | "bytes" | 11 | "bytes" | ||
| 12 | + | "errors" | |||
| 12 | "fmt" | 13 | "fmt" | ||
| 14 | + | "io" | |||
| 13 | "os" | 15 | "os" | ||
| 14 | "path/filepath" | 16 | "path/filepath" | ||
| 15 | "slices" | 17 | "slices" | ||
| 16 | "strings" | 18 | "strings" | ||
| 19 | + | "syscall" | |||
| 17 | "unicode/utf8" | 20 | "unicode/utf8" | ||
| 18 | 21 | ||||
| 19 | "github.com/google/uuid" | 22 | "github.com/google/uuid" | ||
| @@ -90,23 +93,43 @@ type Sink interface { | |||||
| 90 | } | 93 | } | ||
| 91 | 94 | ||||
| 92 | // FileCommands manages the per-step files, like FileCommandManager. | 95 | // FileCommands manages the per-step files, like FileCommandManager. | ||
| 96 | + | // | |||
| 97 | + | // The directory belongs to the user of the steps, and the executor reads the | |||
| 98 | + | // files as root after each step, so every access goes through an os.Root opened | |||
| 99 | + | // before the first step ran: replacing the directory or a file by a symbolic | |||
| 100 | + | // link cannot make the executor read or write anything outside it. Files are | |||
| 101 | + | // opened with O_NOFOLLOW and must be regular files. | |||
| 93 | type FileCommands struct { | 102 | type FileCommands struct { | ||
| 94 | dir string | 103 | dir string | ||
| 104 | + | root *os.Root | |||
| 95 | suffix string | 105 | suffix string | ||
| 96 | - | // Own, when set, gives a file or directory to the user of the steps. | 106 | + | // owner, when set, is the user the files are given to. |
| 97 | - | Own func(path string) | 107 | + | owner *[2]int |
| 98 | } | 108 | } | ||
| 99 | 109 | ||||
| 100 | // NewFileCommands uses dir (RUNNER_TEMP/_runner_file_commands), creating it. | 110 | // NewFileCommands uses dir (RUNNER_TEMP/_runner_file_commands), creating it. | ||
| 111 | + | // Call Close when the job is over. | |||
| 101 | func NewFileCommands(dir string) (*FileCommands, error) { | 112 | func NewFileCommands(dir string) (*FileCommands, error) { | ||
| 102 | if err := os.MkdirAll(dir, 0o755); err != nil { | 113 | if err := os.MkdirAll(dir, 0o755); err != nil { | ||
| 103 | return nil, err | 114 | return nil, err | ||
| 104 | } | 115 | } | ||
| 105 | - | return &FileCommands{dir: dir}, nil | 116 | + | root, err := os.OpenRoot(dir) |
| 117 | + | if err != nil { | |||
| 118 | + | return nil, err | |||
| 119 | + | } | |||
| 120 | + | return &FileCommands{dir: dir, root: root}, nil | |||
| 106 | } | 121 | } | ||
| 107 | 122 | ||||
| 123 | + | // Close releases the directory. | |||
| 124 | + | func (f *FileCommands) Close() error { return f.root.Close() } | |||
| 125 | + | ||||
| 126 | + | // SetOwner gives the files of the next steps to uid:gid. | |||
| 127 | + | func (f *FileCommands) SetOwner(uid, gid int) { f.owner = &[2]int{uid, gid} } | |||
| 128 | + | ||||
| 129 | + | func (f *FileCommands) name(c fileCommand, suffix string) string { return c.FilePrefix + suffix } | |||
| 130 | + | ||||
| 108 | func (f *FileCommands) path(c fileCommand, suffix string) string { | 131 | func (f *FileCommands) path(c fileCommand, suffix string) string { | ||
| 109 | - | return filepath.Join(f.dir, c.FilePrefix+suffix) | 132 | + | return filepath.Join(f.dir, f.name(c, suffix)) |
| 110 | } | 133 | } | ||
| 111 | 134 | ||||
| 112 | // Init creates fresh, empty files for the next step (after deleting the ones | 135 | // Init creates fresh, empty files for the next step (after deleting the ones | ||
| @@ -118,19 +141,12 @@ func (f *FileCommands) Init() (map[string]string, error) { | |||||
| 118 | env := make(map[string]string, len(fileCommands)) | 141 | env := make(map[string]string, len(fileCommands)) | ||
| 119 | for _, c := range fileCommands { | 142 | for _, c := range fileCommands { | ||
| 120 | if old != "" { | 143 | if old != "" { | ||
| 121 | - | _ = os.Remove(f.path(c, old)) | 144 | + | _ = f.root.Remove(f.name(c, old)) |
| 122 | } | 145 | } | ||
| 123 | - | p := f.path(c, f.suffix) | 146 | + | if err := createExcl(f.root, f.name(c, f.suffix), nil, 0o644, f.owner); err != nil { |
| 124 | - | _ = os.Remove(p) | |||
| 125 | - | fh, err := os.Create(p) | |||
| 126 | - | if err != nil { | |||
| 127 | return nil, err | 147 | return nil, err | ||
| 128 | } | 148 | } | ||
| 129 | - | _ = fh.Close() | 149 | + | env[envVarName(c.ContextName)] = f.path(c, f.suffix) |
| 130 | - | if f.Own != nil { | |||
| 131 | - | f.Own(p) | |||
| 132 | - | } | |||
| 133 | - | env[envVarName(c.ContextName)] = p | |||
| 134 | } | 150 | } | ||
| 135 | return env, nil | 151 | return env, nil | ||
| 136 | } | 152 | } | ||
| @@ -139,19 +155,19 @@ func (f *FileCommands) Init() (map[string]string, error) { | |||||
| 139 | // A command that fails does not stop the others (ProcessFiles, :75). | 155 | // A command that fails does not stop the others (ProcessFiles, :75). | ||
| 140 | func (f *FileCommands) Process(sink Sink) { | 156 | func (f *FileCommands) Process(sink Sink) { | ||
| 141 | for _, c := range fileCommands { | 157 | for _, c := range fileCommands { | ||
| 142 | - | p := f.path(c, f.suffix) | 158 | + | p := f.name(c, f.suffix) |
| 143 | var err error | 159 | var err error | ||
| 144 | switch c.ContextName { | 160 | switch c.ContextName { | ||
| 145 | case cmdPath: | 161 | case cmdPath: | ||
| 146 | - | err = processAddPath(sink, p) | 162 | + | err = processAddPath(sink, f.root, p) |
| 147 | case cmdEnv: | 163 | case cmdEnv: | ||
| 148 | - | err = processSetEnv(sink, p) | 164 | + | err = processSetEnv(sink, f.root, p) |
| 149 | case cmdStepSummary: | 165 | case cmdStepSummary: | ||
| 150 | - | err = processStepSummary(sink, p) | 166 | + | err = processStepSummary(sink, f.root, p) |
| 151 | case cmdState: | 167 | case cmdState: | ||
| 152 | - | err = processPairs(sink, p, sink.SaveState, func(k, v string) { sink.Debug(fmt.Sprintf("Save intra-action state %s = %s", k, v)) }) | 168 | + | err = processPairs(sink, f.root, p, sink.SaveState, func(k, v string) { sink.Debug(fmt.Sprintf("Save intra-action state %s = %s", k, v)) }) |
| 153 | case cmdOutput: | 169 | case cmdOutput: | ||
| 154 | - | err = processPairs(sink, p, sink.SetOutput, func(k, v string) { sink.Debug(fmt.Sprintf("Set output %s = %s", k, v)) }) | 170 | + | err = processPairs(sink, f.root, p, sink.SetOutput, func(k, v string) { sink.Debug(fmt.Sprintf("Set output %s = %s", k, v)) }) |
| 155 | } | 171 | } | ||
| 156 | if err != nil { | 172 | if err != nil { | ||
| 157 | sink.Error(fmt.Sprintf("Unable to process file command '%s' successfully.", c.ContextName)) | 173 | sink.Error(fmt.Sprintf("Unable to process file command '%s' successfully.", c.ContextName)) | ||
| @@ -161,26 +177,86 @@ func (f *FileCommands) Process(sink Sink) { | |||||
| 161 | } | 177 | } | ||
| 162 | } | 178 | } | ||
| 163 | 179 | ||||
| 180 | + | // errNotRegular is the error of a file command file that was replaced by | |||
| 181 | + | // something else (a symbolic link, a FIFO, a directory). | |||
| 182 | + | var errNotRegular = ghErrorf("The file is not a regular file") | |||
| 183 | + | ||||
| 184 | + | // openRegular opens name in root for reading without following a symbolic link | |||
| 185 | + | // and without blocking on a FIFO, and checks that it is a regular file. | |||
| 186 | + | func openRegular(root *os.Root, name string) (*os.File, error) { | |||
| 187 | + | fh, err := root.OpenFile(name, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) | |||
| 188 | + | if err != nil { | |||
| 189 | + | if errors.Is(err, syscall.ELOOP) || errors.Is(err, syscall.EMLINK) { | |||
| 190 | + | return nil, errNotRegular | |||
| 191 | + | } | |||
| 192 | + | return nil, err | |||
| 193 | + | } | |||
| 194 | + | st, err := fh.Stat() | |||
| 195 | + | if err != nil { | |||
| 196 | + | _ = fh.Close() | |||
| 197 | + | return nil, err | |||
| 198 | + | } | |||
| 199 | + | if !st.Mode().IsRegular() { | |||
| 200 | + | _ = fh.Close() | |||
| 201 | + | return nil, errNotRegular | |||
| 202 | + | } | |||
| 203 | + | return fh, nil | |||
| 204 | + | } | |||
| 205 | + | ||||
| 164 | // readText is File.ReadAllText: a UTF-8 BOM is dropped and invalid bytes | 206 | // readText is File.ReadAllText: a UTF-8 BOM is dropped and invalid bytes | ||
| 165 | - | // become U+FFFD. A missing file is reported as ok=false (the callers debug-log | 207 | + | // become U+FFFD. A missing file gives ok=false and no error (the callers |
| 166 | - | // it; AddPath and StepSummary skip silently). | 208 | + | // debug-log it; AddPath and StepSummary skip silently); a file that is not a |
| 167 | - | func readText(path string) (text string, ok bool) { | 209 | + | // regular file is an error. |
| 168 | - | b, err := os.ReadFile(path) | 210 | + | func readText(root *os.Root, name string) (text string, ok bool, err error) { |
| 211 | + | fh, err := openRegular(root, name) | |||
| 212 | + | if errors.Is(err, os.ErrNotExist) { | |||
| 213 | + | return "", false, nil | |||
| 214 | + | } | |||
| 215 | + | if err != nil { | |||
| 216 | + | return "", false, err | |||
| 217 | + | } | |||
| 218 | + | defer func() { _ = fh.Close() }() | |||
| 219 | + | b, err := io.ReadAll(fh) | |||
| 169 | if err != nil { | 220 | if err != nil { | ||
| 170 | - | return "", false | 221 | + | return "", false, err |
| 171 | } | 222 | } | ||
| 172 | b = bytes.TrimPrefix(b, []byte("\xef\xbb\xbf")) | 223 | b = bytes.TrimPrefix(b, []byte("\xef\xbb\xbf")) | ||
| 173 | if !utf8.Valid(b) { | 224 | if !utf8.Valid(b) { | ||
| 174 | b = bytes.ToValidUTF8(b, []byte("\ufffd")) | 225 | b = bytes.ToValidUTF8(b, []byte("\ufffd")) | ||
| 175 | } | 226 | } | ||
| 176 | - | return string(b), true | 227 | + | return string(b), true, nil |
| 228 | + | } | |||
| 229 | + | ||||
| 230 | + | // createExcl creates name in root with the content, failing if it exists (an | |||
| 231 | + | // existing entry, a symbolic link included, is removed first, never followed), | |||
| 232 | + | // and gives it to owner when set. The creation uses O_EXCL|O_NOFOLLOW, so | |||
| 233 | + | // something planted between the removal and the creation makes it fail. | |||
| 234 | + | func createExcl(root *os.Root, name string, content []byte, perm os.FileMode, owner *[2]int) error { | |||
| 235 | + | if err := root.Remove(name); err != nil && !errors.Is(err, os.ErrNotExist) { | |||
| 236 | + | return err | |||
| 237 | + | } | |||
| 238 | + | fh, err := root.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL|syscall.O_NOFOLLOW, perm) | |||
| 239 | + | if err != nil { | |||
| 240 | + | return err | |||
| 241 | + | } | |||
| 242 | + | if _, err := fh.Write(content); err != nil { | |||
| 243 | + | _ = fh.Close() | |||
| 244 | + | return err | |||
| 245 | + | } | |||
| 246 | + | if err := fh.Close(); err != nil { | |||
| 247 | + | return err | |||
| 248 | + | } | |||
| 249 | + | if owner != nil { | |||
| 250 | + | return root.Lchown(name, owner[0], owner[1]) | |||
| 251 | + | } | |||
| 252 | + | return nil | |||
| 177 | } | 253 | } | ||
| 178 | 254 | ||||
| 179 | // processAddPath is AddPathFileCommand.ProcessCommand (FileCommandManager.cs:134). | 255 | // processAddPath is AddPathFileCommand.ProcessCommand (FileCommandManager.cs:134). | ||
| 180 | - | func processAddPath(sink Sink, path string) error { | 256 | + | func processAddPath(sink Sink, root *os.Root, name string) error { |
| 181 | - | text, ok := readText(path) | 257 | + | text, ok, err := readText(root, name) |
| 182 | - | if !ok { | 258 | + | if err != nil || !ok { |
| 183 | - | return nil | 259 | + | return err |
| 184 | } | 260 | } | ||
| 185 | // File.ReadAllLines splits on \r\n, \n and \r. | 261 | // File.ReadAllLines splits on \r\n, \n and \r. | ||
| 186 | for _, line := range splitLines(text) { | 262 | for _, line := range splitLines(text) { | ||
| @@ -213,8 +289,8 @@ func splitLines(s string) []string { | |||||
| 213 | 289 | ||||
| 214 | // processSetEnv is SetEnvFileCommand.ProcessCommand (:167). Pairs read before | 290 | // processSetEnv is SetEnvFileCommand.ProcessCommand (:167). Pairs read before | ||
| 215 | // a format error are applied, like the lazy enumeration of the original. | 291 | // a format error are applied, like the lazy enumeration of the original. | ||
| 216 | - | func processSetEnv(sink Sink, path string) error { | 292 | + | func processSetEnv(sink Sink, root *os.Root, name string) error { |
| 217 | - | pairs, err := readEnvFile(sink, path) | 293 | + | pairs, err := readEnvFile(sink, root, name) |
| 218 | for _, p := range pairs { | 294 | for _, p := range pairs { | ||
| 219 | if blocked := blockedEnvName(p.Key); blocked != "" { | 295 | if blocked := blockedEnvName(p.Key); blocked != "" { | ||
| 220 | sink.Error(fmt.Sprintf("Can't store %s output parameter using '$GITHUB_ENV' command.", blocked)) | 296 | sink.Error(fmt.Sprintf("Can't store %s output parameter using '$GITHUB_ENV' command.", blocked)) | ||
| @@ -236,8 +312,8 @@ func blockedEnvName(name string) string { | |||||
| 236 | } | 312 | } | ||
| 237 | 313 | ||||
| 238 | // processPairs serves GITHUB_STATE and GITHUB_OUTPUT. | 314 | // processPairs serves GITHUB_STATE and GITHUB_OUTPUT. | ||
| 239 | - | func processPairs(sink Sink, path string, apply func(k, v string), debug func(k, v string)) error { | 315 | + | func processPairs(sink Sink, root *os.Root, name string, apply func(k, v string), debug func(k, v string)) error { |
| 240 | - | pairs, err := readEnvFile(sink, path) | 316 | + | pairs, err := readEnvFile(sink, root, name) |
| 241 | for _, p := range pairs { | 317 | for _, p := range pairs { | ||
| 242 | apply(p.Key, p.Value) | 318 | apply(p.Key, p.Value) | ||
| 243 | debug(p.Key, p.Value) | 319 | debug(p.Key, p.Value) | ||
| @@ -245,10 +321,13 @@ func processPairs(sink Sink, path string, apply func(k, v string), debug func(k, | |||||
| 245 | return err | 321 | return err | ||
| 246 | } | 322 | } | ||
| 247 | 323 | ||||
| 248 | - | func readEnvFile(sink Sink, path string) ([]Pair, error) { | 324 | + | func readEnvFile(sink Sink, root *os.Root, name string) ([]Pair, error) { |
| 249 | - | text, ok := readText(path) | 325 | + | text, ok, err := readText(root, name) |
| 326 | + | if err != nil { | |||
| 327 | + | return nil, err | |||
| 328 | + | } | |||
| 250 | if !ok { | 329 | if !ok { | ||
| 251 | - | sink.Debug(fmt.Sprintf("File does not exist '%s'", path)) | 330 | + | sink.Debug(fmt.Sprintf("File does not exist '%s'", filepath.Join(root.Name(), name))) |
| 252 | return nil, nil | 331 | return nil, nil | ||
| 253 | } | 332 | } | ||
| 254 | return ParseEnvFile(text) | 333 | return ParseEnvFile(text) | ||
| @@ -257,8 +336,16 @@ func readEnvFile(sink Sink, path string) ([]Pair, error) { | |||||
| 257 | // processStepSummary is CreateStepSummaryCommand.ProcessCommand (:230) for | 336 | // processStepSummary is CreateStepSummaryCommand.ProcessCommand (:230) for | ||
| 258 | // CI-1: the file is only checked (an empty file is ignored, a file above 1 MiB | 337 | // CI-1: the file is only checked (an empty file is ignored, a file above 1 MiB | ||
| 259 | // is an error); ermite does not store summaries yet. | 338 | // is an error); ermite does not store summaries yet. | ||
| 260 | - | func processStepSummary(sink Sink, path string) error { | 339 | + | func processStepSummary(sink Sink, root *os.Root, name string) error { |
| 261 | - | st, err := os.Stat(path) | 340 | + | fh, err := openRegular(root, name) |
| 341 | + | if errors.Is(err, os.ErrNotExist) { | |||
| 342 | + | return nil | |||
| 343 | + | } | |||
| 344 | + | if err != nil { | |||
| 345 | + | return err | |||
| 346 | + | } | |||
| 347 | + | st, err := fh.Stat() | |||
| 348 | + | _ = fh.Close() | |||
| 262 | if err != nil || st.Size() == 0 { | 349 | if err != nil || st.Size() == 0 { | ||
| 263 | return nil | 350 | return nil | ||
| 264 | } | 351 | } | ||
M internal/executor/filecmd_test.go +124 -0
| @@ -7,7 +7,9 @@ import ( | |||
| 7 | 7 | "path/filepath" | |
| 8 | 8 | "reflect" | |
| 9 | 9 | "strings" | |
| 10 | + | "syscall" | |
| 10 | 11 | "testing" | |
| 12 | + | "time" | |
| 11 | 13 | ) | |
| 12 | 14 | ||
| 13 | 15 | // Cases ported from SetEnvFileCommandL0, SetOutputFileCommandL0 and | |
| @@ -229,3 +231,125 @@ func TestPrependedPath(t *testing.T) { | |||
| 229 | 231 | t.Fatal(got) | |
| 230 | 232 | } | |
| 231 | 233 | } | |
| 234 | + | ||
| 235 | + | // ---- the directory belongs to the user of the steps: nothing may be followed | |
| 236 | + | ||
| 237 | + | func TestFileCommandSymlinkIsNotFollowed(t *testing.T) { | |
| 238 | + | for _, name := range []string{"GITHUB_ENV", "GITHUB_OUTPUT", "GITHUB_PATH", "GITHUB_STATE", "GITHUB_STEP_SUMMARY"} { | |
| 239 | + | t.Run(name, func(t *testing.T) { | |
| 240 | + | secret := filepath.Join(t.TempDir(), "secret") | |
| 241 | + | if err := os.WriteFile(secret, []byte("STOLEN=1\n/stolen/bin\n"), 0o600); err != nil { | |
| 242 | + | t.Fatal(err) | |
| 243 | + | } | |
| 244 | + | fc, err := NewFileCommands(t.TempDir()) | |
| 245 | + | if err != nil { | |
| 246 | + | t.Fatal(err) | |
| 247 | + | } | |
| 248 | + | defer func() { _ = fc.Close() }() | |
| 249 | + | env, _ := fc.Init() | |
| 250 | + | // The step replaces its file by a link to a file only root may read. | |
| 251 | + | p := env[name] | |
| 252 | + | if err := os.Remove(p); err != nil { | |
| 253 | + | t.Fatal(err) | |
| 254 | + | } | |
| 255 | + | if err := os.Symlink(secret, p); err != nil { | |
| 256 | + | t.Fatal(err) | |
| 257 | + | } | |
| 258 | + | s := newSink() | |
| 259 | + | fc.Process(s) | |
| 260 | + | if len(s.env)+len(s.outputs)+len(s.state)+len(s.path) != 0 { | |
| 261 | + | t.Fatalf("the content of the target was read: %+v", s) | |
| 262 | + | } | |
| 263 | + | if !s.failed || len(s.errors) == 0 { | |
| 264 | + | t.Fatalf("a replaced file must fail the step: %+v", s) | |
| 265 | + | } | |
| 266 | + | }) | |
| 267 | + | } | |
| 268 | + | } | |
| 269 | + | ||
| 270 | + | func TestFileCommandFifoDoesNotBlock(t *testing.T) { | |
| 271 | + | fc, _ := NewFileCommands(t.TempDir()) | |
| 272 | + | defer func() { _ = fc.Close() }() | |
| 273 | + | env, _ := fc.Init() | |
| 274 | + | p := env["GITHUB_OUTPUT"] | |
| 275 | + | _ = os.Remove(p) | |
| 276 | + | if err := syscall.Mkfifo(p, 0o644); err != nil { | |
| 277 | + | t.Skip(err) | |
| 278 | + | } | |
| 279 | + | done := make(chan *fakeSink, 1) | |
| 280 | + | go func() { s := newSink(); fc.Process(s); done <- s }() | |
| 281 | + | select { | |
| 282 | + | case s := <-done: | |
| 283 | + | if !s.failed { | |
| 284 | + | t.Fatalf("%+v", s) | |
| 285 | + | } | |
| 286 | + | case <-time.After(5 * time.Second): | |
| 287 | + | t.Fatal("reading a FIFO blocked the executor") | |
| 288 | + | } | |
| 289 | + | } | |
| 290 | + | ||
| 291 | + | func TestFileCommandDirectoryReplacedByASymlink(t *testing.T) { | |
| 292 | + | outside := t.TempDir() | |
| 293 | + | if err := os.WriteFile(filepath.Join(outside, "set_env_x"), []byte("LEAK=1\n"), 0o600); err != nil { | |
| 294 | + | t.Fatal(err) | |
| 295 | + | } | |
| 296 | + | parent := t.TempDir() | |
| 297 | + | dir := filepath.Join(parent, "cmds") | |
| 298 | + | fc, err := NewFileCommands(dir) | |
| 299 | + | if err != nil { | |
| 300 | + | t.Fatal(err) | |
| 301 | + | } | |
| 302 | + | defer func() { _ = fc.Close() }() | |
| 303 | + | if _, err := fc.Init(); err != nil { | |
| 304 | + | t.Fatal(err) | |
| 305 | + | } | |
| 306 | + | // The step swaps the directory for a link to another one. | |
| 307 | + | if err := os.Rename(dir, dir+".old"); err != nil { | |
| 308 | + | t.Fatal(err) | |
| 309 | + | } | |
| 310 | + | if err := os.Symlink(outside, dir); err != nil { | |
| 311 | + | t.Fatal(err) | |
| 312 | + | } | |
| 313 | + | fc.suffix = "x" | |
| 314 | + | s := newSink() | |
| 315 | + | fc.Process(s) | |
| 316 | + | if len(s.env) != 0 { | |
| 317 | + | t.Fatalf("a file of the swapped directory was read: %v", s.env) | |
| 318 | + | } | |
| 319 | + | // Init keeps working on the original directory, not on the link's target. | |
| 320 | + | env, err := fc.Init() | |
| 321 | + | if err != nil { | |
| 322 | + | t.Fatal(err) | |
| 323 | + | } | |
| 324 | + | if _, err := os.Stat(filepath.Join(outside, filepath.Base(env["GITHUB_ENV"]))); err == nil { | |
| 325 | + | t.Fatal("a file was created through the swapped directory") | |
| 326 | + | } | |
| 327 | + | } | |
| 328 | + | ||
| 329 | + | func TestCreateExclReplacesAndNeverFollows(t *testing.T) { | |
| 330 | + | dir := t.TempDir() | |
| 331 | + | root, err := os.OpenRoot(dir) | |
| 332 | + | if err != nil { | |
| 333 | + | t.Fatal(err) | |
| 334 | + | } | |
| 335 | + | defer func() { _ = root.Close() }() | |
| 336 | + | victim := filepath.Join(t.TempDir(), "victim") | |
| 337 | + | if err := os.WriteFile(victim, []byte("keep"), 0o600); err != nil { | |
| 338 | + | t.Fatal(err) | |
| 339 | + | } | |
| 340 | + | if err := os.Symlink(victim, filepath.Join(dir, "f")); err != nil { | |
| 341 | + | t.Fatal(err) | |
| 342 | + | } | |
| 343 | + | if err := createExcl(root, "f", []byte("new"), 0o644, nil); err != nil { | |
| 344 | + | t.Fatal(err) | |
| 345 | + | } | |
| 346 | + | if b, _ := os.ReadFile(victim); string(b) != "keep" { | |
| 347 | + | t.Fatalf("the target of the link was written: %q", b) | |
| 348 | + | } | |
| 349 | + | if b, _ := os.ReadFile(filepath.Join(dir, "f")); string(b) != "new" { | |
| 350 | + | t.Fatalf("%q", b) | |
| 351 | + | } | |
| 352 | + | if fi, _ := os.Lstat(filepath.Join(dir, "f")); fi.Mode()&os.ModeSymlink != 0 { | |
| 353 | + | t.Fatal("still a link") | |
| 354 | + | } | |
| 355 | + | } | |
| @@ -7,7 +7,9 @@ import ( | |||||
| 7 | "path/filepath" | 7 | "path/filepath" | ||
| 8 | "reflect" | 8 | "reflect" | ||
| 9 | "strings" | 9 | "strings" | ||
| 10 | + | "syscall" | |||
| 10 | "testing" | 11 | "testing" | ||
| 12 | + | "time" | |||
| 11 | ) | 13 | ) | ||
| 12 | 14 | ||||
| 13 | // Cases ported from SetEnvFileCommandL0, SetOutputFileCommandL0 and | 15 | // Cases ported from SetEnvFileCommandL0, SetOutputFileCommandL0 and | ||
| @@ -229,3 +231,125 @@ func TestPrependedPath(t *testing.T) { | |||||
| 229 | t.Fatal(got) | 231 | t.Fatal(got) | ||
| 230 | } | 232 | } | ||
| 231 | } | 233 | } | ||
| 234 | + | ||||
| 235 | + | // ---- the directory belongs to the user of the steps: nothing may be followed | |||
| 236 | + | ||||
| 237 | + | func TestFileCommandSymlinkIsNotFollowed(t *testing.T) { | |||
| 238 | + | for _, name := range []string{"GITHUB_ENV", "GITHUB_OUTPUT", "GITHUB_PATH", "GITHUB_STATE", "GITHUB_STEP_SUMMARY"} { | |||
| 239 | + | t.Run(name, func(t *testing.T) { | |||
| 240 | + | secret := filepath.Join(t.TempDir(), "secret") | |||
| 241 | + | if err := os.WriteFile(secret, []byte("STOLEN=1\n/stolen/bin\n"), 0o600); err != nil { | |||
| 242 | + | t.Fatal(err) | |||
| 243 | + | } | |||
| 244 | + | fc, err := NewFileCommands(t.TempDir()) | |||
| 245 | + | if err != nil { | |||
| 246 | + | t.Fatal(err) | |||
| 247 | + | } | |||
| 248 | + | defer func() { _ = fc.Close() }() | |||
| 249 | + | env, _ := fc.Init() | |||
| 250 | + | // The step replaces its file by a link to a file only root may read. | |||
| 251 | + | p := env[name] | |||
| 252 | + | if err := os.Remove(p); err != nil { | |||
| 253 | + | t.Fatal(err) | |||
| 254 | + | } | |||
| 255 | + | if err := os.Symlink(secret, p); err != nil { | |||
| 256 | + | t.Fatal(err) | |||
| 257 | + | } | |||
| 258 | + | s := newSink() | |||
| 259 | + | fc.Process(s) | |||
| 260 | + | if len(s.env)+len(s.outputs)+len(s.state)+len(s.path) != 0 { | |||
| 261 | + | t.Fatalf("the content of the target was read: %+v", s) | |||
| 262 | + | } | |||
| 263 | + | if !s.failed || len(s.errors) == 0 { | |||
| 264 | + | t.Fatalf("a replaced file must fail the step: %+v", s) | |||
| 265 | + | } | |||
| 266 | + | }) | |||
| 267 | + | } | |||
| 268 | + | } | |||
| 269 | + | ||||
| 270 | + | func TestFileCommandFifoDoesNotBlock(t *testing.T) { | |||
| 271 | + | fc, _ := NewFileCommands(t.TempDir()) | |||
| 272 | + | defer func() { _ = fc.Close() }() | |||
| 273 | + | env, _ := fc.Init() | |||
| 274 | + | p := env["GITHUB_OUTPUT"] | |||
| 275 | + | _ = os.Remove(p) | |||
| 276 | + | if err := syscall.Mkfifo(p, 0o644); err != nil { | |||
| 277 | + | t.Skip(err) | |||
| 278 | + | } | |||
| 279 | + | done := make(chan *fakeSink, 1) | |||
| 280 | + | go func() { s := newSink(); fc.Process(s); done <- s }() | |||
| 281 | + | select { | |||
| 282 | + | case s := <-done: | |||
| 283 | + | if !s.failed { | |||
| 284 | + | t.Fatalf("%+v", s) | |||
| 285 | + | } | |||
| 286 | + | case <-time.After(5 * time.Second): | |||
| 287 | + | t.Fatal("reading a FIFO blocked the executor") | |||
| 288 | + | } | |||
| 289 | + | } | |||
| 290 | + | ||||
| 291 | + | func TestFileCommandDirectoryReplacedByASymlink(t *testing.T) { | |||
| 292 | + | outside := t.TempDir() | |||
| 293 | + | if err := os.WriteFile(filepath.Join(outside, "set_env_x"), []byte("LEAK=1\n"), 0o600); err != nil { | |||
| 294 | + | t.Fatal(err) | |||
| 295 | + | } | |||
| 296 | + | parent := t.TempDir() | |||
| 297 | + | dir := filepath.Join(parent, "cmds") | |||
| 298 | + | fc, err := NewFileCommands(dir) | |||
| 299 | + | if err != nil { | |||
| 300 | + | t.Fatal(err) | |||
| 301 | + | } | |||
| 302 | + | defer func() { _ = fc.Close() }() | |||
| 303 | + | if _, err := fc.Init(); err != nil { | |||
| 304 | + | t.Fatal(err) | |||
| 305 | + | } | |||
| 306 | + | // The step swaps the directory for a link to another one. | |||
| 307 | + | if err := os.Rename(dir, dir+".old"); err != nil { | |||
| 308 | + | t.Fatal(err) | |||
| 309 | + | } | |||
| 310 | + | if err := os.Symlink(outside, dir); err != nil { | |||
| 311 | + | t.Fatal(err) | |||
| 312 | + | } | |||
| 313 | + | fc.suffix = "x" | |||
| 314 | + | s := newSink() | |||
| 315 | + | fc.Process(s) | |||
| 316 | + | if len(s.env) != 0 { | |||
| 317 | + | t.Fatalf("a file of the swapped directory was read: %v", s.env) | |||
| 318 | + | } | |||
| 319 | + | // Init keeps working on the original directory, not on the link's target. | |||
| 320 | + | env, err := fc.Init() | |||
| 321 | + | if err != nil { | |||
| 322 | + | t.Fatal(err) | |||
| 323 | + | } | |||
| 324 | + | if _, err := os.Stat(filepath.Join(outside, filepath.Base(env["GITHUB_ENV"]))); err == nil { | |||
| 325 | + | t.Fatal("a file was created through the swapped directory") | |||
| 326 | + | } | |||
| 327 | + | } | |||
| 328 | + | ||||
| 329 | + | func TestCreateExclReplacesAndNeverFollows(t *testing.T) { | |||
| 330 | + | dir := t.TempDir() | |||
| 331 | + | root, err := os.OpenRoot(dir) | |||
| 332 | + | if err != nil { | |||
| 333 | + | t.Fatal(err) | |||
| 334 | + | } | |||
| 335 | + | defer func() { _ = root.Close() }() | |||
| 336 | + | victim := filepath.Join(t.TempDir(), "victim") | |||
| 337 | + | if err := os.WriteFile(victim, []byte("keep"), 0o600); err != nil { | |||
| 338 | + | t.Fatal(err) | |||
| 339 | + | } | |||
| 340 | + | if err := os.Symlink(victim, filepath.Join(dir, "f")); err != nil { | |||
| 341 | + | t.Fatal(err) | |||
| 342 | + | } | |||
| 343 | + | if err := createExcl(root, "f", []byte("new"), 0o644, nil); err != nil { | |||
| 344 | + | t.Fatal(err) | |||
| 345 | + | } | |||
| 346 | + | if b, _ := os.ReadFile(victim); string(b) != "keep" { | |||
| 347 | + | t.Fatalf("the target of the link was written: %q", b) | |||
| 348 | + | } | |||
| 349 | + | if b, _ := os.ReadFile(filepath.Join(dir, "f")); string(b) != "new" { | |||
| 350 | + | t.Fatalf("%q", b) | |||
| 351 | + | } | |||
| 352 | + | if fi, _ := os.Lstat(filepath.Join(dir, "f")); fi.Mode()&os.ModeSymlink != 0 { | |||
| 353 | + | t.Fatal("still a link") | |||
| 354 | + | } | |||
| 355 | + | } | |||
M internal/executor/job.go +24 -7
| @@ -118,12 +118,14 @@ func merge(cur *stepResult, coming stepResult) stepResult { | |||
| 118 | 118 | } | |
| 119 | 119 | ||
| 120 | 120 | type job struct { | |
| 121 | - | cfg Config | |
| 122 | - | cancel *Canceller | |
| 123 | - | wf *workflow.Workflow | |
| 124 | - | jb *workflow.Job | |
| 125 | - | fc *FileCommands | |
| 126 | - | trackID string | |
| 121 | + | cfg Config | |
| 122 | + | cancel *Canceller | |
| 123 | + | wf *workflow.Workflow | |
| 124 | + | jb *workflow.Job | |
| 125 | + | fc *FileCommands | |
| 126 | + | // tempRoot is RUNNER_TEMP, opened before the first step (see FileCommands). | |
| 127 | + | tempRoot *os.Root | |
| 128 | + | trackID string | |
| 127 | 129 | ||
| 128 | 130 | mu sync.Mutex // serialises the Reporter and guards state and result | |
| 129 | 131 | state *State | |
| @@ -149,6 +151,14 @@ func Run(cancel *Canceller, cfg Config) Result { | |||
| 149 | 151 | cancel = NewCanceller() | |
| 150 | 152 | } | |
| 151 | 153 | j := &job{cfg: cfg, cancel: cancel, trackID: "github_" + uuid.NewString()} | |
| 154 | + | defer func() { | |
| 155 | + | if j.fc != nil { | |
| 156 | + | _ = j.fc.Close() | |
| 157 | + | } | |
| 158 | + | if j.tempRoot != nil { | |
| 159 | + | _ = j.tempRoot.Close() | |
| 160 | + | } | |
| 161 | + | }() | |
| 152 | 162 | res := j.run() | |
| 153 | 163 | cfg.Reporter.JobCompleted(res) | |
| 154 | 164 | return res | |
| @@ -373,8 +383,15 @@ func (j *job) setUp() ([]*workflow.Step, error) { | |||
| 373 | 383 | if err != nil { | |
| 374 | 384 | return nil, err | |
| 375 | 385 | } | |
| 376 | - | fc.Own = j.own | |
| 386 | + | if c := cfg.Credential; c != nil { | |
| 387 | + | fc.SetOwner(int(c.Uid), int(c.Gid)) | |
| 388 | + | } | |
| 377 | 389 | j.fc = fc | |
| 390 | + | // Everything the executor writes below the temp directory later goes through | |
| 391 | + | // this root (see FileCommands): the user of the steps owns the directory. | |
| 392 | + | if j.tempRoot, err = os.OpenRoot(layout.Temp); err != nil { | |
| 393 | + | return nil, err | |
| 394 | + | } | |
| 378 | 395 | for _, d := range []string{layout.WorkRoot, layout.RunnerWorkspace, layout.Workspace, layout.Temp, fc.dir} { | |
| 379 | 396 | j.own(d) | |
| 380 | 397 | } | |
| @@ -118,12 +118,14 @@ func merge(cur *stepResult, coming stepResult) stepResult { | |||||
| 118 | } | 118 | } | ||
| 119 | 119 | ||||
| 120 | type job struct { | 120 | type job struct { | ||
| 121 | - | cfg Config | 121 | + | cfg Config |
| 122 | - | cancel *Canceller | 122 | + | cancel *Canceller |
| 123 | - | wf *workflow.Workflow | 123 | + | wf *workflow.Workflow |
| 124 | - | jb *workflow.Job | 124 | + | jb *workflow.Job |
| 125 | - | fc *FileCommands | 125 | + | fc *FileCommands |
| 126 | - | trackID string | 126 | + | // tempRoot is RUNNER_TEMP, opened before the first step (see FileCommands). |
| 127 | + | tempRoot *os.Root | |||
| 128 | + | trackID string | |||
| 127 | 129 | ||||
| 128 | mu sync.Mutex // serialises the Reporter and guards state and result | 130 | mu sync.Mutex // serialises the Reporter and guards state and result | ||
| 129 | state *State | 131 | state *State | ||
| @@ -149,6 +151,14 @@ func Run(cancel *Canceller, cfg Config) Result { | |||||
| 149 | cancel = NewCanceller() | 151 | cancel = NewCanceller() | ||
| 150 | } | 152 | } | ||
| 151 | j := &job{cfg: cfg, cancel: cancel, trackID: "github_" + uuid.NewString()} | 153 | j := &job{cfg: cfg, cancel: cancel, trackID: "github_" + uuid.NewString()} | ||
| 154 | + | defer func() { | |||
| 155 | + | if j.fc != nil { | |||
| 156 | + | _ = j.fc.Close() | |||
| 157 | + | } | |||
| 158 | + | if j.tempRoot != nil { | |||
| 159 | + | _ = j.tempRoot.Close() | |||
| 160 | + | } | |||
| 161 | + | }() | |||
| 152 | res := j.run() | 162 | res := j.run() | ||
| 153 | cfg.Reporter.JobCompleted(res) | 163 | cfg.Reporter.JobCompleted(res) | ||
| 154 | return res | 164 | return res | ||
| @@ -373,8 +383,15 @@ func (j *job) setUp() ([]*workflow.Step, error) { | |||||
| 373 | if err != nil { | 383 | if err != nil { | ||
| 374 | return nil, err | 384 | return nil, err | ||
| 375 | } | 385 | } | ||
| 376 | - | fc.Own = j.own | 386 | + | if c := cfg.Credential; c != nil { |
| 387 | + | fc.SetOwner(int(c.Uid), int(c.Gid)) | |||
| 388 | + | } | |||
| 377 | j.fc = fc | 389 | j.fc = fc | ||
| 390 | + | // Everything the executor writes below the temp directory later goes through | |||
| 391 | + | // this root (see FileCommands): the user of the steps owns the directory. | |||
| 392 | + | if j.tempRoot, err = os.OpenRoot(layout.Temp); err != nil { | |||
| 393 | + | return nil, err | |||
| 394 | + | } | |||
| 378 | for _, d := range []string{layout.WorkRoot, layout.RunnerWorkspace, layout.Workspace, layout.Temp, fc.dir} { | 395 | for _, d := range []string{layout.WorkRoot, layout.RunnerWorkspace, layout.Workspace, layout.Temp, fc.dir} { | ||
| 379 | j.own(d) | 396 | j.own(d) | ||
| 380 | } | 397 | } | ||
M internal/executor/job_test.go +27 -0
| @@ -3,6 +3,8 @@ | |||
| 3 | 3 | package executor | |
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | + | "os" | |
| 7 | + | "path/filepath" | |
| 6 | 8 | "strings" | |
| 7 | 9 | "syscall" | |
| 8 | 10 | "testing" | |
| @@ -497,3 +499,28 @@ jobs: | |||
| 497 | 499 | t.Fatalf("%s %s", last.Conclusion, logOf(last)) | |
| 498 | 500 | } | |
| 499 | 501 | } | |
| 502 | + | ||
| 503 | + | // A step that plants a link where the executor writes event.json (before the | |
| 504 | + | // directory exists, or between two steps) must not make it write elsewhere. | |
| 505 | + | func TestEventJSONIsNeverWrittenThroughALink(t *testing.T) { | |
| 506 | + | needTools(t, "bash") | |
| 507 | + | victim := filepath.Join(t.TempDir(), "victim") | |
| 508 | + | if err := os.WriteFile(victim, []byte("keep"), 0o600); err != nil { | |
| 509 | + | t.Fatal(err) | |
| 510 | + | } | |
| 511 | + | rec, res := runLocal(t, wfHead+` steps: | |
| 512 | + | - run: | | |
| 513 | + | rm -f "$RUNNER_TEMP/_github_workflow/event.json" | |
| 514 | + | ln -s "`+victim+`" "$RUNNER_TEMP/_github_workflow/event.json" | |
| 515 | + | - run: cat "$GITHUB_EVENT_PATH" | |
| 516 | + | `, "j", nil) | |
| 517 | + | if res.Conclusion != ConclusionSuccess { | |
| 518 | + | t.Fatalf("%+v %s", res, logOf(rec.steps[2])) | |
| 519 | + | } | |
| 520 | + | if b, _ := os.ReadFile(victim); string(b) != "keep" { | |
| 521 | + | t.Fatalf("the target of the link was overwritten: %q", b) | |
| 522 | + | } | |
| 523 | + | if got := logOf(rec.steps[2]); !strings.Contains(got, `"ref": "refs/heads/scn/x/r1"`) { | |
| 524 | + | t.Fatalf("the second step does not see a fresh event.json:\n%s", got) | |
| 525 | + | } | |
| 526 | + | } | |
| @@ -3,6 +3,8 @@ | |||||
| 3 | package executor | 3 | package executor | ||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | + | "os" | |||
| 7 | + | "path/filepath" | |||
| 6 | "strings" | 8 | "strings" | ||
| 7 | "syscall" | 9 | "syscall" | ||
| 8 | "testing" | 10 | "testing" | ||
| @@ -497,3 +499,28 @@ jobs: | |||||
| 497 | t.Fatalf("%s %s", last.Conclusion, logOf(last)) | 499 | t.Fatalf("%s %s", last.Conclusion, logOf(last)) | ||
| 498 | } | 500 | } | ||
| 499 | } | 501 | } | ||
| 502 | + | ||||
| 503 | + | // A step that plants a link where the executor writes event.json (before the | |||
| 504 | + | // directory exists, or between two steps) must not make it write elsewhere. | |||
| 505 | + | func TestEventJSONIsNeverWrittenThroughALink(t *testing.T) { | |||
| 506 | + | needTools(t, "bash") | |||
| 507 | + | victim := filepath.Join(t.TempDir(), "victim") | |||
| 508 | + | if err := os.WriteFile(victim, []byte("keep"), 0o600); err != nil { | |||
| 509 | + | t.Fatal(err) | |||
| 510 | + | } | |||
| 511 | + | rec, res := runLocal(t, wfHead+` steps: | |||
| 512 | + | - run: | | |||
| 513 | + | rm -f "$RUNNER_TEMP/_github_workflow/event.json" | |||
| 514 | + | ln -s "`+victim+`" "$RUNNER_TEMP/_github_workflow/event.json" | |||
| 515 | + | - run: cat "$GITHUB_EVENT_PATH" | |||
| 516 | + | `, "j", nil) | |||
| 517 | + | if res.Conclusion != ConclusionSuccess { | |||
| 518 | + | t.Fatalf("%+v %s", res, logOf(rec.steps[2])) | |||
| 519 | + | } | |||
| 520 | + | if b, _ := os.ReadFile(victim); string(b) != "keep" { | |||
| 521 | + | t.Fatalf("the target of the link was overwritten: %q", b) | |||
| 522 | + | } | |||
| 523 | + | if got := logOf(rec.steps[2]); !strings.Contains(got, `"ref": "refs/heads/scn/x/r1"`) { | |||
| 524 | + | t.Fatalf("the second step does not see a fresh event.json:\n%s", got) | |||
| 525 | + | } | |||
| 526 | + | } | |||
M internal/executor/shell.go +11 -1
| @@ -7,6 +7,7 @@ import ( | |||
| 7 | 7 | "os" | |
| 8 | 8 | "path/filepath" | |
| 9 | 9 | "strings" | |
| 10 | + | "syscall" | |
| 10 | 11 | ||
| 11 | 12 | "github.com/google/uuid" | |
| 12 | 13 | ) | |
| @@ -295,7 +296,16 @@ func WriteScript(tempDir string, p ShellPlan, contents string) (Script, error) { | |||
| 295 | 296 | return Script{}, err | |
| 296 | 297 | } | |
| 297 | 298 | contents = FixUpScriptContents(p.Command, contents) | |
| 298 | - | if err := os.WriteFile(path, []byte(contents), 0o644); err != nil { | |
| 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 { | |
| 299 | 309 | return Script{}, fmt.Errorf("write script: %w", err) | |
| 300 | 310 | } | |
| 301 | 311 | return Script{Path: path, Argv: append([]string{p.CommandPath}, ParseArguments(args)...)}, nil | |
| @@ -7,6 +7,7 @@ import ( | |||||
| 7 | "os" | 7 | "os" | ||
| 8 | "path/filepath" | 8 | "path/filepath" | ||
| 9 | "strings" | 9 | "strings" | ||
| 10 | + | "syscall" | |||
| 10 | 11 | ||||
| 11 | "github.com/google/uuid" | 12 | "github.com/google/uuid" | ||
| 12 | ) | 13 | ) | ||
| @@ -295,7 +296,16 @@ func WriteScript(tempDir string, p ShellPlan, contents string) (Script, error) { | |||||
| 295 | return Script{}, err | 296 | return Script{}, err | ||
| 296 | } | 297 | } | ||
| 297 | contents = FixUpScriptContents(p.Command, contents) | 298 | contents = FixUpScriptContents(p.Command, contents) | ||
| 298 | - | if err := os.WriteFile(path, []byte(contents), 0o644); err != nil { | 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 { | |||
| 299 | return Script{}, fmt.Errorf("write script: %w", err) | 309 | return Script{}, fmt.Errorf("write script: %w", err) | ||
| 300 | } | 310 | } | ||
| 301 | return Script{Path: path, Argv: append([]string{p.CommandPath}, ParseArguments(args)...)}, nil | 311 | return Script{Path: path, Argv: append([]string{p.CommandPath}, ParseArguments(args)...)}, nil | ||