Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEC: fix github workflow vulnerable to script injection #2787

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

diogoteles08
Copy link
Contributor

Hi! I'm Diogo from Google's Open Source Security Team(GOSST) and I'm dropping by to suggest this small change that will enhance the security of your repository by preventing script injection attacks through your GitHub workflows.

In the piece of code I changed, you were directly using the value of a variable that comes from a user's input, so a malicious user could exploit that input and use it to run arbitrary code. By using an intermediate environment variable, the value of the expression is stored in memory, used as a variable and doesn't interact with the script generation process. This mitigates the script injection risks and also keeps your workflow running exactly as before.

You can find more information about this on this github documentation or in this gitguardian blogpost.

Cheers!

Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
@diogoteles08 diogoteles08 changed the title Fix: GitHub workflow vulnerable to script injection SEC: fix github workflow vulnerable to script injection Aug 2, 2024
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.12%. Comparing base (3ad9234) to head (bb3b517).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2787   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files          51       51           
  Lines        8557     8557           
  Branches     1707     1707           
=======================================
  Hits         8140     8140           
  Misses        263      263           
  Partials      154      154           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stefan6419846
Copy link
Collaborator

Thanks for the PR. In theory, running this workflow on untrusted data should not happen due to the collaborators having to merge each PR anyway - especially release PRs should only be accepted when created by collaborators. Nevertheless, it seems like a good approach to follow best practices.

While we are at it and to avoid doubts: The run condition in line 22 can already be considered safe against such attacks?

@diogoteles08
Copy link
Contributor Author

@stefan6419846 Good question. Yeah, in the if block the injections wouldn't work so it's considered safe.

@stefan6419846 stefan6419846 merged commit 582557e into py-pdf:main Aug 2, 2024
17 of 18 checks passed
@pubpub-zz pubpub-zz mentioned this pull request Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants