Skip to content

Commit 49bbfc3

Browse files
smowtonsmowton
committed
Convert SSRF sinks into url-open CSV sinks
I also drop the previous approach of taint-tracking through various builder objects in favour of assuming that a URI set in a request-builder object is highly likely to end up requested in some way or another. This will cause the `java/non-https-url` query to pick the new sinks up too, and fixes a Spring case that had never worked but went unnoticed until now.
1 parent 0f2139f commit 49bbfc3

File tree

7 files changed

+92
-173
lines changed

7 files changed

+92
-173
lines changed

‎java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,8 @@ private module Frameworks {
8282
private import semmle.code.java.frameworks.guava.Guava
8383
private import semmle.code.java.frameworks.jackson.JacksonSerializability
8484
private import semmle.code.java.frameworks.JaxWS
85+
private import semmle.code.java.frameworks.spring.SpringHttp
86+
private import semmle.code.java.frameworks.spring.SpringWebClient
8587
private import semmle.code.java.security.ResponseSplitting
8688
private import semmle.code.java.security.InformationLeak
8789
private import semmle.code.java.security.XSS
@@ -209,6 +211,8 @@ private predicate sinkModelCsv(string row) {
209211
// Open URL
210212
"java.net;URL;false;openConnection;;;Argument[-1];open-url",
211213
"java.net;URL;false;openStream;;;Argument[-1];open-url",
214+
"java.net.http;HttpRequest;false;newBuilder;;;Argument[0];open-url",
215+
"java.net.http;HttpRequest$Builder;false;uri;;;Argument[0];open-url",
212216
// Create file
213217
"java.io;FileOutputStream;false;FileOutputStream;;;Argument[0];create-file",
214218
"java.io;RandomAccessFile;false;RandomAccessFile;;;Argument[0];create-file",

‎java/ql/src/semmle/code/java/frameworks/ApacheHttp.qll

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,37 @@ private class ApacheHttpXssSink extends SinkModelCsv {
9292
}
9393
}
9494

95+
private class ApacheHttpOpenUrlSink extends SinkModelCsv {
96+
override predicate row(string row) {
97+
row =
98+
[
99+
"org.apache.http;HttpRequest;true;setURI;;;Argument[0];open-url",
100+
"org.apache.http.message;BasicHttpRequest;false;BasicHttpRequest;(RequestLine);;Argument[0];open-url",
101+
"org.apache.http.message;BasicHttpRequest;false;BasicHttpRequest;(String,String);;Argument[1];open-url",
102+
"org.apache.http.message;BasicHttpRequest;false;BasicHttpRequest;(String,String,ProtocolVersion);;Argument[1];open-url",
103+
"org.apache.http.message;BasicHttpEntityEnclosingRequest;false;BasicHttpEntityEnclosingRequest;(RequestLine);;Argument[0];open-url",
104+
"org.apache.http.message;BasicHttpEntityEnclosingRequest;false;BasicHttpEntityEnclosingRequest;(String,String);;Argument[1];open-url",
105+
"org.apache.http.message;BasicHttpEntityEnclosingRequest;false;BasicHttpEntityEnclosingRequest;(String,String,ProtocolVersion);;Argument[1];open-url",
106+
"org.apache.http.client.methods;HttpGet;false;HttpGet;;;Argument[0];open-url",
107+
"org.apache.http.client.methods;HttpHead;false;HttpHead;;;Argument[0];open-url",
108+
"org.apache.http.client.methods;HttpPut;false;HttpPut;;;Argument[0];open-url",
109+
"org.apache.http.client.methods;HttpPost;false;HttpPost;;;Argument[0];open-url",
110+
"org.apache.http.client.methods;HttpDelete;false;HttpDelete;;;Argument[0];open-url",
111+
"org.apache.http.client.methods;HttpOptions;false;HttpOptions;;;Argument[0];open-url",
112+
"org.apache.http.client.methods;HttpTrace;false;HttpTrace;;;Argument[0];open-url",
113+
"org.apache.http.client.methods;HttpPatch;false;HttpPatch;;;Argument[0];open-url",
114+
"org.apache.http.client.methods;HttpRequestBase;true;setURI;;;Argument[0];open-url",
115+
"org.apache.http.client.methods;RequestBuilder;false;setUri;;;Argument[0];open-url",
116+
"org.apache.http.client.methods;RequestBuilder;false;get;;;Argument[0];open-url",
117+
"org.apache.http.client.methods;RequestBuilder;false;post;;;Argument[0];open-url",
118+
"org.apache.http.client.methods;RequestBuilder;false;put;;;Argument[0];open-url",
119+
"org.apache.http.client.methods;RequestBuilder;false;options;;;Argument[0];open-url",
120+
"org.apache.http.client.methods;RequestBuilder;false;head;;;Argument[0];open-url",
121+
"org.apache.http.client.methods;RequestBuilder;false;delete;;;Argument[0];open-url"
122+
]
123+
}
124+
}
125+
95126
private class ApacheHttpFlowStep extends SummaryModelCsv {
96127
override predicate row(string row) {
97128
row =

‎java/ql/src/semmle/code/java/frameworks/JaxWS.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -786,3 +786,9 @@ private class UriBuilderModel extends SummaryModelCsv {
786786
]
787787
}
788788
}
789+
790+
private class JaxRsUrlOpenSink extends SinkModelCsv {
791+
override predicate row(string row) {
792+
row = ["javax.ws.rs.client;Client;true;target;;;Argument[0];open-url"]
793+
}
794+
}

‎java/ql/src/semmle/code/java/frameworks/spring/SpringHttp.qll

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55

66
import java
7+
private import semmle.code.java.dataflow.ExternalFlow
78

89
/** The class `org.springframework.http.HttpEntity` or an instantiation of it. */
910
class SpringHttpEntity extends Class {
@@ -38,3 +39,25 @@ class SpringResponseEntityBodyBuilder extends Interface {
3839
class SpringHttpHeaders extends Class {
3940
SpringHttpHeaders() { this.hasQualifiedName("org.springframework.http", "HttpHeaders") }
4041
}
42+
43+
private class UrlOpenSink extends SinkModelCsv {
44+
override predicate row(string row) {
45+
row =
46+
[
47+
"org.springframework.http;RequestEntity;false;get;;;Argument[0];open-url",
48+
"org.springframework.http;RequestEntity;false;post;;;Argument[0];open-url",
49+
"org.springframework.http;RequestEntity;false;head;;;Argument[0];open-url",
50+
"org.springframework.http;RequestEntity;false;delete;;;Argument[0];open-url",
51+
"org.springframework.http;RequestEntity;false;options;;;Argument[0];open-url",
52+
"org.springframework.http;RequestEntity;false;patch;;;Argument[0];open-url",
53+
"org.springframework.http;RequestEntity;false;put;;;Argument[0];open-url",
54+
"org.springframework.http;RequestEntity;false;method;;;Argument[1];open-url",
55+
"org.springframework.http;RequestEntity;false;RequestEntity;(HttpMethod,URI);;Argument[1];open-url",
56+
"org.springframework.http;RequestEntity;false;RequestEntity;(MultiValueMap,HttpMethod,URI);;Argument[2];open-url",
57+
"org.springframework.http;RequestEntity;false;RequestEntity;(T,HttpMethod,URI);;Argument[2];open-url",
58+
"org.springframework.http;RequestEntity;false;RequestEntity;(T,HttpMethod,URI,Type);;Argument[2];open-url",
59+
"org.springframework.http;RequestEntity;false;RequestEntity;(T,MultiValueMap,HttpMethod,URI);;Argument[3];open-url",
60+
"org.springframework.http;RequestEntity;false;RequestEntity;(T,MultiValueMap,HttpMethod,URI,Type);;Argument[3];open-url"
61+
]
62+
}
63+
}

‎java/ql/src/semmle/code/java/frameworks/spring/SpringWebClient.qll

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import java
66
import SpringHttp
7+
private import semmle.code.java.dataflow.ExternalFlow
78

89
/** The class `org.springframework.web.client.RestTemplate`. */
910
class SpringRestTemplate extends Class {
@@ -27,3 +28,21 @@ class SpringWebClient extends Interface {
2728
this.hasQualifiedName("org.springframework.web.reactive.function.client", "WebClient")
2829
}
2930
}
31+
32+
private class UrlOpenSink extends SinkModelCsv {
33+
override predicate row(string row) {
34+
row =
35+
[
36+
"org.springframework.web.client;RestTemplate;false;doExecute;;;Argument[0];open-url",
37+
"org.springframework.web.client;RestTemplate;false;postForEntity;;;Argument[0];open-url",
38+
"org.springframework.web.client;RestTemplate;false;postForLocation;;;Argument[0];open-url",
39+
"org.springframework.web.client;RestTemplate;false;postForObject;;;Argument[0];open-url",
40+
"org.springframework.web.client;RestTemplate;false;put;;;Argument[0];open-url",
41+
"org.springframework.web.client;RestTemplate;false;exchange;;;Argument[0];open-url",
42+
"org.springframework.web.client;RestTemplate;false;execute;;;Argument[0];open-url",
43+
"org.springframework.web.client;RestTemplate;false;getForEntity;;;Argument[0];open-url",
44+
"org.springframework.web.client;RestTemplate;false;getForObject;;;Argument[0];open-url",
45+
"org.springframework.web.client;RestTemplate;false;patchForObject;;;Argument[0];open-url"
46+
]
47+
}
48+
}

‎java/ql/src/semmle/code/java/security/RequestForgery.qll

Lines changed: 3 additions & 173 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import semmle.code.java.frameworks.javase.Http
99
import semmle.code.java.dataflow.DataFlow
1010
import semmle.code.java.dataflow.TaintTracking
1111
private import semmle.code.java.StringFormat
12+
private import semmle.code.java.dataflow.ExternalFlow
1213

1314
/**
1415
* A unit class for adding additional taint steps that are specific to server-side request forgery (SSRF) attacks.
@@ -30,185 +31,14 @@ private class DefaultRequestForgeryAdditionalTaintStep extends RequestForgeryAdd
3031
or
3132
// propagate to a URL when its host is assigned to
3233
exists(UrlConstructorCall c | c.getHostArg() = pred.asExpr() | succ.asExpr() = c)
33-
or
34-
// propagate to a RequestEntity when its url is assigned to
35-
exists(MethodAccess m |
36-
m.getMethod().getDeclaringType() instanceof SpringRequestEntity and
37-
(
38-
m.getMethod().hasName(["get", "post", "head", "delete", "options", "patch", "put"]) and
39-
m.getArgument(0) = pred.asExpr() and
40-
m = succ.asExpr()
41-
or
42-
m.getMethod().hasName("method") and
43-
m.getArgument(1) = pred.asExpr() and
44-
m = succ.asExpr()
45-
)
46-
)
47-
or
48-
// propagate from a `RequestEntity<>$BodyBuilder` to a `RequestEntity`
49-
// when the builder is tainted
50-
exists(MethodAccess m, RefType t |
51-
m.getMethod().getDeclaringType() = t and
52-
t.hasQualifiedName("org.springframework.http", "RequestEntity<>$BodyBuilder") and
53-
m.getMethod().hasName("body") and
54-
m.getQualifier() = pred.asExpr() and
55-
m = succ.asExpr()
56-
)
5734
}
5835
}
5936

6037
/** A data flow sink for server-side request forgery (SSRF) vulnerabilities. */
6138
abstract class RequestForgerySink extends DataFlow::Node { }
6239

63-
/**
64-
* An argument to a url `openConnection` or `openStream` call
65-
* taken as a sink for request forgery vulnerabilities.
66-
*/
67-
private class UrlOpen extends RequestForgerySink {
68-
UrlOpen() {
69-
exists(MethodAccess ma |
70-
ma.getMethod() instanceof UrlOpenConnectionMethod or
71-
ma.getMethod() instanceof UrlOpenStreamMethod
72-
|
73-
this.asExpr() = ma.getQualifier()
74-
)
75-
}
76-
}
77-
78-
/**
79-
* An argument to an Apache `setURI` call taken as a
80-
* sink for request forgery vulnerabilities.
81-
*/
82-
private class ApacheSetUri extends RequestForgerySink {
83-
ApacheSetUri() {
84-
exists(MethodAccess ma |
85-
ma.getReceiverType() instanceof ApacheHttpRequest and
86-
ma.getMethod().hasName("setURI")
87-
|
88-
this.asExpr() = ma.getArgument(0)
89-
)
90-
}
91-
}
92-
93-
/**
94-
* An argument to any Apache `HttpRequest` instantiation taken as a
95-
* sink for request forgery vulnerabilities.
96-
*/
97-
private class ApacheHttpRequestInstantiation extends RequestForgerySink {
98-
ApacheHttpRequestInstantiation() {
99-
exists(ClassInstanceExpr c | c.getConstructedType() instanceof ApacheHttpRequest |
100-
this.asExpr() = c.getArgument(0)
101-
)
102-
}
103-
}
104-
105-
/**
106-
* An argument to an Apache `RequestBuilder` method call taken as a
107-
* sink for request forgery vulnerabilities.
108-
*/
109-
private class ApacheHttpRequestBuilderArgument extends RequestForgerySink {
110-
ApacheHttpRequestBuilderArgument() {
111-
exists(MethodAccess ma |
112-
ma.getReceiverType() instanceof TypeApacheHttpRequestBuilder and
113-
ma.getMethod().hasName(["setURI", "get", "post", "put", "optons", "head", "delete"])
114-
|
115-
this.asExpr() = ma.getArgument(0)
116-
)
117-
}
118-
}
119-
120-
/**
121-
* An argument to any `java.net.http.HttpRequest` instantiation taken as a
122-
* sink for request forgery vulnerabilities.
123-
*/
124-
private class HttpRequestNewBuilder extends RequestForgerySink {
125-
HttpRequestNewBuilder() {
126-
exists(MethodAccess call |
127-
call.getCallee().hasName("newBuilder") and
128-
call.getMethod().getDeclaringType().hasQualifiedName("java.net.http", "HttpRequest")
129-
|
130-
this.asExpr() = call.getArgument(0)
131-
)
132-
}
133-
}
134-
135-
/**
136-
* An argument to an `HttpBuilder` `uri` call taken as a
137-
* sink for request forgery vulnerabilities.
138-
*/
139-
private class HttpBuilderUriArgument extends RequestForgerySink {
140-
HttpBuilderUriArgument() {
141-
exists(MethodAccess ma | ma.getMethod() instanceof HttpBuilderUri |
142-
this.asExpr() = ma.getArgument(0)
143-
)
144-
}
145-
}
146-
147-
/**
148-
* An argument to a Spring `RestTemplate` method call taken as a
149-
* sink for request forgery vulnerabilities.
150-
*/
151-
private class SpringRestTemplateArgument extends RequestForgerySink {
152-
SpringRestTemplateArgument() {
153-
this.asExpr() = any(SpringRestTemplateUrlMethodAccess m).getUrlArgument()
154-
}
155-
}
156-
157-
/**
158-
* An argument to a `javax.ws.rs.Client` `target` method call taken as a
159-
* sink for request forgery vulnerabilities.
160-
*/
161-
private class JaxRsClientTarget extends RequestForgerySink {
162-
JaxRsClientTarget() {
163-
exists(MethodAccess ma |
164-
ma.getMethod().getDeclaringType() instanceof JaxRsClient and
165-
ma.getMethod().hasName("target")
166-
|
167-
this.asExpr() = ma.getArgument(0)
168-
)
169-
}
170-
}
171-
172-
/**
173-
* A URI argument to an `org.springframework.http.RequestEntity` constructor call
174-
* taken as a sink for request forgery vulnerabilities.
175-
*/
176-
private class RequestEntityUriArg extends RequestForgerySink {
177-
RequestEntityUriArg() {
178-
exists(ClassInstanceExpr e, Argument a |
179-
e.getConstructedType() instanceof SpringRequestEntity and
180-
e.getAnArgument() = a and
181-
a.getType() instanceof TypeUri and
182-
this.asExpr() = a
183-
)
184-
}
185-
}
186-
187-
/**
188-
* A Spring Rest Template method
189-
* that takes a URL as an argument.
190-
*/
191-
private class SpringRestTemplateUrlMethod extends Method {
192-
SpringRestTemplateUrlMethod() {
193-
this.getDeclaringType() instanceof SpringRestTemplate and
194-
this.hasName([
195-
"doExecute", "postForEntity", "postForLocation", "postForObject", "put", "exchange",
196-
"execute", "getForEntity", "getForObject", "patchForObject"
197-
])
198-
}
199-
}
200-
201-
/**
202-
* A call to a Spring Rest Template method
203-
* that takes a URL as an argument.
204-
*/
205-
private class SpringRestTemplateUrlMethodAccess extends MethodAccess {
206-
SpringRestTemplateUrlMethodAccess() { this.getMethod() instanceof SpringRestTemplateUrlMethod }
207-
208-
/**
209-
* Gets the URL argument of this template call.
210-
*/
211-
Argument getUrlArgument() { result = this.getArgument(0) }
40+
private class UrlOpenSinkAsRequestForgerySink extends RequestForgerySink {
41+
UrlOpenSinkAsRequestForgerySink() { sinkNode(this, "open-url") }
21242
}
21343

21444
/** A sanitizer for request forgery vulnerabilities. */

‎java/ql/test/query-tests/security/CWE-918/RequestForgery.expected

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,15 @@ edges
4848
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:37:43:37:56 | fooResourceUrl |
4949
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:41:42:41:55 | fooResourceUrl |
5050
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:45:47:45:60 | fooResourceUrl |
51+
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:50:40:50:62 | new URI(...) |
52+
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:50:48:50:61 | fooResourceUrl : String |
5153
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:54:59:54:72 | fooResourceUrl |
5254
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:58:74:58:96 | new URI(...) |
5355
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:58:82:58:95 | fooResourceUrl : String |
5456
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:62:57:62:70 | fooResourceUrl |
5557
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:66:48:66:61 | fooResourceUrl |
5658
| SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:69:30:69:43 | fooResourceUrl |
59+
| SpringSSRF.java:50:48:50:61 | fooResourceUrl : String | SpringSSRF.java:50:40:50:62 | new URI(...) |
5760
| SpringSSRF.java:58:82:58:95 | fooResourceUrl : String | SpringSSRF.java:58:74:58:96 | new URI(...) |
5861
nodes
5962
| JaxWsSSRF.java:21:22:21:48 | getParameter(...) : String | semmle.label | getParameter(...) : String |
@@ -106,6 +109,8 @@ nodes
106109
| SpringSSRF.java:37:43:37:56 | fooResourceUrl | semmle.label | fooResourceUrl |
107110
| SpringSSRF.java:41:42:41:55 | fooResourceUrl | semmle.label | fooResourceUrl |
108111
| SpringSSRF.java:45:47:45:60 | fooResourceUrl | semmle.label | fooResourceUrl |
112+
| SpringSSRF.java:50:40:50:62 | new URI(...) | semmle.label | new URI(...) |
113+
| SpringSSRF.java:50:48:50:61 | fooResourceUrl : String | semmle.label | fooResourceUrl : String |
109114
| SpringSSRF.java:54:59:54:72 | fooResourceUrl | semmle.label | fooResourceUrl |
110115
| SpringSSRF.java:58:74:58:96 | new URI(...) | semmle.label | new URI(...) |
111116
| SpringSSRF.java:58:82:58:95 | fooResourceUrl : String | semmle.label | fooResourceUrl : String |
@@ -136,6 +141,7 @@ nodes
136141
| SpringSSRF.java:37:43:37:56 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:37:43:37:56 | fooResourceUrl | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
137142
| SpringSSRF.java:41:42:41:55 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:41:42:41:55 | fooResourceUrl | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
138143
| SpringSSRF.java:45:47:45:60 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:45:47:45:60 | fooResourceUrl | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
144+
| SpringSSRF.java:50:40:50:62 | new URI(...) | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:50:40:50:62 | new URI(...) | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
139145
| SpringSSRF.java:54:59:54:72 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:54:59:54:72 | fooResourceUrl | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
140146
| SpringSSRF.java:58:74:58:96 | new URI(...) | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:58:74:58:96 | new URI(...) | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |
141147
| SpringSSRF.java:62:57:62:70 | fooResourceUrl | SpringSSRF.java:26:33:26:60 | getParameter(...) : String | SpringSSRF.java:62:57:62:70 | fooResourceUrl | Potential server-side request forgery due to $@. | SpringSSRF.java:26:33:26:60 | getParameter(...) | a user-provided value |

0 commit comments

Comments
 (0)