From 602f63ad458531606ca08be1a1fbb46d51f3a766 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Sat, 5 Sep 2020 20:15:50 +0530 Subject: [PATCH 1/3] [Java] Add QL for detecting Spring View Manipulation Vulnerabilities. --- .../SpringImplicitViewManipulation.qhelp | 4 + .../CWE-094/SpringImplicitViewManipulation.ql | 64 +++++++++ .../Security/CWE/CWE-094/SpringViewBad.java | 17 +++ .../Security/CWE/CWE-094/SpringViewGood.java | 20 +++ .../CWE/CWE-094/SpringViewManipulation.qhelp | 50 +++++++ .../CWE/CWE-094/SpringViewManipulation.ql | 21 +++ .../CWE/CWE-094/SpringViewManipulationLib.qll | 123 ++++++++++++++++++ .../semmle/code/java/dataflow/FlowSources.qll | 16 +++ .../frameworks/spring/SpringController.qll | 5 + .../code/java/frameworks/spring/SpringWeb.qll | 16 +++ 10 files changed, 336 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/SpringViewBad.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/SpringViewGood.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.qhelp new file mode 100644 index 00000000000..ffacd8f8b03 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.qhelp @@ -0,0 +1,4 @@ + + + + diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.ql new file mode 100644 index 00000000000..e4ec03ed956 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.ql @@ -0,0 +1,64 @@ +/** + * @name Spring Implicit View Manipulation + * @description Untrusted input in a Spring View Controller can lead to RCE. + * @kind problem + * @problem.severity error + * @precision high + * @id java/spring-view-manipulation-implicit + * @tags security + * external/cwe/cwe-094 + */ + +import java +import SpringViewManipulationLib + +private predicate canResultInImplicitViewConversion(Method m) { + m.getReturnType() instanceof VoidType + or + m.getReturnType() instanceof MapType + or + m.getReturnType().(RefType).hasQualifiedName("org.springframework.ui", "Model") +} + +private predicate maybeATestMethod(Method m) { + exists(string s | + s = m.getName() or + s = m.getFile().getRelativePath() or + s = m.getDeclaringType().getName() + | + s.matches(["%test%", "%example%", "%exception%"]) + ) +} + +private predicate mayBeExploitable(Method m) { + // There should be a attacker controlled parameter in the URI for the attack to be exploitable. + // This is possible only when there exists a parameter with the Spring `@PathVariable` annotation + // applied to it. + exists(Parameter p | + p = m.getAParameter() and + p.hasAnnotation("org.springframework.web.bind.annotation", "PathVariable") and + // Having a parameter of say type `Long` is non exploitable as Java type + // checking rules are applied prior to view name resolution, rendering the exploit useless. + // hence, here we check for the param type to be a Java `String`. + p.getType() instanceof TypeString and + // Exclude cases where a regex check is applied on a parameter to prevent false positives. + not m.(SpringRequestMappingMethod).getValue().matches("%{%:[%]%}%") + ) and + not maybeATestMethod(m) +} + +from SpringRequestMappingMethod m +where + thymeleafIsUsed() and + mayBeExploitable(m) and + canResultInImplicitViewConversion(m) and + // If there's a parameter of type`HttpServletResponse`, Spring Framework does not interpret + // it as a view name, but just returns this string in HTTP Response preventing exploitation + // This also applies to `@ResponseBody` annotation. + not m.getParameterType(_) instanceof HttpServletResponse and + // A spring request mapping method which does not have response body annotation applied to it + m.getAnAnnotation().getType() instanceof SpringRequestMappingAnnotationType and + not exists(SpringResponseBodyAnnotationType t | t = m.getAnAnnotation().getType()) and + // `@RestController` inherits `@ResponseBody` internally so it should be ignored. + not m.getDeclaringType() instanceof SpringRestController +select m, "This method may be vulnerable to spring view manipulation vulnerabilities" diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewBad.java b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewBad.java new file mode 100644 index 00000000000..bb8121f7b11 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewBad.java @@ -0,0 +1,17 @@ +@Controller +public class SptingViewManipulationController { + + Logger log = LoggerFactory.getLogger(HelloController.class); + + @GetMapping("/safe/fragment") + public String Fragment(@RequestParam String section) { + // bad as template path is attacker controlled + return "welcome :: " + section; + } + + @GetMapping("/doc/{document}") + public void getDocument(@PathVariable String document) { + // returns void, so view name is taken from URI + log.info("Retrieving " + document); + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewGood.java b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewGood.java new file mode 100644 index 00000000000..046150cae95 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewGood.java @@ -0,0 +1,20 @@ +@Controller +public class SptingViewManipulationController { + + Logger log = LoggerFactory.getLogger(HelloController.class); + + @GetMapping("/safe/fragment") + @ResponseBody + public String Fragment(@RequestParam String section) { + // good, as `@ResponseBody` annotation tells Spring + // to process the return values as body, instead of view name + return "welcome :: " + section; + } + + @GetMapping("/safe/doc/{document}") + public void getDocument(@PathVariable String document, HttpServletResponse response) { + // good as `HttpServletResponse param tells Spring that the response is already + // processed. + log.info("Retrieving " + document); // FP + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.qhelp new file mode 100644 index 00000000000..dadd20dfdb7 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.qhelp @@ -0,0 +1,50 @@ + + + + +

+ The Spring Expression Language (SpEL) is a powerful expression language + provided by Spring Framework. The language offers many features + including invocation of methods available in the JVM. +

+

+ An unrestricted view name manipulation vulnerability in Spring Framework could lead to attacker-controlled arbitary SpEL expressions being evaluated using attacker-controlled data, which may in turn allow an attacker to run arbitrary code. +

+

+ Note: two related variants of this problem are detected by different queries, `java/spring-view-manipulation` and `java/spring-view-manipulation-implicit`. The first detects taint flow problems where the return types is always String. While the latter, `java/spring-view-manipulation-implicit` detects cases where the request mapping method has a non-string return type such as void. +

+ + + +

+ In general, using user input to determine Spring view name should be avoided. + If user input must be included in the expression, the controller can be annotated by + a @ReponseBody annotation. In this case, Spring Framework does not interpret + it as a view name, but just returns this string in HTTP Response. The same applies to using + a @RestController annotation on a class, as internally it inherits @ResponseBody. +

+ + + +

+ In the following example, the Fragment method uses an externally controlled variable section to generate the view name. Hence, it is vulnerable to Spring View Manipulation attacks. +

+ +

+ This can be easily prevented by using the ResponseBody annotation which marks the reponse is already processed preventing exploitation of Spring View Manipulation vulnerabilities. Alternatively, this can also be fixed by adding a HttpServletResponse parameter to the method definition as shown in the example below. +

+ + + + +
  • + Veracode Research : Spring View Manipulation +
    • +
  • + Spring Framework Reference Documentation: Spring Expression Language (SpEL) +
    • +
  • + OWASP: Expression Language Injection +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.ql new file mode 100644 index 00000000000..3c490e6bf68 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.ql @@ -0,0 +1,21 @@ +/** + * @name Spring View Manipulation + * @description Untrusted input in a Spring View can lead to RCE. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/spring-view-manipulation + * @tags security + * external/cwe/cwe-094 + */ + +import java +import SpringViewManipulationLib +import DataFlow::PathGraph + +from DataFlow::PathNode source, DataFlow::PathNode sink, SpringViewManipulationConfig conf +where + thymeleafIsUsed() and + conf.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Potential Spring Expression Language injection from $@.", + source.getNode(), "this user input" diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll new file mode 100644 index 00000000000..b03fefbc366 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll @@ -0,0 +1,123 @@ +/** + * Provides classes for reasoning about Spring View Manipulation vulnerabilities + */ + +import java +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.frameworks.spring.Spring +import SpringFrameworkLib + +/** Holds if `Thymeleaf` templating engine is used in the project. */ +predicate thymeleafIsUsed() { + exists(Pom p | + p.getADependency().getArtifact().getValue() in [ + "spring-boot-starter-thymeleaf", "thymeleaf-spring4", "springmvc-xml-thymeleaf", + "thymeleaf-spring5" + ] + ) + or + exists(SpringBean b | b.getClassNameRaw().matches("org.thymeleaf.spring%")) +} + +/** + * A taint-tracking configuration for unsafe user input + * that can lead to Spring View Manipulation vulnerabilities. + */ +class SpringViewManipulationConfig extends TaintTracking::Configuration { + SpringViewManipulationConfig() { this = "Spring View Manipulation Config" } + + override predicate isSource(DataFlow::Node source) { + source instanceof RemoteFlowSource or + source instanceof WebRequestSource + } + + override predicate isSink(DataFlow::Node sink) { sink instanceof SpringViewManipulationSink } + + override predicate isSanitizer(DataFlow::Node node) { + // Block flows like + // ``` + // a = "redirect:" + taint` + // ``` + exists(AddExpr e, StringLiteral sl | + node.asExpr() = e.getControlFlowNode().getASuccessor*() and + sl = e.getLeftOperand*() and + sl.getRepresentedString().matches(["redirect:%", "ajaxredirect:%", "forward:%"]) + ) + or + // Block flows like + // ``` + // x.append("redirect:"); + // x.append(tainted()); + // return x.toString(); + // + // "redirect:".concat(taint) + // + // String.format("redirect:%s",taint); + // ``` + exists(Call ca, StringLiteral sl | + ( + sl = ca.getArgument(_) + or + sl = ca.getQualifier() + ) and + ca = getAStringCombiningCall() and + sl.getRepresentedString().matches(["redirect:%", "ajaxredirect:%", "forward:%"]) + | + exists(Call cc | DataFlow::localExprFlow(ca.getQualifier(), cc.getQualifier()) | + cc = node.asExpr() + ) + ) + } +} + +private Call getAStringCombiningCall() { + exists(StringCombiningMethod m | result = m.getAReference()) +} + +abstract private class StringCombiningMethod extends Method { } + +private class AppendableAppendMethod extends StringCombiningMethod { + AppendableAppendMethod() { + exists(RefType t | + t.hasQualifiedName("java.lang", "Appendable") and + this.getDeclaringType().extendsOrImplements*(t) and + this.hasName("append") + ) + } +} + +private class StringConcatMethod extends StringCombiningMethod { + StringConcatMethod() { + this.getDeclaringType() instanceof TypeString and + this.hasName("concat") + } +} + +private class StringFormatMethod extends StringCombiningMethod { + StringFormatMethod() { + this.getDeclaringType() instanceof TypeString and + this.hasName("format") + } +} + +/** + * A sink for Spring View Manipulation vulnerabilities, + */ +class SpringViewManipulationSink extends DataFlow::ExprNode { + SpringViewManipulationSink() { + exists(ReturnStmt r, SpringRequestMappingMethod m | + r.getResult() = this.asExpr() and + m.getBody().getAStmt() = r and + not m.isResponseBody() and + r.getResult().getType() instanceof TypeString + ) + or + exists(ConstructorCall c | c.getConstructedType() instanceof ModelAndView | + this.asExpr() = c.getArgument(0) and + c.getConstructor().getParameterType(0) instanceof TypeString + ) + or + exists(SpringModelAndViewSetViewNameCall c | this.asExpr() = c.getArgument(0)) + } +} diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index b4235cb2635..67e67289c4a 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -256,6 +256,7 @@ private class RemoteTaintedMethod extends Method { this instanceof ServletRequestGetParameterMethod or this instanceof ServletRequestGetParameterMapMethod or this instanceof ServletRequestGetParameterNamesMethod or + this instanceof PortletRenderRequestGetParameterMethod or this instanceof HttpServletRequestGetQueryStringMethod or this instanceof HttpServletRequestGetHeaderMethod or this instanceof HttpServletRequestGetPathMethod or @@ -308,6 +309,21 @@ class EnvReadMethod extends Method { } } +private class PortletRenderRequestGetParameterMethod extends Method { + PortletRenderRequestGetParameterMethod() { + exists(RefType c, Interface t | + c.extendsOrImplements*(t) and + t.hasQualifiedName("javax.portlet", "RenderState") and + this = c.getAMethod() + | + this.hasName([ + "getCookies", "getParameter", "getRenderParameters", "getParameterNames", + "getParameterValues", "getParameterMap" + ]) + ) + } +} + /** The type `java.net.InetAddress`. */ class TypeInetAddr extends RefType { TypeInetAddr() { this.getQualifiedName() = "java.net.InetAddress" } diff --git a/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll b/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll index 3abf325dfa2..f935a4803a9 100644 --- a/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll +++ b/java/ql/src/semmle/code/java/frameworks/spring/SpringController.qll @@ -126,6 +126,11 @@ class SpringRequestMappingMethod extends SpringControllerMethod { requestMappingAnnotation.getValue("produces").(CompileTimeConstantExpr).getStringValue() } + /** Gets the "value" @RequestMapping annotation value, if present. */ + string getValue() { + result = requestMappingAnnotation.getValue("value").(CompileTimeConstantExpr).getStringValue() + } + /** Holds if this is considered an `@ResponseBody` method. */ predicate isResponseBody() { getAnAnnotation().getType() instanceof SpringResponseBodyAnnotationType or diff --git a/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll b/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll index 4a71c71295e..ad4e00959dc 100644 --- a/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll +++ b/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll @@ -17,3 +17,19 @@ class SpringNativeWebRequest extends Class { this.hasQualifiedName("org.springframework.web.context.request", "NativeWebRequest") } } + +/** Models Spring `servlet` as well as `portlet` package's `ModelAndView` class. */ +class ModelAndView extends Class { + ModelAndView() { + hasQualifiedName(["org.springframework.web.servlet", "org.springframework.web.portlet"], + "ModelAndView") + } +} + +/** Models a call to the Spring `ModelAndView` class's `setViewName` method. */ +class SpringModelAndViewSetViewNameCall extends MethodAccess { + SpringModelAndViewSetViewNameCall() { + getMethod().getDeclaringType() instanceof ModelAndView and + getMethod().hasName("setViewName") + } +} From 14ec1482721b7fd406a7550de326e6a59b16a7e7 Mon Sep 17 00:00:00 2001 From: Porcuiney Hairs Date: Mon, 1 Mar 2021 18:46:33 +0530 Subject: [PATCH 2/3] refactor to meet experimental guidelines. --- .../CWE/CWE-094/SpringViewManipulationLib.qll | 19 ++++++++++++++++++- .../semmle/code/java/dataflow/FlowSources.qll | 16 ---------------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll index b03fefbc366..b61d64ba7e1 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll @@ -20,6 +20,22 @@ predicate thymeleafIsUsed() { exists(SpringBean b | b.getClassNameRaw().matches("org.thymeleaf.spring%")) } +/** Models methods from the `javax.portlet.RenderState` package which return data from externally controlled sources. */ +class PortletRenderRequestMethod extends Method { + PortletRenderRequestMethod() { + exists(RefType c, Interface t | + c.extendsOrImplements*(t) and + t.hasQualifiedName("javax.portlet", "RenderState") and + this = c.getAMethod() + | + this.hasName([ + "getCookies", "getParameter", "getRenderParameters", "getParameterNames", + "getParameterValues", "getParameterMap" + ]) + ) + } +} + /** * A taint-tracking configuration for unsafe user input * that can lead to Spring View Manipulation vulnerabilities. @@ -29,7 +45,8 @@ class SpringViewManipulationConfig extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource or - source instanceof WebRequestSource + source instanceof WebRequestSource or + source.asExpr().(MethodAccess).getMethod() instanceof PortletRenderRequestMethod } override predicate isSink(DataFlow::Node sink) { sink instanceof SpringViewManipulationSink } diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index 67e67289c4a..b4235cb2635 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll @@ -256,7 +256,6 @@ private class RemoteTaintedMethod extends Method { this instanceof ServletRequestGetParameterMethod or this instanceof ServletRequestGetParameterMapMethod or this instanceof ServletRequestGetParameterNamesMethod or - this instanceof PortletRenderRequestGetParameterMethod or this instanceof HttpServletRequestGetQueryStringMethod or this instanceof HttpServletRequestGetHeaderMethod or this instanceof HttpServletRequestGetPathMethod or @@ -309,21 +308,6 @@ class EnvReadMethod extends Method { } } -private class PortletRenderRequestGetParameterMethod extends Method { - PortletRenderRequestGetParameterMethod() { - exists(RefType c, Interface t | - c.extendsOrImplements*(t) and - t.hasQualifiedName("javax.portlet", "RenderState") and - this = c.getAMethod() - | - this.hasName([ - "getCookies", "getParameter", "getRenderParameters", "getParameterNames", - "getParameterValues", "getParameterMap" - ]) - ) - } -} - /** The type `java.net.InetAddress`. */ class TypeInetAddr extends RefType { TypeInetAddr() { this.getQualifiedName() = "java.net.InetAddress" } From 394c82d5648123f3aed87729f4950995ec0d9ed6 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 2 Mar 2021 10:17:07 +0100 Subject: [PATCH 3/3] Apply suggestions from code review Adjust qldoc. --- .../src/semmle/code/java/frameworks/spring/SpringWeb.qll | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll b/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll index ad4e00959dc..dd8e660fd26 100644 --- a/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll +++ b/java/ql/src/semmle/code/java/frameworks/spring/SpringWeb.qll @@ -18,7 +18,11 @@ class SpringNativeWebRequest extends Class { } } -/** Models Spring `servlet` as well as `portlet` package's `ModelAndView` class. */ +/** + * A Spring `ModelAndView` class. This is either + * `org.springframework.web.servlet.ModelAndView` or + * `org.springframework.web.portlet.ModelAndView`. + */ class ModelAndView extends Class { ModelAndView() { hasQualifiedName(["org.springframework.web.servlet", "org.springframework.web.portlet"], @@ -26,7 +30,7 @@ class ModelAndView extends Class { } } -/** Models a call to the Spring `ModelAndView` class's `setViewName` method. */ +/** A call to the Spring `ModelAndView.setViewName` method. */ class SpringModelAndViewSetViewNameCall extends MethodAccess { SpringModelAndViewSetViewNameCall() { getMethod().getDeclaringType() instanceof ModelAndView and