Skip to content

Commit ce4ce68

Browse files
committed
Fix false positives from static matrix values
1 parent 1c0ae0f commit ce4ce68

8 files changed

Lines changed: 58 additions & 2 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: feature
3+
---
4+
* Added `MatrixExpression.getADeclaredValue()` for accessing scalar values declared for a matrix expression.

actions/ql/lib/codeql/actions/Ast.qll

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,4 +397,7 @@ class InputsExpression extends SimpleReferenceExpression instanceof InputsExpres
397397

398398
class EnvExpression extends SimpleReferenceExpression instanceof EnvExpressionImpl { }
399399

400-
class MatrixExpression extends SimpleReferenceExpression instanceof MatrixExpressionImpl { }
400+
class MatrixExpression extends SimpleReferenceExpression instanceof MatrixExpressionImpl {
401+
/** Gets a scalar value declared for this matrix expression. */
402+
string getADeclaredValue() { result = super.getLiteralValues() }
403+
}

actions/ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,14 @@ abstract class MutableRefCheckoutStep extends PRHeadCheckoutStep { }
227227
/** Checkout of a Pull Request HEAD ref */
228228
abstract class SHACheckoutStep extends PRHeadCheckoutStep { }
229229

230+
private predicate hasOnlyStaticMatrixValues(Expression expr) {
231+
expr instanceof MatrixExpression and
232+
exists(expr.(MatrixExpression).getADeclaredValue()) and
233+
not exists(string value |
234+
value = expr.(MatrixExpression).getADeclaredValue() and value.matches("%${{%")
235+
)
236+
}
237+
230238
/** Checkout of a Pull Request HEAD ref using actions/checkout action */
231239
class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesStep {
232240
ActionsMutableRefCheckout() {
@@ -246,7 +254,8 @@ class ActionsMutableRefCheckout extends MutableRefCheckoutStep instanceof UsesSt
246254
expr.(StepsExpression).getStepId() = value
247255
or
248256
expr.(SimpleReferenceExpression).getFieldName() = value and
249-
not expr instanceof GitHubExpression
257+
not expr instanceof GitHubExpression and
258+
not hasOnlyStaticMatrixValues(expr)
250259
or
251260
expr.(NeedsExpression).getNeededJobId() = value
252261
or
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The untrusted checkout and cache poisoning queries no longer treat references to statically defined matrix values as attacker-controlled checkout references. Matrix values containing GitHub Actions expressions remain reportable.
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
on: issue_comment
2+
3+
jobs:
4+
test:
5+
runs-on: ubuntu-latest
6+
strategy:
7+
matrix:
8+
ref: ["${{ github.event.comment.body }}"]
9+
steps:
10+
- uses: actions/checkout@v4
11+
with:
12+
repository: example/example
13+
ref: ${{ matrix.ref }}
14+
# VULNERABLE: matrix.ref is controlled by the event payload.
15+
- run: npm install
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
on: issue_comment
2+
3+
jobs:
4+
test:
5+
runs-on: ubuntu-latest
6+
strategy:
7+
matrix:
8+
ref: ["main"]
9+
steps:
10+
- uses: actions/checkout@v4
11+
with:
12+
repository: example/example
13+
ref: ${{ matrix.ref }}
14+
# NOT VULNERABLE: matrix.ref can only be the static value "main".
15+
- run: npm install

actions/ql/test/query-tests/Security/CWE-349/CachePoisoningViaDirectCache.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ edges
1717
| .github/workflows/direct_cache5.yml:17:9:21:6 | Uses Step | .github/workflows/direct_cache5.yml:21:9:22:21 | Run Step |
1818
| .github/workflows/direct_cache6.yml:13:9:16:6 | Uses Step | .github/workflows/direct_cache6.yml:16:9:20:6 | Uses Step |
1919
| .github/workflows/direct_cache6.yml:16:9:20:6 | Uses Step | .github/workflows/direct_cache6.yml:20:9:26:46 | Uses Step: cache-pip |
20+
| .github/workflows/dynamic_matrix_ref.yml:10:9:15:6 | Uses Step | .github/workflows/dynamic_matrix_ref.yml:15:9:15:24 | Run Step |
2021
| .github/workflows/neg_direct_cache1.yml:14:9:17:6 | Uses Step | .github/workflows/neg_direct_cache1.yml:17:9:21:6 | Uses Step |
2122
| .github/workflows/neg_direct_cache1.yml:17:9:21:6 | Uses Step | .github/workflows/neg_direct_cache1.yml:21:9:22:21 | Run Step |
2223
| .github/workflows/neg_direct_cache2.yml:14:9:17:6 | Uses Step | .github/workflows/neg_direct_cache2.yml:17:9:21:6 | Uses Step |
@@ -34,6 +35,7 @@ edges
3435
| .github/workflows/neg_poisonable_step1.yml:11:9:14:6 | Uses Step: comment-branch | .github/workflows/neg_poisonable_step1.yml:14:9:19:6 | Uses Step |
3536
| .github/workflows/neg_poisonable_step1.yml:14:9:19:6 | Uses Step | .github/workflows/neg_poisonable_step1.yml:19:9:20:30 | Run Step |
3637
| .github/workflows/neg_poisonable_step2.yml:13:9:16:6 | Uses Step | .github/workflows/neg_poisonable_step2.yml:16:9:17:54 | Run Step |
38+
| .github/workflows/neg_static_matrix_ref.yml:10:9:15:6 | Uses Step | .github/workflows/neg_static_matrix_ref.yml:15:9:15:24 | Run Step |
3739
| .github/workflows/poisonable_step1.yml:10:9:12:6 | Uses Step: comment-branch | .github/workflows/poisonable_step1.yml:12:9:15:6 | Uses Step |
3840
| .github/workflows/poisonable_step1.yml:12:9:15:6 | Uses Step | .github/workflows/poisonable_step1.yml:15:9:17:2 | Run Step |
3941
| .github/workflows/poisonable_step1.yml:21:9:23:6 | Uses Step: comment-branch | .github/workflows/poisonable_step1.yml:23:9:26:6 | Uses Step |

actions/ql/test/query-tests/Security/CWE-349/CachePoisoningViaPoisonableStep.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ edges
2424
| .github/workflows/direct_cache6.yml:13:9:16:6 | Uses Step | .github/workflows/direct_cache6.yml:16:9:20:6 | Uses Step |
2525
| .github/workflows/direct_cache6.yml:15:17:15:57 | github.event.pull_request.head.sha | .github/workflows/direct_cache6.yml:13:9:16:6 | Uses Step |
2626
| .github/workflows/direct_cache6.yml:16:9:20:6 | Uses Step | .github/workflows/direct_cache6.yml:20:9:26:46 | Uses Step: cache-pip |
27+
| .github/workflows/dynamic_matrix_ref.yml:10:9:15:6 | Uses Step | .github/workflows/dynamic_matrix_ref.yml:15:9:15:24 | Run Step |
28+
| .github/workflows/dynamic_matrix_ref.yml:13:17:13:33 | matrix.ref | .github/workflows/dynamic_matrix_ref.yml:10:9:15:6 | Uses Step |
2729
| .github/workflows/neg_direct_cache1.yml:14:9:17:6 | Uses Step | .github/workflows/neg_direct_cache1.yml:17:9:21:6 | Uses Step |
2830
| .github/workflows/neg_direct_cache1.yml:16:17:16:57 | github.event.pull_request.head.sha | .github/workflows/neg_direct_cache1.yml:14:9:17:6 | Uses Step |
2931
| .github/workflows/neg_direct_cache1.yml:17:9:21:6 | Uses Step | .github/workflows/neg_direct_cache1.yml:21:9:22:21 | Run Step |
@@ -46,6 +48,7 @@ edges
4648
| .github/workflows/neg_poisonable_step1.yml:14:9:19:6 | Uses Step | .github/workflows/neg_poisonable_step1.yml:19:9:20:30 | Run Step |
4749
| .github/workflows/neg_poisonable_step1.yml:17:17:17:60 | steps.comment-branch.outputs.head_sha | .github/workflows/neg_poisonable_step1.yml:14:9:19:6 | Uses Step |
4850
| .github/workflows/neg_poisonable_step2.yml:13:9:16:6 | Uses Step | .github/workflows/neg_poisonable_step2.yml:16:9:17:54 | Run Step |
51+
| .github/workflows/neg_static_matrix_ref.yml:10:9:15:6 | Uses Step | .github/workflows/neg_static_matrix_ref.yml:15:9:15:24 | Run Step |
4952
| .github/workflows/poisonable_step1.yml:10:9:12:6 | Uses Step: comment-branch | .github/workflows/poisonable_step1.yml:12:9:15:6 | Uses Step |
5053
| .github/workflows/poisonable_step1.yml:12:9:15:6 | Uses Step | .github/workflows/poisonable_step1.yml:15:9:17:2 | Run Step |
5154
| .github/workflows/poisonable_step1.yml:14:17:14:60 | steps.comment-branch.outputs.head_sha | .github/workflows/poisonable_step1.yml:12:9:15:6 | Uses Step |
@@ -66,6 +69,7 @@ edges
6669
| .github/workflows/poisonable_step5.yml:20:17:20:57 | github.event.pull_request.head.ref | .github/workflows/poisonable_step5.yml:17:9:22:6 | Uses Step |
6770
| .github/workflows/poisonable_step5.yml:22:9:24:6 | Uses Step | .github/workflows/poisonable_step5.yml:24:9:28:31 | Uses Step |
6871
#select
72+
| .github/workflows/dynamic_matrix_ref.yml:15:9:15:24 | Run Step | .github/workflows/dynamic_matrix_ref.yml:13:17:13:33 | matrix.ref | .github/workflows/dynamic_matrix_ref.yml:15:9:15:24 | Run Step | Potential cache poisoning in the context of the default branch due to privilege checkout of untrusted code from $@. ($@). | .github/workflows/dynamic_matrix_ref.yml:13:17:13:33 | matrix.ref | matrix.ref | .github/workflows/dynamic_matrix_ref.yml:1:5:1:17 | issue_comment | issue_comment |
6973
| .github/workflows/poisonable_step1.yml:15:9:17:2 | Run Step | .github/workflows/poisonable_step1.yml:14:17:14:60 | steps.comment-branch.outputs.head_sha | .github/workflows/poisonable_step1.yml:15:9:17:2 | Run Step | Potential cache poisoning in the context of the default branch due to privilege checkout of untrusted code from $@. ($@). | .github/workflows/poisonable_step1.yml:14:17:14:60 | steps.comment-branch.outputs.head_sha | steps.comment-branch.outputs.head_sha | .github/workflows/poisonable_step1.yml:2:3:2:15 | issue_comment | issue_comment |
7074
| .github/workflows/poisonable_step1.yml:26:9:28:2 | Uses Step | .github/workflows/poisonable_step1.yml:25:17:25:60 | steps.comment-branch.outputs.head_sha | .github/workflows/poisonable_step1.yml:26:9:28:2 | Uses Step | Potential cache poisoning in the context of the default branch due to privilege checkout of untrusted code from $@. ($@). | .github/workflows/poisonable_step1.yml:25:17:25:60 | steps.comment-branch.outputs.head_sha | steps.comment-branch.outputs.head_sha | .github/workflows/poisonable_step1.yml:2:3:2:15 | issue_comment | issue_comment |
7175
| .github/workflows/poisonable_step1.yml:37:9:37:75 | Run Step | .github/workflows/poisonable_step1.yml:36:17:36:60 | steps.comment-branch.outputs.head_sha | .github/workflows/poisonable_step1.yml:37:9:37:75 | Run Step | Potential cache poisoning in the context of the default branch due to privilege checkout of untrusted code from $@. ($@). | .github/workflows/poisonable_step1.yml:36:17:36:60 | steps.comment-branch.outputs.head_sha | steps.comment-branch.outputs.head_sha | .github/workflows/poisonable_step1.yml:2:3:2:15 | issue_comment | issue_comment |

0 commit comments

Comments
 (0)