Skip to content

Commit 97c3297

Browse files
owen-mcowen-mc
committed
Make RequestForgery use new API
The extra nodes in .expected files are due to the changes from #13717, which are not applied to configuration classes extending DataFlow::Configuration or TaintTracking::Configuration.
1 parent 1c25363 commit 97c3297

4 files changed

Lines changed: 49 additions & 21 deletions

File tree

‎go/ql/lib/semmle/go/security/RequestForgery.qll‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,11 @@ module RequestForgery {
1717
import RequestForgeryCustomizations::RequestForgery
1818

1919
/**
20+
* DEPRECATED: Use `Flow` instead.
21+
*
2022
* A taint-tracking configuration for reasoning about request forgery.
2123
*/
22-
class Configuration extends TaintTracking::Configuration {
24+
deprecated class Configuration extends TaintTracking::Configuration {
2325
Configuration() { this = "RequestForgery" }
2426

2527
override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -47,4 +49,23 @@ module RequestForgery {
4749
super.isSanitizerGuard(guard) or guard instanceof SanitizerGuard
4850
}
4951
}
52+
53+
private module Config implements DataFlow::ConfigSig {
54+
predicate isSource(DataFlow::Node source) { source instanceof Source }
55+
56+
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
57+
58+
predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
59+
60+
predicate isBarrierOut(DataFlow::Node node) { node instanceof SanitizerEdge }
61+
62+
predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
63+
// propagate to a URL when its host is assigned to
64+
exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Host") |
65+
w.writesField(v.getAUse(), f, pred) and succ = v.getAUse()
66+
)
67+
}
68+
}
69+
70+
module Flow = TaintTracking::Global<Config>;
5071
}

‎go/ql/src/Security/CWE-918/RequestForgery.ql‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,17 +11,17 @@
1111
*/
1212

1313
import go
14-
import semmle.go.security.RequestForgery::RequestForgery
14+
import semmle.go.security.RequestForgery
1515
import semmle.go.security.SafeUrlFlow
16-
import DataFlow::PathGraph
16+
import RequestForgery::Flow::PathGraph
1717

1818
from
19-
Configuration cfg, SafeUrlFlow::Configuration scfg, DataFlow::PathNode source,
20-
DataFlow::PathNode sink, DataFlow::Node request
19+
SafeUrlFlow::Configuration scfg, RequestForgery::Flow::PathNode source,
20+
RequestForgery::Flow::PathNode sink, DataFlow::Node request
2121
where
22-
cfg.hasFlowPath(source, sink) and
23-
request = sink.getNode().(Sink).getARequest() and
22+
RequestForgery::Flow::flowPath(source, sink) and
23+
request = sink.getNode().(RequestForgery::Sink).getARequest() and
2424
// this excludes flow from safe parts of request URLs, for example the full URL
2525
not scfg.hasFlow(_, sink.getNode())
2626
select request, source, sink, "The $@ of this request depends on a $@.", sink.getNode(),
27-
sink.getNode().(Sink).getKind(), source, "user-provided value"
27+
sink.getNode().(RequestForgery::Sink).getKind(), source, "user-provided value"

‎go/ql/test/library-tests/semmle/go/frameworks/Twirp/tests.ql‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,7 @@ query predicate passingPositiveTests(string res, string expectation, InlineTest
6767
exists(Twirp::ServerConstructor n | t.inEntity(n))
6868
or
6969
expectation = "ssrf" and
70-
exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
71-
cfg.hasFlow(_, sink) and t.inNode(sink)
72-
)
70+
exists(DataFlow::Node sink | RequestForgery::Flow::flowTo(sink) and t.inNode(sink))
7371
)
7472
}
7573

@@ -105,9 +103,7 @@ query predicate failingPositiveTests(string res, string expectation, InlineTest
105103
not exists(Twirp::ServerConstructor n | t.inEntity(n))
106104
or
107105
expectation = "ssrf" and
108-
not exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
109-
cfg.hasFlow(_, sink) and t.inNode(sink)
110-
)
106+
not exists(DataFlow::Node sink | RequestForgery::Flow::flowTo(sink) and t.inNode(sink))
111107
)
112108
}
113109

@@ -143,9 +139,7 @@ query predicate passingNegativeTests(string res, string expectation, InlineTest
143139
not exists(Twirp::ServerConstructor n | t.inEntity(n))
144140
or
145141
expectation = "!ssrf" and
146-
not exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
147-
cfg.hasFlow(_, sink) and t.inNode(sink)
148-
)
142+
not exists(DataFlow::Node sink | RequestForgery::Flow::flowTo(sink) and t.inNode(sink))
149143
)
150144
}
151145

@@ -181,8 +175,6 @@ query predicate failingNegativeTests(string res, string expectation, InlineTest
181175
exists(Twirp::ServerConstructor n | t.inEntity(n))
182176
or
183177
expectation = "!ssrf" and
184-
exists(RequestForgery::Configuration cfg, DataFlow::Node sink |
185-
cfg.hasFlow(_, sink) and t.inNode(sink)
186-
)
178+
exists(DataFlow::Node sink | RequestForgery::Flow::flowTo(sink) and t.inNode(sink))
187179
)
188180
}

‎go/ql/test/query-tests/Security/CWE-918/RequestForgery.expected‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,17 @@ edges
66
| tst.go:10:13:10:35 | call to FormValue | tst.go:24:66:24:72 | tainted |
77
| tst.go:10:13:10:35 | call to FormValue | tst.go:27:11:27:29 | ...+... |
88
| tst.go:10:13:10:35 | call to FormValue | tst.go:29:11:29:40 | ...+... |
9-
| tst.go:10:13:10:35 | call to FormValue | tst.go:37:11:37:11 | u |
9+
| tst.go:10:13:10:35 | call to FormValue | tst.go:36:11:36:17 | tainted |
10+
| tst.go:35:2:35:2 | definition of u [pointer] | tst.go:36:2:36:2 | u [pointer] |
11+
| tst.go:36:2:36:2 | implicit dereference | tst.go:35:2:35:2 | definition of u [pointer] |
12+
| tst.go:36:2:36:2 | implicit dereference | tst.go:36:2:36:2 | u |
13+
| tst.go:36:2:36:2 | implicit dereference | tst.go:37:11:37:11 | u |
14+
| tst.go:36:2:36:2 | u | tst.go:36:2:36:2 | implicit dereference |
15+
| tst.go:36:2:36:2 | u | tst.go:36:2:36:2 | u |
16+
| tst.go:36:2:36:2 | u | tst.go:37:11:37:11 | u |
17+
| tst.go:36:2:36:2 | u [pointer] | tst.go:36:2:36:2 | implicit dereference |
18+
| tst.go:36:11:36:17 | tainted | tst.go:36:2:36:2 | u |
19+
| tst.go:36:11:36:17 | tainted | tst.go:37:11:37:11 | u |
1020
| tst.go:37:11:37:11 | u | tst.go:37:11:37:20 | call to String |
1121
| websocket.go:60:21:60:31 | call to Referer | websocket.go:65:27:65:40 | untrustedInput |
1222
| websocket.go:74:21:74:31 | call to Referer | websocket.go:78:36:78:49 | untrustedInput |
@@ -27,6 +37,11 @@ nodes
2737
| tst.go:24:66:24:72 | tainted | semmle.label | tainted |
2838
| tst.go:27:11:27:29 | ...+... | semmle.label | ...+... |
2939
| tst.go:29:11:29:40 | ...+... | semmle.label | ...+... |
40+
| tst.go:35:2:35:2 | definition of u [pointer] | semmle.label | definition of u [pointer] |
41+
| tst.go:36:2:36:2 | implicit dereference | semmle.label | implicit dereference |
42+
| tst.go:36:2:36:2 | u | semmle.label | u |
43+
| tst.go:36:2:36:2 | u [pointer] | semmle.label | u [pointer] |
44+
| tst.go:36:11:36:17 | tainted | semmle.label | tainted |
3045
| tst.go:37:11:37:11 | u | semmle.label | u |
3146
| tst.go:37:11:37:20 | call to String | semmle.label | call to String |
3247
| websocket.go:60:21:60:31 | call to Referer | semmle.label | call to Referer |

0 commit comments

Comments
 (0)