Skip to content

Commit 186f9d0

Browse files
author
Alvaro Muñoz
authored
Merge pull request #2 from github/separate_sources
Split sources by taint type
2 parents 27d0a34 + 831b8cf commit 186f9d0

34 files changed

+383
-786
lines changed

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

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,18 +46,39 @@ module Utils {
4646

4747
bindingset[var]
4848
private string multilineAssignmentRegex(string var) {
49+
// eg:
50+
// echo "PR_TITLE<<EOF" >> $GITHUB_ENV
51+
// echo "$TITLE" >> $GITHUB_ENV
52+
// echo "EOF" >> $GITHUB_ENV
4953
result =
50-
".*(echo|Write-Output)\\s+(.*)<<\\s*([A-Z]*)EOF(.+)(echo|Write-Output)\\s+(\"|')?([A-Z]*)EOF(\"|')?\\s*>>\\s*(\"|')?\\$(\\{)?GITHUB_"
54+
".*(echo|Write-Output)\\s+(.*)<<[\\-]*\\s*([A-Z]*)EOF(.+)(echo|Write-Output)\\s+(\"|')?([A-Z]*)EOF(\"|')?\\s*>>\\s*(\"|')?\\$(\\{)?GITHUB_"
5155
+ var.toUpperCase() + "(\\})?(\"|')?.*"
5256
}
5357

5458
bindingset[var]
5559
private string multilineBlockAssignmentRegex(string var) {
60+
// eg:
61+
// {
62+
// echo 'JSON_RESPONSE<<EOF'
63+
// echo "$TITLE" >> "$GITHUB_ENV"
64+
// echo EOF
65+
// } >> "$GITHUB_ENV"
5666
result =
57-
".*\\{(\\s|::NEW_LINE::)*(echo|Write-Output)\\s+(.*)<<\\s*([A-Z]*)EOF(.+)(echo|Write-Output)\\s+(\"|')?([A-Z]*)EOF(\"|')?(\\s|::NEW_LINE::)*\\}\\s*>>\\s*(\"|')?\\$(\\{)?GITHUB_"
67+
".*\\{(\\s|::NEW_LINE::)*(echo|Write-Output)\\s+(.*)<<[\\-]*\\s*([A-Z]*)EOF(.+)(echo|Write-Output)\\s+(\"|')?([A-Z]*)EOF(\"|')?(\\s|::NEW_LINE::)*\\}\\s*>>\\s*(\"|')?\\$(\\{)?GITHUB_"
5868
+ var.toUpperCase() + "(\\})?(\"|')?.*"
5969
}
6070

71+
bindingset[var]
72+
private string multilineHereDocAssignmentRegex(string var) {
73+
// eg:
74+
// cat <<-EOF >> "$GITHUB_ENV"
75+
// echo "FOO=$TITLE"
76+
// EOF
77+
result =
78+
".*cat\\s*<<[\\-]*\\s*[A-Z]*EOF\\s*>>\\s*[\"']*\\$[\\{]*GITHUB_.*" + var.toUpperCase() +
79+
"[\\}]*[\"']*.*(echo|Write-Output)\\s+([^=]+)=(.*)::NEW_LINE::.*EOF.*"
80+
}
81+
6182
bindingset[script, var]
6283
predicate extractMultilineAssignment(string script, string var, string key, string value) {
6384
// multiline assignment
@@ -87,6 +108,19 @@ module Utils {
87108
.splitAt("\n") + ")" and
88109
key = trimQuotes(flattenedScript.regexpCapture(multilineBlockAssignmentRegex(var), 3))
89110
)
111+
or
112+
// multiline heredoc assignment
113+
exists(string flattenedScript |
114+
flattenedScript = script.replaceAll("\n", "::NEW_LINE::") and
115+
value =
116+
trimQuotes(flattenedScript.regexpCapture(multilineHereDocAssignmentRegex(var), 3))
117+
.regexpReplaceAll("\\s*>>\\s*(\"|')?\\$(\\{)?GITHUB_" + var.toUpperCase() +
118+
"(\\})?(\"|')?", "")
119+
.replaceAll("::NEW_LINE::", "\n")
120+
.trim()
121+
.splitAt("\n") and
122+
key = trimQuotes(flattenedScript.regexpCapture(multilineHereDocAssignmentRegex(var), 2))
123+
)
90124
}
91125

92126
bindingset[line]

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

Lines changed: 133 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
private import actions
2-
private import codeql.actions.DataFlow
31
private import codeql.actions.dataflow.ExternalFlow
42
private import codeql.actions.security.ArtifactPoisoningQuery
53

@@ -22,152 +20,218 @@ abstract class RemoteFlowSource extends SourceNode {
2220
}
2321

2422
bindingset[context]
25-
private predicate isExternalUserControlled(string context) {
26-
exists(string reg | reg = "github\\.event" |
23+
private predicate titleEvent(string context) {
24+
exists(string reg |
25+
reg =
26+
[
27+
// title
28+
"github\\.event\\.issue\\.title", // issue
29+
"github\\.event\\.pull_request\\.title", // pull request
30+
"github\\.event\\.discussion\\.title", // discussion
31+
"github\\.event\\.pages\\[[0-9]+\\]\\.page_name",
32+
"github\\.event\\.pages\\[[0-9]+\\]\\.title",
33+
"github\\.event\\.workflow_run\\.display_title", // The event-specific title associated with the run or the run-name if set, or the value of run-name if it is set in the workflow.
34+
]
35+
|
2736
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
2837
)
2938
}
3039

3140
bindingset[context]
32-
private predicate isExternalUserControlledIssue(string context) {
33-
exists(string reg | reg = ["github\\.event\\.issue\\.title", "github\\.event\\.issue\\.body"] |
41+
private predicate urlEvent(string context) {
42+
exists(string reg |
43+
reg =
44+
[
45+
// url
46+
"github\\.event\\.pull_request\\.head\\.repo\\.homepage",
47+
]
48+
|
3449
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
3550
)
3651
}
3752

3853
bindingset[context]
39-
private predicate isExternalUserControlledPullRequest(string context) {
54+
private predicate textEvent(string context) {
4055
exists(string reg |
4156
reg =
4257
[
43-
"github\\.event\\.pull_request\\.title", "github\\.event\\.pull_request\\.body",
44-
"github\\.event\\.pull_request\\.head\\.label",
45-
"github\\.event\\.pull_request\\.head\\.repo\\.default_branch",
46-
"github\\.event\\.pull_request\\.head\\.repo\\.description",
47-
"github\\.event\\.pull_request\\.head\\.repo\\.homepage",
48-
"github\\.event\\.pull_request\\.head\\.ref", "github\\.head_ref"
58+
// text
59+
"github\\.event\\.issue\\.body", // body
60+
"github\\.event\\.pull_request\\.body", // body
61+
"github\\.event\\.discussion\\.body", // body
62+
"github\\.event\\.review\\.body", // body
63+
"github\\.event\\.comment\\.body", // body
64+
"github\\.event\\.commits\\[[0-9]+\\]\\.message", // messsage
65+
"github\\.event\\.head_commit\\.message", // message
66+
"github\\.event\\.workflow_run\\.head_commit\\.message", // message
67+
"github\\.event\\.pull_request\\.head\\.repo\\.description", // description
68+
"github\\.event\\.workflow_run\\.head_repository\\.description", // description
69+
"github\\.event\\.client_payload\\[[0-9]+\\]", // payload
70+
"github\\.event\\.client_payload", // payload
71+
"github\\.event\\.inputs\\[[0-9]+\\]", // input
72+
"github\\.event\\.inputs", // input
4973
]
5074
|
5175
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
5276
)
5377
}
5478

5579
bindingset[context]
56-
private predicate isExternalUserControlledReview(string context) {
57-
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp("github\\.event\\.review\\.body"))
80+
private predicate repoNameEvent(string context) {
81+
exists(string reg |
82+
reg =
83+
[
84+
// repo name
85+
// Owner: All characters must be either a hyphen (-) or alphanumeric
86+
// Repo: All code points must be either a hyphen (-), an underscore (_), a period (.), or an ASCII alphanumeric code point
87+
"github\\.event\\.workflow_run\\.pull_requests\\[[0-9]+\\]\\.head\\.repo\\.name", // repo name
88+
"github\\.event\\.workflow_run\\.head_repository\\.name", // repo name
89+
"github\\.event\\.workflow_run\\.head_repository\\.full_name", // nwo
90+
]
91+
|
92+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
93+
)
5894
}
5995

6096
bindingset[context]
61-
private predicate isExternalUserControlledComment(string context) {
62-
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp("github\\.event\\.comment\\.body"))
97+
private predicate branchEvent(string context) {
98+
exists(string reg |
99+
reg =
100+
[
101+
// branch
102+
// https://docs.github.com/en/get-started/using-git/dealing-with-special-characters-in-branch-and-tag-names
103+
// - They can include slash / for hierarchical (directory) grouping, but no slash-separated component can begin with a dot . or end with the sequence .lock.
104+
// - They must contain at least one /
105+
// - They cannot have two consecutive dots .. anywhere.
106+
// - They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere.
107+
// - They cannot have question-mark ?, asterisk *, or open bracket [ anywhere.
108+
// - They cannot begin or end with a slash / or contain multiple consecutive slashes
109+
// - They cannot end with a dot .
110+
// - They cannot contain a sequence @{
111+
// - They cannot be the single character @
112+
// - They cannot contain a \
113+
// eg: zzz";echo${IFS}"hello";# would be a valid branch name
114+
"github\\.event\\.pull_request\\.head\\.repo\\.default_branch",
115+
"github\\.event\\.pull_request\\.head\\.ref", "github\\.head_ref",
116+
"github\\.event\\.workflow_run\\.head_branch",
117+
"github\\.event\\.workflow_run\\.head_branch",
118+
"github\\.event\\.workflow_run\\.pull_requests\\[[0-9]+\\]\\.head\\.ref",
119+
]
120+
|
121+
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
122+
)
63123
}
64124

65125
bindingset[context]
66-
private predicate isExternalUserControlledGollum(string context) {
126+
private predicate labelEvent(string context) {
67127
exists(string reg |
68128
reg =
69129
[
70-
"github\\.event\\.pages\\[[0-9]+\\]\\.page_name",
71-
"github\\.event\\.pages\\[[0-9]+\\]\\.title"
130+
// label
131+
// - They cannot contain a escaping \
132+
"github\\.event\\.pull_request\\.head\\.label",
72133
]
73134
|
74135
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
75136
)
76137
}
77138

78139
bindingset[context]
79-
private predicate isExternalUserControlledCommit(string context) {
140+
private predicate emailEvent(string context) {
80141
exists(string reg |
81142
reg =
82143
[
83-
"github\\.event\\.commits\\[[0-9]+\\]\\.message", "github\\.event\\.head_commit\\.message",
144+
// email
145+
// `echo${IFS}hello`@domain.com
84146
"github\\.event\\.head_commit\\.author\\.email",
85-
"github\\.event\\.head_commit\\.author\\.name",
86147
"github\\.event\\.head_commit\\.committer\\.email",
87-
"github\\.event\\.head_commit\\.committer\\.name",
88148
"github\\.event\\.commits\\[[0-9]+\\]\\.author\\.email",
89-
"github\\.event\\.commits\\[[0-9]+\\]\\.author\\.name",
90149
"github\\.event\\.commits\\[[0-9]+\\]\\.committer\\.email",
91-
"github\\.event\\.commits\\[[0-9]+\\]\\.committer\\.name",
150+
"github\\.event\\.workflow_run\\.head_commit\\.author\\.email",
151+
"github\\.event\\.workflow_run\\.head_commit\\.committer\\.email",
92152
]
93153
|
94154
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
95155
)
96156
}
97157

98158
bindingset[context]
99-
private predicate isExternalUserControlledDiscussion(string context) {
159+
private predicate usernameEvent(string context) {
100160
exists(string reg |
101-
reg = ["github\\.event\\.discussion\\.title", "github\\.event\\.discussion\\.body"]
161+
reg =
162+
[
163+
// username
164+
// All characters must be either a hyphen (-) or alphanumeric
165+
"github\\.event\\.head_commit\\.author\\.name",
166+
"github\\.event\\.head_commit\\.committer\\.name",
167+
"github\\.event\\.commits\\[[0-9]+\\]\\.author\\.name",
168+
"github\\.event\\.commits\\[[0-9]+\\]\\.committer\\.name",
169+
"github\\.event\\.workflow_run\\.head_commit\\.author\\.name",
170+
"github\\.event\\.workflow_run\\.head_commit\\.committer\\.name",
171+
]
102172
|
103173
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
104174
)
105175
}
106176

107177
bindingset[context]
108-
private predicate isExternalUserControlledWorkflowRun(string context) {
178+
private predicate pathEvent(string context) {
109179
exists(string reg |
110180
reg =
111181
[
112-
"github\\.event\\.workflow\\.path", "github\\.event\\.workflow_run\\.head_branch",
113-
"github\\.event\\.workflow_run\\.display_title",
114-
"github\\.event\\.workflow_run\\.head_branch",
115-
"github\\.event\\.workflow_run\\.head_repository\\.description",
116-
"github\\.event\\.workflow_run\\.head_repository\\.full_name",
117-
"github\\.event\\.workflow_run\\.head_repository\\.name",
118-
"github\\.event\\.workflow_run\\.head_commit\\.message",
119-
"github\\.event\\.workflow_run\\.head_commit\\.author\\.email",
120-
"github\\.event\\.workflow_run\\.head_commit\\.author\\.name",
121-
"github\\.event\\.workflow_run\\.head_commit\\.committer\\.email",
122-
"github\\.event\\.workflow_run\\.head_commit\\.committer\\.name",
123-
"github\\.event\\.workflow_run\\.pull_requests\\[[0-9]+\\]\\.head\\.ref",
124-
"github\\.event\\.workflow_run\\.pull_requests\\[[0-9]+\\]\\.head\\.repo\\.name",
182+
// filename
183+
"github\\.event\\.workflow\\.path",
125184
]
126185
|
127186
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
128187
)
129188
}
130189

131190
bindingset[context]
132-
private predicate isExternalUserControlledRepositoryDispatch(string context) {
191+
private predicate jsonEvent(string context) {
133192
exists(string reg |
134-
reg = ["github\\.event\\.client_payload\\[[0-9]+\\]", "github\\.event\\.client_payload",]
193+
reg =
194+
[
195+
// json
196+
"github\\.event",
197+
]
135198
|
136199
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
137200
)
138201
}
139202

140-
bindingset[context]
141-
private predicate isExternalUserControlledWorkflowDispatch(string context) {
142-
exists(string reg | reg = ["github\\.event\\.inputs\\[[0-9]+\\]", "github\\.event\\.inputs",] |
143-
Utils::normalizeExpr(context).regexpMatch(Utils::wrapRegexp(reg))
144-
)
145-
}
203+
class EventSource extends RemoteFlowSource {
204+
string flag;
146205

147-
private class EventSource extends RemoteFlowSource {
148206
EventSource() {
149207
exists(Expression e, string context | this.asExpr() = e and context = e.getExpression() |
150-
isExternalUserControlled(context) or
151-
isExternalUserControlledIssue(context) or
152-
isExternalUserControlledPullRequest(context) or
153-
isExternalUserControlledReview(context) or
154-
isExternalUserControlledComment(context) or
155-
isExternalUserControlledGollum(context) or
156-
isExternalUserControlledCommit(context) or
157-
isExternalUserControlledDiscussion(context) or
158-
isExternalUserControlledWorkflowRun(context) or
159-
isExternalUserControlledRepositoryDispatch(context) or
160-
isExternalUserControlledWorkflowDispatch(context)
208+
titleEvent(context) and flag = "title"
209+
or
210+
urlEvent(context) and flag = "url"
211+
or
212+
textEvent(context) and flag = "text"
213+
or
214+
branchEvent(context) and flag = "branch"
215+
or
216+
labelEvent(context) and flag = "label"
217+
or
218+
emailEvent(context) and flag = "email"
219+
or
220+
usernameEvent(context) and flag = "username"
221+
or
222+
pathEvent(context) and flag = "filename"
223+
or
224+
jsonEvent(context) and flag = "json"
161225
)
162226
}
163227

164-
override string getSourceType() { result = "User-controlled events" }
228+
override string getSourceType() { result = flag }
165229
}
166230

167231
/**
168232
* A Source of untrusted data defined in a MaD specification
169233
*/
170-
private class ExternallyDefinedSource extends RemoteFlowSource {
234+
class ExternallyDefinedSource extends RemoteFlowSource {
171235
string sourceType;
172236

173237
ExternallyDefinedSource() { externallyDefinedSource(this, sourceType, _) }
@@ -178,19 +242,19 @@ private class ExternallyDefinedSource extends RemoteFlowSource {
178242
/**
179243
* An input for a Composite Action
180244
*/
181-
private class CompositeActionInputSource extends RemoteFlowSource {
245+
class CompositeActionInputSource extends RemoteFlowSource {
182246
CompositeAction c;
183247

184248
CompositeActionInputSource() { c.getAnInput() = this.asExpr() }
185249

186-
override string getSourceType() { result = "Composite action input" }
250+
override string getSourceType() { result = "input" }
187251
}
188252

189253
/**
190254
* A downloadeded artifact.
191255
*/
192-
private class ArtifactToOptionSource extends RemoteFlowSource {
193-
ArtifactToOptionSource() { this.asExpr() instanceof UntrustedArtifactDownloadStep }
256+
private class ArtifactSource extends RemoteFlowSource {
257+
ArtifactSource() { this.asExpr() instanceof UntrustedArtifactDownloadStep }
194258

195-
override string getSourceType() { result = "Step output from Artifact" }
259+
override string getSourceType() { result = "artifact" }
196260
}

0 commit comments

Comments
 (0)