Merge branch 'main' into java/merge-5226

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

java/ql/src/experimental/Security/CWE/CWE-489/EJBMain.ql

@@ -9,13 +9,13 @@
 
 import java
 import semmle.code.java.J2EE
-import MainLib
+import TestLib
 
 /** The `main` method in an Enterprise Java Bean. */
 class EnterpriseBeanMainMethod extends Method {
   EnterpriseBeanMainMethod() {
     this.getDeclaringType() instanceof EnterpriseBean and
-    isMainMethod(this) and
+    this instanceof MainMethod and
     not isTestMethod(this)
   }
 }

java/ql/src/experimental/Security/CWE/CWE-489/StrutsBad.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!DOCTYPE struts PUBLIC
+	"-//Apache Software Foundation//DTD Struts Configuration 2.3//EN"
+	"http://struts.apache.org/dtds/struts-2.3.dtd">
+
+<struts>
+    <constant name="struts.enable.DynamicMethodInvocation" value="true" />
+    <constant name="struts.devMode" value="true" />
+    <constant name="struts.i18n.encoding" value="utf-8" />
+    <include file="login.xml" />
+</struts>

java/ql/src/experimental/Security/CWE/CWE-489/StrutsGood.xml

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!DOCTYPE struts PUBLIC
+	"-//Apache Software Foundation//DTD Struts Configuration 2.3//EN"
+	"http://struts.apache.org/dtds/struts-2.3.dtd">
+
+<struts>
+    <constant name="struts.enable.DynamicMethodInvocation" value="true" />
+    <constant name="struts.devMode" value="false" />
+    <constant name="struts.i18n.encoding" value="utf-8"></constant>
+    <include file="login.xml" />
+</struts>

java/ql/src/experimental/Security/CWE/CWE-489/TestLib.qll

@@ -1,17 +1,7 @@
-/** Definitions related to the main method in a test program. */
+/** Definitions related to test methods. */
 
 import java
 
-/** Holds if `m` is the main method of a Java class with the signature `public static void main(String[] args)`. */
-predicate isMainMethod(Method m) {
-  m.hasName("main") and
-  m.isStatic() and
-  m.getReturnType() instanceof VoidType and
-  m.isPublic() and
-  m.getNumberOfParameters() = 1 and
-  m.getParameter(0).getType() instanceof Array
-}
-
 /**
  * Holds if `m` is a test method indicated by:
  *    a) in a test directory such as `src/test/java`

java/ql/src/experimental/Security/CWE/CWE-489/WebComponentMain.ql

@@ -9,7 +9,7 @@
 
 import java
 import semmle.code.java.frameworks.Servlets
-import MainLib
+import TestLib
 
 /** The java type `javax.servlet.Filter`. */
 class ServletFilterClass extends Class {
@@ -48,7 +48,7 @@ class WebComponentMainMethod extends Method {
           .getASupertype+()
           .hasQualifiedName("org.springframework.webflow.execution", "Action") // Spring actions
     ) and
-    isMainMethod(this) and
+    this instanceof MainMethod and
     not isTestMethod(this)
   }
 }

java/ql/src/experimental/Security/CWE/CWE-489/devMode.qhelp

@@ -0,0 +1,32 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Turning Apache Struts' development mode configuration on while deploying applications to production environments can lead to remote code execution.</p>
+
+</overview>
+<recommendation>
+
+<p>An application should disable the development mode at the time of deployment.</p>
+
+</recommendation>
+<example>
+
+<p>The following example shows a `struts.xml` file with `struts.devmode` enabled.</p>
+
+<sample src="StrutsBad.xml" />
+
+<p>This can be easily corrected by setting the value of the `struts.devmode` parameter to false.</p>
+
+<sample src="StrutsGood.xml" />
+
+</example>
+<references>
+
+<li>
+  Apache Struts:
+  <a href="https://struts.apache.org/core-developers/development-mode.html">Struts development mode configuration</a>
+</li>
+
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-489/devMode.ql

@@ -0,0 +1,24 @@
+/**
+ * @name Apache Struts development mode enabled
+ * @description Enabling struts development mode in production environment
+ *  can lead to remote code execution.
+ * @kind problem
+ * @problem.severity error
+ * @precision high
+ * @id java/struts-development-mode
+ * @tags security
+ *       external/cwe/cwe-489
+ */
+
+import java
+import experimental.semmle.code.xml.StrutsXML
+
+bindingset[path]
+predicate isLikelyDemoProject(string path) { path.regexpMatch("(?i).*(demo|test|example).*") }
+
+from ConstantParameter c
+where
+  c.getNameValue() = "struts.devMode" and
+  c.getValueValue() = "true" and
+  not isLikelyDemoProject(c.getFile().getRelativePath())
+select c, "Enabling development mode in production environments is dangerous"

java/ql/src/experimental/semmle/code/xml/StrutsXML.qll

@@ -0,0 +1,40 @@
+import java
+
+/**
+ * A deployment descriptor file, typically called `struts.xml`.
+ */
+class StrutsXMLFile extends XMLFile {
+  StrutsXMLFile() {
+    count(XMLElement e | e = this.getAChild()) = 1 and
+    this.getAChild().getName() = "struts"
+  }
+}
+
+/**
+ * An XML element in a `StrutsXMLFile`.
+ */
+class StrutsXMLElement extends XMLElement {
+  StrutsXMLElement() { this.getFile() instanceof StrutsXMLFile }
+
+  /**
+   * Gets the value for this element, with leading and trailing whitespace trimmed.
+   */
+  string getValue() { result = allCharactersString().trim() }
+}
+
+/**
+ * A `<constant>` element in a `StrutsXMLFile`.
+ */
+class ConstantParameter extends StrutsXMLElement {
+  ConstantParameter() { this.getName() = "constant" }
+
+  /**
+   * Gets the value of the `name` attribute of this `<constant>`.
+   */
+  string getNameValue() { result = getAttributeValue("name") }
+
+  /**
+   * Gets the value of the `value` attribute of this `<constant>`.
+   */
+  string getValueValue() { result = getAttributeValue("value") }
+}