What to do about runner.temp in template injection check?

[funnelfiasco]

In #128, I proposed to exclude runner.temp in the template injection check because it's a value provided by GitHub and thus not exploitable the way, say, a branch name could be.

@woodruffw said:

I think in practice it can't be attacker controlled, but I'm having trouble proving that (and in particular, if it can contain spaces anything else special, it can potentially be an injection source).

I also am having trouble proving it. I can't find in GitHub's docs a description of how it's defined, but it appears to be a predictable directory with a UUID (for example /home/runner/work/_temp/bf7c360e-7ad0-4c65-b10f-ce00c3516531). Even if it does contain spaces or the like, it's not clear to me how it could be used as an attack vector.

But this may be a failure in imagination on my part, and I agree with the principle of not excluding things we aren't very sure about, so I'm starting this discussion to see what I may have missed.

[woodruffw]

Thanks! I appreciate you starting the discussion.

I'll do some more research tonight as well, but I agree currently with your appraisal that it seems like runner.temp can't be attacker controlled in any meaningful way.

I think the only case where this might not be true is a fully attacker controlled/compromised runner, in which case all contexts are potentially injectable and the entire audit is moot. So that's not worth considering 🙂

[funnelfiasco]

If you could figure out how to write an audit rule for "this will be run on an adversarial GitHub runner", you would get a lot of money 😆

[lazka]

I had a quick look for where the value is coming from, maybe it is helpful:

• https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Worker/TempDirectoryManager.cs#L24-L31
• https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Common/HostContext.cs#L267-L325
• https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Listener/CommandSettings.cs#L324-L330
• https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Listener/Configuration/ConfigurationManager.cs#L410
• https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Common/Constants.cs#L225-L228
• https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Worker/RunnerContext.cs#L9-L15

So in summary, the workdir is set via the CLI when the runner is set up and defaults to "_work". This is then written to a JSON file and loaded by the runner. That is combined with the "root" directory, which is the directory the runner .exe lives in. Then this is combined with "_temp". And that is written to runner.temp. And that is then written to the RUNNER_TEMP env var.

So. from what I see, the only variable input here is the path to the runner .exe and what the admin entered as the workdir when the runner was set up.

And further more, the github example in the docs use it as is in a shell context: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-usage-of-the-runner-context

[woodruffw]

Thanks for the deep dive here @lazka!

Given all that, it sounds like there's no problem with considering runner.temp 100% safe. I'd be happy to review a PR that adds it to the safe list if someone wants, otherwise I'll get to it in the coming days.

[woodruffw]

#277 will allowlist runner.temp. Thanks again for opening this @funnelfiasco, and to @lazka for drilling down and proving that this is fine to filter!