Skip to content

Commit b4b5ae2

Browse files
erik-krogherik-krogh
committed
add some request-forgery sanitizers, inspired from C#
1 parent bbeee8f commit b4b5ae2

File tree

3 files changed

+131
-0
lines changed

3 files changed

+131
-0
lines changed

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

Lines changed: 77 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,79 @@ 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 `List.contains()` that is a sanitizer for URL redirects.
90+
*/
91+
private predicate isContainsUrlSanitizer(Guard guard, Expr e, boolean branch) {
92+
guard =
93+
any(MethodCall method |
94+
method.getMethod().getName() = "contains" and
95+
e = method.getArgument(0) and
96+
branch = true
97+
)
98+
}
99+
100+
/**
101+
* An URL argument to a call to `.contains()` that is a sanitizer for URL redirects.
102+
*
103+
* This `contains` method is usually called on a list, but the sanitizer matches any call to a method
104+
* called `contains`, so other methods with the same name will also be considered sanitizers.
105+
*/
106+
class ContainsUrlSanitizer extends RequestForgerySanitizer {
107+
ContainsUrlSanitizer() {
108+
this = DataFlow::BarrierGuard<isContainsUrlSanitizer/3>::getABarrierNode()
109+
}
110+
}
111+
112+
/**
113+
* A check that the URL is relative, and therefore safe for URL redirects.
114+
*/
115+
private predicate isRelativeUrlSanitizer(Guard guard, Expr e, boolean branch) {
116+
guard =
117+
any(MethodCall call |
118+
exists(Method method |
119+
call.getMethod() = method and
120+
method.getName() = "isAbsolute" and
121+
method.getDeclaringType().hasQualifiedName("java.net", "URI")
122+
) and
123+
e = call.getQualifier() and
124+
branch = false
125+
)
126+
}
127+
128+
/**
129+
* A check that the URL is relative, and therefore safe for URL redirects.
130+
*/
131+
class RelativeUrlSanitizer extends RequestForgerySanitizer {
132+
RelativeUrlSanitizer() {
133+
this = DataFlow::BarrierGuard<isRelativeUrlSanitizer/3>::getABarrierNode()
134+
}
135+
}
136+
137+
/**
138+
* A comparison on the host of a url, that is a sanitizer for URL redirects.
139+
* E.g. `"example.org".equals(url.getHost())"`
140+
*/
141+
private predicate isHostComparisonSanitizer(Guard guard, Expr e, boolean branch) {
142+
guard =
143+
any(MethodCall equalsCall |
144+
equalsCall.getMethod().getName() = "equals" and
145+
branch = true and
146+
exists(MethodCall hostCall |
147+
hostCall = [equalsCall.getQualifier(), equalsCall.getArgument(0)] and
148+
hostCall.getMethod().getName() = "getHost" and
149+
hostCall.getMethod().getDeclaringType().hasQualifiedName("java.net", "URI") and
150+
e = hostCall.getQualifier()
151+
)
152+
)
153+
}
154+
155+
/**
156+
* A comparison on the `Host` property of a url, that is a sanitizer for URL redirects.
157+
*/
158+
class HostComparisonSanitizer extends RequestForgerySanitizer {
159+
HostComparisonSanitizer() {
160+
this = DataFlow::BarrierGuard<isHostComparisonSanitizer/3>::getABarrierNode()
161+
}
162+
}

‎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)