Skip to content

Commit 962069c

Browse files
luchua-bcluchua-bc
committed
Add path check in a security context (redirect)
1 parent 48f143e commit 962069c

File tree

4 files changed

+88
-35
lines changed

4 files changed

+88
-35
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-625/PermissiveDotRegex.ql

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,20 +12,37 @@
1212
*/
1313

1414
import java
15+
import semmle.code.java.controlflow.Guards
1516
import semmle.code.java.dataflow.ExternalFlow
1617
import semmle.code.java.dataflow.FlowSources
18+
import semmle.code.java.security.UrlRedirect
1719
import DataFlow::PathGraph
1820
import Regex
1921

22+
/** Source model of remote flow source with servlets. */
23+
private class GetServletUriSource extends SourceModelCsv {
24+
override predicate row(string row) {
25+
row =
26+
[
27+
"javax.servlet.http;HttpServletRequest;false;getPathInfo;();;ReturnValue;uri-path;manual",
28+
"javax.servlet.http;HttpServletRequest;false;getPathTranslated;();;ReturnValue;uri-path;manual",
29+
"javax.servlet.http;HttpServletRequest;false;getRequestURI;();;ReturnValue;uri-path;manual",
30+
"javax.servlet.http;HttpServletRequest;false;getRequestURL;();;ReturnValue;uri-path;manual",
31+
"javax.servlet.http;HttpServletRequest;false;getServletPath;();;ReturnValue;uri-path;manual"
32+
]
33+
}
34+
}
35+
2036
/**
2137
* `.` without a `\` prefix, which is likely not a character literal in regex
2238
*/
2339
class PermissiveDotStr extends StringLiteral {
2440
PermissiveDotStr() {
25-
// Find `.` in a string that is not prefixed with `\`
41+
// Find `.` in a string that is not prefixed with `\` and ends with `.*` (no suffix like file extension)
2642
exists(string s, int i | this.getValue() = s |
27-
s.indexOf(".") = i and
28-
not s.charAt(i - 1) = "\\"
43+
s.indexOf(".*") = i and
44+
not s.charAt(i - 1) = "\\" and
45+
s.length() = i + 2
2946
)
3047
}
3148
}
@@ -86,9 +103,26 @@ class PermissiveDotRegexConfig extends DataFlow::Configuration {
86103
class MatchRegexConfiguration extends TaintTracking::Configuration {
87104
MatchRegexConfiguration() { this = "PermissiveDotRegex::MatchRegexConfiguration" }
88105

89-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
106+
override predicate isSource(DataFlow::Node source) { sourceNode(source, "uri-path") }
90107

91-
override predicate isSink(DataFlow::Node sink) { sink instanceof MatchRegexSink }
108+
override predicate isSink(DataFlow::Node sink) {
109+
sink instanceof MatchRegexSink and
110+
exists(
111+
Guard guard, Expr se, Expr ce // used in a condition to control url redirect, which is a typical security enforcement
112+
|
113+
(
114+
sink.asExpr() = ce.(MethodAccess).getQualifier() or
115+
sink.asExpr() = ce.(MethodAccess).getAnArgument() or
116+
sink.asExpr() = ce
117+
) and
118+
(
119+
DataFlow::localExprFlow(ce, guard.(MethodAccess).getQualifier()) or
120+
DataFlow::localExprFlow(ce, guard.(MethodAccess).getAnArgument())
121+
) and
122+
DataFlow::exprNode(se) instanceof UrlRedirectSink and
123+
guard.controls(se.getBasicBlock(), true)
124+
)
125+
}
92126
}
93127

94128
from

‎java/ql/src/experimental/Security/CWE/CWE-625/Regex.qll

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,4 +95,3 @@ class PatternMatcherRegexSink extends MatchRegexSink {
9595
)
9696
}
9797
}
98-

‎java/ql/test/experimental/query-tests/security/CWE-625/DotRegexServlet.java

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,12 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response)
2222

2323
if (m.matches()) {
2424
// Protected page - check access token and redirect to login page
25-
} else {
26-
// Not protected page - render content
25+
if (!request.getSession().getAttribute("secAttr").equals("secValue")) {
26+
response.sendRedirect("/login.html");
27+
return;
28+
} else {
29+
// Not protected page - render content
30+
}
2731
}
2832
}
2933

@@ -37,22 +41,30 @@ protected void doGet2(HttpServletRequest request, HttpServletResponse response)
3741

3842
if (m.matches()) {
3943
// Protected page - check access token and redirect to login page
40-
} else {
41-
// Not protected page - render content
44+
if (!request.getSession().getAttribute("secAttr").equals("secValue")) {
45+
response.sendRedirect("/login.html");
46+
return;
47+
} else {
48+
// Not protected page - render content
49+
}
4250
}
4351
}
4452

4553
// BAD: A string with line return e.g. `/protected/%0axyz` can bypass the path check
4654
protected void doGet3(HttpServletRequest request, HttpServletResponse response)
4755
throws ServletException, IOException {
48-
String source = request.getPathInfo();
56+
String source = request.getRequestURI();
4957

5058
boolean matches = source.matches(PROTECTED_PATTERN);
5159

5260
if (matches) {
5361
// Protected page - check access token and redirect to login page
54-
} else {
55-
// Not protected page - render content
62+
if (!request.getSession().getAttribute("secAttr").equals("secValue")) {
63+
response.sendRedirect("/login.html");
64+
return;
65+
} else {
66+
// Not protected page - render content
67+
}
5668
}
5769
}
5870

@@ -65,8 +77,12 @@ protected void doGet4(HttpServletRequest request, HttpServletResponse response)
6577

6678
if (matches) {
6779
// Protected page - check access token and redirect to login page
68-
} else {
69-
// Not protected page - render content
80+
if (!request.getSession().getAttribute("secAttr").equals("secValue")) {
81+
response.sendRedirect("/login.html");
82+
return;
83+
} else {
84+
// Not protected page - render content
85+
}
7086
}
7187
}
7288

@@ -80,8 +96,12 @@ protected void doGet5(HttpServletRequest request, HttpServletResponse response)
8096

8197
if (m.matches()) {
8298
// Protected page - check access token and redirect to login page
83-
} else {
84-
// Not protected page - render content
99+
if (!request.getSession().getAttribute("secAttr").equals("secValue")) {
100+
response.sendRedirect("/login.html");
101+
return;
102+
} else {
103+
// Not protected page - render content
104+
}
85105
}
86106
}
87107
}

‎java/ql/test/experimental/query-tests/security/CWE-625/PermissiveDotRegex.expected

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,31 @@
11
edges
22
| DotRegexServlet.java:11:30:11:46 | PROTECTED_PATTERN : String | DotRegexServlet.java:20:31:20:47 | PROTECTED_PATTERN |
3-
| DotRegexServlet.java:11:30:11:46 | PROTECTED_PATTERN : String | DotRegexServlet.java:50:36:50:52 | PROTECTED_PATTERN |
4-
| DotRegexServlet.java:11:30:11:46 | PROTECTED_PATTERN : String | DotRegexServlet.java:64:37:64:53 | PROTECTED_PATTERN |
3+
| DotRegexServlet.java:11:30:11:46 | PROTECTED_PATTERN : String | DotRegexServlet.java:58:36:58:52 | PROTECTED_PATTERN |
4+
| DotRegexServlet.java:11:30:11:46 | PROTECTED_PATTERN : String | DotRegexServlet.java:76:37:76:53 | PROTECTED_PATTERN |
55
| DotRegexServlet.java:11:50:11:64 | "/protected/.*" : String | DotRegexServlet.java:11:30:11:46 | PROTECTED_PATTERN : String |
66
| DotRegexServlet.java:18:19:18:39 | getPathInfo(...) : String | DotRegexServlet.java:21:25:21:30 | source |
7-
| DotRegexServlet.java:33:19:33:39 | getPathInfo(...) : String | DotRegexServlet.java:36:25:36:30 | source |
8-
| DotRegexServlet.java:48:19:48:39 | getPathInfo(...) : String | DotRegexServlet.java:50:21:50:26 | source |
9-
| DotRegexServlet.java:62:19:62:39 | getPathInfo(...) : String | DotRegexServlet.java:64:56:64:61 | source |
10-
| DotRegexServlet.java:76:19:76:39 | getPathInfo(...) : String | DotRegexServlet.java:79:25:79:30 | source |
7+
| DotRegexServlet.java:37:19:37:39 | getPathInfo(...) : String | DotRegexServlet.java:40:25:40:30 | source |
8+
| DotRegexServlet.java:56:19:56:41 | getRequestURI(...) : String | DotRegexServlet.java:58:21:58:26 | source |
9+
| DotRegexServlet.java:74:19:74:39 | getPathInfo(...) : String | DotRegexServlet.java:76:56:76:61 | source |
10+
| DotRegexServlet.java:92:19:92:39 | getPathInfo(...) : String | DotRegexServlet.java:95:25:95:30 | source |
1111
nodes
1212
| DotRegexServlet.java:11:30:11:46 | PROTECTED_PATTERN : String | semmle.label | PROTECTED_PATTERN : String |
1313
| DotRegexServlet.java:11:50:11:64 | "/protected/.*" : String | semmle.label | "/protected/.*" : String |
1414
| DotRegexServlet.java:18:19:18:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
1515
| DotRegexServlet.java:20:31:20:47 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN |
1616
| DotRegexServlet.java:21:25:21:30 | source | semmle.label | source |
17-
| DotRegexServlet.java:33:19:33:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
18-
| DotRegexServlet.java:36:25:36:30 | source | semmle.label | source |
19-
| DotRegexServlet.java:48:19:48:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
20-
| DotRegexServlet.java:50:21:50:26 | source | semmle.label | source |
21-
| DotRegexServlet.java:50:36:50:52 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN |
22-
| DotRegexServlet.java:62:19:62:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
23-
| DotRegexServlet.java:64:37:64:53 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN |
24-
| DotRegexServlet.java:64:56:64:61 | source | semmle.label | source |
25-
| DotRegexServlet.java:76:19:76:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
26-
| DotRegexServlet.java:79:25:79:30 | source | semmle.label | source |
17+
| DotRegexServlet.java:37:19:37:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
18+
| DotRegexServlet.java:40:25:40:30 | source | semmle.label | source |
19+
| DotRegexServlet.java:56:19:56:41 | getRequestURI(...) : String | semmle.label | getRequestURI(...) : String |
20+
| DotRegexServlet.java:58:21:58:26 | source | semmle.label | source |
21+
| DotRegexServlet.java:58:36:58:52 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN |
22+
| DotRegexServlet.java:74:19:74:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
23+
| DotRegexServlet.java:76:37:76:53 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN |
24+
| DotRegexServlet.java:76:56:76:61 | source | semmle.label | source |
25+
| DotRegexServlet.java:92:19:92:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
26+
| DotRegexServlet.java:95:25:95:30 | source | semmle.label | source |
2727
subpaths
2828
#select
2929
| DotRegexServlet.java:21:25:21:30 | source | DotRegexServlet.java:18:19:18:39 | getPathInfo(...) : String | DotRegexServlet.java:21:25:21:30 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:18:19:18:39 | getPathInfo(...) | user-provided value |
30-
| DotRegexServlet.java:50:21:50:26 | source | DotRegexServlet.java:48:19:48:39 | getPathInfo(...) : String | DotRegexServlet.java:50:21:50:26 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:48:19:48:39 | getPathInfo(...) | user-provided value |
31-
| DotRegexServlet.java:64:56:64:61 | source | DotRegexServlet.java:62:19:62:39 | getPathInfo(...) : String | DotRegexServlet.java:64:56:64:61 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:62:19:62:39 | getPathInfo(...) | user-provided value |
30+
| DotRegexServlet.java:58:21:58:26 | source | DotRegexServlet.java:56:19:56:41 | getRequestURI(...) : String | DotRegexServlet.java:58:21:58:26 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:56:19:56:41 | getRequestURI(...) | user-provided value |
31+
| DotRegexServlet.java:76:56:76:61 | source | DotRegexServlet.java:74:19:74:39 | getPathInfo(...) : String | DotRegexServlet.java:76:56:76:61 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:74:19:74:39 | getPathInfo(...) | user-provided value |

0 commit comments

Comments
 (0)