Skip to content

Commit a0cd551

Browse files
haby0haby0
committed
Add filtering of String.format
1 parent 498c99e commit a0cd551

File tree

4 files changed

+37
-59
lines changed

4 files changed

+37
-59
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,16 @@ class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration {
3333
ae.getRightOperand() = node.asExpr() and
3434
not ae instanceof RedirectBuilderExpr
3535
)
36+
or
37+
exists(MethodAccess ma, int index |
38+
ma.getMethod().hasName("format") and
39+
ma.getMethod().getDeclaringType() instanceof TypeString and
40+
ma.getArgument(index) = node.asExpr() and
41+
(
42+
index != 0 and
43+
not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().regexpMatch("^%s.*")
44+
)
45+
)
3646
}
3747
}
3848

‎java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll

Lines changed: 3 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -51,23 +51,22 @@ class SpringUrlRedirectSink extends DataFlow::Node {
5151
SpringUrlRedirectSink() {
5252
exists(RedirectBuilderExpr rbe |
5353
rbe.getRightOperand() = this.asExpr() and
54-
exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(rbe), _))
54+
any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
5555
)
5656
or
5757
exists(MethodAccess ma, RedirectAppendCall rac |
5858
DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and
5959
ma.getMethod().hasName("append") and
6060
ma.getArgument(0) = this.asExpr() and
61-
exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(ma.getQualifier()), _))
61+
any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
6262
)
6363
or
6464
exists(MethodAccess ma |
6565
ma.getMethod().hasName("setUrl") and
6666
ma.getMethod()
6767
.getDeclaringType()
6868
.hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and
69-
ma.getArgument(0) = this.asExpr() and
70-
exists(RedirectViewFlowConfig rvfc | rvfc.hasFlowToExpr(ma.getQualifier()))
69+
ma.getArgument(0) = this.asExpr()
7170
)
7271
or
7372
exists(ClassInstanceExpr cie |
@@ -84,57 +83,3 @@ class SpringUrlRedirectSink extends DataFlow::Node {
8483
)
8584
}
8685
}
87-
88-
/** A data flow configuration tracing flow from redirect builder expression to spring controller method return expression. */
89-
private class RedirectBuilderFlowConfig extends DataFlow2::Configuration {
90-
RedirectBuilderFlowConfig() { this = "RedirectBuilderFlowConfig" }
91-
92-
override predicate isSource(DataFlow::Node src) {
93-
exists(RedirectBuilderExpr rbe | rbe = src.asExpr())
94-
or
95-
exists(MethodAccess ma, RedirectAppendCall rac |
96-
DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and
97-
ma.getMethod().hasName("append") and
98-
ma.getQualifier() = src.asExpr()
99-
)
100-
}
101-
102-
override predicate isSink(DataFlow::Node sink) {
103-
exists(ReturnStmt rs, SpringRequestMappingMethod sqmm |
104-
rs.getResult() = sink.asExpr() and
105-
sqmm.getBody().getAStmt() = rs
106-
)
107-
}
108-
109-
override predicate isAdditionalFlowStep(Node prod, Node succ) {
110-
exists(MethodAccess ma |
111-
ma.getMethod().hasName("toString") and
112-
ma.getMethod().getDeclaringType() instanceof StringBuildingType and
113-
ma.getQualifier() = prod.asExpr() and
114-
ma = succ.asExpr()
115-
)
116-
}
117-
}
118-
119-
/** A data flow configuration tracing flow from RedirectView object to calling setUrl method. */
120-
private class RedirectViewFlowConfig extends DataFlow2::Configuration {
121-
RedirectViewFlowConfig() { this = "RedirectViewFlowConfig" }
122-
123-
override predicate isSource(DataFlow::Node src) {
124-
exists(ClassInstanceExpr cie |
125-
cie.getConstructedType()
126-
.hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and
127-
cie = src.asExpr()
128-
)
129-
}
130-
131-
override predicate isSink(DataFlow::Node sink) {
132-
exists(MethodAccess ma |
133-
ma.getMethod().hasName("setUrl") and
134-
ma.getMethod()
135-
.getDeclaringType()
136-
.hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and
137-
ma.getQualifier() = sink.asExpr()
138-
)
139-
}
140-
}

‎java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ edges
55
| SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | SpringUrlRedirect.java:33:47:33:57 | redirectUrl |
66
| SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | SpringUrlRedirect.java:40:29:40:39 | redirectUrl |
77
| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | SpringUrlRedirect.java:48:30:48:40 | redirectUrl |
8+
| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | SpringUrlRedirect.java:54:30:54:66 | format(...) |
9+
| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | SpringUrlRedirect.java:59:30:59:76 | format(...) |
810
nodes
911
| SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | semmle.label | redirectUrl : String |
1012
| SpringUrlRedirect.java:15:19:15:29 | redirectUrl | semmle.label | redirectUrl |
@@ -18,10 +20,16 @@ nodes
1820
| SpringUrlRedirect.java:40:29:40:39 | redirectUrl | semmle.label | redirectUrl |
1921
| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | semmle.label | redirectUrl : String |
2022
| SpringUrlRedirect.java:48:30:48:40 | redirectUrl | semmle.label | redirectUrl |
23+
| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | semmle.label | redirectUrl : String |
24+
| SpringUrlRedirect.java:54:30:54:66 | format(...) | semmle.label | format(...) |
25+
| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | semmle.label | redirectUrl : String |
26+
| SpringUrlRedirect.java:59:30:59:76 | format(...) | semmle.label | format(...) |
2127
#select
2228
| SpringUrlRedirect.java:15:19:15:29 | redirectUrl | SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | SpringUrlRedirect.java:15:19:15:29 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:13:30:13:47 | redirectUrl | user-provided value |
2329
| SpringUrlRedirect.java:21:36:21:46 | redirectUrl | SpringUrlRedirect.java:20:24:20:41 | redirectUrl : String | SpringUrlRedirect.java:21:36:21:46 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:20:24:20:41 | redirectUrl | user-provided value |
2430
| SpringUrlRedirect.java:27:44:27:54 | redirectUrl | SpringUrlRedirect.java:26:30:26:47 | redirectUrl : String | SpringUrlRedirect.java:27:44:27:54 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:26:30:26:47 | redirectUrl | user-provided value |
2531
| SpringUrlRedirect.java:33:47:33:57 | redirectUrl | SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | SpringUrlRedirect.java:33:47:33:57 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:32:30:32:47 | redirectUrl | user-provided value |
2632
| SpringUrlRedirect.java:40:29:40:39 | redirectUrl | SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | SpringUrlRedirect.java:40:29:40:39 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:37:24:37:41 | redirectUrl | user-provided value |
2733
| SpringUrlRedirect.java:48:30:48:40 | redirectUrl | SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | SpringUrlRedirect.java:48:30:48:40 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:45:24:45:41 | redirectUrl | user-provided value |
34+
| SpringUrlRedirect.java:54:30:54:66 | format(...) | SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | SpringUrlRedirect.java:54:30:54:66 | format(...) | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:53:24:53:41 | redirectUrl | user-provided value |
35+
| SpringUrlRedirect.java:59:30:59:76 | format(...) | SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | SpringUrlRedirect.java:59:30:59:76 | format(...) | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:58:24:58:41 | redirectUrl | user-provided value |

‎java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,16 @@ public String bad6(String redirectUrl) {
5050
}
5151

5252
@GetMapping("url7")
53+
public String bad7(String redirectUrl) {
54+
return "redirect:" + String.format("%s/?aaa", redirectUrl);
55+
}
56+
57+
@GetMapping("url8")
58+
public String bad8(String redirectUrl, String token) {
59+
return "redirect:" + String.format(redirectUrl + "?token=%s", token);
60+
}
61+
62+
@GetMapping("url9")
5363
public RedirectView good1(String redirectUrl) {
5464
RedirectView rv = new RedirectView();
5565
if (redirectUrl.startsWith(VALID_REDIRECT)){
@@ -60,9 +70,14 @@ public RedirectView good1(String redirectUrl) {
6070
return rv;
6171
}
6272

63-
@GetMapping("url8")
73+
@GetMapping("url10")
6474
public ModelAndView good2(String token) {
6575
String url = "/edit?token=" + token;
6676
return new ModelAndView("redirect:" + url);
6777
}
78+
79+
@GetMapping("url11")
80+
public String good3(String status) {
81+
return "redirect:" + String.format("/stories/search/criteria?status=%s", status);
82+
}
6883
}

0 commit comments

Comments
 (0)