Don't force set the executable bit on scripts to be set

[kuroneko]

At present, buildkite sets the executable bit on any script it attempts to
execute as part of a job.

This is actually quite hazardous as there may be files that are valid
interpreter scripts, but are dangerous to run -- by having this behaviour, it
actually subverts any good that no-command-eval can achieve by leaving another
potentially gaping hole which we can't turn off!

It's also relatively unnecessary - git, by default, tries to store the state of
the execute bit into the repo. We should not subvert this if the user doesn't
want us to.

This patch adds a flag that can disable this behaviour, so, if you're security
minded, and you really need to be able to ensure that arbitrary files in your
repo cannot be invoked, without use of a whitelist, you have the ability to do
so!

[lox]

Thanks very much for this @kuroneko!

by having this behaviour, it actually subverts any good that no-command-eval can achieve by leaving another potentially gaping hole which we can't turn off!

Could you expand a bit on this? What would be an example of an attack on this?

This patch adds a flag that can disable this behaviour

If we can, I'd love to remove the behaviour entirely if we think it's exploitable!

[kuroneko]

The issue created is that now any file in your repository becomes a potential entrypoint for the agent to use, not just the ones you intended, subverting the general UNIX file permissions mechanism (and the level to which Git supports that)..

All attack vectors depend upon either the buildkite masters being compromised and/or impersonated, or attacks via the pipeline definition. The pipeline definition attack is less of a concern for us since it's usually visible - Our concerns were more oriented towards the former. The risks of this aren't treated here because they're outside of our control as the endusers of your service.

I assume this is the vector that no-command-eval was intended to protect against too.

The problem is, because the behaviour is one where any file can become set +x (or at least, that's what it looked like to me in my quick analysis of the behaviour in bootstrap), including scripts not intended to be executable, or binaries that just happen to have valid executable headers, it becomes possible for an attacker to try to introduce a payload they can exploit as a seemingly innocent part of another change, but doesn't look exploitable because it's a fragment of another part that's missing the execute bit, or because it's obfuscated into another type of binary, and then through any failure to control what is passed to a no-command-eval agent, invoke that payload.

[lox]

it becomes possible for an attacker to try to introduce a payload they can exploit as a seemingly innocent part of another change, but doesn't look exploitable because it's a fragment of another part that's missing the execute bit, or because it's obfuscated into another type of binary, and then through any failure to control what is passed to a no-command-eval agent, invoke that payload.

That makes a lot of sense, thanks!

Will discuss with the team what approach we want to take. Appreciate the write up!

[lox]

What would you think of disabling the setting of the executable bit when no-command-eval is set? That way we avoid another security config.

[kuroneko]

What would you think of disabling the setting of the executable bit when no-command-eval is set? That way we avoid another security config.

Sounds good to me - I only put in an extra option in the event that somebody relied on the combinational behaviour.

[kuroneko]

New commit now only relies on the existing CommandEval flag, eliminating all the config change requirements.

[lox]

Thanks for the contribution and interesting discussion @kuroneko!