Security Hardening for Environment Variables

[RedYetiDev]

This PR prevents a workflow run executing arbitrary code using environment variables.

Caution

This is a security patch.
I do not see this as an immediate threat, as the workflow that sets the variables would also need to be vulnerable.

[platisd]

Thanks for the PR. Can you please elaborate on a scenario where this would be an issue?

[RedYetiDev]

Yes! In this scenario, a user has set up a workflow and is using environment variables to store arbitrary data. For example:

echo "DATA=$(cat path/to/arbitrary/file.txt)" >> $GITHUB_ENV

The user might assume their runner is safe because they are not using a runtime (e.g., bash or python) elsewhere (even if they are using this action).

However, this action uses bash and python, both of which are vulnerable to environment variable injection.

[oleg-derevenetz]

Hi @RedYetiDev Although I agree that the introduction of environment variables is a potential attack vector in certain cases, hardening this way looks somewhat inefficient. You set the BASH_ENV and PYTHONWARNINGS, but not, e.g. PATH that could also be tampered with using such an injection, and the wrong binary can be used in this case - for example, in the action_launcher.bash script the wrong ln binary could be used (since ln IIRC isn't a bash builtin command).

[RedYetiDev]

Okay, what do you recommend? You can replace the binaries with /usr/bin/... if I recall

[oleg-derevenetz]

You can replace the binaries with /usr/bin/... if I recall

Initially this Action was created as a Docker action, and has additional protection against those injections by design. But later it was converted to the composite action to make it easier to run it on Windows runners (including self-hosted ones), so hardcoding the paths as /usr/bin/... or whatever also doesn't look as a good idea IMHO.

[RedYetiDev]

Okay, well I'm not quite sure what's the best approach. I think that users should be aware of the risk, or it should be reduced (to the best of the actions ability)

[oleg-derevenetz]

I think that users should be aware of the risk, or it should be reduced (to the best of the actions ability)

I believe that:

1. If a workflow that calls this Action is already vulnerable, then there will be more attack vectors than just this Action. For example, if PATH is tampered with, then literally any command in that workflow that is called without specifying the full path (a common practice) becomes the target. We can't do anything with that.
2. I suspect that there are much more "sensitive" environment variables that could be tampered with. For instance, how about PYTHONPATH (possibility to load Python modules from a wrong place)? CDPATH (cd some_dir && ./some_script)? Even HOME - yes, it also could be set externally when bash is not executed in the login mode (~/bin/some_script)? Etc, etc. This plugging of holes does not help much because it is most likely impossible to take everything into account, and even if you close all potential holes of this kind now, who guarantees that support for new "sensitive" environment variables will not appear, for instance, in the newer Python versions?

[RedYetiDev]

Okay, so it's up to the implementations and how people use this.

[platisd]

Very nice and educational discussion 🚀