Refactored the docker run command in the run-in-docker-action to use environment variables instead of direct GitHub context interpolation.
Directly embedding context data like github.workspace or inputs.command into shell scripts can lead to injection if the data contains unexpected characters. Mapping these to environment variables and referencing them within the script is more robust.
Tested this change by ensuring the variable mapping correctly passes data to the docker container.
~ 0
While this strictly follows general security practices, I don’t see the value in these specific changes for two reasons:
Neither github.workspace nor inputs.command appear to be under direct user control in this context.
The content of inputs.command is intended to be executed in the shell, so treating it as an executable string seems appropriate.
Implementation note: Regardless of the above, there is no need to introduce a new environment variable WORKSPACE, as GHA already provides GITHUB_WORKSPACE.
@hebasto Thanks for the review. I’ve updated the action to use the built-in GITHUB_WORKSPACE and removed the redundant variable.
Regarding the injection hardening: while these contexts aren’t directly user-controlled here, mapping them to environment variables is a more robust way to handle multiline inputs and prevents potential quote-mangling in the bash -c wrapping.
@real-or-random Thanks for the ACK. Glad to see support for establishing safer patterns in the CI scripts.
39@@ -40,14 +40,16 @@ runs:
40 shell: bash
41
42 - # Tell Docker to pass environment variables in `env` into the container.
43- run: >
44+ shell: bash
45+ env:
46+ CMD_TO_RUN: ${{ inputs.command }}
CMD_TO_RUN now leaks into the container.
I think unsetting it within the command substitution would avoid it (if we want to):
0$(unset CMD_TO_RUN; echo '${{ toJSON(env) }}' | jq -r 'keys[] | "--env \(.) "') \
jq filter to specifically exclude CMD_TO_RUN so it doesn’t leak into the container environment. Fixed in the latest commit.
47+ run: |
48 docker run \
49- $(echo '${{ toJSON(env) }}' | jq -r 'keys[] | "--env \(.) "') \
50- --volume ${{ github.workspace }}:${{ github.workspace }} \
51- --workdir ${{ github.workspace }} \
52+ $(echo '${{ toJSON(env) }}' | jq -r 'keys[] | select(. != "CMD_TO_RUN") | "--env \(.) "') \
${{ toJSON(env) }}.
echo should close that gap, as it avoids shell interpretation of the JSON string entirely. I’ve updated the action to use this more isolated approach and squashed the commits.
Refactored the docker run command in the run-in-docker-action to use
environment variables instead of direct GitHub context interpolation.
To prevent potential shell injection when processing environment
variables, the context data is written to a temporary file, which is
then parsed by jq. This avoids passing raw context strings through
shell command substitution.
Tested this change by ensuring the variable mapping correctly passes
data to the docker container.
$(echo '${{ toJSON(env) }}' ...) (because env contains a '), then you can also break out of echo '${{ toJSON(env) }}' not inside a command substitution.
Concept NACK
Sorry for the sudden 180. I didn’t think about this enough when I gave a Concept ACK. I believe @hebasto is right:
github.workspace is determined by GitHub Actions. If the attacker controls this, it’s game over. (It’s very different from the example in the blog post, e.g., an issue title that could be set by a user.)inputs.command, then the attacker can run arbitrary code anyway.With that in mind, the changes here add complexity without providing a tangible benefit.
If you think I’m missing something and these changes have some value, then I’d ask you to provide a clear model of an attacker that they (should) protect against. Then we can clearly evaluate whether it’s worth protecting against this kind of attacker.
I’m sorry for wasting your time! :/
@real-or-random Valid point. Switched to mapping the JSON blob to a dedicated environment variable before writing it to the temporary file. This keeps the data entirely out of the shell’s command line and avoids interpretation issues if env contains single quotes.
Also excluded the new ENV_JSON variable from the container’s --env flags to keep things clean.
Labels
ci