Skip to content

Commit 64f9235

Browse files
committed
Fix backwards flow through TaintTracking::FunctionModel
We only do this for taint models as there isn't any backwards flow through data flow function models.
1 parent 6f5ceeb commit 64f9235

File tree

3 files changed

+71
-32
lines changed

3 files changed

+71
-32
lines changed

go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,9 @@ class AdditionalTaintStep extends Unit {
8383
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
8484
}
8585

86-
/**
87-
* Holds if the additional step from `pred` to `succ` should be included in all
88-
* global taint flow configurations.
89-
*/
90-
predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ, string model) {
86+
private predicate localAdditionalForwardTaintStep(
87+
DataFlow::Node pred, DataFlow::Node succ, string model
88+
) {
9189
exists(DataFlow::Node pred2 |
9290
pred2 = pred
9391
or
@@ -103,14 +101,45 @@ predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ, str
103101
) and
104102
model = ""
105103
or
106-
any(FunctionModel fm).taintStep(pred, succ) and model = "FunctionModel"
104+
any(FunctionModel fm).forwardTaintStep(pred, succ) and model = "FunctionModel"
107105
or
108106
any(AdditionalTaintStep a).step(pred, succ) and model = "AdditionalTaintStep"
109107
or
110108
FlowSummaryImpl::Private::Steps::summaryLocalStep(pred.(DataFlowPrivate::FlowSummaryNode)
111109
.getSummaryNode(), succ.(DataFlowPrivate::FlowSummaryNode).getSummaryNode(), false, model)
112110
}
113111

112+
private predicate localForwardTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
113+
DataFlow::localFlow(pred, succ) or
114+
localAdditionalForwardTaintStep(pred, succ, _) or
115+
// Simple flow through library code is included in the exposed local
116+
// step relation, even though flow is technically inter-procedural
117+
FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(pred, succ, _)
118+
}
119+
120+
/**
121+
* Holds if taint flows backwards from `pred` to `succ` via a function model.
122+
*/
123+
private predicate localAdditionalBackwardTaintStep(
124+
DataFlow::Node pred, DataFlow::Node succ, string model
125+
) {
126+
// backward step through function model
127+
exists(FunctionModel m, DataFlow::Node resultNode |
128+
m.backwardTaintStep(resultNode, succ) and
129+
localForwardTaintStep+(resultNode, pred.(DataFlow::PostUpdateNode).getPreUpdateNode())
130+
) and
131+
model = "FunctionModel"
132+
}
133+
134+
/**
135+
* Holds if the additional step from `pred` to `succ` should be included in all
136+
* global taint flow configurations.
137+
*/
138+
predicate localAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ, string model) {
139+
localAdditionalForwardTaintStep(pred, succ, model) or
140+
localAdditionalBackwardTaintStep(pred, succ, model)
141+
}
142+
114143
/**
115144
* Holds if taint flows from `pred` to `succ` via a reference or dereference.
116145
*
@@ -199,23 +228,36 @@ abstract class FunctionModel extends Function {
199228
abstract predicate hasTaintFlow(FunctionInput input, FunctionOutput output);
200229

201230
/** Gets an input node for this model for the call `c`. */
202-
DataFlow::Node getAnInputNode(DataFlow::CallNode c) { this.taintStepForCall(result, _, c) }
231+
DataFlow::Node getAnInputNode(DataFlow::CallNode c) { this.taintStepForCall(result, _, c, _) }
203232

204233
/** Gets an output node for this model for the call `c`. */
205-
DataFlow::Node getAnOutputNode(DataFlow::CallNode c) { this.taintStepForCall(_, result, c) }
234+
DataFlow::Node getAnOutputNode(DataFlow::CallNode c) { this.taintStepForCall(_, result, c, _) }
206235

207236
/** Holds if this function model causes taint to flow from `pred` to `succ` for the call `c`. */
208-
predicate taintStepForCall(DataFlow::Node pred, DataFlow::Node succ, DataFlow::CallNode c) {
237+
predicate taintStepForCall(
238+
DataFlow::Node pred, DataFlow::Node succ, DataFlow::CallNode c, Boolean forward
239+
) {
209240
c = this.getACall() and
210241
exists(FunctionInput inp, FunctionOutput outp | this.hasTaintFlow(inp, outp) |
211242
pred = pragma[only_bind_out](inp).getNode(c) and
212-
succ = pragma[only_bind_out](outp).getNode(c)
243+
succ = pragma[only_bind_out](outp).getNode(c) and
244+
if inp.isResult() or inp.isResult(_) then forward = false else forward = true
213245
)
214246
}
215247

216248
/** Holds if this function model causes taint to flow from `pred` to `succ`. */
217249
predicate taintStep(DataFlow::Node pred, DataFlow::Node succ) {
218-
this.taintStepForCall(pred, succ, _)
250+
this.taintStepForCall(pred, succ, _, _)
251+
}
252+
253+
/** Holds if this function model causes taint to flow forward from `pred` to `succ`. */
254+
predicate forwardTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
255+
this.taintStepForCall(pred, succ, _, true)
256+
}
257+
258+
/** Holds if this function model causes taint to flow backwards from `pred` to `succ`. */
259+
predicate backwardTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
260+
this.taintStepForCall(pred, succ, _, false)
219261
}
220262
}
221263

go/ql/test/library-tests/semmle/go/frameworks/TaintSteps/TaintStep.expected

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ invalidModelRow
88
| crypto.go:11:18:11:57 | call to Open | crypto.go:11:2:11:57 | ... := ...[1] |
99
| crypto.go:11:42:11:51 | ciphertext | crypto.go:11:2:11:57 | ... := ...[0] |
1010
| io.go:14:31:14:43 | "some string" | io.go:14:13:14:44 | call to NewReader |
11-
| io.go:16:3:16:3 | definition of w | io.go:16:23:16:27 | &... [postupdate] |
12-
| io.go:16:3:16:3 | definition of w | io.go:16:30:16:34 | &... [postupdate] |
1311
| io.go:16:23:16:27 | &... | io.go:16:24:16:27 | buf1 [postupdate] |
1412
| io.go:16:23:16:27 | &... [postupdate] | io.go:16:24:16:27 | buf1 [postupdate] |
1513
| io.go:16:24:16:27 | buf1 | io.go:16:23:16:27 | &... |
@@ -18,6 +16,8 @@ invalidModelRow
1816
| io.go:16:30:16:34 | &... [postupdate] | io.go:16:31:16:34 | buf2 [postupdate] |
1917
| io.go:16:31:16:34 | buf2 | io.go:16:30:16:34 | &... |
2018
| io.go:16:31:16:34 | buf2 [postupdate] | io.go:16:30:16:34 | &... |
19+
| io.go:18:11:18:11 | w [postupdate] | io.go:16:23:16:27 | &... [postupdate] |
20+
| io.go:18:11:18:11 | w [postupdate] | io.go:16:30:16:34 | &... [postupdate] |
2121
| io.go:18:14:18:19 | reader | io.go:18:11:18:11 | w [postupdate] |
2222
| io.go:22:31:22:43 | "some string" | io.go:22:13:22:44 | call to NewReader |
2323
| io.go:25:19:25:23 | &... | io.go:25:20:25:23 | buf1 [postupdate] |
@@ -31,9 +31,9 @@ invalidModelRow
3131
| io.go:33:20:33:23 | buf1 | io.go:33:19:33:23 | &... |
3232
| io.go:33:20:33:23 | buf1 [postupdate] | io.go:33:19:33:23 | &... |
3333
| io.go:35:16:35:21 | reader | io.go:35:12:35:13 | w2 [postupdate] |
34-
| io.go:39:6:39:6 | definition of w | io.go:39:3:39:19 | ... := ...[0] |
3534
| io.go:39:11:39:19 | call to Pipe | io.go:39:3:39:19 | ... := ...[0] |
3635
| io.go:39:11:39:19 | call to Pipe | io.go:39:3:39:19 | ... := ...[1] |
36+
| io.go:40:14:40:14 | w [postupdate] | io.go:39:3:39:19 | ... := ...[0] |
3737
| io.go:40:17:40:31 | "some string\\n" | io.go:40:14:40:14 | w [postupdate] |
3838
| io.go:43:16:43:16 | r | io.go:43:3:43:5 | buf [postupdate] |
3939
| io.go:44:13:44:15 | buf | io.go:44:13:44:24 | call to String |

go/ql/test/query-tests/Security/CWE-640/EmailInjection.expected

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
#select
22
| EmailBad.go:12:56:12:67 | type conversion | EmailBad.go:9:10:9:17 | selection of Header | EmailBad.go:12:56:12:67 | type conversion | Email content may contain $@. | EmailBad.go:9:10:9:17 | selection of Header | untrusted input |
33
| main.go:33:57:33:78 | type conversion | main.go:31:21:31:31 | call to Referer | main.go:33:57:33:78 | type conversion | Email content may contain $@. | main.go:31:21:31:31 | call to Referer | untrusted input |
4-
| main.go:42:3:42:7 | definition of write | main.go:39:21:39:31 | call to Referer | main.go:42:3:42:7 | definition of write | Email content may contain $@. | main.go:39:21:39:31 | call to Referer | untrusted input |
54
| main.go:54:46:54:59 | untrustedInput | main.go:48:21:48:31 | call to Referer | main.go:54:46:54:59 | untrustedInput | Email content may contain $@. | main.go:48:21:48:31 | call to Referer | untrusted input |
65
| main.go:55:52:55:65 | untrustedInput | main.go:48:21:48:31 | call to Referer | main.go:55:52:55:65 | untrustedInput | Email content may contain $@. | main.go:48:21:48:31 | call to Referer | untrusted input |
76
| main.go:65:16:65:22 | content | main.go:60:21:60:31 | call to Referer | main.go:65:16:65:22 | content | Email content may contain $@. | main.go:60:21:60:31 | call to Referer | untrusted input |
@@ -16,8 +15,6 @@ edges
1615
| EmailBad.go:9:10:9:17 | selection of Header | EmailBad.go:9:10:9:29 | call to Get | provenance | Src:MaD:1 MaD:7 |
1716
| EmailBad.go:9:10:9:29 | call to Get | EmailBad.go:12:56:12:67 | type conversion | provenance | |
1817
| main.go:31:21:31:31 | call to Referer | main.go:33:57:33:78 | type conversion | provenance | Src:MaD:2 |
19-
| main.go:39:21:39:31 | call to Referer | main.go:43:25:43:38 | untrustedInput | provenance | Src:MaD:2 |
20-
| main.go:43:25:43:38 | untrustedInput | main.go:42:3:42:7 | definition of write | provenance | MaD:5 |
2118
| main.go:48:21:48:31 | call to Referer | main.go:54:46:54:59 | untrustedInput | provenance | Src:MaD:2 |
2219
| main.go:48:21:48:31 | call to Referer | main.go:55:52:55:65 | untrustedInput | provenance | Src:MaD:2 |
2320
| main.go:60:21:60:31 | call to Referer | main.go:62:47:62:60 | untrustedInput | provenance | Src:MaD:2 |
@@ -33,15 +30,15 @@ edges
3330
| main.go:93:15:93:62 | call to NewContent | main.go:95:16:95:23 | content2 | provenance | |
3431
| main.go:93:48:93:61 | untrustedInput | main.go:93:15:93:62 | call to NewContent | provenance | MaD:4 |
3532
| main.go:113:21:113:31 | call to Referer | main.go:119:28:119:41 | untrustedInput | provenance | Src:MaD:2 |
36-
| main.go:116:3:116:4 | definition of mw | main.go:116:29:116:30 | &... | provenance | FunctionModel |
37-
| main.go:116:29:116:30 | &... | main.go:124:57:124:57 | b | provenance | |
38-
| main.go:119:28:119:41 | untrustedInput | main.go:116:3:116:4 | definition of mw | provenance | MaD:6 |
33+
| main.go:116:29:116:30 | &... [postupdate] | main.go:124:57:124:57 | b | provenance | |
34+
| main.go:119:3:119:4 | mw [postupdate] | main.go:116:29:116:30 | &... [postupdate] | provenance | FunctionModel |
35+
| main.go:119:28:119:41 | untrustedInput | main.go:119:3:119:4 | mw [postupdate] | provenance | MaD:6 |
3936
| main.go:124:57:124:57 | b | main.go:124:57:124:65 | call to Bytes | provenance | MaD:3 |
4037
| main.go:129:21:129:31 | call to Referer | main.go:136:30:136:43 | untrustedInput | provenance | Src:MaD:2 |
41-
| main.go:132:3:132:4 | definition of mw | main.go:132:29:132:30 | &... | provenance | FunctionModel |
42-
| main.go:132:29:132:30 | &... | main.go:141:57:141:57 | b | provenance | |
43-
| main.go:135:3:135:12 | definition of formWriter | main.go:132:3:132:4 | definition of mw | provenance | FunctionModel |
44-
| main.go:136:30:136:43 | untrustedInput | main.go:135:3:135:12 | definition of formWriter | provenance | MaD:5 |
38+
| main.go:132:29:132:30 | &... [postupdate] | main.go:141:57:141:57 | b | provenance | |
39+
| main.go:135:20:135:21 | mw [postupdate] | main.go:132:29:132:30 | &... [postupdate] | provenance | FunctionModel |
40+
| main.go:136:18:136:27 | formWriter [postupdate] | main.go:135:20:135:21 | mw [postupdate] | provenance | FunctionModel |
41+
| main.go:136:30:136:43 | untrustedInput | main.go:136:18:136:27 | formWriter [postupdate] | provenance | MaD:5 |
4542
| main.go:141:57:141:57 | b | main.go:141:57:141:65 | call to Bytes | provenance | MaD:3 |
4643
models
4744
| 1 | Source: net/http; Request; true; Header; ; ; ; remote; manual |
@@ -57,9 +54,6 @@ nodes
5754
| EmailBad.go:12:56:12:67 | type conversion | semmle.label | type conversion |
5855
| main.go:31:21:31:31 | call to Referer | semmle.label | call to Referer |
5956
| main.go:33:57:33:78 | type conversion | semmle.label | type conversion |
60-
| main.go:39:21:39:31 | call to Referer | semmle.label | call to Referer |
61-
| main.go:42:3:42:7 | definition of write | semmle.label | definition of write |
62-
| main.go:43:25:43:38 | untrustedInput | semmle.label | untrustedInput |
6357
| main.go:48:21:48:31 | call to Referer | semmle.label | call to Referer |
6458
| main.go:54:46:54:59 | untrustedInput | semmle.label | untrustedInput |
6559
| main.go:55:52:55:65 | untrustedInput | semmle.label | untrustedInput |
@@ -79,16 +73,19 @@ nodes
7973
| main.go:93:48:93:61 | untrustedInput | semmle.label | untrustedInput |
8074
| main.go:95:16:95:23 | content2 | semmle.label | content2 |
8175
| main.go:113:21:113:31 | call to Referer | semmle.label | call to Referer |
82-
| main.go:116:3:116:4 | definition of mw | semmle.label | definition of mw |
83-
| main.go:116:29:116:30 | &... | semmle.label | &... |
76+
| main.go:116:29:116:30 | &... [postupdate] | semmle.label | &... [postupdate] |
77+
| main.go:119:3:119:4 | mw [postupdate] | semmle.label | mw [postupdate] |
8478
| main.go:119:28:119:41 | untrustedInput | semmle.label | untrustedInput |
8579
| main.go:124:57:124:57 | b | semmle.label | b |
8680
| main.go:124:57:124:65 | call to Bytes | semmle.label | call to Bytes |
8781
| main.go:129:21:129:31 | call to Referer | semmle.label | call to Referer |
88-
| main.go:132:3:132:4 | definition of mw | semmle.label | definition of mw |
89-
| main.go:132:29:132:30 | &... | semmle.label | &... |
90-
| main.go:135:3:135:12 | definition of formWriter | semmle.label | definition of formWriter |
82+
| main.go:132:29:132:30 | &... [postupdate] | semmle.label | &... [postupdate] |
83+
| main.go:135:20:135:21 | mw [postupdate] | semmle.label | mw [postupdate] |
84+
| main.go:136:18:136:27 | formWriter [postupdate] | semmle.label | formWriter [postupdate] |
9185
| main.go:136:30:136:43 | untrustedInput | semmle.label | untrustedInput |
9286
| main.go:141:57:141:57 | b | semmle.label | b |
9387
| main.go:141:57:141:65 | call to Bytes | semmle.label | call to Bytes |
9488
subpaths
89+
testFailures
90+
| main.go:39:33:39:43 | comment | Missing result: Source |
91+
| main.go:42:24:42:33 | comment | Missing result: Alert |

0 commit comments

Comments
 (0)