+
+
+
+Directly incorporating user input into a URL redirect request without validating the input
+can facilitate phishing attacks. In these attacks, unsuspecting users can be redirected to a
+malicious site that looks very similar to the real site they intend to visit, but which is
+controlled by the attacker.
+
+
+
+
+To guard against untrusted URL redirection, it is advisable to avoid putting user input
+directly into a redirect URL. Instead, maintain a list of authorized
+redirects on the server; then choose from that list based on the user input provided.
+
+
+
+
+The following examples show the bad case and the good case respectively.
+In bad1 method and bad2 method and bad3 method and
+bad4 method, shows an HTTP request parameter being used directly in a URL redirect
+without validating the input, which facilitates phishing attacks. In good1 method,
+shows how to solve this problem by verifying whether the user input is a known fixed string beginning.
+
+
+
+
+
+
+A Guide To Spring Redirects: Spring Redirects.
+Url redirection - attack and defense: Url Redirection.
+
+
diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql
new file mode 100644
index 000000000000..b02bd3e4c302
--- /dev/null
+++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql
@@ -0,0 +1,66 @@
+/**
+ * @name Spring url redirection from remote source
+ * @description Spring url redirection based on unvalidated user-input
+ * may cause redirection to malicious web sites.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/spring-unvalidated-url-redirection
+ * @tags security
+ * external/cwe-601
+ */
+
+import java
+import SpringUrlRedirect
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+private class StartsWithSanitizer extends DataFlow::BarrierGuard {
+ StartsWithSanitizer() {
+ this.(MethodAccess).getMethod().hasName("startsWith") and
+ this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and
+ this.(MethodAccess).getMethod().getNumberOfParameters() = 1
+ }
+
+ override predicate checks(Expr e, boolean branch) {
+ e = this.(MethodAccess).getQualifier() and branch = true
+ }
+}
+
+class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration {
+ SpringUrlRedirectFlowConfig() { this = "SpringUrlRedirectFlowConfig" }
+
+ override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+ override predicate isSink(DataFlow::Node sink) { sink instanceof SpringUrlRedirectSink }
+
+ override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+ guard instanceof StartsWithSanitizer
+ }
+
+ override predicate isSanitizer(DataFlow::Node node) {
+ // Exclude the case where the left side of the concatenated string is not `redirect:`.
+ // E.g: `String url = "/path?token=" + request.getParameter("token");`
+ // Note this is quite a broad sanitizer (it will also sanitize the right-hand side of `url = "http://" + request.getParameter("token")`);
+ // Consider making this stricter in future.
+ exists(AddExpr ae |
+ ae.getRightOperand() = node.asExpr() and
+ not ae instanceof RedirectBuilderExpr
+ )
+ or
+ exists(MethodAccess ma, int index |
+ ma.getMethod().hasName("format") and
+ ma.getMethod().getDeclaringType() instanceof TypeString and
+ ma.getArgument(index) = node.asExpr() and
+ (
+ index != 0 and
+ not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().regexpMatch("^%s.*")
+ )
+ )
+ }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, SpringUrlRedirectFlowConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Potentially untrusted URL redirection due to $@.",
+ source.getNode(), "user-provided value"
diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll
new file mode 100644
index 000000000000..4a86640d4d45
--- /dev/null
+++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll
@@ -0,0 +1,73 @@
+import java
+import DataFlow
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.DataFlow2
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.frameworks.spring.SpringController
+
+/**
+ * A concatenate expression using the string `redirect:` or `ajaxredirect:` or `forward:` on the left.
+ *
+ * E.g: `"redirect:" + redirectUrl`
+ */
+class RedirectBuilderExpr extends AddExpr {
+ RedirectBuilderExpr() {
+ this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() in [
+ "redirect:", "ajaxredirect:", "forward:"
+ ]
+ }
+}
+
+/**
+ * A call to `StringBuilder.append` or `StringBuffer.append` method, and the parameter value is
+ * `"redirect:"` or `"ajaxredirect:"` or `"forward:"`.
+ *
+ * E.g: `StringBuilder.append("redirect:")`
+ */
+class RedirectAppendCall extends MethodAccess {
+ RedirectAppendCall() {
+ this.getMethod().hasName("append") and
+ this.getMethod().getDeclaringType() instanceof StringBuildingType and
+ this.getArgument(0).(CompileTimeConstantExpr).getStringValue() in [
+ "redirect:", "ajaxredirect:", "forward:"
+ ]
+ }
+}
+
+/** A URL redirection sink from spring controller method. */
+class SpringUrlRedirectSink extends DataFlow::Node {
+ SpringUrlRedirectSink() {
+ exists(RedirectBuilderExpr rbe |
+ rbe.getRightOperand() = this.asExpr() and
+ any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
+ )
+ or
+ exists(MethodAccess ma, RedirectAppendCall rac |
+ DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and
+ ma.getMethod().hasName("append") and
+ ma.getArgument(0) = this.asExpr() and
+ any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
+ )
+ or
+ exists(MethodAccess ma |
+ ma.getMethod().hasName("setUrl") and
+ ma.getMethod()
+ .getDeclaringType()
+ .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and
+ ma.getArgument(0) = this.asExpr()
+ )
+ or
+ exists(ClassInstanceExpr cie |
+ cie.getConstructedType()
+ .hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and
+ cie.getArgument(0) = this.asExpr()
+ )
+ or
+ exists(ClassInstanceExpr cie |
+ cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
+ exists(RedirectBuilderExpr rbe |
+ rbe = cie.getArgument(0) and rbe.getRightOperand() = this.asExpr()
+ )
+ )
+ }
+}
diff --git a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected
new file mode 100644
index 000000000000..bcf5e892e1ba
--- /dev/null
+++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected
@@ -0,0 +1,35 @@
+edges
+| SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | SpringUrlRedirect.java:15:19:15:29 | redirectUrl |
+| SpringUrlRedirect.java:20:24:20:41 | redirectUrl : String | SpringUrlRedirect.java:21:36:21:46 | redirectUrl |
+| SpringUrlRedirect.java:26:30:26:47 | redirectUrl : String | SpringUrlRedirect.java:27:44:27:54 | redirectUrl |
+| SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | SpringUrlRedirect.java:33:47:33:57 | redirectUrl |
+| SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | SpringUrlRedirect.java:40:29:40:39 | redirectUrl |
+| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | SpringUrlRedirect.java:48:30:48:40 | redirectUrl |
+| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | SpringUrlRedirect.java:54:30:54:66 | format(...) |
+| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | SpringUrlRedirect.java:59:30:59:76 | format(...) |
+nodes
+| SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:15:19:15:29 | redirectUrl | semmle.label | redirectUrl |
+| SpringUrlRedirect.java:20:24:20:41 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:21:36:21:46 | redirectUrl | semmle.label | redirectUrl |
+| SpringUrlRedirect.java:26:30:26:47 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:27:44:27:54 | redirectUrl | semmle.label | redirectUrl |
+| SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:33:47:33:57 | redirectUrl | semmle.label | redirectUrl |
+| SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:40:29:40:39 | redirectUrl | semmle.label | redirectUrl |
+| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:48:30:48:40 | redirectUrl | semmle.label | redirectUrl |
+| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:54:30:54:66 | format(...) | semmle.label | format(...) |
+| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | semmle.label | redirectUrl : String |
+| SpringUrlRedirect.java:59:30:59:76 | format(...) | semmle.label | format(...) |
+#select
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
+| 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 |
diff --git a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java
new file mode 100644
index 000000000000..f3958cba102d
--- /dev/null
+++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java
@@ -0,0 +1,83 @@
+import javax.servlet.http.HttpServletResponse;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.servlet.ModelAndView;
+import org.springframework.web.servlet.view.RedirectView;
+
+@Controller
+public class SpringUrlRedirect {
+
+ private final static String VALID_REDIRECT = "http://127.0.0.1";
+
+ @GetMapping("url1")
+ public RedirectView bad1(String redirectUrl, HttpServletResponse response) throws Exception {
+ RedirectView rv = new RedirectView();
+ rv.setUrl(redirectUrl);
+ return rv;
+ }
+
+ @GetMapping("url2")
+ public String bad2(String redirectUrl) {
+ String url = "redirect:" + redirectUrl;
+ return url;
+ }
+
+ @GetMapping("url3")
+ public RedirectView bad3(String redirectUrl) {
+ RedirectView rv = new RedirectView(redirectUrl);
+ return rv;
+ }
+
+ @GetMapping("url4")
+ public ModelAndView bad4(String redirectUrl) {
+ return new ModelAndView("redirect:" + redirectUrl);
+ }
+
+ @GetMapping("url5")
+ public String bad5(String redirectUrl) {
+ StringBuffer stringBuffer = new StringBuffer();
+ stringBuffer.append("redirect:");
+ stringBuffer.append(redirectUrl);
+ return stringBuffer.toString();
+ }
+
+ @GetMapping("url6")
+ public String bad6(String redirectUrl) {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append("redirect:");
+ stringBuilder.append(redirectUrl);
+ return stringBuilder.toString();
+ }
+
+ @GetMapping("url7")
+ public String bad7(String redirectUrl) {
+ return "redirect:" + String.format("%s/?aaa", redirectUrl);
+ }
+
+ @GetMapping("url8")
+ public String bad8(String redirectUrl, String token) {
+ return "redirect:" + String.format(redirectUrl + "?token=%s", token);
+ }
+
+ @GetMapping("url9")
+ public RedirectView good1(String redirectUrl) {
+ RedirectView rv = new RedirectView();
+ if (redirectUrl.startsWith(VALID_REDIRECT)){
+ rv.setUrl(redirectUrl);
+ }else {
+ rv.setUrl(VALID_REDIRECT);
+ }
+ return rv;
+ }
+
+ @GetMapping("url10")
+ public ModelAndView good2(String token) {
+ String url = "/edit?token=" + token;
+ return new ModelAndView("redirect:" + url);
+ }
+
+ @GetMapping("url11")
+ public String good3(String status) {
+ return "redirect:" + String.format("/stories/search/criteria?status=%s", status);
+ }
+}
diff --git a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.qlref b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.qlref
new file mode 100644
index 000000000000..418be1d307be
--- /dev/null
+++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.qlref
@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql
diff --git a/java/ql/test/experimental/query-tests/security/CWE-601/options b/java/ql/test/experimental/query-tests/security/CWE-601/options
new file mode 100644
index 000000000000..a92891087477
--- /dev/null
+++ b/java/ql/test/experimental/query-tests/security/CWE-601/options
@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/
\ No newline at end of file
diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/ModelAndView.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/ModelAndView.java
new file mode 100644
index 000000000000..53e337d50538
--- /dev/null
+++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/ModelAndView.java
@@ -0,0 +1,107 @@
+package org.springframework.web.servlet;
+
+import java.util.Map;
+import org.springframework.http.HttpStatus;
+import org.springframework.lang.Nullable;
+
+public class ModelAndView {
+ @Nullable
+ private Object view;
+ @Nullable
+ private HttpStatus status;
+ private boolean cleared = false;
+
+ public ModelAndView() {
+ }
+
+ public ModelAndView(String viewName) {
+ this.view = viewName;
+ }
+
+ public ModelAndView(View view) {
+ this.view = view;
+ }
+
+ public ModelAndView(String viewName, @Nullable Map