Mitigate shell injection attack

[ad-m-ss]

Hi,

I have impression that the current version of workflows is vulnerable to shell injection attack. See https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#good-practices-for-mitigating-script-injection-attacks about this class of vulnerability.

I follow mitigations recommendation and added intermediary environment variable.

Kind regards

[ad-m-ss]

@sondrelg could you take a look? You introduced most of affected code.

[codecov[bot]]

Codecov Report

Merging #60 (23af218) into main (9a06221) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #60   +/-   ##
=======================================
  Coverage   93.15%   93.15%           
=======================================
  Files           1        1           
  Lines         219      219           
  Branches       53       53           
=======================================
  Hits          204      204           
  Misses          7        7           
  Partials        8        8           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sondrelg]

What would inject what? This is only run in your own workflows and inputs are parsed and passed in by the Github workflow engine. Not against this change, just trying to understand why it's needed 🙂

[JonasKs]

I am also unsure how this makes it more secure 🤔 The example is about "do not use user-input in your actions", e.g. do not allow a PR title to specify which images should be deleted.

In other words, as I read it, it is guidelines to the ones implementing a workflow, not the workflow implementation itself?

[ad-m-ss]

Users of this action do not need to be aware that they should not pass user-controller input to parameters of it. It is difficult to control how users use the action (they may pass a parameter as a result of some previous step), so there is no need to put them at risk.

The risk here is low, but as I read the code, it caught my eye, so I proposed a fix as a general good practice to mitigate this.

[sondrelg]

I appreciate the contribution :clap: Just still trying to understand:

• How is it currently unsafe? Can you give me an example of how the current action could be abused?
• How does this mitigate the risk?

[JonasKs]

I’m with Sondre - really appreciate the contribution, just trying to understand it properly before we merge. 😊

[ad-m-ss]

How is it currently unsafe? Can you give me an example of how the current action could be abused?

Sure, see here https://github.com/ad-m-ss/container-retention-policy-poc/actions/runs/4679651354/workflow that I injected shell command to parameter:

That bash script can be used eg. to leak PAT by encrypting it and outputing it to console.

How does this mitigate the risk?

Parameters are never evaluated as bash script. It's always passed to script as is. See:

https://github.com/ad-m-ss/container-retention-policy-poc/actions/runs/4679675956/workflow

[ad-m-ss]

@JonasKs @sondrelg Is there anything that I can do to help move it forward?

[JonasKs]

Sorry, we're both a bit busy at the moment, and I'm out of the country for two weeks. I'll try to investigate your findings early next week.

[ad-m-ss]

Take your time. I was afraid it was overlooked, but I fully understand the priorities. It's not critical :)

[sondrelg]

Thanks a lot @ad-m-ss :clap: Do we all agree this is a bugfix?