Skip to content

Commit 55476af

Browse files
author
Alvaro Muñoz
authored
Merge pull request #107 from github/query_if
query: split if expression is always true query
2 parents 80f2b24 + db6f174 commit 55476af

12 files changed

+236
-22
lines changed

‎ql/src/Security/CWE-571/ExpressionIsAlwaysTrue.md renamed to ‎ql/src/Security/CWE-571/ExpressionIsAlwaysTrueCritical.md

File renamed without changes.

‎ql/src/Security/CWE-571/ExpressionIsAlwaysTrue.ql renamed to ‎ql/src/Security/CWE-571/ExpressionIsAlwaysTrueCritical.ql

+8-9
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,26 @@
11
/**
2-
*:
3-
*
42
* @name If expression always true
53
* @description Expressions used in If conditions with extra spaces are always true.
64
* @kind problem
75
* @security-severity 9.0
86
* @problem.severity error
9-
* @precision high
10-
* @id actions/if-expression-always-true
7+
* @precision very-high
8+
* @id actions/if-expression-always-true/critical
119
* @tags actions
1210
* maintainability
1311
* external/cwe/cwe-275
1412
*/
1513

1614
import actions
15+
import codeql.actions.security.ControlChecks
1716

18-
from If i
17+
from ControlCheck i
1918
where
20-
i.getCondition().matches("%${{%") and
19+
i.(If).getCondition().matches("%${{%") and
2120
(
22-
not i.getCondition().matches("${{%") or
23-
not i.getCondition().matches("%}}")
21+
not i.(If).getCondition().matches("${{%") or
22+
not i.(If).getCondition().matches("%}}")
2423
)
2524
or
26-
count(i.getCondition().splitAt("${{")) > 2
25+
count(i.(If).getCondition().splitAt("${{")) > 2
2726
select i, "Expression always evaluates to true"

‎ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.md

+63
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
# If Condition Always Evaluates to True
2+
3+
## Description
4+
5+
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`.
6+
7+
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.
8+
9+
## Recommendation
10+
11+
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:
12+
13+
1. Do not use `${{` and `}}` for Workflow Expressions in `if` conditions.
14+
2. Avoid multiline or spaced-out conditional expressions that might inadvertently introduce unwanted characters or formatting.
15+
3. Test the workflow to ensure the `if` conditions behave as expected under different scenarios.
16+
17+
## Examples
18+
19+
### Correct Usage
20+
21+
1. Omit `${{` and `}}` in `if` conditions:
22+
23+
```yaml
24+
if: steps.checks.outputs.safe_to_run == true
25+
if: |-
26+
steps.checks.outputs.safe_to_run == true
27+
if: |
28+
steps.checks.outputs.safe_to_run == true
29+
```
30+
31+
2. If using `${{` and `}}` Workflow Expressions, ensure the `if` condition is formatted correctly without extra spaces or characters:
32+
33+
```yaml
34+
if: ${{ steps.checks.outputs.safe_to_run == true }}
35+
if: |-
36+
${{ steps.checks.outputs.safe_to_run == true }}
37+
```
38+
39+
### Incorrect Usage
40+
41+
1. Do not mix Workflow Expressions with un-delimited expressions:
42+
43+
```yaml
44+
if: ${{ steps.checks.outputs.safe_to_run }} == true
45+
```
46+
47+
2. Do not include trailing new lines or spaces:
48+
49+
```yaml
50+
if: |
51+
${{ steps.checks.outputs.safe_to_run == true }}
52+
if: >
53+
${{ steps.checks.outputs.safe_to_run == true }}
54+
if: " ${{ steps.checks.outputs.safe_to_run == true }}"
55+
if: |+
56+
${{ steps.checks.outputs.safe_to_run == true }}
57+
if: >+
58+
${{ steps.checks.outputs.safe_to_run == true }}
59+
```
60+
61+
## References
62+
63+
- [Expression Always True Github Issue](https://github.com/actions/runner/issues/1173)

‎ql/src/Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
/**
2+
* @name If expression always true
3+
* @description Expressions used in If conditions with extra spaces are always true.
4+
* @kind problem
5+
* @problem.severity error
6+
* @precision high
7+
* @security-severity 7.5
8+
* @id actions/if-expression-always-true/high
9+
* @tags actions
10+
* maintainability
11+
* external/cwe/cwe-275
12+
*/
13+
14+
import actions
15+
import codeql.actions.security.ControlChecks
16+
17+
from If i
18+
where
19+
not i instanceof ControlCheck and
20+
(
21+
i.getCondition().matches("%${{%") and
22+
(
23+
not i.getCondition().matches("${{%") or
24+
not i.getCondition().matches("%}}")
25+
)
26+
or
27+
count(i.getCondition().splitAt("${{")) > 2
28+
)
29+
select i, "Expression always evaluates to true"

‎ql/test/query-tests/Security/CWE-571/.github/workflows/test.yml renamed to ‎ql/test/query-tests/Security/CWE-571/.github/workflows/test1.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ jobs:
9191
if: ${{ github.event_name }} == 'foo'
9292
run: echo "Test 18 should not be printed"
9393
- name: Test 19
94-
if: ${{ contains(fromJSON('["OWNER", "MEMBER"]'), github.event.pull_request.author_association )}} || github.actor == 'renovate[bot]'
94+
if: ${{ contains(fromJSON('["OWNER", "MEMBER"]'), github.event.pull_request.foo )}} || github.event_name == 'foo'
9595
run: echo "Test 19 should not be printed"
9696
- name: Test 20
9797
if: ${{ hashFiles('./docker/Dockerfile.debian') }} != ""

‎ql/test/query-tests/Security/CWE-571/.github/workflows/test2.yml

+111
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
name: Event
2+
3+
on:
4+
workflow_dispatch:
5+
6+
jobs:
7+
if-tests:
8+
runs-on: ubuntu-latest
9+
permissions: {}
10+
steps:
11+
- name: Test 1
12+
if: github.actor == "foo"
13+
run: echo "Test 1 should not be printed"
14+
- name: Test 2
15+
if: |
16+
${{
17+
github.actor == "foo" ||
18+
3 == 4
19+
}}
20+
run: echo "Test 2 should not be printed"
21+
- name: Test 3
22+
if: ${{ github.actor == "foo" }}
23+
run: echo "Test 3 should not be printed"
24+
- name: Test 4
25+
if: ${{ github.actor == "foo" }}
26+
run: echo "Test 4 should not be printed"
27+
- name: Test 5
28+
if: ${{
29+
github.actor == "foo" ||
30+
3 == 4
31+
}}
32+
run: echo "Test 5 should not be printed"
33+
- name: Test 6
34+
if: ${{ 1 == 1 }} ${{ github.actor == "foo" }}
35+
run: echo "Test 6 should not be printed"
36+
- name: Test 7
37+
run: echo "Test 7 should not be printed"
38+
if: ${{
39+
github.actor == "foo" ||
40+
3 == 4
41+
}}
42+
43+
- name: Test 8
44+
run: echo "Test 8 should not be printed"
45+
if: >
46+
${{
47+
github.actor == "foo" ||
48+
3 == 4 }}
49+
- name: Test 9
50+
if: '${{ github.actor == "foo" }}'
51+
run: echo "Test 9 should not be printed"
52+
- name: Test 10
53+
if: "${{ github.actor == 111 }}"
54+
run: echo "Test 10 should not be printed"
55+
- name: Test 11
56+
if: " ${{ github.actor == 111 }}"
57+
run: echo "Test 11 should not be printed"
58+
- name: Test 12
59+
if: " ${{ github.actor == 111 }}"
60+
run: echo "Test 12 should not be printed"
61+
- name: Test 13
62+
if: |
63+
github.actor == "foo" ||
64+
3 == 4
65+
run: echo "Test 13 should not be printed"
66+
- name: Test 14
67+
if: >-
68+
${{(
69+
false || github.actor == "foo"
70+
)}}
71+
run: echo "Test 14 should not be printed"
72+
- name: Test 15
73+
if: |-
74+
${{(
75+
false || github.actor == "foo"
76+
)}}
77+
run: echo "Test 15 should not be printed"
78+
- name: Test 16
79+
if: |+
80+
${{(
81+
false || github.actor == "foo"
82+
)}}
83+
run: echo "Test 16 should not be printed"
84+
- name: Test 17
85+
if: >+
86+
${{(
87+
false || github.actor == "foo"
88+
)}}
89+
run: echo "Test 17 should not be printed"
90+
- name: Test 18
91+
if: ${{ github.actor }} == 'foo'
92+
run: echo "Test 18 should not be printed"
93+
- name: Test 19
94+
if: ${{ contains(fromJSON('["OWNER", "MEMBER"]'), github.event.pull_request.author_association )}} || github.actor == 'renovate[bot]'
95+
run: echo "Test 19 should not be printed"
96+
- name: Test 20
97+
if: ${{ github.actor }} != ""
98+
run: echo "Test 20 should not be printed"
99+
- name: Test 21
100+
if: >
101+
${{ github.actor == 'foo' &&
102+
github.event.workflow_run.conclusion == 'success' }}
103+
run: echo "Test 21 should not be printed"
104+
- name: Test 22
105+
if: |
106+
runner.os == 'Windows' && (
107+
startsWith(inputs.node, 'v10.') ||
108+
startsWith(inputs.node, 'v12.') ||
109+
startsWith(inputs.node, 'v14.')
110+
)
111+
run: echo "Test 22 should not be printed"

‎ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.expected

-11
This file was deleted.

‎ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrue.qlref

-1
This file was deleted.

‎ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.expected

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
| .github/workflows/test2.yml:15:13:19:13 | \| | Expression always evaluates to true |
2+
| .github/workflows/test2.yml:34:13:34:54 | ${{ 1 = ... foo" }} | Expression always evaluates to true |
3+
| .github/workflows/test2.yml:45:13:48:24 | > | Expression always evaluates to true |
4+
| .github/workflows/test2.yml:56:15:56:44 | " ${{ g ... 11 }}" | Expression always evaluates to true |
5+
| .github/workflows/test2.yml:59:15:59:44 | " ${{ g ... 11 }}" | Expression always evaluates to true |
6+
| .github/workflows/test2.yml:79:13:82:14 | \|+ | Expression always evaluates to true |
7+
| .github/workflows/test2.yml:85:13:88:14 | >+ | Expression always evaluates to true |
8+
| .github/workflows/test2.yml:91:13:91:40 | ${{ git ... = 'foo' | Expression always evaluates to true |
9+
| .github/workflows/test2.yml:94:13:94:141 | ${{ con ... e[bot]' | Expression always evaluates to true |
10+
| .github/workflows/test2.yml:97:13:97:37 | ${{ git ... } != "" | Expression always evaluates to true |
11+
| .github/workflows/test2.yml:100:13:102:63 | > | Expression always evaluates to true |

‎ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueCritical.qlref

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-571/ExpressionIsAlwaysTrueCritical.ql

‎ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.expected

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
| .github/workflows/test1.yml:15:13:19:13 | \| | Expression always evaluates to true |
2+
| .github/workflows/test1.yml:34:13:34:39 | ${{ 1 = ... == 2 }} | Expression always evaluates to true |
3+
| .github/workflows/test1.yml:45:13:48:24 | > | Expression always evaluates to true |
4+
| .github/workflows/test1.yml:56:15:56:31 | " ${{ 1 == 2 }}" | Expression always evaluates to true |
5+
| .github/workflows/test1.yml:59:15:59:31 | " ${{ 1 == 2 }}" | Expression always evaluates to true |
6+
| .github/workflows/test1.yml:79:13:82:14 | \|+ | Expression always evaluates to true |
7+
| .github/workflows/test1.yml:85:13:88:14 | >+ | Expression always evaluates to true |
8+
| .github/workflows/test1.yml:91:13:91:45 | ${{ git ... = 'foo' | Expression always evaluates to true |
9+
| .github/workflows/test1.yml:94:13:94:121 | ${{ con ... = 'foo' | Expression always evaluates to true |
10+
| .github/workflows/test1.yml:97:13:97:64 | ${{ has ... } != "" | Expression always evaluates to true |
11+
| .github/workflows/test1.yml:100:13:102:63 | > | Expression always evaluates to true |

‎ql/test/query-tests/Security/CWE-571/ExpressionIsAlwaysTrueHigh.qlref

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-571/ExpressionIsAlwaysTrueHigh.ql

0 commit comments

Comments
 (0)