diff --git a/ql/src/Security/CWE-571/ExpressionIsAlwaysTrue.md b/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueCritical.md similarity index 100% rename from ql/src/Security/CWE-571/ExpressionIsAlwaysTrue.md rename to ql/src/Security/CWE-571/ExpressionIsAlwaysTrueCritical.md diff --git a/ql/src/Security/CWE-571/ExpressionIsAlwaysTrue.ql b/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueCritical.ql similarity index 51% rename from ql/src/Security/CWE-571/ExpressionIsAlwaysTrue.ql rename to ql/src/Security/CWE-571/ExpressionIsAlwaysTrueCritical.ql index 58eab4c6022..6eaaca6e05d 100644 --- a/ql/src/Security/CWE-571/ExpressionIsAlwaysTrue.ql +++ b/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueCritical.ql @@ -1,27 +1,26 @@ /** - *: - * * @name If expression always true * @description Expressions used in If conditions with extra spaces are always true. * @kind problem * @security-severity 9.0 * @problem.severity error - * @precision high - * @id actions/if-expression-always-true + * @precision very-high + * @id actions/if-expression-always-true/critical * @tags actions * maintainability * external/cwe/cwe-275 */ import actions +import codeql.actions.security.ControlChecks -from If i +from ControlCheck i where - i.getCondition().matches("%${{%") and + i.(If).getCondition().matches("%${{%") and ( - not i.getCondition().matches("${{%") or - not i.getCondition().matches("%}}") + not i.(If).getCondition().matches("${{%") or + not i.(If).getCondition().matches("%}}") ) or - count(i.getCondition().splitAt("${{")) > 2 + count(i.(If).getCondition().splitAt("${{")) > 2 select i, "Expression always evaluates to true" diff --git a/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.md b/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.md new file mode 100644 index 00000000000..1e7ea120cba --- /dev/null +++ b/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.md @@ -0,0 +1,63 @@ +# If Condition Always Evaluates to True + +## Description + +GitHub Workflow Expressions (`${{ ... }}`) used in the `if` condition of jobs or steps must not contain extra characters or spaces. Otherwise, the condition is invariably evaluated to `true`. + +When an `if` condition erroneously evaluates to `true`, unintended steps may be executed, leading to logic bugs and potentially exposing parts of the workflow designed to run only in secure scenarios. This behavior subverts the intended conditional logic of the workflow, leading to potential security vulnerabilities and unintentional consequences. + +## Recommendation + +To avoid the vulnerability where an `if` condition always evaluates to `true`, it is crucial to eliminate any extra characters or spaces in your GitHub Actions expressions: + +1. Do not use `${{` and `}}` for Workflow Expressions in `if` conditions. +2. Avoid multiline or spaced-out conditional expressions that might inadvertently introduce unwanted characters or formatting. +3. Test the workflow to ensure the `if` conditions behave as expected under different scenarios. + +## Examples + +### Correct Usage + +1. Omit `${{` and `}}` in `if` conditions: + + ```yaml + if: steps.checks.outputs.safe_to_run == true + if: |- + steps.checks.outputs.safe_to_run == true + if: | + steps.checks.outputs.safe_to_run == true + ``` + +2. If using `${{` and `}}` Workflow Expressions, ensure the `if` condition is formatted correctly without extra spaces or characters: + + ```yaml + if: ${{ steps.checks.outputs.safe_to_run == true }} + if: |- + ${{ steps.checks.outputs.safe_to_run == true }} + ``` + +### Incorrect Usage + +1. Do not mix Workflow Expressions with un-delimited expressions: + + ```yaml + if: ${{ steps.checks.outputs.safe_to_run }} == true + ``` + +2. Do not include trailing new lines or spaces: + + ```yaml + if: | + ${{ steps.checks.outputs.safe_to_run == true }} + if: > + ${{ steps.checks.outputs.safe_to_run == true }} + if: " ${{ steps.checks.outputs.safe_to_run == true }}" + if: |+ + ${{ steps.checks.outputs.safe_to_run == true }} + if: >+ + ${{ steps.checks.outputs.safe_to_run == true }} + ``` + +## References + +- [Expression Always True Github Issue](https://github.com/actions/runner/issues/1173) diff --git a/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql b/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql new file mode 100644 index 00000000000..6b0c6997761 --- /dev/null +++ b/ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql @@ -0,0 +1,29 @@ +/** + * @name If expression always true + * @description Expressions used in If conditions with extra spaces are always true. + * @kind problem + * @problem.severity error + * @precision high + * @security-severity 7.5 + * @id actions/if-expression-always-true/high + * @tags actions + * maintainability + * external/cwe/cwe-275 + */ + +import actions +import codeql.actions.security.ControlChecks + +from If i +where + not i instanceof ControlCheck and + ( + i.getCondition().matches("%${{%") and + ( + not i.getCondition().matches("${{%") or + not i.getCondition().matches("%}}") + ) + or + count(i.getCondition().splitAt("${{")) > 2 + ) +select i, "Expression always evaluates to true" diff --git a/ql/test/query-tests/Security/CWE-571/.github/workflows/test.yml b/ql/test/query-tests/Security/CWE-571/.github/workflows/test1.yml similarity index 97% rename from ql/test/query-tests/Security/CWE-571/.github/workflows/test.yml rename to ql/test/query-tests/Security/CWE-571/.github/workflows/test1.yml index 4ed45ff973e..bbbcc5aaa79 100644 --- a/ql/test/query-tests/Security/CWE-571/.github/workflows/test.yml +++ b/ql/test/query-tests/Security/CWE-571/.github/workflows/test1.yml @@ -91,7 +91,7 @@ jobs: if: ${{ github.event_name }} == 'foo' run: echo "Test 18 should not be printed" - name: Test 19 - if: ${{ contains(fromJSON('["OWNER", "MEMBER"]'), github.event.pull_request.author_association )}} || github.actor == 'renovate[bot]' + if: ${{ contains(fromJSON('["OWNER", "MEMBER"]'), github.event.pull_request.foo )}} || github.event_name == 'foo' run: echo "Test 19 should not be printed" - name: Test 20 if: ${{ hashFiles('./docker/Dockerfile.debian') }} != "" diff --git a/ql/test/query-tests/Security/CWE-571/.github/workflows/test2.yml b/ql/test/query-tests/Security/CWE-571/.github/workflows/test2.yml new file mode 100644 index 00000000000..8b863037e29 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-571/.github/workflows/test2.yml @@ -0,0 +1,111 @@ +name: Event + +on: + workflow_dispatch: + +jobs: + if-tests: + runs-on: ubuntu-latest + permissions: {} + steps: + - name: Test 1 + if: github.actor == "foo" + run: echo "Test 1 should not be printed" + - name: Test 2 + if: | + ${{ + github.actor == "foo" || + 3 == 4 + }} + run: echo "Test 2 should not be printed" + - name: Test 3 + if: ${{ github.actor == "foo" }} + run: echo "Test 3 should not be printed" + - name: Test 4 + if: ${{ github.actor == "foo" }} + run: echo "Test 4 should not be printed" + - name: Test 5 + if: ${{ + github.actor == "foo" || + 3 == 4 + }} + run: echo "Test 5 should not be printed" + - name: Test 6 + if: ${{ 1 == 1 }} ${{ github.actor == "foo" }} + run: echo "Test 6 should not be printed" + - name: Test 7 + run: echo "Test 7 should not be printed" + if: ${{ + github.actor == "foo" || + 3 == 4 + }} + + - name: Test 8 + run: echo "Test 8 should not be printed" + if: > + ${{ + github.actor == "foo" || + 3 == 4 }} + - name: Test 9 + if: '${{ github.actor == "foo" }}' + run: echo "Test 9 should not be printed" + - name: Test 10 + if: "${{ github.actor == 111 }}" + run: echo "Test 10 should not be printed" + - name: Test 11 + if: " ${{ github.actor == 111 }}" + run: echo "Test 11 should not be printed" + - name: Test 12 + if: " ${{ github.actor == 111 }}" + run: echo "Test 12 should not be printed" + - name: Test 13 + if: | + github.actor == "foo" || + 3 == 4 + run: echo "Test 13 should not be printed" + - name: Test 14 + if: >- + ${{( + false || github.actor == "foo" + )}} + run: echo "Test 14 should not be printed" + - name: Test 15 + if: |- + ${{( + false || github.actor == "foo" + )}} + run: echo "Test 15 should not be printed" + - name: Test 16 + if: |+ + ${{( + false || github.actor == "foo" + )}} + run: echo "Test 16 should not be printed" + - name: Test 17 + if: >+ + ${{( + false || github.actor == "foo" + )}} + run: echo "Test 17 should not be printed" + - name: Test 18 + if: ${{ github.actor }} == 'foo' + run: echo "Test 18 should not be printed" + - name: Test 19 + if: ${{ contains(fromJSON('["OWNER", "MEMBER"]'), github.event.pull_request.author_association )}} || github.actor == 'renovate[bot]' + run: echo "Test 19 should not be printed" + - name: Test 20 + if: ${{ github.actor }} != "" + run: echo "Test 20 should not be printed" + - name: Test 21 + if: > + ${{ github.actor == 'foo' && + github.event.workflow_run.conclusion == 'success' }} + run: echo "Test 21 should not be printed" + - name: Test 22 + if: | + runner.os == 'Windows' && ( + startsWith(inputs.node, 'v10.') || + startsWith(inputs.node, 'v12.') || + startsWith(inputs.node, 'v14.') + ) + run: echo "Test 22 should not be printed" diff --git a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.expected b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.expected deleted file mode 100644 index d4c16131cc2..00000000000 --- a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.expected +++ /dev/null @@ -1,11 +0,0 @@ -| .github/workflows/test.yml:15:13:19:13 | \| | Expression always evaluates to true | -| .github/workflows/test.yml:34:13:34:39 | ${{ 1 = ... == 2 }} | Expression always evaluates to true | -| .github/workflows/test.yml:45:13:48:24 | > | Expression always evaluates to true | -| .github/workflows/test.yml:56:15:56:31 | " ${{ 1 == 2 }}" | Expression always evaluates to true | -| .github/workflows/test.yml:59:15:59:31 | " ${{ 1 == 2 }}" | Expression always evaluates to true | -| .github/workflows/test.yml:79:13:82:14 | \|+ | Expression always evaluates to true | -| .github/workflows/test.yml:85:13:88:14 | >+ | Expression always evaluates to true | -| .github/workflows/test.yml:91:13:91:45 | ${{ git ... = 'foo' | Expression always evaluates to true | -| .github/workflows/test.yml:94:13:94:141 | ${{ con ... e[bot]' | Expression always evaluates to true | -| .github/workflows/test.yml:97:13:97:64 | ${{ has ... } != "" | Expression always evaluates to true | -| .github/workflows/test.yml:100:13:102:63 | > | Expression always evaluates to true | diff --git a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.qlref b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.qlref deleted file mode 100644 index 01235fb6a20..00000000000 --- a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE-571/ExpressionIsAlwaysTrue.ql diff --git a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.expected b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.expected new file mode 100644 index 00000000000..2ef457d9e01 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.expected @@ -0,0 +1,11 @@ +| .github/workflows/test2.yml:15:13:19:13 | \| | Expression always evaluates to true | +| .github/workflows/test2.yml:34:13:34:54 | ${{ 1 = ... foo" }} | Expression always evaluates to true | +| .github/workflows/test2.yml:45:13:48:24 | > | Expression always evaluates to true | +| .github/workflows/test2.yml:56:15:56:44 | " ${{ g ... 11 }}" | Expression always evaluates to true | +| .github/workflows/test2.yml:59:15:59:44 | " ${{ g ... 11 }}" | Expression always evaluates to true | +| .github/workflows/test2.yml:79:13:82:14 | \|+ | Expression always evaluates to true | +| .github/workflows/test2.yml:85:13:88:14 | >+ | Expression always evaluates to true | +| .github/workflows/test2.yml:91:13:91:40 | ${{ git ... = 'foo' | Expression always evaluates to true | +| .github/workflows/test2.yml:94:13:94:141 | ${{ con ... e[bot]' | Expression always evaluates to true | +| .github/workflows/test2.yml:97:13:97:37 | ${{ git ... } != "" | Expression always evaluates to true | +| .github/workflows/test2.yml:100:13:102:63 | > | Expression always evaluates to true | diff --git a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.qlref b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.qlref new file mode 100644 index 00000000000..823f802a70f --- /dev/null +++ b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.qlref @@ -0,0 +1 @@ +Security/CWE-571/ExpressionIsAlwaysTrueCritical.ql diff --git a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.expected b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.expected new file mode 100644 index 00000000000..c853603377c --- /dev/null +++ b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.expected @@ -0,0 +1,11 @@ +| .github/workflows/test1.yml:15:13:19:13 | \| | Expression always evaluates to true | +| .github/workflows/test1.yml:34:13:34:39 | ${{ 1 = ... == 2 }} | Expression always evaluates to true | +| .github/workflows/test1.yml:45:13:48:24 | > | Expression always evaluates to true | +| .github/workflows/test1.yml:56:15:56:31 | " ${{ 1 == 2 }}" | Expression always evaluates to true | +| .github/workflows/test1.yml:59:15:59:31 | " ${{ 1 == 2 }}" | Expression always evaluates to true | +| .github/workflows/test1.yml:79:13:82:14 | \|+ | Expression always evaluates to true | +| .github/workflows/test1.yml:85:13:88:14 | >+ | Expression always evaluates to true | +| .github/workflows/test1.yml:91:13:91:45 | ${{ git ... = 'foo' | Expression always evaluates to true | +| .github/workflows/test1.yml:94:13:94:121 | ${{ con ... = 'foo' | Expression always evaluates to true | +| .github/workflows/test1.yml:97:13:97:64 | ${{ has ... } != "" | Expression always evaluates to true | +| .github/workflows/test1.yml:100:13:102:63 | > | Expression always evaluates to true | diff --git a/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.qlref b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.qlref new file mode 100644 index 00000000000..f12135bd1b8 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.qlref @@ -0,0 +1 @@ +Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql