Actions: Sequester issue_comment triggered untrusted checkout from other triggers#18838
Open
KyFaSt wants to merge 10 commits intogithub:mainfrom
Open
Actions: Sequester issue_comment triggered untrusted checkout from other triggers#18838KyFaSt wants to merge 10 commits intogithub:mainfrom
KyFaSt wants to merge 10 commits intogithub:mainfrom
Conversation
* issue_comment triggered untrusted checkouts present a security risk but mitigating the risk cannot be done wholly in the workflow relying on the event and those mitigations cannot be detected by CodeQL so these triggers should be moved to separate alerts with level warning
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql
Fixed
Show fixed
Hide fixed
| @@ -0,0 +1 @@ | |||
| Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -0,0 +1 @@ | |||
| Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
KyFaSt
changed the title
…b.com/KyFaSt/codeql into kyfast/untrusted-checkout-refinements
| string checkoutTriggers() { | ||
| string checkoutTriggers() { result = ["pull_request_target", "workflow_run", "workflow_call"] } | ||
|
|
||
| string issueCommentTriggers() { result = ["issue_comment"] } |
Check warning
Code scanning / CodeQL
Singleton set literal Warning
| * @problem.severity warning | ||
| * @precision high | ||
| * @security-severity 7.5 | ||
| * @security-severity 0.0 |
Collaborator
There was a problem hiding this comment.
I think we'll want to avoid this. We can discuss internally. Do we have known CVEs involving this problem, that we can use to compute the severity based on CVSS?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
issue_commenttriggered untrusted checkouts present a security risk but mitigating the risk cannot be done wholly in the workflow relying on the event and those mitigations cannot be detected by CodeQL so these triggers should be moved to separate alerts with levelwarning. See https://github.blog/security/application-security/how-to-secure-your-github-actions-workflows-with-codeql/#issueoops-security-pitfalls-with-issue_comment-trigger for more details.I removed the
issue_commenttrigger from the untrusted checkout high and critical and created new alerts with mitigation advice more suited towardsissue_comment. I think it's important to warn developers about the risks of this workflow trigger, but understand it may not be possible for projects that rely heavily on IssueOps