Skip to content

Commit ff3759e

Browse files
author
Alvaro Muñoz
authored
Merge pull request #40 from GitHubSecurityLab/refactor_source_checks
feat(sources): Do not take triggers into consideration
2 parents 9d5b026 + 2ed3ace commit ff3759e

24 files changed

+123
-138
lines changed

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,9 @@ private import actions
88
* - action: Fully-qualified action name (NWO)
99
* - version: Either '*' or a specific SHA/Tag
1010
* - output arg: To node (prefixed with either `env.` or `output.`)
11-
* - trigger: Triggering event under which this model introduces tainted data. Use `*` for any event.
1211
*/
13-
predicate sourceModel(string action, string version, string output, string trigger, string kind) {
14-
Extensions::sourceModel(action, version, output, trigger, kind)
12+
predicate sourceModel(string action, string version, string output, string kind) {
13+
Extensions::sourceModel(action, version, output, kind)
1514
}
1615

1716
/**
@@ -39,11 +38,9 @@ predicate sinkModel(string action, string version, string input, string kind) {
3938
Extensions::sinkModel(action, version, input, kind)
4039
}
4140

42-
predicate externallyDefinedSource(
43-
DataFlow::Node source, string sourceType, string fieldName, string trigger
44-
) {
41+
predicate externallyDefinedSource(DataFlow::Node source, string sourceType, string fieldName) {
4542
exists(Uses uses, string action, string version, string kind |
46-
sourceModel(action, version, fieldName, trigger, kind) and
43+
sourceModel(action, version, fieldName, kind) and
4744
uses.getCallee() = action.toLowerCase() and
4845
(
4946
if version.trim() = "*"

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

Lines changed: 9 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@ abstract class RemoteFlowSource extends SourceNode {
1818
/** Gets a string that describes the type of this remote flow source. */
1919
abstract string getSourceType();
2020

21-
abstract string getATriggerEvent();
22-
2321
override string getThreatModel() { result = "remote" }
2422
}
2523

@@ -122,47 +120,31 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
122120
}
123121

124122
private class EventSource extends RemoteFlowSource {
125-
string trigger;
126-
127123
EventSource() {
128124
exists(Expression e, string context | this.asExpr() = e and context = e.getExpression() |
129-
trigger = ["issues", "issue_comment"] and isExternalUserControlledIssue(context)
130-
or
131-
trigger = ["pull_request_target", "pull_request_review", "pull_request_review_comment"] and
132-
isExternalUserControlledPullRequest(context)
133-
or
134-
trigger = ["pull_request_review"] and isExternalUserControlledReview(context)
135-
or
136-
trigger = ["pull_request_review_comment", "issue_comment", "discussion_comment"] and
137-
isExternalUserControlledComment(context)
138-
or
139-
trigger = ["gollum"] and isExternalUserControlledGollum(context)
140-
or
141-
trigger = ["push"] and isExternalUserControlledCommit(context)
142-
or
143-
trigger = ["discussion", "discussion_comment"] and isExternalUserControlledDiscussion(context)
144-
or
145-
trigger = ["workflow_run"] and isExternalUserControlledWorkflowRun(context)
125+
isExternalUserControlledIssue(context) or
126+
isExternalUserControlledPullRequest(context) or
127+
isExternalUserControlledReview(context) or
128+
isExternalUserControlledComment(context) or
129+
isExternalUserControlledGollum(context) or
130+
isExternalUserControlledCommit(context) or
131+
isExternalUserControlledDiscussion(context) or
132+
isExternalUserControlledWorkflowRun(context)
146133
)
147134
}
148135

149136
override string getSourceType() { result = "User-controlled events" }
150-
151-
override string getATriggerEvent() { result = trigger }
152137
}
153138

154139
/**
155140
* A Source of untrusted data defined in a MaD specification
156141
*/
157142
private class ExternallyDefinedSource extends RemoteFlowSource {
158143
string sourceType;
159-
string trigger;
160144

161-
ExternallyDefinedSource() { externallyDefinedSource(this, sourceType, _, trigger) }
145+
ExternallyDefinedSource() { externallyDefinedSource(this, sourceType, _) }
162146

163147
override string getSourceType() { result = sourceType }
164-
165-
override string getATriggerEvent() { result = trigger }
166148
}
167149

168150
/**
@@ -174,6 +156,4 @@ private class CompositeActionInputSource extends RemoteFlowSource {
174156
CompositeActionInputSource() { c.getAnInput() = this.asExpr() }
175157

176158
override string getSourceType() { result = "Composite action input" }
177-
178-
override string getATriggerEvent() { result = "*" }
179159
}

‎ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos =
175175
*/
176176
predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
177177
exists(Uses astFrom, StepsExpression astTo |
178-
externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName(), _) and
178+
externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName()) and
179179
astFrom = nodeFrom.asExpr() and
180180
astTo = nodeTo.asExpr() and
181181
astTo.getTarget() = astFrom
@@ -192,7 +192,7 @@ predicate stepsCtxLocalStep(Node nodeFrom, Node nodeTo) {
192192
*/
193193
predicate needsCtxLocalStep(Node nodeFrom, Node nodeTo) {
194194
exists(Uses astFrom, NeedsExpression astTo |
195-
externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName(), _) and
195+
externallyDefinedSource(nodeFrom, _, "output." + astTo.getFieldName()) and
196196
astFrom = nodeFrom.asExpr() and
197197
astTo = nodeTo.asExpr() and
198198
astTo.getTarget() = astFrom
@@ -232,7 +232,7 @@ predicate envCtxLocalStep(Node nodeFrom, Node nodeTo) {
232232
astFrom = nodeFrom.asExpr() and
233233
astTo = nodeTo.asExpr() and
234234
(
235-
externallyDefinedSource(nodeFrom, _, "env." + astTo.getFieldName(), _) or
235+
externallyDefinedSource(nodeFrom, _, "env." + astTo.getFieldName()) or
236236
astTo.getTarget() = astFrom
237237
)
238238
)

‎ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@
55
/**
66
* Holds if a source model exists for the given parameters.
77
*/
8-
extensible predicate sourceModel(
9-
string action, string version, string output, string trigger, string kind
10-
);
8+
extensible predicate sourceModel(string action, string version, string output, string kind);
119

1210
/**
1311
* Holds if a summary model exists for the given parameters.

‎ql/lib/ext/TEST-RW-MODELS.model.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ extensions:
99
pack: githubsecuritylab/actions-all
1010
extensible: sourceModel
1111
data:
12-
- ["octo-org/source-repo/.github/workflows/workflow.yml", "*", "output.workflow-output", "*", "Foo"]
12+
- ["octo-org/source-repo/.github/workflows/workflow.yml", "*", "output.workflow-output", "Foo"]
1313
- addsTo:
1414
pack: githubsecuritylab/actions-all
1515
extensible: sinkModel

‎ql/lib/ext/ahmadnassri_action-changed-files.model.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,5 @@ extensions:
33
pack: githubsecuritylab/actions-all
44
extensible: sourceModel
55
data:
6-
- ["ahmadnassri/action-changed-files", "*", "output.files", "pull_request_target", "PR changed files"]
7-
- ["ahmadnassri/action-changed-files", "*", "output.json", "pull_request_target", "PR changed files"]
6+
- ["ahmadnassri/action-changed-files", "*", "output.files", "PR changed files"]
7+
- ["ahmadnassri/action-changed-files", "*", "output.json", "PR changed files"]

‎ql/lib/ext/amannn_action-semantic-pull-request.model.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@ extensions:
33
pack: githubsecuritylab/actions-all
44
extensible: sourceModel
55
data:
6-
- ["amannn/action-semantic-pull-request", "*", "output.error_message", "pull_request_target", "PR title"]
6+
- ["amannn/action-semantic-pull-request", "*", "output.error_message", "PR title"]

‎ql/lib/ext/cypress-io_github-action.model.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@ extensions:
33
pack: githubsecuritylab/actions-all
44
extensible: sourceModel
55
data:
6-
- ["cypress-io/github-action", "*", "env.GH_BRANCH", "pull_request_target", "PR branch"]
6+
- ["cypress-io/github-action", "*", "env.GH_BRANCH", "PR branch"]

‎ql/lib/ext/dawidd6_action-download-artifact.model.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@ extensions:
33
pack: githubsecuritylab/actions-all
44
extensible: sourceModel
55
data:
6-
- ["dawidd6/action-download-artifact", "*", "output.artifacts", "*", "Artifact details"]
6+
- ["dawidd6/action-download-artifact", "*", "output.artifacts", "Artifact details"]

‎ql/lib/ext/dorny_paths-filter.model.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@ extensions:
33
pack: githubsecuritylab/actions-all
44
extensible: sourceModel
55
data:
6-
- ["dorny/paths-filter", "*", "output.changes", "pull_request_target", "PR changed files"]
6+
- ["dorny/paths-filter", "*", "output.changes", "PR changed files"]

0 commit comments

Comments
 (0)