Java: Add a query for SpEL injections

- Added experimental/Security/CWE/CWE-094/SpelInjection.ql
  and a couple of libraries
- Added a qhelp file with a few examples
- Added tests and stubs for Spring

java/ql/src/experimental/Security/CWE/CWE-094/SaferSpelExpressionEvaluation.java

@@ -0,0 +1,12 @@
+public Object evaluate(Socket socket) throws IOException {
+  try (BufferedReader reader = new BufferedReader(
+      new InputStreamReader(socket.getInputStream()))) {
+
+    String string = reader.readLine();
+    ExpressionParser parser = new SpelExpressionParser();
+    Expression expression = parser.parseExpression(string);
+    SimpleEvaluationContext context 
+        = SimpleEvaluationContext.forReadWriteDataBinding().build();
+    return expression.getValue(context);
+  }
+}

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

@@ -0,0 +1,56 @@
+<!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.
+If a SpEL expression is built using attacker-controlled data, 
+and then evaluated in a powerful context,
+then it may allow the attacker to run arbitrary code.
+</p>
+<p>
+The <code>SpelExpressionParser</code> class parses a SpEL expression string
+and returns an <code>Expression</code> instance
+that can be then evaluated by calling one of its methods.
+By default, an expression is evaluated in a powerful <code>StandardEvaluationContext</code>
+that allows the expression to access other methods available in the JVM.
+</p>
+</overview>
+
+<recommendation>
+<p>
+In general, including user input in a SpEL expression should be avoided.
+If user input must be included in the expression,
+it should be then evaluated in a limited context
+that doesn't allow arbitrary method invocation.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example uses untrusted data to build a SpEL expression
+and then runs it in the default powerfull context.
+</p>
+<sample src="UnsafeSpelExpressionEvaluation.java" />
+
+<p>
+The next example shows how an untrusted SpEL expression can be run
+in <code>SimpleEvaluationContext</code> that doesn't allow accessing arbitrary methods.
+However, it's recommended to avoid using untrusted input in SpEL expressions.
+</p>
+<sample src="SaferSpelExpressionEvaluation.java" />
+</example>
+
+<references>
+<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/SpelInjection.ql

@@ -0,0 +1,19 @@
+/**
+ * @name Expression language injection (Spring)
+ * @description Evaluation of a user-controlled Spring Expression Language (SpEL) expression
+ *              may lead to remote code execution.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/spel-expression-injection
+ * @tags security
+ *       external/cwe/cwe-094
+ */
+
+import java
+import SpelInjectionLib
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, ExpressionInjectionConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "SpEL injection from $@.", source.getNode(), "this user input"

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

@@ -0,0 +1,100 @@
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking2
+import SpringFrameworkLib
+
+/**
+ * A taint-tracking configuration for unsafe user input
+ * that is used to construct and evaluate a SpEL expression.
+ */
+class ExpressionInjectionConfig extends TaintTracking::Configuration {
+  ExpressionInjectionConfig() { this = "ExpressionInjectionConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource or
+    source instanceof WebRequestSource
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof ExpressionEvaluationSink }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    expressionParsingStep(node1, node2) or
+    springPropertiesStep(node1, node2)
+  }
+}
+
+/**
+ * A sink for SpEL injection vulnerabilities,
+ * i.e. methods that run evaluation of a SpEL expression in a powerfull context.
+ */
+class ExpressionEvaluationSink extends DataFlow::ExprNode {
+  ExpressionEvaluationSink() {
+    exists(MethodAccess ma, Method m | m = ma.getMethod() |
+      m instanceof ExpressionEvaluationMethod and
+      getExpr() = ma.getQualifier() and
+      not exists(SafeEvaluationContextFlowConfig config |
+        config.hasFlowTo(DataFlow::exprNode(ma.getArgument(0)))
+      )
+    )
+  }
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that parses a SpEL expression,
+ * i.e. `parser.parseExpression(tainted)`.
+ */
+predicate expressionParsingStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(MethodAccess ma, Method m | ma.getMethod() = m |
+    m.getDeclaringType().getAnAncestor*() instanceof ExpressionParser and
+    m.hasName("parseExpression") and
+    ma.getAnArgument() = node1.asExpr() and
+    node2.asExpr() = ma
+  )
+}
+
+/**
+ * A configuration for safe evaluation context that may be used in expression evaluation.
+ */
+class SafeEvaluationContextFlowConfig extends DataFlow2::Configuration {
+  SafeEvaluationContextFlowConfig() { this = "SpelInjection::SafeEvaluationContextFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof SafeContextSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma, Method m | m = ma.getMethod() |
+      m instanceof ExpressionEvaluationMethod and
+      ma.getArgument(0) = sink.asExpr()
+    )
+  }
+
+  override int fieldFlowBranchLimit() { result = 0 }
+}
+
+class SafeContextSource extends DataFlow::ExprNode {
+  SafeContextSource() {
+    isSimpleEvaluationContextConstructorCall(getExpr()) or
+    isSimpleEvaluationContextBuilderCall(getExpr())
+  }
+}
+
+/**
+ * Holds if `expr` constructs `SimpleEvaluationContext`.
+ */
+predicate isSimpleEvaluationContextConstructorCall(Expr expr) {
+  exists(ConstructorCall cc |
+    cc.getConstructedType() instanceof SimpleEvaluationContext and
+    cc = expr
+  )
+}
+
+/**
+ * Holds if `expr` builds `SimpleEvaluationContext` via `SimpleEvaluationContext.Builder`,
+ * e.g. `SimpleEvaluationContext.forReadWriteDataBinding().build()`.
+ */
+predicate isSimpleEvaluationContextBuilderCall(Expr expr) {
+  exists(MethodAccess ma, Method m | ma.getMethod() = m |
+    m.getDeclaringType() instanceof SimpleEvaluationContextBuilder and
+    m.hasName("build") and
+    ma = expr
+  )
+}

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

@@ -0,0 +1,136 @@
+import java
+import semmle.code.java.dataflow.DataFlow
+
+/**
+ * Methods that trigger evaluation of an expression.
+ */
+class ExpressionEvaluationMethod extends Method {
+  ExpressionEvaluationMethod() {
+    getDeclaringType() instanceof Expression and
+    (
+      hasName("getValue") or
+      hasName("getValueTypeDescriptor") or
+      hasName("getValueType") or
+      hasName("setValue")
+    )
+  }
+}
+
+/**
+ * `WebRequest` interface is a source of tainted data.
+ */
+class WebRequestSource extends DataFlow::Node {
+  WebRequestSource() {
+    exists(MethodAccess ma, Method m | ma.getMethod() = m |
+      m.getDeclaringType() instanceof WebRequest and
+      (
+        m.hasName("getHeader") or
+        m.hasName("getHeaderValues") or
+        m.hasName("getHeaderNames") or
+        m.hasName("getParameter") or
+        m.hasName("getParameterValues") or
+        m.hasName("getParameterNames") or
+        m.hasName("getParameterMap")
+      ) and
+      ma = asExpr()
+    )
+  }
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that converts `PropertyValues`
+ * to an array of `PropertyValue`, i.e. `tainted.getPropertyValues()`.
+ */
+predicate getPropertyValuesStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(MethodAccess ma, Method m | m = ma.getMethod() |
+    node1.asExpr() = ma.getQualifier() and
+    node2.asExpr() = ma and
+    m.getDeclaringType() instanceof PropertyValues and
+    m.hasName("getPropertyValues")
+  )
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that constructs `MutablePropertyValues`,
+ * i.e. `new MutablePropertyValues(tainted)`.
+ */
+predicate createMutablePropertyValuesStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(ConstructorCall cc | cc.getConstructedType() instanceof MutablePropertyValues |
+    node1.asExpr() = cc.getAnArgument() and
+    node2.asExpr() = cc
+  )
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that returns a name of `PropertyValue`,
+ * i.e. `tainted.getName()`.
+ */
+predicate getPropertyNameStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(MethodAccess ma, Method m | m = ma.getMethod() |
+    node1.asExpr() = ma.getQualifier() and
+    node2.asExpr() = ma and
+    m.getDeclaringType() instanceof PropertyValue and
+    m.hasName("getName")
+  )
+}
+
+/**
+ * Holds if `node1` to `node2` is a dataflow step that converts `MutablePropertyValues`
+ * to a list of `PropertyValue`, i.e. `tainted.getPropertyValueList()`.
+ */
+predicate getPropertyValueListStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(MethodAccess ma, Method m | m = ma.getMethod() |
+    node1.asExpr() = ma.getQualifier() and
+    node2.asExpr() = ma and
+    m.getDeclaringType() instanceof MutablePropertyValues and
+    m.hasName("getPropertyValueList")
+  )
+}
+
+/**
+ * Holds if `node1` to `node2` is one of the dataflow steps that propagate
+ * tainted data via Spring properties.
+ */
+predicate springPropertiesStep(DataFlow::Node node1, DataFlow::Node node2) {
+  createMutablePropertyValuesStep(node1, node2) or
+  getPropertyNameStep(node1, node2) or
+  getPropertyValuesStep(node1, node2) or
+  getPropertyValueListStep(node1, node2)
+}
+
+class PropertyValue extends RefType {
+  PropertyValue() { hasQualifiedName("org.springframework.beans", "PropertyValue") }
+}
+
+class PropertyValues extends RefType {
+  PropertyValues() { hasQualifiedName("org.springframework.beans", "PropertyValues") }
+}
+
+class MutablePropertyValues extends RefType {
+  MutablePropertyValues() { hasQualifiedName("org.springframework.beans", "MutablePropertyValues") }
+}
+
+class SimpleEvaluationContext extends RefType {
+  SimpleEvaluationContext() {
+    hasQualifiedName("org.springframework.expression.spel.support", "SimpleEvaluationContext")
+  }
+}
+
+class SimpleEvaluationContextBuilder extends RefType {
+  SimpleEvaluationContextBuilder() {
+    hasQualifiedName("org.springframework.expression.spel.support",
+      "SimpleEvaluationContext$Builder")
+  }
+}
+
+class WebRequest extends RefType {
+  WebRequest() { hasQualifiedName("org.springframework.web.context.request", "WebRequest") }
+}
+
+class Expression extends RefType {
+  Expression() { hasQualifiedName("org.springframework.expression", "Expression") }
+}
+
+class ExpressionParser extends RefType {
+  ExpressionParser() { hasQualifiedName("org.springframework.expression", "ExpressionParser") }
+}

java/ql/src/experimental/Security/CWE/CWE-094/UnsafeSpelExpressionEvaluation.java

@@ -0,0 +1,10 @@
+public Object evaluate(Socket socket) throws IOException {
+  try (BufferedReader reader = new BufferedReader(
+      new InputStreamReader(socket.getInputStream()))) {
+
+    String string = reader.readLine();
+    ExpressionParser parser = new SpelExpressionParser();
+    Expression expression = parser.parseExpression(string);
+    return expression.getValue();
+  }
+}

java/ql/test/experimental/Security/CWE/CWE-094/SpelInjection.expected

@@ -0,0 +1,27 @@
+edges
+| SpelInjection.java:15:22:15:44 | getInputStream(...) : InputStream | SpelInjection.java:23:5:23:14 | expression |
+| SpelInjection.java:27:22:27:44 | getInputStream(...) : InputStream | SpelInjection.java:34:5:34:14 | expression |
+| SpelInjection.java:38:22:38:44 | getInputStream(...) : InputStream | SpelInjection.java:48:5:48:14 | expression |
+| SpelInjection.java:52:22:52:44 | getInputStream(...) : InputStream | SpelInjection.java:59:5:59:14 | expression |
+| SpelInjection.java:63:22:63:44 | getInputStream(...) : InputStream | SpelInjection.java:70:5:70:14 | expression |
+| SpelInjection.java:74:22:74:44 | getInputStream(...) : InputStream | SpelInjection.java:83:5:83:14 | expression |
+nodes
+| SpelInjection.java:15:22:15:44 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| SpelInjection.java:23:5:23:14 | expression | semmle.label | expression |
+| SpelInjection.java:27:22:27:44 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| SpelInjection.java:34:5:34:14 | expression | semmle.label | expression |
+| SpelInjection.java:38:22:38:44 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| SpelInjection.java:48:5:48:14 | expression | semmle.label | expression |
+| SpelInjection.java:52:22:52:44 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| SpelInjection.java:59:5:59:14 | expression | semmle.label | expression |
+| SpelInjection.java:63:22:63:44 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| SpelInjection.java:70:5:70:14 | expression | semmle.label | expression |
+| SpelInjection.java:74:22:74:44 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| SpelInjection.java:83:5:83:14 | expression | semmle.label | expression |
+#select
+| SpelInjection.java:23:5:23:14 | expression | SpelInjection.java:15:22:15:44 | getInputStream(...) : InputStream | SpelInjection.java:23:5:23:14 | expression | SpEL injection from $@. | SpelInjection.java:15:22:15:44 | getInputStream(...) | this user input |
+| SpelInjection.java:34:5:34:14 | expression | SpelInjection.java:27:22:27:44 | getInputStream(...) : InputStream | SpelInjection.java:34:5:34:14 | expression | SpEL injection from $@. | SpelInjection.java:27:22:27:44 | getInputStream(...) | this user input |
+| SpelInjection.java:48:5:48:14 | expression | SpelInjection.java:38:22:38:44 | getInputStream(...) : InputStream | SpelInjection.java:48:5:48:14 | expression | SpEL injection from $@. | SpelInjection.java:38:22:38:44 | getInputStream(...) | this user input |
+| SpelInjection.java:59:5:59:14 | expression | SpelInjection.java:52:22:52:44 | getInputStream(...) : InputStream | SpelInjection.java:59:5:59:14 | expression | SpEL injection from $@. | SpelInjection.java:52:22:52:44 | getInputStream(...) | this user input |
+| SpelInjection.java:70:5:70:14 | expression | SpelInjection.java:63:22:63:44 | getInputStream(...) : InputStream | SpelInjection.java:70:5:70:14 | expression | SpEL injection from $@. | SpelInjection.java:63:22:63:44 | getInputStream(...) | this user input |
+| SpelInjection.java:83:5:83:14 | expression | SpelInjection.java:74:22:74:44 | getInputStream(...) : InputStream | SpelInjection.java:83:5:83:14 | expression | SpEL injection from $@. | SpelInjection.java:74:22:74:44 | getInputStream(...) | this user input |

java/ql/test/experimental/Security/CWE/CWE-094/SpelInjection.java

@@ -0,0 +1,100 @@
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.Socket;
+import org.springframework.expression.Expression;
+import org.springframework.expression.ExpressionParser;
+import org.springframework.expression.spel.standard.SpelExpressionParser;
+import org.springframework.expression.spel.support.SimpleEvaluationContext;
+import org.springframework.expression.spel.support.StandardEvaluationContext;
+
+public class SpelInjection {
+
+  private static final ExpressionParser PARSER = new SpelExpressionParser();
+
+  public void testGetValue(Socket socket) throws IOException {
+    InputStream in = socket.getInputStream();
+
+    byte[] bytes = new byte[1024];
+    int n = in.read(bytes);
+    String input = new String(bytes, 0, n);
+
+    ExpressionParser parser = new SpelExpressionParser();
+    Expression expression = parser.parseExpression(input);
+    expression.getValue();
+  }
+
+  public void testGetValueWithChainedCalls(Socket socket) throws IOException {
+    InputStream in = socket.getInputStream();
+
+    byte[] bytes = new byte[1024];
+    int n = in.read(bytes);
+    String input = new String(bytes, 0, n);
+
+    Expression expression = new SpelExpressionParser().parseExpression(input);
+    expression.getValue();
+  }
+
+  public void testSetValueWithRootObject(Socket socket) throws IOException {
+    InputStream in = socket.getInputStream();
+
+    byte[] bytes = new byte[1024];
+    int n = in.read(bytes);
+    String input = new String(bytes, 0, n);
+
+    Expression expression = new SpelExpressionParser().parseExpression(input);
+
+    Object root = new Object();
+    Object value = new Object();
+    expression.setValue(root, value);
+  }
+
+  public void testGetValueWithStaticParser(Socket socket) throws IOException {
+    InputStream in = socket.getInputStream();
+
+    byte[] bytes = new byte[1024];
+    int n = in.read(bytes);
+    String input = new String(bytes, 0, n);
+
+    Expression expression = PARSER.parseExpression(input);
+    expression.getValue();
+  }
+
+  public void testGetValueType(Socket socket) throws IOException {
+    InputStream in = socket.getInputStream();
+
+    byte[] bytes = new byte[1024];
+    int n = in.read(bytes);
+    String input = new String(bytes, 0, n);
+
+    Expression expression = PARSER.parseExpression(input);
+    expression.getValueType();
+  }
+
+  public void testWithStandardEvaluationContext(Socket socket) throws IOException {
+    InputStream in = socket.getInputStream();
+
+    byte[] bytes = new byte[1024];
+    int n = in.read(bytes);
+    String input = new String(bytes, 0, n);
+
+    Expression expression = PARSER.parseExpression(input);
+
+    StandardEvaluationContext context = new StandardEvaluationContext();
+    expression.getValue(context);
+  }
+
+  public void testWithSimpleEvaluationContext(Socket socket) throws IOException {
+    InputStream in = socket.getInputStream();
+
+    byte[] bytes = new byte[1024];
+    int n = in.read(bytes);
+    String input = new String(bytes, 0, n);
+
+    Expression expression = PARSER.parseExpression(input);
+    SimpleEvaluationContext context = SimpleEvaluationContext.forReadWriteDataBinding().build();
+
+    // the expression is evaluated in a limited context
+    expression.getValue(context);
+  }
+
+}

java/ql/test/experimental/Security/CWE/CWE-094/SpelInjection.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-094/SpelInjection.ql

java/ql/test/experimental/Security/CWE/CWE-094/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/springframework-5.2.3

java/ql/test/stubs/springframework-5.2.3/org/springframework/expression/EvaluationContext.java

@@ -0,0 +1,3 @@
+package org.springframework.expression;
+
+public interface EvaluationContext {}

java/ql/test/stubs/springframework-5.2.3/org/springframework/expression/EvaluationException.java

@@ -0,0 +1,3 @@
+package org.springframework.expression;
+
+public class EvaluationException extends RuntimeException {}

java/ql/test/stubs/springframework-5.2.3/org/springframework/expression/Expression.java

@@ -0,0 +1,14 @@
+package org.springframework.expression;
+
+public interface Expression {
+
+	Object getValue() throws EvaluationException;
+
+	Object getValue(EvaluationContext context) throws EvaluationException;
+
+	Class<?> getValueType() throws EvaluationException;
+
+	Class<?> getValueType(EvaluationContext context) throws EvaluationException;
+
+	void setValue(Object rootObject, Object value) throws EvaluationException;
+}

java/ql/test/stubs/springframework-5.2.3/org/springframework/expression/ExpressionParser.java

@@ -0,0 +1,6 @@
+package org.springframework.expression;
+
+public interface ExpressionParser {
+
+    Expression parseExpression(String string);
+}

java/ql/test/stubs/springframework-5.2.3/org/springframework/expression/spel/standard/SpelExpressionParser.java

@@ -0,0 +1,10 @@
+package org.springframework.expression.spel.standard;
+
+import org.springframework.expression.*;
+
+public class SpelExpressionParser implements ExpressionParser {
+
+    public SpelExpressionParser() {}
+    
+    public Expression parseExpression(String string) { return null; }
+}

java/ql/test/stubs/springframework-5.2.3/org/springframework/expression/spel/support/SimpleEvaluationContext.java

@@ -0,0 +1,13 @@
+
+package org.springframework.expression.spel.support;
+
+import org.springframework.expression.*;
+
+public class SimpleEvaluationContext implements EvaluationContext {
+
+    public static Builder forReadWriteDataBinding() { return null; }
+    
+    public static class Builder {
+        public SimpleEvaluationContext build() { return null; }
+    }
+}

java/ql/test/stubs/springframework-5.2.3/org/springframework/expression/spel/support/StandardEvaluationContext.java

@@ -0,0 +1,5 @@
+package org.springframework.expression.spel.support;
+
+import org.springframework.expression.*;
+
+public class StandardEvaluationContext implements EvaluationContext {}