From 01afda8b3bce4caae17ff136a776510ffa752c2e Mon Sep 17 00:00:00 2001 From: Kylie Stradley <4666485+KyFaSt@users.noreply.github.com> Date: Fri, 21 Feb 2025 18:14:52 +0000 Subject: [PATCH 1/8] sequester issue_comment triggered untrusted checkout from other triggers * 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 --- .../codeql/actions/security/ControlChecks.qll | 11 +- .../security/UntrustedCheckoutQuery.qll | 10 +- .../CWE-829/UntrustedCheckoutCritical.md | 2 +- .../Security/CWE-829/UntrustedCheckoutHigh.md | 2 +- .../Security/CWE-829/UntrustedCheckoutHigh.ql | 18 +- .../UntrustedCheckoutIssueCommentCritical.md | 14 + .../UntrustedCheckoutIssueCommentCritical.ql | 55 +++ .../UntrustedCheckoutIssueCommentHigh.md | 14 + .../UntrustedCheckoutIssueCommentHigh.ql | 30 ++ .../UntrustedCheckoutCritical.expected | 13 - .../CWE-829/UntrustedCheckoutHigh.expected | 16 - ...ustedCheckoutIssueCommentCritical.expected | 337 ++++++++++++++++++ ...ntrustedCheckoutIssueCommentCritical.qlref | 1 + ...UntrustedCheckoutIssueCommentHigh.expected | 15 + .../UntrustedCheckoutIssueCommentHigh.qlref | 1 + 15 files changed, 489 insertions(+), 50 deletions(-) create mode 100644 actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.md create mode 100644 actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql create mode 100644 actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.md create mode 100644 actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql create mode 100644 actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.expected create mode 100644 actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.qlref create mode 100644 actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.expected create mode 100644 actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.qlref diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 244c04310d6d..8f170086de32 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -32,13 +32,22 @@ string actor_is_attacker_event() { } string actor_not_attacker_event() { + result = + [ + // actor and attacker can be different + // actor may be a collaborator, but the attacker is may be the author of the PR that gets commented + // therefore it may be vulnerable to TOCTOU races where the actor reviews one thing and the attacker changes it + "pull_request_comment", "issue_comment" + ] +} + +string actor_not_attacker_issue_comment_event() { result = [ // actor and attacker can be different // actor may be a collaborator, but the attacker is may be the author of the PR that gets commented // therefore it may be vulnerable to TOCTOU races where the actor reviews one thing and the attacker changes it "issue_comment", - "pull_request_comment", ] } diff --git a/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll b/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll index 9668fce2ae00..a7641808b87f 100644 --- a/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll +++ b/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll @@ -4,6 +4,14 @@ private import codeql.actions.dataflow.FlowSources private import codeql.actions.TaintTracking string checkoutTriggers() { + result = ["pull_request_target", "workflow_run", "workflow_call"] +} + +string issueCommentTriggers() { + result = ["issue_comment"] +} + +string allCheckoutTriggers() { result = ["pull_request_target", "workflow_run", "workflow_call", "issue_comment"] } @@ -77,7 +85,7 @@ module ActionsMutableRefCheckoutFlow = TaintTracking::Global Date: Fri, 21 Feb 2025 18:45:27 +0000 Subject: [PATCH 2/8] remove unused predicate --- .../ql/lib/codeql/actions/security/ControlChecks.qll | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/actions/ql/lib/codeql/actions/security/ControlChecks.qll b/actions/ql/lib/codeql/actions/security/ControlChecks.qll index 8f170086de32..3bb69b65f42c 100644 --- a/actions/ql/lib/codeql/actions/security/ControlChecks.qll +++ b/actions/ql/lib/codeql/actions/security/ControlChecks.qll @@ -41,16 +41,6 @@ string actor_not_attacker_event() { ] } -string actor_not_attacker_issue_comment_event() { - result = - [ - // actor and attacker can be different - // actor may be a collaborator, but the attacker is may be the author of the PR that gets commented - // therefore it may be vulnerable to TOCTOU races where the actor reviews one thing and the attacker changes it - "issue_comment", - ] -} - /** An If node that contains an actor, user or label check */ abstract class ControlCheck extends AstNode { ControlCheck() { From 3b5d8b55c080e5ab7315f8a0b641205cd6d0bce4 Mon Sep 17 00:00:00 2001 From: Kylie Stradley <4666485+KyFaSt@users.noreply.github.com> Date: Fri, 21 Feb 2025 19:06:36 +0000 Subject: [PATCH 3/8] rename test --- .../CWE-829/UntrustedCheckoutIssueCommentCritical.qlref | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.qlref b/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.qlref index cd855d053122..be0ad9813d6e 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.qlref +++ b/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.qlref @@ -1 +1 @@ -Security/CWE-829/UntrustedCheckoutTriggeredByIssueCommentCritical.ql +Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql From e343646ad74b541083ac1ad1a13f29126462ff8a Mon Sep 17 00:00:00 2001 From: Kylie Stradley <4666485+KyFaSt@users.noreply.github.com> Date: Fri, 21 Feb 2025 19:06:52 +0000 Subject: [PATCH 4/8] fix id typos --- .../Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql | 2 +- .../src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql index 34c7f99359bd..c5567a409d47 100644 --- a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql +++ b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql @@ -7,7 +7,7 @@ * @problem.severity warning * @precision high * @security-severity 7.5 - * @id actions/untrusted-checkout-triggered-by-issue-comment/critical + * @id actions/untrusted-checkout-issue-comment/critical * @tags actions * security * external/cwe/cwe-829 diff --git a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql index e83007810166..b102a7bf1c3d 100644 --- a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql +++ b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql @@ -7,7 +7,7 @@ * @problem.severity warning * @precision high * @security-severity 7.5 - * @id actions/untrusted-checkout/high + * @id actions/untrusted-checkout-issue-comment/high * @tags actions * security * external/cwe/cwe-829 From 8ab3ce0fb87f11e4092a8db39d8c745ade6cf130 Mon Sep 17 00:00:00 2001 From: Kylie Stradley <4666485+KyFaSt@users.noreply.github.com> Date: Tue, 25 Feb 2025 19:42:36 +0000 Subject: [PATCH 5/8] autoformat untrusted checkout query qll --- .../actions/security/UntrustedCheckoutQuery.qll | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll b/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll index a7641808b87f..605008c2402d 100644 --- a/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll +++ b/actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll @@ -3,13 +3,9 @@ private import codeql.actions.DataFlow private import codeql.actions.dataflow.FlowSources private import codeql.actions.TaintTracking -string checkoutTriggers() { - result = ["pull_request_target", "workflow_run", "workflow_call"] -} +string checkoutTriggers() { result = ["pull_request_target", "workflow_run", "workflow_call"] } -string issueCommentTriggers() { - result = ["issue_comment"] -} +string issueCommentTriggers() { result = ["issue_comment"] } string allCheckoutTriggers() { result = ["pull_request_target", "workflow_run", "workflow_call", "issue_comment"] @@ -84,8 +80,7 @@ module ActionsMutableRefCheckoutFlow = TaintTracking::Global Date: Thu, 27 Feb 2025 21:33:20 +0000 Subject: [PATCH 6/8] set security-severity to 0 --- .../Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql | 2 +- .../src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql index c5567a409d47..6341f8eee608 100644 --- a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql +++ b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql @@ -6,7 +6,7 @@ * @kind path-problem * @problem.severity warning * @precision high - * @security-severity 7.5 + * @security-severity 0.0 * @id actions/untrusted-checkout-issue-comment/critical * @tags actions * security diff --git a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql index b102a7bf1c3d..683b80db3ffe 100644 --- a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql +++ b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql @@ -6,7 +6,7 @@ * @kind problem * @problem.severity warning * @precision high - * @security-severity 7.5 + * @security-severity 0.0 * @id actions/untrusted-checkout-issue-comment/high * @tags actions * security From aad59c654d845ac63f21573089eed0caec90527d Mon Sep 17 00:00:00 2001 From: Kylie Stradley <4666485+KyFaSt@users.noreply.github.com> Date: Thu, 27 Feb 2025 21:33:32 +0000 Subject: [PATCH 7/8] linting full stop --- .../Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql | 2 +- .../src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql index 6341f8eee608..d3b8d2e0ecd4 100644 --- a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql +++ b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentCritical.ql @@ -52,4 +52,4 @@ where not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout")) and not exists(ControlCheck check | check.protects(poisonable, event, "untrusted-checkout")) select poisonable, checkout, poisonable, - "Potential execution of untrusted code on a privileged workflow ($@)", event, event.getName() + "Potential execution of untrusted code on a privileged workflow ($@).", event, event.getName() diff --git a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql index 683b80db3ffe..51c4d40be628 100644 --- a/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql +++ b/actions/ql/src/Security/CWE-829/UntrustedCheckoutIssueCommentHigh.ql @@ -26,5 +26,5 @@ where inPrivilegedContext(checkout, event) and event.getName() = issueCommentTriggers() and not exists(ControlCheck check | check.protects(checkout, event, "untrusted-checkout")) -select checkout, "Potential execution of untrusted code on a privileged workflow ($@)", event, +select checkout, "Potential execution of untrusted code on a privileged workflow ($@).", event, event.getName() From 9b1a10a782d591c6f98714164cc8d3833926f85a Mon Sep 17 00:00:00 2001 From: Kylie Stradley <4666485+KyFaSt@users.noreply.github.com> Date: Fri, 28 Feb 2025 21:42:34 +0000 Subject: [PATCH 8/8] add change-notes --- actions/ql/src/change-notes/released/0.6.0.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 actions/ql/src/change-notes/released/0.6.0.md diff --git a/actions/ql/src/change-notes/released/0.6.0.md b/actions/ql/src/change-notes/released/0.6.0.md new file mode 100644 index 000000000000..7973f4745def --- /dev/null +++ b/actions/ql/src/change-notes/released/0.6.0.md @@ -0,0 +1,5 @@ +## 0.6.0 + +### Major Analysis Improvements + +* Moved the trigger `issueComment` in the actions/untrusted-checkout-high (CWE-829) and actions/untrusted-checkout-critical (CWE-829) queries to separate queries (actions/untrusted-checkout-issue-comment-high and actions/untrusted-checkout-issue-comment-critical) because while they are vulnerable to the same attack, they do not have the same resolution. Further work is needed to make this trigger safer to use.