mirror of
https://github.com/github/codeql.git
synced 2026-01-06 11:10:23 +01:00
query: split if expression is always true query
critical - if the if statement contains a known control check high - otherwise
This commit is contained in:
Alvaro Muñoz
@@ -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"
|
||||
63
ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.md
Normal file
63
ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.md
Normal file
@@ -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)
|
||||
29
ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql
Normal file
29
ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql
Normal file
@@ -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"
|
||||
@@ -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') }} != ""
|
||||
111
ql/test/query-tests/Security/CWE-571/.github/workflows/test2.yml
vendored
Normal file
111
ql/test/query-tests/Security/CWE-571/.github/workflows/test2.yml
vendored
Normal file
@@ -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"
|
||||
@@ -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 |
|
||||
@@ -1 +0,0 @@
|
||||
Security/CWE-571/ExpressionIsAlwaysTrue.ql
|
||||
@@ -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 |
|
||||
@@ -0,0 +1 @@
|
||||
Security/CWE-571/ExpressionIsAlwaysTrueCritical.ql
|
||||
@@ -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 |
|
||||
@@ -0,0 +1 @@
|
||||
Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql
|
||||
Reference in New Issue
Block a user