From b69eba923831d895971915fe34f9fdbac8ca1783 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 29 Jul 2022 01:59:47 +0000 Subject: [PATCH] Add check for Spring redirect --- .../CWE/CWE-625/PermissiveDotRegex.ql | 33 +++++- .../CWE/CWE-625/SpringUrlRedirect.qll | 109 ++++++++++++++++++ .../security/CWE-625/DotRegexSpring.java | 75 ++++++++++++ .../CWE-625/PermissiveDotRegex.expected | 45 ++++++++ .../query-tests/security/CWE-625/options | 2 +- 5 files changed, 261 insertions(+), 3 deletions(-) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-625/SpringUrlRedirect.qll create mode 100644 java/ql/test/experimental/query-tests/security/CWE-625/DotRegexSpring.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-625/PermissiveDotRegex.ql b/java/ql/src/experimental/Security/CWE/CWE-625/PermissiveDotRegex.ql index d5b59dd7638..693cc77a562 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-625/PermissiveDotRegex.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-625/PermissiveDotRegex.ql @@ -18,6 +18,7 @@ import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.UrlRedirect import DataFlow::PathGraph import Regex +import SpringUrlRedirect /** Source model of remote flow source with servlets. */ private class GetServletUriSource extends SourceModelCsv { @@ -51,6 +52,30 @@ private class UrlFilterSink extends SinkModelCsv { } } +/** A Spring framework annotation indicating remote uri user input. */ +class SpringUriInputAnnotation extends Annotation { + SpringUriInputAnnotation() { + exists(AnnotationType a | + a = this.getType() and + a.getPackage().getName() = "org.springframework.web.bind.annotation" + | + ( + a.hasName("PathVariable") or + a.hasName("RequestParam") + ) + ) + } +} + +class SpringUriInputParameterSource extends DataFlow::Node { + SpringUriInputParameterSource() { + this.asParameter() = + any(SpringRequestMappingParameter srmp | + srmp.getAnAnnotation() instanceof SpringUriInputAnnotation + ) + } +} + /** * `.` without a `\` prefix, which is likely not a character literal in regex */ @@ -121,7 +146,10 @@ class PermissiveDotRegexConfig extends DataFlow::Configuration { class MatchRegexConfiguration extends TaintTracking::Configuration { MatchRegexConfiguration() { this = "PermissiveDotRegex::MatchRegexConfiguration" } - override predicate isSource(DataFlow::Node source) { sourceNode(source, "uri-path") } + override predicate isSource(DataFlow::Node source) { + sourceNode(source, "uri-path") or // Servlet uri source + source instanceof SpringUriInputParameterSource // Spring uri source + } override predicate isSink(DataFlow::Node sink) { sink instanceof MatchRegexSink and @@ -140,7 +168,8 @@ class MatchRegexConfiguration extends TaintTracking::Configuration { ( DataFlow::exprNode(se) instanceof UrlRedirectSink or sinkNode(DataFlow::exprNode(se), "url-dispatch") or - sinkNode(DataFlow::exprNode(se), "url-filter") + sinkNode(DataFlow::exprNode(se), "url-filter") or + DataFlow::exprNode(se) instanceof SpringUrlRedirectSink ) and guard.controls(se.getBasicBlock(), true) ) diff --git a/java/ql/src/experimental/Security/CWE/CWE-625/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-625/SpringUrlRedirect.qll new file mode 100644 index 00000000000..113afcded5b --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-625/SpringUrlRedirect.qll @@ -0,0 +1,109 @@ +/** Provides methods related to Spring URL redirect from /src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll. */ + +private import java +private import semmle.code.java.dataflow.FlowSources + +/** + * 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. */ +abstract class SpringUrlRedirectSink extends DataFlow::Node { } + +/** + * A sink for URL Redirection via the Spring View classes. + */ +private class SpringViewUrlRedirectSink extends SpringUrlRedirectSink { + SpringViewUrlRedirectSink() { + // Hardcoded redirect such as "redirect:login" + this.asExpr() + .(CompileTimeConstantExpr) + .getStringValue() + .indexOf(["redirect:", "ajaxredirect:", "forward:"]) = 0 and + any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) + or + 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() + ) + ) + } +} + +/** + * A sink for URL Redirection via the `ResponseEntity` class. + */ +private class SpringResponseEntityUrlRedirectSink extends SpringUrlRedirectSink { + SpringResponseEntityUrlRedirectSink() { + // Find `new ResponseEntity(httpHeaders, ...)` or + // `new ResponseEntity(..., httpHeaders, ...)` sinks + exists(ClassInstanceExpr cie, Argument argument | + cie.getConstructedType() instanceof SpringResponseEntity and + argument.getType() instanceof SpringHttpHeaders and + argument = cie.getArgument([0, 1]) and + this.asExpr() = argument + ) + or + // Find `ResponseEntity.status(...).headers(taintHeaders).build()` or + // `ResponseEntity.status(...).location(URI.create(taintURL)).build()` sinks + exists(MethodAccess ma | + ma.getMethod() + .getDeclaringType() + .hasQualifiedName("org.springframework.http", "ResponseEntity$HeadersBuilder") and + ma.getMethod().getName() in ["headers", "location"] and + this.asExpr() = ma.getArgument(0) + ) + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-625/DotRegexSpring.java b/java/ql/test/experimental/query-tests/security/CWE-625/DotRegexSpring.java new file mode 100644 index 00000000000..4651508fe19 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-625/DotRegexSpring.java @@ -0,0 +1,75 @@ +import org.springframework.stereotype.Controller; +import org.springframework.ui.Model; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.servlet.view.RedirectView; + +import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +@Controller +public class DotRegexSpring { + private static final String PROTECTED_PATTERN = "/protected/.*"; + private static final String CONSTRAINT_PATTERN = "/protected/xyz\\.xml"; + + @GetMapping("param") + // BAD: A string with line return e.g. `/protected/%0dxyz` can bypass the path check + public String withParam(@RequestParam String path, Model model) throws UnsupportedEncodingException { + Pattern p = Pattern.compile(PROTECTED_PATTERN); + path = decodePath(path); + Matcher m = p.matcher(path); + + if (m.matches()) { + // Protected page - check access token and redirect to login page + if (model.getAttribute("secAttr") == null || !model.getAttribute("secAttr").equals("secValue")) { + return "redirect:login"; + } + } + // Not protected page - render content + return path; + } + + @GetMapping("{path}") + // BAD: A string with line return e.g. `%252Fprotected%252F%250dxyz` can bypass the path check + public RedirectView withPathVariable1(@PathVariable String path, Model model) throws UnsupportedEncodingException { + Pattern p = Pattern.compile(PROTECTED_PATTERN); + path = decodePath(path); + Matcher m = p.matcher(path); + + if (m.matches()) { + // Protected page - check access token and redirect to login page + if (model.getAttribute("secAttr") == null || !model.getAttribute("secAttr").equals("secValue")) { + RedirectView redirectView = new RedirectView("login", true); + return redirectView; + } + } + return null; + } + + @GetMapping("/sp/{path}") + // GOOD: A string with line return e.g. `%252Fprotected%252F%250dxyz` cannot bypass the path check + public String withPathVariable2(@PathVariable String path, Model model) throws UnsupportedEncodingException { + Pattern p = Pattern.compile(CONSTRAINT_PATTERN); + path = decodePath(path); + Matcher m = p.matcher(path); + + if (m.matches()) { + // Protected page - check access token and redirect to login page + if (model.getAttribute("secAttr") == null || !model.getAttribute("secAttr").equals("secValue")) { + return "redirect:login"; + } + } + // Not protected page - render content + return path; + } + + private String decodePath(String path) throws UnsupportedEncodingException { + while (path.indexOf("%") > -1) { + path = URLDecoder.decode(path, "UTF-8"); + } + return path; + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-625/PermissiveDotRegex.expected b/java/ql/test/experimental/query-tests/security/CWE-625/PermissiveDotRegex.expected index 3d7844219b1..48d8bfb72b8 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-625/PermissiveDotRegex.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-625/PermissiveDotRegex.expected @@ -15,6 +15,26 @@ edges | DotRegexServlet.java:93:19:93:39 | getPathInfo(...) : String | DotRegexServlet.java:96:25:96:30 | source | | DotRegexServlet.java:112:19:112:39 | getPathInfo(...) : String | DotRegexServlet.java:115:25:115:30 | source | | DotRegexServlet.java:133:19:133:39 | getPathInfo(...) : String | DotRegexServlet.java:136:25:136:30 | source | +| DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String | DotRegexSpring.java:21:31:21:47 | PROTECTED_PATTERN | +| DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String | DotRegexSpring.java:38:31:38:47 | PROTECTED_PATTERN | +| DotRegexSpring.java:15:50:15:64 | "/protected/.*" : String | DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String | +| DotRegexSpring.java:20:26:20:50 | path : String | DotRegexSpring.java:22:21:22:24 | path : String | +| DotRegexSpring.java:22:10:22:25 | decodePath(...) : String | DotRegexSpring.java:23:25:23:28 | path | +| DotRegexSpring.java:22:21:22:24 | path : String | DotRegexSpring.java:22:10:22:25 | decodePath(...) : String | +| DotRegexSpring.java:22:21:22:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | +| DotRegexSpring.java:37:40:37:64 | path : String | DotRegexSpring.java:39:21:39:24 | path : String | +| DotRegexSpring.java:39:10:39:25 | decodePath(...) : String | DotRegexSpring.java:40:25:40:28 | path | +| DotRegexSpring.java:39:21:39:24 | path : String | DotRegexSpring.java:39:10:39:25 | decodePath(...) : String | +| DotRegexSpring.java:39:21:39:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | +| DotRegexSpring.java:54:34:54:58 | path : String | DotRegexSpring.java:56:21:56:24 | path : String | +| DotRegexSpring.java:56:10:56:25 | decodePath(...) : String | DotRegexSpring.java:57:25:57:28 | path | +| DotRegexSpring.java:56:21:56:24 | path : String | DotRegexSpring.java:56:10:56:25 | decodePath(...) : String | +| DotRegexSpring.java:56:21:56:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | +| DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:71:29:71:32 | path : String | +| DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String | +| DotRegexSpring.java:71:11:71:42 | decode(...) : String | DotRegexSpring.java:71:29:71:32 | path : String | +| DotRegexSpring.java:71:11:71:42 | decode(...) : String | DotRegexSpring.java:73:10:73:13 | path : String | +| DotRegexSpring.java:71:29:71:32 | path : String | DotRegexSpring.java:71:11:71:42 | decode(...) : String | nodes | DotRegexFilter.java:16:30:16:46 | PROTECTED_PATTERN : String | semmle.label | PROTECTED_PATTERN : String | | DotRegexFilter.java:16:50:16:64 | "/protected/.*" : String | semmle.label | "/protected/.*" : String | @@ -43,10 +63,35 @@ nodes | DotRegexServlet.java:115:25:115:30 | source | semmle.label | source | | DotRegexServlet.java:133:19:133:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String | | DotRegexServlet.java:136:25:136:30 | source | semmle.label | source | +| DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String | semmle.label | PROTECTED_PATTERN : String | +| DotRegexSpring.java:15:50:15:64 | "/protected/.*" : String | semmle.label | "/protected/.*" : String | +| DotRegexSpring.java:20:26:20:50 | path : String | semmle.label | path : String | +| DotRegexSpring.java:21:31:21:47 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN | +| DotRegexSpring.java:22:10:22:25 | decodePath(...) : String | semmle.label | decodePath(...) : String | +| DotRegexSpring.java:22:21:22:24 | path : String | semmle.label | path : String | +| DotRegexSpring.java:23:25:23:28 | path | semmle.label | path | +| DotRegexSpring.java:37:40:37:64 | path : String | semmle.label | path : String | +| DotRegexSpring.java:38:31:38:47 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN | +| DotRegexSpring.java:39:10:39:25 | decodePath(...) : String | semmle.label | decodePath(...) : String | +| DotRegexSpring.java:39:21:39:24 | path : String | semmle.label | path : String | +| DotRegexSpring.java:40:25:40:28 | path | semmle.label | path | +| DotRegexSpring.java:54:34:54:58 | path : String | semmle.label | path : String | +| DotRegexSpring.java:56:10:56:25 | decodePath(...) : String | semmle.label | decodePath(...) : String | +| DotRegexSpring.java:56:21:56:24 | path : String | semmle.label | path : String | +| DotRegexSpring.java:57:25:57:28 | path | semmle.label | path | +| DotRegexSpring.java:69:28:69:38 | path : String | semmle.label | path : String | +| DotRegexSpring.java:71:11:71:42 | decode(...) : String | semmle.label | decode(...) : String | +| DotRegexSpring.java:71:29:71:32 | path : String | semmle.label | path : String | +| DotRegexSpring.java:73:10:73:13 | path : String | semmle.label | path : String | subpaths +| DotRegexSpring.java:22:21:22:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String | DotRegexSpring.java:22:10:22:25 | decodePath(...) : String | +| DotRegexSpring.java:39:21:39:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String | DotRegexSpring.java:39:10:39:25 | decodePath(...) : String | +| DotRegexSpring.java:56:21:56:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String | DotRegexSpring.java:56:10:56:25 | decodePath(...) : String | #select | DotRegexFilter.java:32:25:32:30 | source | DotRegexFilter.java:29:19:29:43 | getPathInfo(...) : String | DotRegexFilter.java:32:25:32:30 | source | Potentially authentication bypass due to $@. | DotRegexFilter.java:29:19:29:43 | getPathInfo(...) | user-provided value | | DotRegexServlet.java:22:25:22:30 | source | DotRegexServlet.java:19:19:19:39 | getPathInfo(...) : String | DotRegexServlet.java:22:25:22:30 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:19:19:19:39 | getPathInfo(...) | user-provided value | | DotRegexServlet.java:59:21:59:26 | source | DotRegexServlet.java:57:19:57:41 | getRequestURI(...) : String | DotRegexServlet.java:59:21:59:26 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:57:19:57:41 | getRequestURI(...) | user-provided value | | DotRegexServlet.java:77:56:77:61 | source | DotRegexServlet.java:75:19:75:39 | getPathInfo(...) : String | DotRegexServlet.java:77:56:77:61 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:75:19:75:39 | getPathInfo(...) | user-provided value | | DotRegexServlet.java:115:25:115:30 | source | DotRegexServlet.java:112:19:112:39 | getPathInfo(...) : String | DotRegexServlet.java:115:25:115:30 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:112:19:112:39 | getPathInfo(...) | user-provided value | +| DotRegexSpring.java:23:25:23:28 | path | DotRegexSpring.java:20:26:20:50 | path : String | DotRegexSpring.java:23:25:23:28 | path | Potentially authentication bypass due to $@. | DotRegexSpring.java:20:26:20:50 | path | user-provided value | +| DotRegexSpring.java:40:25:40:28 | path | DotRegexSpring.java:37:40:37:64 | path : String | DotRegexSpring.java:40:25:40:28 | path | Potentially authentication bypass due to $@. | DotRegexSpring.java:37:40:37:64 | path | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-625/options b/java/ql/test/experimental/query-tests/security/CWE-625/options index 79df30ff8e9..5f11b982510 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-625/options +++ b/java/ql/test/experimental/query-tests/security/CWE-625/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4 +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8