Skip to content

Commit 839d16c

Browse files
author
Alvaro Muñoz
committed
Treat If's values as expression no matter the delimiters
1 parent 1bf2431 commit 839d16c

File tree

10 files changed

+215
-24
lines changed

10 files changed

+215
-24
lines changed

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,16 @@
11
private import codeql.actions.ast.internal.Ast
22
private import codeql.Locations
33

4+
module Utils {
5+
bindingset[expr]
6+
string normalizeExpr(string expr) {
7+
result =
8+
expr.regexpReplaceAll("[\\.\\'\\[\\]\"]+", ".")
9+
.regexpReplaceAll("\\.$", "")
10+
.regexpReplaceAll("\\.\\s", " ")
11+
}
12+
}
13+
414
class AstNode instanceof AstNodeImpl {
515
AstNode getAChildNode() { result = super.getAChildNode() }
616

@@ -188,6 +198,8 @@ class Step extends AstNode instanceof StepImpl {
188198
*/
189199
class If extends AstNode instanceof IfImpl {
190200
string getCondition() { result = super.getCondition() }
201+
202+
Expression getConditionExpr() { result = super.getConditionExpr() }
191203
}
192204

193205
abstract class Uses extends AstNode instanceof UsesImpl {

‎ql/lib/codeql/actions/ast/internal/Ast.qll

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,14 @@ private newtype TAstNode =
4545
)
4646
)
4747
)
48+
or
49+
// if's conditions do not need to be delimted with ${{}}
50+
exists(YamlMapping m |
51+
m.maps(key, value) and
52+
key.(YamlScalar).getValue() = ["if"] and
53+
value.getValue() = raw and
54+
exprOffset = 1
55+
)
4856
} or
4957
TCompositeAction(YamlMapping n) {
5058
n instanceof YamlDocument and
@@ -123,7 +131,7 @@ class ScalarValueImpl extends AstNodeImpl, TScalarValueNode {
123131

124132
override Location getLocation() { result = value.getLocation() }
125133

126-
override YamlNode getNode() { result = value }
134+
override YamlScalar getNode() { result = value }
127135
}
128136

129137
class ExpressionImpl extends AstNodeImpl, TExpressionNode {
@@ -135,15 +143,16 @@ class ExpressionImpl extends AstNodeImpl, TExpressionNode {
135143

136144
ExpressionImpl() {
137145
this = TExpressionNode(key, value, rawExpression, exprOffset - 1) and
138-
expression =
139-
rawExpression.regexpCapture("\\$\\{\\{\\s*([A-Za-z0-9_\\[\\]\\*\\((\\)\\.\\-]+)\\s*\\}\\}", 1)
146+
if rawExpression.trim().regexpMatch("\\$\\{\\{.*\\}\\}")
147+
then expression = rawExpression.trim().regexpCapture("\\$\\{\\{\\s*(.*)\\s*\\}\\}", 1).trim()
148+
else expression = rawExpression.trim()
140149
}
141150

142151
override string toString() { result = expression }
143152

144153
override AstNodeImpl getAChildNode() { none() }
145154

146-
override AstNodeImpl getParentNode() { result.getNode() = value }
155+
override ScalarValueImpl getParentNode() { result.getNode() = value }
147156

148157
override string getAPrimaryQlClass() { result = "ExpressionNode" }
149158

@@ -638,6 +647,9 @@ class IfImpl extends AstNodeImpl, TIfNode {
638647

639648
/** Gets the condition that must be satisfied for this job to run. */
640649
string getCondition() { result = n.(YamlScalar).getValue() }
650+
651+
/** Gets the condition that must be satisfied for this job to run. */
652+
ExpressionImpl getConditionExpr() { result.getParentNode().getNode() = n }
641653
}
642654

643655
class EnvImpl extends AstNodeImpl, TEnvNode {

‎ql/src/Security/CWE-829/UntrustedCheckout.ql

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,29 +18,32 @@ import actions
1818
/** An If node that contains an actor, user or label check */
1919
class ControlCheck extends If {
2020
ControlCheck() {
21-
this.getCondition().regexpMatch(".*github\\.(triggering_)?actor.*") or
22-
this.getCondition().regexpMatch(".*github\\.event\\.pull_request\\.user\\.login.*") or
23-
this.getCondition().regexpMatch(".*github\\.event\\.pull_request\\.labels.*") or
24-
this.getCondition().regexpMatch(".*github\\.event\\.label\\.name.*")
21+
Utils::normalizeExpr(this.getCondition())
22+
.regexpMatch([
23+
".*github\\.actor.*", ".*github\\.triggering_actor.*",
24+
".*github\\.event\\.pull_request\\.user\\.login.*",
25+
".*github\\.event\\.pull_request\\.labels.*", ".*github\\.event\\.label\\.name.*"
26+
])
2527
}
2628
}
2729

2830
bindingset[s]
2931
predicate containsHeadRef(string s) {
30-
s.matches("%" +
31-
[
32-
"github.event.number", // The pull request number.
33-
"github.event.pull_request.head.ref", // The ref name of head.
34-
"github.event.pull_request.head.sha", // The commit SHA of head.
35-
"github.event.pull_request.id", // The pull request ID.
36-
"github.event.pull_request.number", // The pull request number.
37-
"github.event.pull_request.merge_commit_sha", // The SHA of the merge commit.
38-
"github.head_ref", // The head_ref or source branch of the pull request in a workflow run.
39-
"github.event.workflow_run.head_branch", // The branch of the head commit.
40-
"github.event.workflow_run.head_commit.id", // The SHA of the head commit.
41-
"github.event.workflow_run.head_sha", // The SHA of the head commit.
42-
"env.GITHUB_HEAD_REF",
43-
] + "%")
32+
Utils::normalizeExpr(s)
33+
.matches("%" +
34+
[
35+
"github.event.number", // The pull request number.
36+
"github.event.pull_request.head.ref", // The ref name of head.
37+
"github.event.pull_request.head.sha", // The commit SHA of head.
38+
"github.event.pull_request.id", // The pull request ID.
39+
"github.event.pull_request.number", // The pull request number.
40+
"github.event.pull_request.merge_commit_sha", // The SHA of the merge commit.
41+
"github.head_ref", // The head_ref or source branch of the pull request in a workflow run.
42+
"github.event.workflow_run.head_branch", // The branch of the head commit.
43+
"github.event.workflow_run.head_commit.id", // The SHA of the head commit.
44+
"github.event.workflow_run.head_sha", // The SHA of the head commit.
45+
"env.GITHUB_HEAD_REF",
46+
] + "%")
4447
}
4548

4649
/** Checkout of a Pull Request HEAD ref */

‎ql/test/library-tests/test.expected

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ parentNodes
190190
| .github/workflows/test.yml:34:9:34:23 | ${{ always() }} | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
191191
| .github/workflows/test.yml:34:9:34:23 | ${{ always() }} | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
192192
| .github/workflows/test.yml:34:10:34:24 | always() | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
193-
| .github/workflows/test.yml:34:10:34:24 | always() | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
193+
| .github/workflows/test.yml:34:11:34:25 | always() | .github/workflows/test.yml:34:9:34:23 | ${{ always() }} |
194194
| .github/workflows/test.yml:36:12:36:15 | job1 | .github/workflows/test.yml:1:1:40:53 | on: push |
195195
| .github/workflows/test.yml:36:12:36:15 | job1 | .github/workflows/test.yml:32:5:40:53 | Job: job2 |
196196
| .github/workflows/test.yml:36:12:36:15 | job1 | .github/workflows/test.yml:32:5:40:53 | Job: job2 |
@@ -415,3 +415,8 @@ calls
415415
| .github/workflows/test.yml:19:9:26:6 | Uses Step: step | mad9000/actions-find-and-replace-string |
416416
needs
417417
| .github/workflows/test.yml:40:20:40:53 | needs.job1.outputs.job_output |
418+
testNormalizeExpr
419+
| foo['bar'] == baz | foo.bar == baz |
420+
| github.event.pull_request.user["login"] | github.event.pull_request.user.login |
421+
| github.event.pull_request.user['login'] | github.event.pull_request.user.login |
422+
| github.event.pull_request['user']['login'] | github.event.pull_request.user.login |

‎ql/test/library-tests/test.ql

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import codeql.actions.Ast
2+
import codeql.actions.Ast::Utils as Utils
23
import codeql.actions.Cfg as Cfg
34
import codeql.actions.DataFlow
45
import codeql.Locations
@@ -59,3 +60,12 @@ query predicate summaries(string action, string version, string input, string ou
5960
query predicate calls(DataFlow::CallNode call, string callee) { callee = call.getCallee() }
6061

6162
query predicate needs(DataFlow::Node e) { e.asExpr() instanceof NeedsExpression }
63+
64+
query string testNormalizeExpr(string s) {
65+
s =
66+
[
67+
"github.event.pull_request.user['login']", "github.event.pull_request.user[\"login\"]",
68+
"github.event.pull_request['user']['login']", "foo['bar'] == baz"
69+
] and
70+
result = Utils::normalizeExpr(s)
71+
}

‎ql/test/query-tests/Security/CWE-094/CriticalExpressionInjection.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ edges
33
| .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE |
44
| .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] |
55
| .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files |
6+
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log |
67
| .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced |
78
| .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:57:29:57:73 | steps.remove_quotations.outputs.replaced |
89
| .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] |
@@ -58,6 +59,8 @@ nodes
5859
| .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | semmle.label | steps.remove_quotations.outputs.replaced |
5960
| .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | semmle.label | Uses Step: changed-files |
6061
| .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | semmle.label | steps.changed-files.outputs.all_changed_files |
62+
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
63+
| .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log |
6164
| .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | semmle.label | github.event.comment.body |
6265
| .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | semmle.label | github.event.comment.body |
6366
| .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | semmle.label | github.event.issue.body |
@@ -187,6 +190,7 @@ nodes
187190
subpaths
188191
#select
189192
| .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | steps.remove_quotations.outputs.replaced |
193+
| .github/workflows/changelog.yml:58:26:58:39 | env.log | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/changelog.yml:58:26:58:39 | env.log | env.log |
190194
| .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | github.event.comment.body |
191195
| .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | github.event.comment.body |
192196
| .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | github.event.issue.body |

‎ql/test/query-tests/Security/CWE-094/ExpressionInjection.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ edges
33
| .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE |
44
| .github/workflows/argus_case_study.yml:22:20:22:39 | env.ISSUE_TITLE | .github/workflows/argus_case_study.yml:15:9:24:6 | Uses Step: remove_quotations [replaced] |
55
| .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files |
6+
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log |
67
| .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:39:31:39:75 | steps.remove_quotations.outputs.replaced |
78
| .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] | .github/workflows/cross3.yml:57:29:57:73 | steps.remove_quotations.outputs.replaced |
89
| .github/workflows/cross3.yml:32:18:32:53 | github.event.commits[0].message | .github/workflows/cross3.yml:27:7:37:4 | Uses Step: remove_quotations [replaced] |
@@ -58,6 +59,8 @@ nodes
5859
| .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | semmle.label | steps.remove_quotations.outputs.replaced |
5960
| .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | semmle.label | Uses Step: changed-files |
6061
| .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | semmle.label | steps.changed-files.outputs.all_changed_files |
62+
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
63+
| .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log |
6164
| .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | semmle.label | github.event.comment.body |
6265
| .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | semmle.label | github.event.comment.body |
6366
| .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | semmle.label | github.event.issue.body |
@@ -188,6 +191,7 @@ subpaths
188191
#select
189192
| .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | .github/workflows/argus_case_study.yml:17:25:17:53 | github.event.issue.title | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/argus_case_study.yml:27:33:27:77 | steps.remove_quotations.outputs.replaced | ${{steps.remove_quotations.outputs.replaced}} |
190193
| .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:22:24:22:75 | steps.changed-files.outputs.all_changed_files | ${{ steps.changed-files.outputs.all_changed_files }} |
194+
| .github/workflows/changelog.yml:58:26:58:39 | env.log | .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/changelog.yml:58:26:58:39 | env.log | ${{ env.log }} |
191195
| .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:9:15:9:46 | github.event.comment.body | ${{ github.event.comment.body }} |
192196
| .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:15:19:15:50 | github.event.comment.body | ${{ github.event.comment.body }} |
193197
| .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | Potential expression injection in $@, which may be controlled by an external user. | .github/workflows/comment_issue.yml:16:19:16:48 | github.event.issue.body | ${{ github.event.issue.body }} |

0 commit comments

Comments
 (0)