Skip to content

Commit 498c99e

Browse files
haby0haby0
committed
Add left value, Add return expression tracing flow
1 parent 02e4150 commit 498c99e

File tree

3 files changed

+90
-6
lines changed

3 files changed

+90
-6
lines changed

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

Lines changed: 65 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,48 @@ class StartsWithSanitizer extends DataFlow::BarrierGuard {
1818
}
1919

2020
/**
21-
* A concatenate expression using the string `redirect:` on the left.
21+
* A concatenate expression using the string `redirect:` or `ajaxredirect:` or `forward:` on the left.
2222
*
2323
* E.g: `"redirect:" + redirectUrl`
2424
*/
2525
class RedirectBuilderExpr extends AddExpr {
2626
RedirectBuilderExpr() {
27-
this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "redirect:"
27+
this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() in [
28+
"redirect:", "ajaxredirect:", "forward:"
29+
]
30+
}
31+
}
32+
33+
/**
34+
* A call to `StringBuilder.append` or `StringBuffer.append` method, and the parameter value is
35+
* `"redirect:"` or `"ajaxredirect:"` or `"forward:"`.
36+
*
37+
* E.g: `StringBuilder.append("redirect:")`
38+
*/
39+
class RedirectAppendCall extends MethodAccess {
40+
RedirectAppendCall() {
41+
this.getMethod().hasName("append") and
42+
this.getMethod().getDeclaringType() instanceof StringBuildingType and
43+
this.getArgument(0).(CompileTimeConstantExpr).getStringValue() in [
44+
"redirect:", "ajaxredirect:", "forward:"
45+
]
2846
}
2947
}
3048

3149
/** A URL redirection sink from spring controller method. */
3250
class SpringUrlRedirectSink extends DataFlow::Node {
3351
SpringUrlRedirectSink() {
34-
exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = this.asExpr())
52+
exists(RedirectBuilderExpr rbe |
53+
rbe.getRightOperand() = this.asExpr() and
54+
exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(rbe), _))
55+
)
56+
or
57+
exists(MethodAccess ma, RedirectAppendCall rac |
58+
DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and
59+
ma.getMethod().hasName("append") and
60+
ma.getArgument(0) = this.asExpr() and
61+
exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(ma.getQualifier()), _))
62+
)
3563
or
3664
exists(MethodAccess ma |
3765
ma.getMethod().hasName("setUrl") and
@@ -50,8 +78,40 @@ class SpringUrlRedirectSink extends DataFlow::Node {
5078
or
5179
exists(ClassInstanceExpr cie |
5280
cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
53-
cie.getArgument(0) = this.asExpr() and
54-
exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = this.asExpr())
81+
exists(RedirectBuilderExpr rbe |
82+
rbe = cie.getArgument(0) and rbe.getRightOperand() = this.asExpr()
83+
)
84+
)
85+
}
86+
}
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()
55115
)
56116
}
57117
}

‎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
@@ -3,6 +3,8 @@ edges
33
| SpringUrlRedirect.java:20:24:20:41 | redirectUrl : String | SpringUrlRedirect.java:21:36:21:46 | redirectUrl |
44
| SpringUrlRedirect.java:26:30:26:47 | redirectUrl : String | SpringUrlRedirect.java:27:44:27:54 | redirectUrl |
55
| SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | SpringUrlRedirect.java:33:47:33:57 | redirectUrl |
6+
| SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | SpringUrlRedirect.java:40:29:40:39 | redirectUrl |
7+
| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | SpringUrlRedirect.java:48:30:48:40 | redirectUrl |
68
nodes
79
| SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | semmle.label | redirectUrl : String |
810
| SpringUrlRedirect.java:15:19:15:29 | redirectUrl | semmle.label | redirectUrl |
@@ -12,8 +14,14 @@ nodes
1214
| SpringUrlRedirect.java:27:44:27:54 | redirectUrl | semmle.label | redirectUrl |
1315
| SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | semmle.label | redirectUrl : String |
1416
| SpringUrlRedirect.java:33:47:33:57 | redirectUrl | semmle.label | redirectUrl |
17+
| SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | semmle.label | redirectUrl : String |
18+
| SpringUrlRedirect.java:40:29:40:39 | redirectUrl | semmle.label | redirectUrl |
19+
| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | semmle.label | redirectUrl : String |
20+
| SpringUrlRedirect.java:48:30:48:40 | redirectUrl | semmle.label | redirectUrl |
1521
#select
1622
| 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 |
1723
| 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 |
1824
| 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 |
1925
| 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 |
26+
| 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 |
27+
| 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 |

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,22 @@ public ModelAndView bad4(String redirectUrl) {
3434
}
3535

3636
@GetMapping("url5")
37+
public String bad5(String redirectUrl) {
38+
StringBuffer stringBuffer = new StringBuffer();
39+
stringBuffer.append("redirect:");
40+
stringBuffer.append(redirectUrl);
41+
return stringBuffer.toString();
42+
}
43+
44+
@GetMapping("url6")
45+
public String bad6(String redirectUrl) {
46+
StringBuilder stringBuilder = new StringBuilder();
47+
stringBuilder.append("redirect:");
48+
stringBuilder.append(redirectUrl);
49+
return stringBuilder.toString();
50+
}
51+
52+
@GetMapping("url7")
3753
public RedirectView good1(String redirectUrl) {
3854
RedirectView rv = new RedirectView();
3955
if (redirectUrl.startsWith(VALID_REDIRECT)){
@@ -44,7 +60,7 @@ public RedirectView good1(String redirectUrl) {
4460
return rv;
4561
}
4662

47-
@GetMapping("url6")
63+
@GetMapping("url8")
4864
public ModelAndView good2(String token) {
4965
String url = "/edit?token=" + token;
5066
return new ModelAndView("redirect:" + url);

0 commit comments

Comments
 (0)