Skip to content

Commit 9c30b82

Browse files
author
porcupineyhairs
authored
Merge pull request github#2 from aschackmull/java/ssrf-review
Java: Review fixes.
2 parents ebc6c49 + 3f04099 commit 9c30b82

File tree

20 files changed

+219
-360
lines changed

20 files changed

+219
-360
lines changed

‎java/ql/src/Security/CWE/CWE-319/HttpsUrls.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ class HTTPStringToURLOpenMethodFlowConfig extends TaintTracking::Configuration {
5252
}
5353

5454
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
55-
exists(UrlConstructor u |
55+
exists(UrlConstructorCall u |
5656
node1.asExpr() = u.protocolArg() and
5757
node2.asExpr() = u
5858
)

‎java/ql/src/experimental/CWE-918/RequestForgery.ql

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* @name Server Sider Request Forgery (SSRF) from remote source
2+
* @name Server Side Request Forgery (SSRF)
33
* @description Making web requests based on unvalidated user-input
44
* may cause server to communicate with malicious servers.
55
* @kind path-problem
@@ -12,10 +12,22 @@
1212

1313
import java
1414
import semmle.code.java.dataflow.FlowSources
15-
import RequestForgery::RequestForgery
15+
import RequestForgery
1616
import DataFlow::PathGraph
1717

18-
from DataFlow::PathNode source, DataFlow::PathNode sink, RequestForgeryRemoteConfiguration conf
18+
class RequestForgeryConfiguration extends TaintTracking::Configuration {
19+
RequestForgeryConfiguration() { this = "Server Side Request Forgery" }
20+
21+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgerySink }
24+
25+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
26+
requestForgeryStep(pred, succ)
27+
}
28+
}
29+
30+
from DataFlow::PathNode source, DataFlow::PathNode sink, RequestForgeryConfiguration conf
1931
where conf.hasFlowPath(source, sink)
2032
select sink.getNode(), source, sink, "Potential server side request forgery due to $@.",
2133
source.getNode(), "a user-provided value"

‎java/ql/src/experimental/CWE-918/RequestForgery.qll

Lines changed: 164 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,17 @@
11
import java
2-
import semmle.code.java.dataflow.FlowSources
2+
import semmle.code.java.frameworks.Networking
3+
import semmle.code.java.frameworks.ApacheHttp
4+
import semmle.code.java.frameworks.spring.Spring
5+
import semmle.code.java.frameworks.JaxWS
36
import semmle.code.java.frameworks.javase.Http
47
import semmle.code.java.dataflow.DataFlow
58

6-
module RequestForgery {
7-
import RequestForgeryCustomizations::RequestForgery
8-
9-
/**
10-
* A taint-tracking configuration for reasoning about request forgery.
11-
*/
12-
class RequestForgeryRemoteConfiguration extends TaintTracking::Configuration {
13-
RequestForgeryRemoteConfiguration() { this = "Server Side Request Forgery" }
14-
15-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
16-
17-
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
18-
19-
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
20-
additionalStep(pred, succ)
21-
}
22-
}
23-
}
24-
25-
predicate additionalStep(DataFlow::Node pred, DataFlow::Node succ) {
9+
predicate requestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) {
2610
// propagate to a URI when its host is assigned to
2711
exists(UriCreation c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
2812
or
2913
// propagate to a URL when its host is assigned to
30-
exists(UrlConstructor c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
14+
exists(UrlConstructorCall c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
3115
or
3216
// propagate to a RequestEntity when its url is assigned to
3317
exists(MethodAccess m |
@@ -36,11 +20,11 @@ predicate additionalStep(DataFlow::Node pred, DataFlow::Node succ) {
3620
m.getMethod().hasName(["get", "post", "head", "delete", "options", "patch", "put"]) and
3721
m.getArgument(0) = pred.asExpr() and
3822
m = succ.asExpr()
23+
or
24+
m.getMethod().hasName("method") and
25+
m.getArgument(1) = pred.asExpr() and
26+
m = succ.asExpr()
3927
)
40-
or
41-
m.getMethod().hasName("method") and
42-
m.getArgument(1) = pred.asExpr() and
43-
m = succ.asExpr()
4428
)
4529
or
4630
// propagate from a `RequestEntity<>$BodyBuilder` to a `RequestEntity`
@@ -53,3 +37,157 @@ predicate additionalStep(DataFlow::Node pred, DataFlow::Node succ) {
5337
m = succ.asExpr()
5438
)
5539
}
40+
41+
/** A data flow sink for request forgery vulnerabilities. */
42+
abstract class RequestForgerySink extends DataFlow::Node { }
43+
44+
/**
45+
* An argument to an url `openConnection` or `openStream` call
46+
* taken as a sink for request forgery vulnerabilities.
47+
*/
48+
private class UrlOpen extends RequestForgerySink {
49+
UrlOpen() {
50+
exists(MethodAccess ma |
51+
ma.getMethod() instanceof UrlOpenConnectionMethod or
52+
ma.getMethod() instanceof UrlOpenStreamMethod
53+
|
54+
this.asExpr() = ma.getQualifier()
55+
)
56+
}
57+
}
58+
59+
/**
60+
* An argument to an Apache `setURI` call taken as a
61+
* sink for request forgery vulnerabilities.
62+
*/
63+
private class ApacheSetUri extends RequestForgerySink {
64+
ApacheSetUri() {
65+
exists(MethodAccess ma |
66+
ma.getReceiverType() instanceof ApacheHttpRequest and
67+
ma.getMethod().hasName("setURI")
68+
|
69+
this.asExpr() = ma.getArgument(0)
70+
)
71+
}
72+
}
73+
74+
/**
75+
* An argument to any Apache Request Instantiation call taken as a
76+
* sink for request forgery vulnerabilities.
77+
*/
78+
private class ApacheHttpRequestInstantiation extends RequestForgerySink {
79+
ApacheHttpRequestInstantiation() {
80+
exists(ClassInstanceExpr c | c.getConstructedType() instanceof ApacheHttpRequest |
81+
this.asExpr() = c.getArgument(0)
82+
)
83+
}
84+
}
85+
86+
/**
87+
* An argument to a Apache RequestBuilder method call taken as a
88+
* sink for request forgery vulnerabilities.
89+
*/
90+
private class ApacheHttpRequestBuilderArgument extends RequestForgerySink {
91+
ApacheHttpRequestBuilderArgument() {
92+
exists(MethodAccess ma |
93+
ma.getReceiverType() instanceof TypeApacheHttpRequestBuilder and
94+
ma.getMethod().hasName(["setURI", "get", "post", "put", "optons", "head", "delete"])
95+
|
96+
this.asExpr() = ma.getArgument(0)
97+
)
98+
}
99+
}
100+
101+
/**
102+
* An argument to any Java.net.http.request Instantiation call taken as a
103+
* sink for request forgery vulnerabilities.
104+
*/
105+
private class HttpRequestNewBuilder extends RequestForgerySink {
106+
HttpRequestNewBuilder() {
107+
exists(MethodAccess call |
108+
call.getCallee().hasName("newBuilder") and
109+
call.getMethod().getDeclaringType().getName() = "HttpRequest"
110+
|
111+
this.asExpr() = call.getArgument(0)
112+
)
113+
}
114+
}
115+
116+
/**
117+
* An argument to an Http Builder `uri` call taken as a
118+
* sink for request forgery vulnerabilities.
119+
*/
120+
private class HttpBuilderUriArgument extends RequestForgerySink {
121+
HttpBuilderUriArgument() {
122+
exists(MethodAccess ma | ma.getMethod() instanceof HttpBuilderUri |
123+
this.asExpr() = ma.getArgument(0)
124+
)
125+
}
126+
}
127+
128+
/**
129+
* An argument to a Spring Rest Template method call taken as a
130+
* sink for request forgery vulnerabilities.
131+
*/
132+
private class SpringRestTemplateArgument extends RequestForgerySink {
133+
SpringRestTemplateArgument() {
134+
exists(MethodAccess ma |
135+
this.asExpr() = ma.getMethod().(SpringRestTemplateUrlMethods).getUrlArgument(ma)
136+
)
137+
}
138+
}
139+
140+
/**
141+
* An argument to `javax.ws.rs.Client`s `target` method call taken as a
142+
* sink for request forgery vulnerabilities.
143+
*/
144+
private class JaxRsClientTarget extends RequestForgerySink {
145+
JaxRsClientTarget() {
146+
exists(MethodAccess ma |
147+
ma.getMethod().getDeclaringType() instanceof JaxRsClient and
148+
ma.getMethod().hasName("target")
149+
|
150+
this.asExpr() = ma.getArgument(0)
151+
)
152+
}
153+
}
154+
155+
/**
156+
* An argument to `org.springframework.http.RequestEntity`s constructor call
157+
* which is an URI taken as a sink for request forgery vulnerabilities.
158+
*/
159+
private class RequestEntityUriArg extends RequestForgerySink {
160+
RequestEntityUriArg() {
161+
exists(ClassInstanceExpr e, Argument a |
162+
e.getConstructedType() instanceof SpringRequestEntity and
163+
e.getAnArgument() = a and
164+
a.getType() instanceof TypeUri and
165+
this.asExpr() = a
166+
)
167+
}
168+
}
169+
170+
/**
171+
* A class representing all Spring Rest Template methods
172+
* which take an URL as an argument.
173+
*/
174+
private class SpringRestTemplateUrlMethods extends Method {
175+
SpringRestTemplateUrlMethods() {
176+
this.getDeclaringType() instanceof SpringRestTemplate and
177+
this
178+
.hasName([
179+
"doExecute", "postForEntity", "postForLocation", "postForObject", "put", "exchange",
180+
"execute", "getForEntity", "getForObject", "patchForObject"
181+
])
182+
}
183+
184+
/**
185+
* Gets the argument which corresponds to a URL argument
186+
* passed as a `java.net.___URL` object or as a string or the like
187+
*/
188+
Argument getUrlArgument(MethodAccess ma) {
189+
// doExecute(URI url, HttpMethod method, RequestCallback requestCallback,
190+
// ResponseExtractor<T> responseExtractor)
191+
result = ma.getArgument(0)
192+
}
193+
}

0 commit comments

Comments
 (0)