Skip to content

Commit 70d1741

Browse files
author
Alvaro Muñoz
authored
Merge pull request #4 from GitHubSecurityLab/improve_mad
Refactor MaD semantics
2 parents f2fc411 + 4f0b66e commit 70d1741

12 files changed

+153
-60
lines changed

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

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,9 @@ class JobStmt extends Statement instanceof Actions::Job {
115115
*/
116116
string getId() { result = super.getId() }
117117

118+
/** Gets the workflow that this job is a part of. */
119+
WorkflowStmt getWorkflowStmt() { result = super.getWorkflow() }
120+
118121
/** Gets the step at the given index within this job. */
119122
StepStmt getStepStmt(int index) { result = super.getStep(index) }
120123

@@ -181,6 +184,26 @@ class StepStmt extends Statement instanceof Actions::Step {
181184
string getId() { result = super.getId() }
182185

183186
JobStmt getJobStmt() { result = super.getJob() }
187+
188+
/**
189+
* Gets a environment variable expression by name in the scope of the current step.
190+
*/
191+
Expression getEnvExpr(string name) {
192+
exists(Actions::StepEnv env |
193+
env.getStep() = this and
194+
env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
195+
)
196+
or
197+
exists(Actions::JobEnv env |
198+
env.getJob() = this.getJobStmt() and
199+
env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
200+
)
201+
or
202+
exists(Actions::WorkflowEnv env |
203+
env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
204+
env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
205+
)
206+
}
184207
}
185208

186209
/**
@@ -192,6 +215,8 @@ abstract class UsesExpr extends Expression {
192215
abstract string getVersion();
193216

194217
abstract Expression getArgumentExpr(string key);
218+
219+
abstract Expression getEnvExpr(string name);
195220
}
196221

197222
/**
@@ -212,6 +237,8 @@ class StepUsesExpr extends StepStmt, UsesExpr {
212237
result = with.lookup(key)
213238
)
214239
}
240+
241+
override Expression getEnvExpr(string name) { result = this.(StepStmt).getEnvExpr(name) }
215242
}
216243

217244
/**
@@ -260,6 +287,23 @@ class JobUsesExpr extends UsesExpr instanceof YamlMapping {
260287
override Expression getArgumentExpr(string key) {
261288
this.(YamlMapping).lookup("with").(YamlMapping).lookup(key) = result
262289
}
290+
291+
/**
292+
* Gets a environment variable expression by name in the scope of the current node.
293+
*/
294+
override Expression getEnvExpr(string name) {
295+
this.(YamlMapping).lookup("env").(YamlMapping).lookup(name) = result
296+
or
297+
exists(Actions::JobEnv env |
298+
env.getJob() = this.getJobStmt() and
299+
env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
300+
)
301+
or
302+
exists(Actions::WorkflowEnv env |
303+
env.getWorkflow() = this.getJobStmt().getWorkflowStmt() and
304+
env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
305+
)
306+
}
263307
}
264308

265309
/**
@@ -272,13 +316,6 @@ class RunExpr extends StepStmt, Expression {
272316

273317
Expression getScriptExpr() { result = scriptExpr }
274318

275-
Expression getEnvExpr(string name) {
276-
exists(Actions::StepEnv env |
277-
env.getStep() = this and
278-
env.(YamlMapping).maps(any(YamlScalar s | s.getValue() = name), result)
279-
)
280-
}
281-
282319
string getScript() { result = scriptExpr.getValue() }
283320
}
284321

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ import codeql.actions.DataFlow
33
import actions
44

55
/** Holds if a source model exists for the given parameters. */
6-
predicate sourceModel(string action, string version, string output, string kind) {
7-
Extensions::sourceModel(action, version, output, kind)
6+
predicate sourceModel(string action, string version, string output, string trigger, string kind) {
7+
Extensions::sourceModel(action, version, output, trigger, kind)
88
}
99

1010
/** Holds if a sink model exists for the given parameters. */
@@ -17,15 +17,27 @@ predicate sinkModel(string action, string version, string input, string kind) {
1717
Extensions::sinkModel(action, version, input, kind)
1818
}
1919

20+
/**
21+
* MaD sinks
22+
* Fields:
23+
* - action: Fully-qualified action name (NWO)
24+
* - version: Either '*' or a specific SHA/Tag
25+
* - input arg: sink node (prefixed with either `env.` or `input.`)
26+
* - kind: sink kind
27+
*/
2028
predicate sinkNode(DataFlow::ExprNode sink, string kind) {
2129
exists(UsesExpr uses, string action, string version, string input |
22-
uses.getArgumentExpr(input.splitAt(",").trim()) = sink.asExpr() and
30+
(
31+
if input.trim().matches("env.%")
32+
then sink.asExpr() = uses.getEnvExpr(input.trim().replaceAll("input\\.", ""))
33+
else sink.asExpr() = uses.getArgumentExpr(input.trim())
34+
) and
2335
sinkModel(action, version, input, kind) and
2436
uses.getCallee() = action and
2537
(
2638
if version.trim() = "*"
2739
then uses.getVersion() = any(string v)
28-
else uses.getVersion() = version.splitAt(",").trim()
40+
else uses.getVersion() = version.trim()
2941
)
3042
)
3143
}

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,19 +125,36 @@ private class EventSource extends RemoteFlowSource {
125125
override string getSourceType() { result = "User-controlled events" }
126126
}
127127

128+
/**
129+
* MaD sources
130+
* Fields:
131+
* - action: Fully-qualified action name (NWO)
132+
* - version: Either '*' or a specific SHA/Tag
133+
* - output arg: To node (prefixed with either `env.` or `output.`)
134+
* - trigger: Triggering event under which this model introduces tainted data. Use `*` for any event.
135+
*/
128136
private class ExternallyDefinedSource extends RemoteFlowSource {
129137
string soutceType;
130138

131139
ExternallyDefinedSource() {
132-
exists(UsesExpr uses, string action, string version, /*string output,*/ string kind |
133-
sourceModel(action, version, _, kind) and
140+
exists(
141+
UsesExpr uses, string action, string version, string output, string trigger, string kind
142+
|
143+
sourceModel(action, version, output, trigger, kind) and
134144
uses.getCallee() = action and
135145
(
136146
if version.trim() = "*"
137147
then uses.getVersion() = any(string v)
138-
else uses.getVersion() = version.splitAt(",").trim()
148+
else uses.getVersion() = version.trim()
149+
) and
150+
(
151+
if output.trim().matches("env.%")
152+
then this.asExpr() = uses.getEnvExpr(output.trim().replaceAll("output\\.", ""))
153+
else
154+
// 'output.' is the default qualifier
155+
// TODO: Taint just the specified output
156+
this.asExpr() = uses
139157
) and
140-
uses = this.asExpr() and
141158
soutceType = kind
142159
)
143160
}

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,32 @@ class AdditionalTaintStep extends Unit {
2121
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
2222
}
2323

24+
/**
25+
* MaD summaries
26+
* Fields:
27+
* - action: Fully-qualified action name (NWO)
28+
* - version: Either '*' or a specific SHA/Tag
29+
* - input arg: From node (prefixed with either `env.` or `input.`)
30+
* - output arg: To node (prefixed with either `env.` or `output.`)
31+
* - kind: Either 'Taint' or 'Value'
32+
*/
2433
predicate externallyDefinedSummary(DataFlow::Node pred, DataFlow::Node succ) {
2534
exists(UsesExpr uses, string action, string version, string input |
26-
/*, string output */ summaryModel(action, version, input, _, "taint") and
35+
// `output` not used yet
36+
summaryModel(action, version, input, _, "taint") and
2737
uses.getCallee() = action and
2838
(
2939
if version.trim() = "*"
3040
then uses.getVersion() = any(string v)
31-
else uses.getVersion() = version.splitAt(",").trim()
41+
else uses.getVersion() = version.trim()
42+
) and
43+
(
44+
if input.trim().matches("env.%")
45+
then pred.asExpr() = uses.getEnvExpr(input.trim().replaceAll("env\\.", ""))
46+
else
47+
// 'input.' is the default qualifier
48+
pred.asExpr() = uses.getArgumentExpr(input.trim().replaceAll("input\\.", ""))
3249
) and
33-
pred.asExpr() = uses.getArgumentExpr(input.splitAt(",").trim()) and
3450
succ.asExpr() = uses
3551
)
3652
}

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

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

1012
/**
1113
* Holds if a summary model exists for the given parameters.

‎ql/lib/ext/REMOVEME.model.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/actions-all
4+
extensible: sinkModel
5+
data:
6+
- [ "FAKE-mad9000/actions-find-and-replace-string", "*", "source", "expression-injection" ]

‎ql/lib/ext/frabert-replace-string-action.model.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/actions-all
4+
extensible: summaryModel
5+
data:
6+
- [ "frabert/replace-string-action", "*", "string", "replaced", "taint" ]
7+
- [ "frabert/replace-string-action", "*", "replace-with", "replaced", "taint" ]

‎ql/lib/ext/mad9000-actions-find-and-replace-string.model.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/actions-all
4+
extensible: summaryModel
5+
data:
6+
- [ "mad9000/actions-find-and-replace-string", "*", "source", "value", "taint" ]
7+
- [ "mad9000/actions-find-and-replace-string", "*", "replace", "value", "taint" ]
8+
- [ "frabert/replace-string-action", "*", "string", "replaced", "taint" ]
9+
- [ "frabert/replace-string-action", "*", "replace-with", "replaced", "taint" ]

‎ql/lib/ext/sinks.model.yml

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

‎ql/lib/ext/sources.model.yml

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

0 commit comments

Comments
 (0)