github / docs

The open-source repo for docs.github.com
https://docs.github.com
Creative Commons Attribution 4.0 International
16.33k stars 59.84k forks source link

Misleading script injection mitigation #17902

Closed laurentsimon closed 1 year ago

laurentsimon commented 2 years ago

Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-action-instead-of-an-inline-script-recommended

What part(s) of the article would you like to see updated?

The current "recommended" mitigation to script injections in GitHub workflows is to use a GitHub Action, see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-action-instead-of-an-inline-script-recommended

I think this is mis-leading. All this recommendation is doing is tell users to use an action, basically moving the vulnerability inside the action instead of the workflow itself.

As a user, I may create a local Action and introduce the exact same vulnerability using a script, which is not going to mitigate the problem. If the intention is to say "don't use a script and use javascript, possibly inside an Action", I think that would be fine. But the way it's phrased right now is mis-leading. Using an Action does not mitigate script injections.

A drawback of GHA is that it's yet another dependency to maintain and another attack surface.

The right way to fix this vulnerability is to declare an env variable, which is given as a second option. Users worried about script injection in their script are unlikely to move to an Action, which takes additional work. Users use inline scripts because it's the easy option, so I think we should meet them where they're at.

Additional information

Content Plan

Content plan here

No response

welcome[bot] commented 2 years ago

Thanks for opening this issue. A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

steveward commented 2 years ago

Thanks for the issue @laurentsimon, I'll get this triaged for the team to review.

github-actions[bot] commented 2 years ago

Thanks for opening an issue! We've triaged this issue for technical review by a subject matter expert :eyes:

github-actions[bot] commented 2 years ago

This is a gentle bump for the docs team that this issue is waiting for technical review.

juliandunn commented 2 years ago

If the intention is to say "don't use a script and use javascript, possibly inside an Action", I think that would be fine.

That is the intention and we should reword it to be clearer that this is the recommendation.

I hear you about "Users use inline scripts because it's the easy option" but that is the reason for giving the second option. It still doesn't change the fact that to truly improve security they need to check & sanitize their inputs and the better way to do that is not in an inline shell script.

janiceilene commented 2 years ago

@laurentsimon You or anyone else are welcome to make the update outlined in the comment above ☝️

laurentsimon commented 2 years ago

I hear you about "Users use inline scripts because it's the easy option" but that is the reason for giving the second option. It still doesn't change the fact that to truly improve security they need to check & sanitize their inputs

+1 for that. Using env variables in inline script is sanitization, in some sense.

and the better way to do that is not in an inline shell script.

I don't disagree. Just that it begs the question: who is writing the Action? The same user will make the same mistake in the Action.