Skip to content

Commit 9c7eecf

Browse files
JarLobJarLob
committed
Add support for composite actions
1 parent baefeab commit 9c7eecf

4 files changed

Lines changed: 183 additions & 71 deletions

File tree

‎javascript/ql/lib/semmle/javascript/Actions.qll‎

Lines changed: 57 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,62 @@ import javascript
1010
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions.
1111
*/
1212
module Actions {
13-
/** A YAML node in a GitHub Actions workflow file. */
13+
/** A YAML node in a GitHub Actions workflow or custom action file. */
1414
private class Node extends YamlNode {
1515
Node() {
16-
this.getLocation()
17-
.getFile()
18-
.getRelativePath()
19-
.regexpMatch("(^|.*/)\\.github/workflows/.*\\.y(?:a?)ml$")
16+
exists(File f |
17+
f = this.getLocation().getFile() and
18+
(
19+
f.getRelativePath().regexpMatch("(^|.*/)\\.github/workflows/.*\\.y(?:a?)ml$")
20+
or
21+
f.getBaseName() = "action.yml"
22+
)
23+
)
2024
}
2125
}
2226

27+
/**
28+
* A custom action. This is a mapping at the top level of an Actions YAML action file.
29+
* See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions.
30+
*/
31+
class Action extends Node, YamlDocument, YamlMapping {
32+
/** Gets the `runs` mapping. */
33+
Runs getRuns() { result = this.lookup("runs") }
34+
}
35+
36+
/**
37+
* An `runs` mapping in a custom action YAML.
38+
* See https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#runs
39+
*/
40+
class Runs extends StepsContainer {
41+
Action action;
42+
43+
Runs() { action.lookup("runs") = this }
44+
45+
/** Gets the action that this `runs` mapping is in. */
46+
Action getAction() { result = action }
47+
}
48+
49+
/**
50+
* The parent class of the class that can contain `steps` mappings. (`Job` or `Runs` currently.)
51+
*/
52+
abstract class StepsContainer extends YamlNode, YamlMapping {
53+
/** Gets the sequence of `steps` within this YAML node. */
54+
YamlSequence getSteps() { result = this.lookup("steps") }
55+
}
56+
57+
/**
58+
* A `using` mapping in a custom action YAML.
59+
*/
60+
class Using extends YamlNode, YamlScalar {
61+
Runs runs;
62+
63+
Using() { runs.lookup("using") = this }
64+
65+
/** Gets the `runs` mapping that this `using` mapping is in. */
66+
Runs getRuns() { result = runs }
67+
}
68+
2369
/**
2470
* An Actions workflow. This is a mapping at the top level of an Actions YAML workflow file.
2571
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions.
@@ -109,7 +155,7 @@ module Actions {
109155
* An Actions job within a workflow.
110156
* See https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobs.
111157
*/
112-
class Job extends YamlNode, YamlMapping {
158+
class Job extends StepsContainer {
113159
string jobId;
114160
Workflow workflow;
115161

@@ -136,9 +182,6 @@ module Actions {
136182
/** Gets the step at the given index within this job. */
137183
Step getStep(int index) { result.getJob() = this and result.getIndex() = index }
138184

139-
/** Gets the sequence of `steps` within this job. */
140-
YamlSequence getSteps() { result = this.lookup("steps") }
141-
142185
/** Gets the `env` mapping in this job. */
143186
YamlMapping getEnv() { result = this.lookup("env") }
144187

@@ -184,15 +227,17 @@ module Actions {
184227
*/
185228
class Step extends YamlNode, YamlMapping {
186229
int index;
187-
Job job;
230+
StepsContainer parent;
188231

189-
Step() { this = job.getSteps().getElement(index) }
232+
Step() { this = parent.getSteps().getElement(index) }
190233

191234
/** Gets the 0-based position of this step within the sequence of `steps`. */
192235
int getIndex() { result = index }
193236

194237
/** Gets the job this step belongs to. */
195-
Job getJob() { result = job }
238+
Job getJob() { result = parent.(Job) }
239+
240+
Runs getRuns() { result = parent.(Runs) }
196241

197242
/** Gets the value of the `uses` field in this step, if any. */
198243
Uses getUses() { result.getStep() = this }

‎javascript/ql/src/Security/CWE-094/ExpressionInjection.ql‎

Lines changed: 111 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -103,70 +103,122 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
103103
)
104104
}
105105

106-
from YamlNode node, string injection, string context, Actions::On on
107-
where
106+
/**
107+
* The env variable name in `${{ env.name }}`
108+
* is where the external user controlled value was assigned to.
109+
*/
110+
bindingset[injection]
111+
predicate isEnvTainted(Actions::Env env, string injection, string context) {
112+
Actions::getEnvName(injection) = env.getName() and
113+
Actions::getASimpleReferenceExpression(env) = context
114+
}
115+
116+
/**
117+
* Holds if the `run` contains any expression interpolation `${{ e }}`.
118+
* Sets `context` to the initial untrusted value assignment in case of `${{ env... }}` interpolation
119+
*/
120+
predicate isRunInjectable(Actions::Run run, string injection, string context) {
121+
Actions::getASimpleReferenceExpression(run) = injection and
108122
(
109-
exists(Actions::Run run |
110-
node = run and
111-
Actions::getASimpleReferenceExpression(run) = injection and
112-
run.getStep().getJob().getWorkflow().getOn() = on and
113-
(
114-
injection = context
115-
or
116-
exists(Actions::Env env |
117-
Actions::getEnvName(injection) = env.getName() and
118-
Actions::getASimpleReferenceExpression(env) = context
119-
)
123+
injection = context
124+
or
125+
exists(Actions::Env env | isEnvTainted(env, injection, context))
126+
)
127+
}
128+
129+
/**
130+
* Holds if the `actions/github-script` contains any expression interpolation `${{ e }}`.
131+
* Sets `context` to the initial untrusted value assignment in case of `${{ env... }}` interpolation
132+
*/
133+
predicate isScriptInjectable(Actions::Script script, string injection, string context) {
134+
exists(Actions::Step step, Actions::Uses uses |
135+
script.getWith().getStep() = step and
136+
uses.getStep() = step and
137+
uses.getGitHubRepository() = "actions/github-script" and
138+
Actions::getASimpleReferenceExpression(script) = injection and
139+
(
140+
injection = context
141+
or
142+
exists(Actions::Env env | isEnvTainted(env, injection, context))
143+
)
144+
)
145+
}
146+
147+
from YamlNode node, string injection, string context
148+
where
149+
exists(Actions::Using u, Actions::Runs runs |
150+
u.getValue() = "composite" and
151+
u.getRuns() = runs and
152+
(
153+
exists(Actions::Run run |
154+
isRunInjectable(run, injection, context) and
155+
node = run and
156+
run.getStep().getRuns() = runs
120157
)
158+
or
159+
exists(Actions::Script script |
160+
node = script and
161+
script.getWith().getStep().getRuns() = runs and
162+
isScriptInjectable(script, injection, context)
163+
)
164+
) and
165+
(
166+
isExternalUserControlledIssue(context) or
167+
isExternalUserControlledPullRequest(context) or
168+
isExternalUserControlledReview(context) or
169+
isExternalUserControlledComment(context) or
170+
isExternalUserControlledGollum(context) or
171+
isExternalUserControlledCommit(context) or
172+
isExternalUserControlledDiscussion(context) or
173+
isExternalUserControlledWorkflowRun(context)
121174
)
122-
or
123-
exists(Actions::Script script, Actions::Step step, Actions::Uses uses |
124-
node = script and
125-
script.getWith().getStep().getJob().getWorkflow().getOn() = on and
126-
script.getWith().getStep() = step and
127-
uses.getStep() = step and
128-
uses.getGitHubRepository() = "actions/github-script" and
129-
Actions::getASimpleReferenceExpression(script) = injection and
130-
(
131-
injection = context
132-
or
133-
exists(Actions::Env env |
134-
Actions::getEnvName(injection) = env.getName() and
135-
Actions::getASimpleReferenceExpression(env) = context
136-
)
175+
)
176+
or
177+
exists(Actions::On on |
178+
(
179+
exists(Actions::Run run |
180+
isRunInjectable(run, injection, context) and
181+
node = run and
182+
run.getStep().getJob().getWorkflow().getOn() = on
183+
)
184+
or
185+
exists(Actions::Script script |
186+
node = script and
187+
script.getWith().getStep().getJob().getWorkflow().getOn() = on and
188+
isScriptInjectable(script, injection, context)
137189
)
190+
) and
191+
(
192+
exists(on.getNode("issues")) and
193+
isExternalUserControlledIssue(context)
194+
or
195+
exists(on.getNode("pull_request_target")) and
196+
isExternalUserControlledPullRequest(context)
197+
or
198+
exists(on.getNode("pull_request_review")) and
199+
(isExternalUserControlledReview(context) or isExternalUserControlledPullRequest(context))
200+
or
201+
exists(on.getNode("pull_request_review_comment")) and
202+
(isExternalUserControlledComment(context) or isExternalUserControlledPullRequest(context))
203+
or
204+
exists(on.getNode("issue_comment")) and
205+
(isExternalUserControlledComment(context) or isExternalUserControlledIssue(context))
206+
or
207+
exists(on.getNode("gollum")) and
208+
isExternalUserControlledGollum(context)
209+
or
210+
exists(on.getNode("push")) and
211+
isExternalUserControlledCommit(context)
212+
or
213+
exists(on.getNode("discussion")) and
214+
isExternalUserControlledDiscussion(context)
215+
or
216+
exists(on.getNode("discussion_comment")) and
217+
(isExternalUserControlledDiscussion(context) or isExternalUserControlledComment(context))
218+
or
219+
exists(on.getNode("workflow_run")) and
220+
isExternalUserControlledWorkflowRun(context)
138221
)
139-
) and
140-
(
141-
exists(on.getNode("issues")) and
142-
isExternalUserControlledIssue(context)
143-
or
144-
exists(on.getNode("pull_request_target")) and
145-
isExternalUserControlledPullRequest(context)
146-
or
147-
exists(on.getNode("pull_request_review")) and
148-
(isExternalUserControlledReview(context) or isExternalUserControlledPullRequest(context))
149-
or
150-
exists(on.getNode("pull_request_review_comment")) and
151-
(isExternalUserControlledComment(context) or isExternalUserControlledPullRequest(context))
152-
or
153-
exists(on.getNode("issue_comment")) and
154-
(isExternalUserControlledComment(context) or isExternalUserControlledIssue(context))
155-
or
156-
exists(on.getNode("gollum")) and
157-
isExternalUserControlledGollum(context)
158-
or
159-
exists(on.getNode("push")) and
160-
isExternalUserControlledCommit(context)
161-
or
162-
exists(on.getNode("discussion")) and
163-
isExternalUserControlledDiscussion(context)
164-
or
165-
exists(on.getNode("discussion_comment")) and
166-
(isExternalUserControlledDiscussion(context) or isExternalUserControlledComment(context))
167-
or
168-
exists(on.getNode("workflow_run")) and
169-
isExternalUserControlledWorkflowRun(context)
170222
)
171223
select node,
172224
"Potential injection from the ${ " + injection +

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,3 +62,4 @@
6262
| .github/workflows/workflow_run.yml:14:12:14:77 | echo '$ ... ame }}' | Potential injection from the ${ github.event.workflow_run.head_commit.committer.name }, which may be controlled by an external user. |
6363
| .github/workflows/workflow_run.yml:15:12:15:62 | echo '$ ... nch }}' | Potential injection from the ${ github.event.workflow_run.head_branch }, which may be controlled by an external user. |
6464
| .github/workflows/workflow_run.yml:16:12:16:78 | echo '$ ... ion }}' | Potential injection from the ${ github.event.workflow_run.head_repository.description }, which may be controlled by an external user. |
65+
| action.yml:14:12:14:50 | echo '$ ... ody }}' | Potential injection from the ${ github.event.comment.body }, which may be controlled by an external user. |

‎javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/action.yml‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
name: 'test'
2+
description: 'test'
3+
branding:
4+
icon: 'test'
5+
color: 'test'
6+
inputs:
7+
test:
8+
description: test
9+
required: false
10+
default: 'test'
11+
runs:
12+
using: "composite"
13+
steps:
14+
- run: echo '${{ github.event.comment.body }}'

0 commit comments

Comments
 (0)