From effa2b162a77031186bf26114e2e6e0e6e847154 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 6 May 2021 12:05:26 +0800 Subject: [PATCH 1/6] Add spring url redirection detect --- .../CWE/CWE-601/SpringUrlRedirect.java | 46 +++++++ .../CWE/CWE-601/SpringUrlRedirect.qhelp | 37 +++++ .../Security/CWE/CWE-601/SpringUrlRedirect.ql | 42 ++++++ .../CWE/CWE-601/SpringUrlRedirect.qll | 91 ++++++++++++ .../CWE-601/SpringUrlRedirect.expected | 19 +++ .../security/CWE-601/SpringUrlRedirect.java | 52 +++++++ .../security/CWE-601/SpringUrlRedirect.qlref | 1 + .../query-tests/security/CWE-601/options | 1 + .../web/servlet/ModelAndView.java | 107 +++++++++++++++ .../org/springframework/web/servlet/View.java | 20 +++ .../servlet/view/AbstractUrlBasedView.java | 39 ++++++ .../web/servlet/view/RedirectView.java | 129 ++++++++++++++++++ 12 files changed, 584 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll create mode 100644 java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected create mode 100644 java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.qlref create mode 100644 java/ql/test/experimental/query-tests/security/CWE-601/options create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/ModelAndView.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/View.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/AbstractUrlBasedView.java create mode 100644 java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/RedirectView.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.java b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.java new file mode 100644 index 000000000000..eba64aab6a86 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.java @@ -0,0 +1,46 @@ +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 RedirectView good1(String redirectUrl) { + RedirectView rv = new RedirectView(); + if (redirectUrl.startsWith(VALID_REDIRECT)){ + rv.setUrl(redirectUrl); + }else { + rv.setUrl(VALID_REDIRECT); + } + return rv; + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qhelp b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qhelp new file mode 100644 index 000000000000..6fe70dfb1139 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qhelp @@ -0,0 +1,37 @@ + + + + + +

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..138bce57ac97 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql @@ -0,0 +1,42 @@ +/** + * @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 + +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");` + exists(AddExpr ae | + ae.getRightOperand() = node.asExpr() and + not ae instanceof RedirectBuilderExpr + ) + } +} + +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..0ea88e846731 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll @@ -0,0 +1,91 @@ +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 + +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 + } +} + +/** + * A concatenate expression using the string `redirect:` on the left. + * + * E.g: `"redirect:" + redirectUrl` + */ +class RedirectBuilderExpr extends AddExpr { + RedirectBuilderExpr() { + this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "redirect:" + } +} + +/** A URL redirection sink from spring controller method. */ +class SpringUrlRedirectSink extends DataFlow::Node { + SpringUrlRedirectSink() { + exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = this.asExpr()) + 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() and + exists(RedirectViewFlowConfig rvfc | rvfc.hasFlowToExpr(ma.getQualifier())) + ) + 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 + cie.getArgument(0) = this.asExpr() and + exists(RedirectBuilderFlowConfig rstrbfc | rstrbfc.hasFlowToExpr(cie.getArgument(0))) + ) + } +} + +/** A data flow configuration tracing flow from remote sources to redirect builder expression. */ +private class RedirectBuilderFlowConfig extends DataFlow2::Configuration { + RedirectBuilderFlowConfig() { this = "RedirectBuilderFlowConfig" } + + override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { + exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = sink.asExpr()) + } +} + +/** A data flow configuration tracing flow from RedirectView object to calling setUrl method. */ +private class RedirectViewFlowConfig extends DataFlow2::Configuration { + RedirectViewFlowConfig() { this = "RedirectViewFlowConfig" } + + override predicate isSource(DataFlow::Node src) { + exists(ClassInstanceExpr cie | + cie.getConstructedType() + .hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and + cie = src.asExpr() + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod().hasName("setUrl") and + ma.getMethod() + .getDeclaringType() + .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and + ma.getQualifier() = sink.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..fee0598bbeef --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected @@ -0,0 +1,19 @@ +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 | +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 | +#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 | 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..1438b0a63a18 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java @@ -0,0 +1,52 @@ +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 RedirectView good1(String redirectUrl) { + RedirectView rv = new RedirectView(); + if (redirectUrl.startsWith(VALID_REDIRECT)){ + rv.setUrl(redirectUrl); + }else { + rv.setUrl(VALID_REDIRECT); + } + return rv; + } + + @GetMapping("url6") + public ModelAndView good2(String token) { + String url = "/edit?token=" + token; + return new ModelAndView("redirect:" + url); + } +} 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 model) { } + + public ModelAndView(View view, @Nullable Map model) { } + + public ModelAndView(String viewName, HttpStatus status) { } + + public ModelAndView(@Nullable String viewName, @Nullable Map model, @Nullable HttpStatus status) { } + + public ModelAndView(String viewName, String modelName, Object modelObject) { } + + public ModelAndView(View view, String modelName, Object modelObject) { } + + public void setViewName(@Nullable String viewName) { + this.view = viewName; + } + + @Nullable + public String getViewName() { + return ""; + } + + public void setView(@Nullable View view) { } + + @Nullable + public View getView() { + return null; + } + + public boolean hasView() { + return true; + } + + public boolean isReference() { + return true; + } + + @Nullable + protected Map getModelInternal() { + return null; + } + + public Map getModel() { + return null; + } + + public void setStatus(@Nullable HttpStatus status) { } + + @Nullable + public HttpStatus getStatus() { + return this.status; + } + + public ModelAndView addObject(String attributeName, @Nullable Object attributeValue) { + return null; + } + + public ModelAndView addObject(Object attributeValue) { + return null; + } + + public ModelAndView addAllObjects(@Nullable Map modelMap) { + return null; + } + + public void clear() { } + + public boolean isEmpty() { + return true; + } + + public boolean wasCleared() { + return true; + } + + public String toString() { + return ""; + } + + private String formatView() { + return ""; + } +} + diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/View.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/View.java new file mode 100644 index 000000000000..b2281b8c2505 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/View.java @@ -0,0 +1,20 @@ +package org.springframework.web.servlet; + +import java.util.Map; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.springframework.lang.Nullable; + +public interface View { + String RESPONSE_STATUS_ATTRIBUTE = View.class.getName() + ".responseStatus"; + String PATH_VARIABLES = View.class.getName() + ".pathVariables"; + String SELECTED_CONTENT_TYPE = View.class.getName() + ".selectedContentType"; + + @Nullable + default String getContentType() { + return null; + } + + void render(@Nullable Map var1, HttpServletRequest var2, HttpServletResponse var3) throws Exception; +} + diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/AbstractUrlBasedView.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/AbstractUrlBasedView.java new file mode 100644 index 000000000000..9efd87af12f3 --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/AbstractUrlBasedView.java @@ -0,0 +1,39 @@ +package org.springframework.web.servlet.view; + +import java.util.Locale; +import org.springframework.lang.Nullable; + +public abstract class AbstractUrlBasedView { + @Nullable + private String url; + + protected AbstractUrlBasedView() { } + + protected AbstractUrlBasedView(String url) { + this.url = url; + } + + public void setUrl(@Nullable String url) { + this.url = url; + } + + @Nullable + public String getUrl() { + return ""; + } + + public void afterPropertiesSet() throws Exception { } + + protected boolean isUrlRequired() { + return true; + } + + public boolean checkResource(Locale locale) throws Exception { + return true; + } + + public String toString() { + return ""; + } +} + diff --git a/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/RedirectView.java b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/RedirectView.java new file mode 100644 index 000000000000..ee18868231ae --- /dev/null +++ b/java/ql/test/stubs/springframework-5.2.3/org/springframework/web/servlet/view/RedirectView.java @@ -0,0 +1,129 @@ +package org.springframework.web.servlet.view; + +import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.lang.reflect.Array; +import java.net.URLEncoder; +import java.util.Collection; +import java.util.Collections; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Map.Entry; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.springframework.http.HttpStatus; +import org.springframework.lang.Nullable; + +public class RedirectView extends AbstractUrlBasedView { + private static final Pattern URI_TEMPLATE_VARIABLE_PATTERN = Pattern.compile("\\{([^/]+?)\\}"); + private boolean contextRelative = false; + private boolean http10Compatible = true; + private boolean exposeModelAttributes = true; + @Nullable + private String encodingScheme; + @Nullable + private HttpStatus statusCode; + private boolean expandUriTemplateVariables = true; + private boolean propagateQueryParams = false; + @Nullable + private String[] hosts; + + public RedirectView() { } + + public RedirectView(String url) { } + + public RedirectView(String url, boolean contextRelative) { } + + public RedirectView(String url, boolean contextRelative, boolean http10Compatible) { } + + public RedirectView(String url, boolean contextRelative, boolean http10Compatible, boolean exposeModelAttributes) { } + + public void setContextRelative(boolean contextRelative) { } + + public void setHttp10Compatible(boolean http10Compatible) { } + + public void setExposeModelAttributes(boolean exposeModelAttributes) { } + + public void setEncodingScheme(String encodingScheme) { } + + public void setStatusCode(HttpStatus statusCode) { } + + public void setExpandUriTemplateVariables(boolean expandUriTemplateVariables) { } + + public void setPropagateQueryParams(boolean propagateQueryParams) { } + + public boolean isPropagateQueryProperties() { + return true; + } + + public void setHosts(@Nullable String... hosts) { } + + @Nullable + public String[] getHosts() { + return this.hosts; + } + + public boolean isRedirectView() { + return true; + } + + protected boolean isContextRequired() { + return false; + } + + protected void renderMergedOutputModel(Map model, HttpServletRequest request, HttpServletResponse response) throws IOException { } + + protected final String createTargetUrl(Map model, HttpServletRequest request) throws UnsupportedEncodingException { + return ""; + } + + private String getContextPath(HttpServletRequest request) { + return ""; + } + + protected StringBuilder replaceUriTemplateVariables(String targetUrl, Map model, Map currentUriVariables, String encodingScheme) throws UnsupportedEncodingException { + return null; + } + + private Map getCurrentRequestUriVariables(HttpServletRequest request) { + return null; + } + + protected void appendCurrentQueryParams(StringBuilder targetUrl, HttpServletRequest request) { } + + protected void appendQueryProperties(StringBuilder targetUrl, Map model, String encodingScheme) throws UnsupportedEncodingException { } + + protected Map queryProperties(Map model) { + return null; + } + + protected boolean isEligibleProperty(String key, @Nullable Object value) { + return true; + } + + protected boolean isEligibleValue(@Nullable Object value) { + return true; + } + + protected String urlEncode(String input, String encodingScheme) throws UnsupportedEncodingException { + return ""; + } + + protected String updateTargetUrl(String targetUrl, Map model, HttpServletRequest request, HttpServletResponse response) { + return ""; + } + + protected void sendRedirect(HttpServletRequest request, HttpServletResponse response, String targetUrl, boolean http10Compatible) throws IOException { } + + protected boolean isRemoteHost(String targetUrl) { + return true; + } + + protected HttpStatus getHttp11StatusCode(HttpServletRequest request, HttpServletResponse response, String targetUrl) { + return this.statusCode; + } +} + From 02e415045f04957a33049907bd3256ce84ad7378 Mon Sep 17 00:00:00 2001 From: haby0 Date: Thu, 13 May 2021 15:48:15 +0800 Subject: [PATCH 2/6] Delete RedirectBuilderFlowConfig --- .../Security/CWE/CWE-601/SpringUrlRedirect.qll | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll index 0ea88e846731..1ab5f3cd0b13 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll @@ -51,22 +51,11 @@ class SpringUrlRedirectSink extends DataFlow::Node { exists(ClassInstanceExpr cie | cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and cie.getArgument(0) = this.asExpr() and - exists(RedirectBuilderFlowConfig rstrbfc | rstrbfc.hasFlowToExpr(cie.getArgument(0))) + exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = this.asExpr()) ) } } -/** A data flow configuration tracing flow from remote sources to redirect builder expression. */ -private class RedirectBuilderFlowConfig extends DataFlow2::Configuration { - RedirectBuilderFlowConfig() { this = "RedirectBuilderFlowConfig" } - - override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { - exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = sink.asExpr()) - } -} - /** A data flow configuration tracing flow from RedirectView object to calling setUrl method. */ private class RedirectViewFlowConfig extends DataFlow2::Configuration { RedirectViewFlowConfig() { this = "RedirectViewFlowConfig" } From 498c99e26ca556e466f00015a6203f3ce64a5362 Mon Sep 17 00:00:00 2001 From: haby0 Date: Fri, 14 May 2021 16:31:59 +0800 Subject: [PATCH 3/6] Add left value, Add return expression tracing flow --- .../CWE/CWE-601/SpringUrlRedirect.qll | 70 +++++++++++++++++-- .../CWE-601/SpringUrlRedirect.expected | 8 +++ .../security/CWE-601/SpringUrlRedirect.java | 18 ++++- 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll index 1ab5f3cd0b13..866eaae1c34f 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll @@ -18,20 +18,48 @@ class StartsWithSanitizer extends DataFlow::BarrierGuard { } /** - * A concatenate expression using the string `redirect:` on the left. + * 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() = "redirect:" + 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()) + exists(RedirectBuilderExpr rbe | + rbe.getRightOperand() = this.asExpr() and + exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(rbe), _)) + ) + or + exists(MethodAccess ma, RedirectAppendCall rac | + DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and + ma.getMethod().hasName("append") and + ma.getArgument(0) = this.asExpr() and + exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(ma.getQualifier()), _)) + ) or exists(MethodAccess ma | ma.getMethod().hasName("setUrl") and @@ -50,8 +78,40 @@ class SpringUrlRedirectSink extends DataFlow::Node { or exists(ClassInstanceExpr cie | cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and - cie.getArgument(0) = this.asExpr() and - exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = this.asExpr()) + exists(RedirectBuilderExpr rbe | + rbe = cie.getArgument(0) and rbe.getRightOperand() = this.asExpr() + ) + ) + } +} + +/** A data flow configuration tracing flow from redirect builder expression to spring controller method return expression. */ +private class RedirectBuilderFlowConfig extends DataFlow2::Configuration { + RedirectBuilderFlowConfig() { this = "RedirectBuilderFlowConfig" } + + override predicate isSource(DataFlow::Node src) { + exists(RedirectBuilderExpr rbe | rbe = src.asExpr()) + or + exists(MethodAccess ma, RedirectAppendCall rac | + DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and + ma.getMethod().hasName("append") and + ma.getQualifier() = src.asExpr() + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(ReturnStmt rs, SpringRequestMappingMethod sqmm | + rs.getResult() = sink.asExpr() and + sqmm.getBody().getAStmt() = rs + ) + } + + override predicate isAdditionalFlowStep(Node prod, Node succ) { + exists(MethodAccess ma | + ma.getMethod().hasName("toString") and + ma.getMethod().getDeclaringType() instanceof StringBuildingType and + ma.getQualifier() = prod.asExpr() and + ma = succ.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 index fee0598bbeef..26b8acd77700 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected @@ -3,6 +3,8 @@ edges | 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 | nodes | SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | semmle.label | redirectUrl : String | | SpringUrlRedirect.java:15:19:15:29 | redirectUrl | semmle.label | redirectUrl | @@ -12,8 +14,14 @@ nodes | 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 | #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 | 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 index 1438b0a63a18..5124d8cd8c62 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java @@ -34,6 +34,22 @@ public ModelAndView bad4(String 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 RedirectView good1(String redirectUrl) { RedirectView rv = new RedirectView(); if (redirectUrl.startsWith(VALID_REDIRECT)){ @@ -44,7 +60,7 @@ public RedirectView good1(String redirectUrl) { return rv; } - @GetMapping("url6") + @GetMapping("url8") public ModelAndView good2(String token) { String url = "/edit?token=" + token; return new ModelAndView("redirect:" + url); From a0cd551bae9ae005c59657ef3463817e226cfe29 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 18 May 2021 11:05:10 +0800 Subject: [PATCH 4/6] Add filtering of String.format --- .../Security/CWE/CWE-601/SpringUrlRedirect.ql | 10 +++ .../CWE/CWE-601/SpringUrlRedirect.qll | 61 +------------------ .../CWE-601/SpringUrlRedirect.expected | 8 +++ .../security/CWE-601/SpringUrlRedirect.java | 17 +++++- 4 files changed, 37 insertions(+), 59 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql index 138bce57ac97..5c8a8ea78d8c 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql @@ -33,6 +33,16 @@ class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration { 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.*") + ) + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll index 866eaae1c34f..84bb5d9adf80 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll @@ -51,14 +51,14 @@ class SpringUrlRedirectSink extends DataFlow::Node { SpringUrlRedirectSink() { exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = this.asExpr() and - exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(rbe), _)) + 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 - exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(ma.getQualifier()), _)) + any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) ) or exists(MethodAccess ma | @@ -66,8 +66,7 @@ class SpringUrlRedirectSink extends DataFlow::Node { ma.getMethod() .getDeclaringType() .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and - ma.getArgument(0) = this.asExpr() and - exists(RedirectViewFlowConfig rvfc | rvfc.hasFlowToExpr(ma.getQualifier())) + ma.getArgument(0) = this.asExpr() ) or exists(ClassInstanceExpr cie | @@ -84,57 +83,3 @@ class SpringUrlRedirectSink extends DataFlow::Node { ) } } - -/** A data flow configuration tracing flow from redirect builder expression to spring controller method return expression. */ -private class RedirectBuilderFlowConfig extends DataFlow2::Configuration { - RedirectBuilderFlowConfig() { this = "RedirectBuilderFlowConfig" } - - override predicate isSource(DataFlow::Node src) { - exists(RedirectBuilderExpr rbe | rbe = src.asExpr()) - or - exists(MethodAccess ma, RedirectAppendCall rac | - DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and - ma.getMethod().hasName("append") and - ma.getQualifier() = src.asExpr() - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(ReturnStmt rs, SpringRequestMappingMethod sqmm | - rs.getResult() = sink.asExpr() and - sqmm.getBody().getAStmt() = rs - ) - } - - override predicate isAdditionalFlowStep(Node prod, Node succ) { - exists(MethodAccess ma | - ma.getMethod().hasName("toString") and - ma.getMethod().getDeclaringType() instanceof StringBuildingType and - ma.getQualifier() = prod.asExpr() and - ma = succ.asExpr() - ) - } -} - -/** A data flow configuration tracing flow from RedirectView object to calling setUrl method. */ -private class RedirectViewFlowConfig extends DataFlow2::Configuration { - RedirectViewFlowConfig() { this = "RedirectViewFlowConfig" } - - override predicate isSource(DataFlow::Node src) { - exists(ClassInstanceExpr cie | - cie.getConstructedType() - .hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and - cie = src.asExpr() - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod().hasName("setUrl") and - ma.getMethod() - .getDeclaringType() - .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and - ma.getQualifier() = sink.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 index 26b8acd77700..bcf5e892e1ba 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected @@ -5,6 +5,8 @@ edges | 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 | @@ -18,6 +20,10 @@ nodes | 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 | @@ -25,3 +31,5 @@ nodes | 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 index 5124d8cd8c62..f3958cba102d 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java @@ -50,6 +50,16 @@ public String bad6(String redirectUrl) { } @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)){ @@ -60,9 +70,14 @@ public RedirectView good1(String redirectUrl) { return rv; } - @GetMapping("url8") + @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); + } } From caf5f4d605bbdcb67fa65de5b65f9288a39d8ef4 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 18 May 2021 19:10:03 +0800 Subject: [PATCH 5/6] modified comment --- .../src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql | 2 ++ .../src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql index 5c8a8ea78d8c..210067fde2ac 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql @@ -29,6 +29,8 @@ class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration { 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 diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll index 84bb5d9adf80..6ae87a37bcc6 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll @@ -5,7 +5,7 @@ import semmle.code.java.dataflow.DataFlow2 import semmle.code.java.dataflow.TaintTracking import semmle.code.java.frameworks.spring.SpringController -class StartsWithSanitizer extends DataFlow::BarrierGuard { +private class StartsWithSanitizer extends DataFlow::BarrierGuard { StartsWithSanitizer() { this.(MethodAccess).getMethod().hasName("startsWith") and this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and From e46de44473791493bd73068dabced3be868da819 Mon Sep 17 00:00:00 2001 From: haby0 Date: Tue, 18 May 2021 19:56:32 +0800 Subject: [PATCH 6/6] Solve errors caused by private ownership --- .../Security/CWE/CWE-601/SpringUrlRedirect.ql | 12 ++++++++++++ .../Security/CWE/CWE-601/SpringUrlRedirect.qll | 12 ------------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql index 210067fde2ac..b02bd3e4c302 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql @@ -15,6 +15,18 @@ 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" } diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll index 6ae87a37bcc6..4a86640d4d45 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll @@ -5,18 +5,6 @@ import semmle.code.java.dataflow.DataFlow2 import semmle.code.java.dataflow.TaintTracking import semmle.code.java.frameworks.spring.SpringController -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 - } -} - /** * A concatenate expression using the string `redirect:` or `ajaxredirect:` or `forward:` on the left. *