Merge pull request #4214 from porcupineyhairs/springViewManipulation

[Java] Add QL for detecting Spring View Manipulation Vulnerabilities.

java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.qhelp

@@ -0,0 +1,4 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+  <include src="SpringViewManipulation.qhelp" />
+</qhelp>

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"

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);
+    }
+}

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
+    }
+}

java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.qhelp

@@ -0,0 +1,50 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+  <overview>
+    <p>
+      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.
+    </p>
+    <p>
+      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.
+    </p>
+    <p>
+      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 <code>String</code>. While the latter, `java/spring-view-manipulation-implicit` detects cases where the request mapping method has a non-string return type such as <code>void</code>.
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+      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 <code>@ReponseBody</code> 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 <code>@RestController</code> annotation on a class, as internally it inherits <code>@ResponseBody</code>.
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+      In the following example, the <code>Fragment</code> method uses an externally controlled variable <code>section</code> to generate the view name. Hence, it is vulnerable to Spring View Manipulation attacks.
+    </p>
+    <sample src="SpringViewBad.java" />
+    <p>
+      This can be easily prevented by using the <code>ResponseBody</code> annotation which marks the reponse is already processed preventing exploitation of Spring View Manipulation vulnerabilities. Alternatively, this can also be fixed by adding a <code>HttpServletResponse</code> parameter to the method definition as shown in the example below.
+    </p>
+    <sample src="SpringViewGood.java" />
+  </example>
+
+  <references>
+    <li>
+      Veracode Research : <a href="https://github.com/veracode-research/spring-view-manipulation/">Spring View Manipulation </a>
+    </li>
+    <li>
+      Spring Framework Reference Documentation: <a href="https://docs.spring.io/spring/docs/4.2.x/spring-framework-reference/html/expressions.html">Spring Expression Language (SpEL)</a>
+    </li>
+    <li>
+      OWASP: <a href="https://owasp.org/www-community/vulnerabilities/Expression_Language_Injection">Expression Language Injection</a>
+    </li>
+  </references>
+</qhelp>

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"

java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulationLib.qll

@@ -0,0 +1,140 @@
+/**
+ * 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%"))
+}
+
+/** 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.
+ */
+class SpringViewManipulationConfig extends TaintTracking::Configuration {
+  SpringViewManipulationConfig() { this = "Spring View Manipulation Config" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource or
+    source instanceof WebRequestSource or
+    source.asExpr().(MethodAccess).getMethod() instanceof PortletRenderRequestMethod
+  }
+
+  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))
+  }
+}