Skip to content

Commit 778c6ad

Browse files
author
Alvaro Muñoz
committed
Fix tj-actions/changed-files sources
1 parent 2a84b9c commit 778c6ad

File tree

7 files changed

+117
-59
lines changed

7 files changed

+117
-59
lines changed

‎ql/lib/codeql/actions/dataflow/FlowSources.qll

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ private class ArtifactSource extends RemoteFlowSource {
263263
/**
264264
* A list of file names returned by dorny/paths-filter.
265265
*/
266-
private class DornyPathsFilterSource extends RemoteFlowSource {
266+
class DornyPathsFilterSource extends RemoteFlowSource {
267267
DornyPathsFilterSource() {
268268
exists(UsesStep u |
269269
u.getCallee() = "dorny/paths-filter" and
@@ -274,3 +274,39 @@ private class DornyPathsFilterSource extends RemoteFlowSource {
274274

275275
override string getSourceType() { result = "filename" }
276276
}
277+
278+
/**
279+
* A list of file names returned by tj-actions/changed-files.
280+
*/
281+
class TJActionsChangedFilesSource extends RemoteFlowSource {
282+
TJActionsChangedFilesSource() {
283+
exists(UsesStep u |
284+
u.getCallee() = "tj-actions/changed-files" and
285+
(
286+
u.getArgument("safe_output") = "false" or
287+
u.getVersion().regexpReplaceAll("^v", "").regexpReplaceAll("\\..*", "").toInt() < 41
288+
) and
289+
this.asExpr() = u
290+
)
291+
}
292+
293+
override string getSourceType() { result = "filename" }
294+
}
295+
296+
/**
297+
* A list of file names returned by tj-actions/verify-changed-files.
298+
*/
299+
class TJActionsVerifyChangedFilesSource extends RemoteFlowSource {
300+
TJActionsVerifyChangedFilesSource() {
301+
exists(UsesStep u |
302+
u.getCallee() = "tj-actions/verify-changed-files" and
303+
(
304+
u.getArgument("safe_output") = "false" or
305+
u.getVersion().regexpReplaceAll("^v", "").regexpReplaceAll("\\..*", "").toInt() < 17
306+
) and
307+
this.asExpr() = u
308+
)
309+
}
310+
311+
override string getSourceType() { result = "filename" }
312+
}

‎ql/lib/codeql/actions/dataflow/FlowSteps.qll

Lines changed: 43 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
private import actions
66
private import codeql.util.Unit
77
private import codeql.actions.DataFlow
8+
private import codeql.actions.dataflow.FlowSources
89
private import codeql.actions.dataflow.ExternalFlow
910
private import codeql.actions.security.ArtifactPoisoningQuery
1011

@@ -43,10 +44,6 @@ predicate envToRunStep(DataFlow::Node pred, DataFlow::Node succ) {
4344
)
4445
}
4546

46-
class EnvToRunTaintStep extends AdditionalTaintStep {
47-
override predicate step(DataFlow::Node node1, DataFlow::Node node2) { envToRunStep(node1, node2) }
48-
}
49-
5047
/**
5148
* Holds if a Run step declares an environment variable, uses it in its script and sets an output in its script.
5249
* e.g.
@@ -119,28 +116,57 @@ predicate artifactDownloadToUseStep(DataFlow::Node pred, DataFlow::Node succ) {
119116
)
120117
}
121118

122-
class ArtifactDownloadToUseTaintStep extends AdditionalTaintStep {
123-
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
124-
artifactDownloadToUseStep(node1, node2)
125-
}
126-
}
127-
128119
/**
129120
* A read of the _files field of the dorny/paths-filter action.
130121
*/
131122
predicate dornyPathsFilterTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
132-
exists(UsesStep u, StepsExpression o |
133-
u.getCallee() = "dorny/paths-filter" and
134-
u.getArgument("list-files") = ["csv", "json"] and
135-
u = pred.asExpr() and
136-
o.getStepId() = u.getId() and
123+
exists(StepsExpression o |
124+
pred instanceof DornyPathsFilterSource and
125+
o.getStepId() = pred.asExpr().(UsesStep).getId() and
137126
o.getFieldName().matches("%_files") and
138127
succ.asExpr() = o
139128
)
140129
}
141130

142-
class DornyPathsFilterTaintStep extends AdditionalTaintStep {
131+
/**
132+
* A read of user-controlled field of the tj-actions/changed-files action.
133+
*/
134+
predicate tjActionsChangedFilesTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
135+
exists(StepsExpression o |
136+
pred instanceof TJActionsChangedFilesSource and
137+
o.getTarget() = pred.asExpr() and
138+
o.getStepId() = pred.asExpr().(UsesStep).getId() and
139+
o.getFieldName() =
140+
[
141+
"added_files", "copied_files", "deleted_files", "modified_files", "renamed_files",
142+
"all_old_new_renamed_files", "type_changed_files", "unmerged_files", "unknown_files",
143+
"all_changed_and_modified_files", "all_changed_files", "other_changed_files",
144+
"all_modified_files", "other_modified_files", "other_deleted_files", "modified_keys",
145+
"changed_keys"
146+
] and
147+
succ.asExpr() = o
148+
)
149+
}
150+
151+
/**
152+
* A read of user-controlled field of the tj-actions/verify-changed-files action.
153+
*/
154+
predicate tjActionsVerifyChangedFilesTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
155+
exists(StepsExpression o |
156+
pred instanceof TJActionsChangedFilesSource and
157+
o.getTarget() = pred.asExpr() and
158+
o.getStepId() = pred.asExpr().(UsesStep).getId() and
159+
o.getFieldName() = "changed_files" and
160+
succ.asExpr() = o
161+
)
162+
}
163+
164+
class TaintSteps extends AdditionalTaintStep {
143165
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
144-
dornyPathsFilterTaintStep(node1, node2)
166+
envToRunStep(node1, node2) or
167+
artifactDownloadToUseStep(node1, node2) or
168+
dornyPathsFilterTaintStep(node1, node2) or
169+
tjActionsChangedFilesTaintStep(node1, node2) or
170+
tjActionsVerifyChangedFilesTaintStep(node1, node2)
145171
}
146172
}

‎ql/lib/ext/tj-actions_changed-files.model.yml

Lines changed: 0 additions & 22 deletions
This file was deleted.

‎ql/lib/ext/tj-actions_verify-changed-files.model.yml

Lines changed: 0 additions & 6 deletions
This file was deleted.

‎ql/test/query-tests/Security/CWE-094/.github/workflows/changed-files.yml

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,6 @@ name: CI
22

33
on:
44
pull_request:
5-
branches:
6-
- main
75

86
jobs:
97
changed_files:
@@ -13,13 +11,32 @@ jobs:
1311
- uses: actions/checkout@v4
1412
with:
1513
fetch-depth: 0
16-
- name: Get changed files
17-
id: changed-files
14+
15+
- name: Get changed files 1
16+
id: changed-files1
1817
uses: tj-actions/changed-files@v40
18+
- name: List all changed files 1
19+
run: |
20+
for file in ${{ steps.changed-files1.outputs.all_changed_files }}; do
21+
echo "$file was changed"
22+
done
1923
20-
- name: List all changed files
24+
- name: Get changed files 2
25+
id: changed-files2
26+
uses: tj-actions/changed-files@v41
27+
- name: List all changed files 2
2128
run: |
22-
for file in ${{ steps.changed-files.outputs.all_changed_files }}; do
29+
for file in ${{ steps.changed-files2.outputs.all_changed_files }}; do
2330
echo "$file was changed"
2431
done
2532
33+
- name: Get changed files 3
34+
id: changed-files3
35+
uses: tj-actions/changed-files@v41
36+
with:
37+
safe_output: false
38+
- name: List all changed files 3
39+
run: |
40+
for file in ${{ steps.changed-files3.outputs.all_changed_files }}; do
41+
echo "$file was changed"
42+
done

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ edges
66
| .github/workflows/artifactpoisoning1.yml:20:9:24:6 | Run Step: pr [id] | .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id |
77
| .github/workflows/artifactpoisoning1.yml:22:14:22:55 | echo "::set-output name=id::$(<pr-id.txt)" | .github/workflows/artifactpoisoning1.yml:20:9:24:6 | Run Step: pr [id] |
88
| .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id |
9-
| .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 |
9+
| .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files |
10+
| .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files |
1011
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log |
1112
| .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log |
1213
| .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 |
@@ -82,8 +83,10 @@ nodes
8283
| .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id | semmle.label | steps.pr.outputs.id |
8384
| .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | semmle.label | Uses Step: pr |
8485
| .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | semmle.label | steps.pr.outputs.id |
85-
| .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | semmle.label | Uses Step: changed-files |
86-
| .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 |
86+
| .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | semmle.label | Uses Step: changed-files1 |
87+
| .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | semmle.label | steps.changed-files1.outputs.all_changed_files |
88+
| .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | semmle.label | Uses Step: changed-files3 |
89+
| .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | semmle.label | steps.changed-files3.outputs.all_changed_files |
8790
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
8891
| .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log |
8992
| .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
@@ -242,7 +245,8 @@ nodes
242245
| action1/action.yml:14:19:14:50 | github.event.comment.body | semmle.label | github.event.comment.body |
243246
subpaths
244247
#select
245-
| .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 code 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 }} |
248+
| .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | ${{ steps.changed-files1.outputs.all_changed_files }} |
249+
| .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | ${{ steps.changed-files3.outputs.all_changed_files }} |
246250
| .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 code injection in $@, which may be controlled by an external user. | .github/workflows/changelog.yml:58:26:58:39 | env.log | ${{ env.log }} |
247251
| .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | Potential code injection in $@, which may be controlled by an external user. | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log | ${{ env.log }} |
248252
| action1/action.yml:14:19:14:50 | github.event.comment.body | action1/action.yml:14:19:14:50 | github.event.comment.body | action1/action.yml:14:19:14:50 | github.event.comment.body | Potential code injection in $@, which may be controlled by an external user. | action1/action.yml:14:19:14:50 | github.event.comment.body | ${{ github.event.comment.body }} |

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ edges
66
| .github/workflows/artifactpoisoning1.yml:20:9:24:6 | Run Step: pr [id] | .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id |
77
| .github/workflows/artifactpoisoning1.yml:22:14:22:55 | echo "::set-output name=id::$(<pr-id.txt)" | .github/workflows/artifactpoisoning1.yml:20:9:24:6 | Run Step: pr [id] |
88
| .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id |
9-
| .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 |
9+
| .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files |
10+
| .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files |
1011
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog.yml:58:26:58:39 | env.log |
1112
| .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | .github/workflows/changelog_from_prt.yml:58:26:58:39 | env.log |
1213
| .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 |
@@ -82,8 +83,10 @@ nodes
8283
| .github/workflows/artifactpoisoning1.yml:34:67:34:92 | steps.pr.outputs.id | semmle.label | steps.pr.outputs.id |
8384
| .github/workflows/artifactpoisoning2.yml:13:9:19:6 | Uses Step: pr | semmle.label | Uses Step: pr |
8485
| .github/workflows/artifactpoisoning2.yml:22:17:22:42 | steps.pr.outputs.id | semmle.label | steps.pr.outputs.id |
85-
| .github/workflows/changed-files.yml:16:9:20:6 | Uses Step: changed-files | semmle.label | Uses Step: changed-files |
86-
| .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 |
86+
| .github/workflows/changed-files.yml:15:9:18:6 | Uses Step: changed-files1 | semmle.label | Uses Step: changed-files1 |
87+
| .github/workflows/changed-files.yml:20:24:20:76 | steps.changed-files1.outputs.all_changed_files | semmle.label | steps.changed-files1.outputs.all_changed_files |
88+
| .github/workflows/changed-files.yml:33:9:38:6 | Uses Step: changed-files3 | semmle.label | Uses Step: changed-files3 |
89+
| .github/workflows/changed-files.yml:40:24:40:76 | steps.changed-files3.outputs.all_changed_files | semmle.label | steps.changed-files3.outputs.all_changed_files |
8790
| .github/workflows/changelog.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |
8891
| .github/workflows/changelog.yml:58:26:58:39 | env.log | semmle.label | env.log |
8992
| .github/workflows/changelog_from_prt.yml:49:19:49:56 | github.event.pull_request.title | semmle.label | github.event.pull_request.title |

0 commit comments

Comments
 (0)