Skip to content

Commit 863e3f7

Browse files
erik-krogherik-krogh
authored
Merge pull request #15731 from erik-krogh/java-url
Java: More sanitizers for request-forgery
2 parents 8d1ee10 + f613823 commit 863e3f7

File tree

4 files changed

+132
-0
lines changed

4 files changed

+132
-0
lines changed

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import semmle.code.java.frameworks.JaxWS
88
import semmle.code.java.frameworks.javase.Http
99
import semmle.code.java.dataflow.DataFlow
1010
import semmle.code.java.frameworks.Properties
11+
private import semmle.code.java.controlflow.Guards
1112
private import semmle.code.java.dataflow.StringPrefixes
1213
private import semmle.code.java.dataflow.ExternalFlow
1314
private import semmle.code.java.security.Sanitizers
@@ -83,3 +84,76 @@ private class HostnameSanitizingPrefix extends InterestingPrefix {
8384
private class HostnameSantizer extends RequestForgerySanitizer {
8485
HostnameSantizer() { this.asExpr() = any(HostnameSanitizingPrefix hsp).getAnAppendedExpression() }
8586
}
87+
88+
/**
89+
* An argument to a call to a `.contains()` method that is a sanitizer for URL redirects.
90+
*
91+
* Matches any method call where the method is named `contains`.
92+
*/
93+
private predicate isContainsUrlSanitizer(Guard guard, Expr e, boolean branch) {
94+
guard =
95+
any(MethodCall method |
96+
method.getMethod().getName() = "contains" and
97+
e = method.getArgument(0) and
98+
branch = true
99+
)
100+
}
101+
102+
/**
103+
* An URL argument to a call to `.contains()` that is a sanitizer for URL redirects.
104+
*
105+
* This `contains` method is usually called on a list, but the sanitizer matches any call to a method
106+
* called `contains`, so other methods with the same name will also be considered sanitizers.
107+
*/
108+
private class ContainsUrlSanitizer extends RequestForgerySanitizer {
109+
ContainsUrlSanitizer() {
110+
this = DataFlow::BarrierGuard<isContainsUrlSanitizer/3>::getABarrierNode()
111+
}
112+
}
113+
114+
/**
115+
* A check that the URL is relative, and therefore safe for URL redirects.
116+
*/
117+
private predicate isRelativeUrlSanitizer(Guard guard, Expr e, boolean branch) {
118+
guard =
119+
any(MethodCall call |
120+
call.getMethod().hasQualifiedName("java.net", "URI", "isAbsolute") and
121+
e = call.getQualifier() and
122+
branch = false
123+
)
124+
}
125+
126+
/**
127+
* A check that the URL is relative, and therefore safe for URL redirects.
128+
*/
129+
private class RelativeUrlSanitizer extends RequestForgerySanitizer {
130+
RelativeUrlSanitizer() {
131+
this = DataFlow::BarrierGuard<isRelativeUrlSanitizer/3>::getABarrierNode()
132+
}
133+
}
134+
135+
/**
136+
* A comparison on the host of a url, that is a sanitizer for URL redirects.
137+
* E.g. `"example.org".equals(url.getHost())"`
138+
*/
139+
private predicate isHostComparisonSanitizer(Guard guard, Expr e, boolean branch) {
140+
guard =
141+
any(MethodCall equalsCall |
142+
equalsCall.getMethod().getName() = "equals" and
143+
branch = true and
144+
exists(MethodCall hostCall |
145+
hostCall = [equalsCall.getQualifier(), equalsCall.getArgument(0)] and
146+
hostCall.getMethod().hasQualifiedName("java.net", "URI", "getHost") and
147+
e = hostCall.getQualifier()
148+
)
149+
)
150+
}
151+
152+
/**
153+
* A comparison on the `Host` property of a url, that is a sanitizer for URL redirects.
154+
*/
155+
private class HostComparisonSanitizer extends RequestForgerySanitizer {
156+
HostComparisonSanitizer() {
157+
this = DataFlow::BarrierGuard<isHostComparisonSanitizer/3>::getABarrierNode()
158+
}
159+
}

‎java/ql/src/change-notes/2024-03-12-request-sanitizers.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added sanitizers for relative URLs, `List.contains()`, and checking the host of a URI to the `java/ssrf` and `java/unvalidated-url-redirection` queries.

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ edges
66
| mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:31:14:38 | source(...) : String | provenance | |
77
| mad/Test.java:14:31:14:38 | source(...) : String | mad/Test.java:14:22:14:38 | (...)... | provenance | |
88
nodes
9+
| UrlRedirect2.java:27:25:27:54 | getParameter(...) | semmle.label | getParameter(...) |
910
| UrlRedirect.java:23:25:23:54 | getParameter(...) | semmle.label | getParameter(...) |
1011
| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | semmle.label | weakCleanup(...) |
1112
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | semmle.label | getParameter(...) : String |
@@ -20,6 +21,7 @@ nodes
2021
subpaths
2122
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) |
2223
#select
24+
| UrlRedirect2.java:27:25:27:54 | getParameter(...) | UrlRedirect2.java:27:25:27:54 | getParameter(...) | UrlRedirect2.java:27:25:27:54 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect2.java:27:25:27:54 | getParameter(...) | user-provided value |
2325
| UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:23:25:23:54 | getParameter(...) | user-provided value |
2426
| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:32:37:32:66 | getParameter(...) | user-provided value |
2527
| UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:39:34:39:63 | getParameter(...) | user-provided value |

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect2.java‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// Test case for
2+
// CWE-601: URL Redirection to Untrusted Site ('Open Redirect')
3+
// http://cwe.mitre.org/data/definitions/601.html
4+
5+
package test.cwe601.cwe.examples;
6+
7+
import java.io.IOException;
8+
import java.util.Arrays;
9+
import java.util.List;
10+
import java.net.URI;
11+
import java.net.URISyntaxException;
12+
13+
import javax.servlet.ServletException;
14+
import javax.servlet.http.HttpServlet;
15+
import javax.servlet.http.HttpServletRequest;
16+
import javax.servlet.http.HttpServletResponse;
17+
18+
public class UrlRedirect2 extends HttpServlet {
19+
private static final List<String> VALID_REDIRECTS = Arrays.asList(
20+
"http://cwe.mitre.org/data/definitions/601.html",
21+
"http://cwe.mitre.org/data/definitions/79.html"
22+
);
23+
24+
protected void doGet(HttpServletRequest request, HttpServletResponse response)
25+
throws ServletException, IOException {
26+
// BAD: a request parameter is incorporated without validation into a URL redirect
27+
response.sendRedirect(request.getParameter("target"));
28+
29+
// GOOD: the request parameter is validated against a known list of strings
30+
String target = request.getParameter("target");
31+
if (VALID_REDIRECTS.contains(target)) {
32+
response.sendRedirect(target);
33+
}
34+
35+
try {
36+
String urlString = request.getParameter("page");
37+
URI url = new URI(urlString);
38+
39+
if (!url.isAbsolute()) {
40+
// GOOD: The redirect is to a relative URL
41+
response.sendRedirect(url.toString());
42+
}
43+
44+
if ("example.org".equals(url.getHost())) {
45+
// GOOD: The redirect is to a known host
46+
response.sendRedirect(url.toString());
47+
}
48+
} catch (URISyntaxException e) {
49+
// handle exception
50+
}
51+
}
52+
}

0 commit comments

Comments
 (0)