Merge pull request #3294 from ggolawski/ognl-injection

CodeQL query to detect OGNL injections
2025-12-24 12:46:34 +01:00 · 2020-06-30 09:46:02 +02:00
parent b57cfc965a aff0e0eb25
commit 13cb853af5
14 changed files with 397 additions and 0 deletions
--- a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.java
+++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.java
@@ -0,0 +1,17 @@
+import ognl.Ognl;
+import ognl.OgnlException;
+
+public void evaluate(HttpServletRequest request, Object root) throws OgnlException {
+  String expression = request.getParameter("expression");
+
+  // BAD: User provided expression is evaluated
+  Ognl.getValue(expression, root);
+  
+  // GOOD: The name is validated and expression is evaluated in sandbox
+  System.setProperty("ognl.security.manager", ""); // Or add -Dognl.security.manager to JVM args
+  if (isValid(expression)) {
+    Ognl.getValue(expression, root);
+  } else {
+    // Reject the request
+  }
+} 
--- a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.qhelp
+++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.qhelp
@@ -0,0 +1,33 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Object-Graph Navigation Language (OGNL) is an open-source Expression Language (EL) for Java. Due
+to its ability to create or change executable code, OGNL is capable of introducing critical
+security flaws to any application that uses it. Evaluation of unvalidated expressions can let
+attacker to modify Java objects' properties or execute arbitrary code.</p>
+</overview>
+
+<recommendation>
+<p>The general recommendation is to not evaluate untrusted ONGL expressions. If user provided OGNL
+expressions must be evaluated, do this in sandbox (add `-Dognl.security.manager` to JVM arguments)
+and validate the expressions before evaluation.</p>
+</recommendation>
+
+<example>
+<p>In the following examples, the code accepts an OGNL expression from the user and evaluates it.
+</p>
+
+<p>In the first example, the user provided OGNL expression is parsed and evaluated.</p>
+
+<p>The second example validates the expression and evaluates it inside the sandbox.</p>
+
+<sample src="OgnlInjection.java" />
+</example>
+
+<references>
+<li><a href="https://github.com/jkuhnert/ognl/">OGNL library</a>.</li>
+<li>Struts security: <a href="https://struts.apache.org/security/#proactively-protect-from-ognl-expression-injections-attacks-if-easily-applicable">Proactively protect from OGNL Expression Injections attacks</a>.</li>
+</references>
+</qhelp>
--- a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.ql
+++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjection.ql
@@ -0,0 +1,22 @@
+/**
+ * @name OGNL Expression Language statement with user-controlled input
+ * @description Evaluation of OGNL Expression Language statement with user-controlled input can
+ *                lead to execution of arbitrary code.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/ognl-injection
+ * @tags security
+ *       external/cwe/cwe-917
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import DataFlow
+import DataFlow::PathGraph
+import OgnlInjectionLib
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, OgnlInjectionFlowConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "OGNL expression might include input from $@.",
+  source.getNode(), "this user input"
--- a/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll
+++ b/java/ql/src/experimental/Security/CWE/CWE-917/OgnlInjectionLib.qll
@@ -0,0 +1,109 @@
+import java
+import semmle.code.java.dataflow.FlowSources
+import DataFlow
+import DataFlow::PathGraph
+
+/**
+ * A taint-tracking configuration for unvalidated user input that is used in OGNL EL evaluation.
+ */
+class OgnlInjectionFlowConfig extends TaintTracking::Configuration {
+  OgnlInjectionFlowConfig() { this = "OgnlInjectionFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof OgnlInjectionSink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    parseCompileExpressionStep(node1, node2)
+  }
+}
+
+/** The class `org.apache.commons.ognl.Ognl` or `ognl.Ognl`. */
+class TypeOgnl extends Class {
+  TypeOgnl() {
+    this.hasQualifiedName("org.apache.commons.ognl", "Ognl") or
+    this.hasQualifiedName("ognl", "Ognl")
+  }
+}
+
+/** The interface `org.apache.commons.ognl.Node` or `ognl.Node`. */
+class TypeNode extends Interface {
+  TypeNode() {
+    this.hasQualifiedName("org.apache.commons.ognl", "Node") or
+    this.hasQualifiedName("ognl", "Node")
+  }
+}
+
+/** The class `com.opensymphony.xwork2.ognl.OgnlUtil`. */
+class TypeOgnlUtil extends Class {
+  TypeOgnlUtil() { this.hasQualifiedName("com.opensymphony.xwork2.ognl", "OgnlUtil") }
+}
+
+/**
+ * OGNL sink for OGNL injection vulnerabilities, i.e. 1st argument to `getValue` or `setValue`
+ * method from `Ognl` or `getValue` or `setValue` method from `Node`.
+ */
+predicate ognlSinkMethod(Method m, int index) {
+  (
+    m.getDeclaringType() instanceof TypeOgnl
+    or
+    m.getDeclaringType().getAnAncestor*() instanceof TypeNode
+  ) and
+  (
+    m.hasName("getValue") or
+    m.hasName("setValue")
+  ) and
+  index = 0
+}
+
+/**
+ * Struts sink for OGNL injection vulnerabilities, i.e. 1st argument to `getValue`, `setValue` or
+ * `callMethod` method from `OgnlUtil`.
+ */
+predicate strutsSinkMethod(Method m, int index) {
+  m.getDeclaringType() instanceof TypeOgnlUtil and
+  (
+    m.hasName("getValue") or
+    m.hasName("setValue") or
+    m.hasName("callMethod")
+  ) and
+  index = 0
+}
+
+/** Holds if parameter at index `index` in method `m` is OGNL injection sink. */
+predicate ognlInjectionSinkMethod(Method m, int index) {
+  ognlSinkMethod(m, index) or
+  strutsSinkMethod(m, index)
+}
+
+/** A data flow sink for unvalidated user input that is used in OGNL EL evaluation. */
+class OgnlInjectionSink extends DataFlow::ExprNode {
+  OgnlInjectionSink() {
+    exists(MethodAccess ma, Method m, int index |
+      ma.getMethod() = m and
+      (ma.getArgument(index) = this.getExpr() or ma.getQualifier() = this.getExpr()) and
+      ognlInjectionSinkMethod(m, index)
+    )
+  }
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `Object` or `Node`,
+ * i.e. `Ognl.parseExpression(tainted)` or `Ognl.compileExpression(tainted)`.
+ */
+predicate parseCompileExpressionStep(ExprNode n1, ExprNode n2) {
+  exists(MethodAccess ma, Method m, int index |
+    n1.asExpr() = ma.getArgument(index) and
+    n2.asExpr() = ma and
+    ma.getMethod() = m and
+    m.getDeclaringType() instanceof TypeOgnl
+  |
+    m.hasName("parseExpression") and index = 0
+    or
+    m.hasName("compileExpression") and index = 2
+  )
+}
--- a/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.expected
+++ b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.expected
@@ -0,0 +1,48 @@
+edges
+| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:13:19:13:22 | tree |
+| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:14:19:14:22 | tree |
+| OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:16:17:16:27 | (...)... : Object |
+| OgnlInjection.java:16:17:16:27 | (...)... : Object | OgnlInjection.java:17:5:17:8 | node |
+| OgnlInjection.java:16:17:16:27 | (...)... : Object | OgnlInjection.java:18:5:18:8 | node |
+| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:23:19:23:22 | tree |
+| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:24:19:24:22 | tree |
+| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:26:5:26:8 | tree |
+| OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:27:5:27:8 | tree |
+| OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:31:19:31:22 | expr |
+| OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:32:19:32:22 | expr |
+| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:37:19:37:22 | expr |
+| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:38:19:38:22 | expr |
+| OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:39:31:39:34 | expr |
+nodes
+| OgnlInjection.java:11:39:11:63 | expr : String | semmle.label | expr : String |
+| OgnlInjection.java:13:19:13:22 | tree | semmle.label | tree |
+| OgnlInjection.java:14:19:14:22 | tree | semmle.label | tree |
+| OgnlInjection.java:16:17:16:27 | (...)... : Object | semmle.label | (...)... : Object |
+| OgnlInjection.java:17:5:17:8 | node | semmle.label | node |
+| OgnlInjection.java:18:5:18:8 | node | semmle.label | node |
+| OgnlInjection.java:21:41:21:65 | expr : String | semmle.label | expr : String |
+| OgnlInjection.java:23:19:23:22 | tree | semmle.label | tree |
+| OgnlInjection.java:24:19:24:22 | tree | semmle.label | tree |
+| OgnlInjection.java:26:5:26:8 | tree | semmle.label | tree |
+| OgnlInjection.java:27:5:27:8 | tree | semmle.label | tree |
+| OgnlInjection.java:30:40:30:64 | expr : String | semmle.label | expr : String |
+| OgnlInjection.java:31:19:31:22 | expr | semmle.label | expr |
+| OgnlInjection.java:32:19:32:22 | expr | semmle.label | expr |
+| OgnlInjection.java:35:26:35:50 | expr : String | semmle.label | expr : String |
+| OgnlInjection.java:37:19:37:22 | expr | semmle.label | expr |
+| OgnlInjection.java:38:19:38:22 | expr | semmle.label | expr |
+| OgnlInjection.java:39:31:39:34 | expr | semmle.label | expr |
+#select
+| OgnlInjection.java:13:19:13:22 | tree | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:13:19:13:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input |
+| OgnlInjection.java:14:19:14:22 | tree | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:14:19:14:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input |
+| OgnlInjection.java:17:5:17:8 | node | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:17:5:17:8 | node | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input |
+| OgnlInjection.java:18:5:18:8 | node | OgnlInjection.java:11:39:11:63 | expr : String | OgnlInjection.java:18:5:18:8 | node | OGNL expression might include input from $@. | OgnlInjection.java:11:39:11:63 | expr | this user input |
+| OgnlInjection.java:23:19:23:22 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:23:19:23:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input |
+| OgnlInjection.java:24:19:24:22 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:24:19:24:22 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input |
+| OgnlInjection.java:26:5:26:8 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:26:5:26:8 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input |
+| OgnlInjection.java:27:5:27:8 | tree | OgnlInjection.java:21:41:21:65 | expr : String | OgnlInjection.java:27:5:27:8 | tree | OGNL expression might include input from $@. | OgnlInjection.java:21:41:21:65 | expr | this user input |
+| OgnlInjection.java:31:19:31:22 | expr | OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:31:19:31:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:30:40:30:64 | expr | this user input |
+| OgnlInjection.java:32:19:32:22 | expr | OgnlInjection.java:30:40:30:64 | expr : String | OgnlInjection.java:32:19:32:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:30:40:30:64 | expr | this user input |
+| OgnlInjection.java:37:19:37:22 | expr | OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:37:19:37:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:35:26:35:50 | expr | this user input |
+| OgnlInjection.java:38:19:38:22 | expr | OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:38:19:38:22 | expr | OGNL expression might include input from $@. | OgnlInjection.java:35:26:35:50 | expr | this user input |
+| OgnlInjection.java:39:31:39:34 | expr | OgnlInjection.java:35:26:35:50 | expr : String | OgnlInjection.java:39:31:39:34 | expr | OGNL expression might include input from $@. | OgnlInjection.java:35:26:35:50 | expr | this user input |
--- a/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.java
+++ b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.java
@@ -0,0 +1,41 @@
+import ognl.Node;
+import ognl.Ognl;
+
+import java.util.HashMap;
+
+import com.opensymphony.xwork2.ognl.OgnlUtil;
+
+import org.springframework.web.bind.annotation.RequestParam;
+
+public class OgnlInjection {
+  public void testOgnlParseExpression(@RequestParam String expr) throws Exception {
+    Object tree = Ognl.parseExpression(expr);
+    Ognl.getValue(tree, new HashMap<>(), new Object());
+    Ognl.setValue(tree, new HashMap<>(), new Object());
+
+    Node node = (Node) tree;
+    node.getValue(null, new Object());
+    node.setValue(null, new Object(), new Object());
+  }
+
+  public void testOgnlCompileExpression(@RequestParam String expr) throws Exception {
+    Node tree = Ognl.compileExpression(null, new Object(), expr);
+    Ognl.getValue(tree, new HashMap<>(), new Object());
+    Ognl.setValue(tree, new HashMap<>(), new Object());
+
+    tree.getValue(null, new Object());
+    tree.setValue(null, new Object(), new Object());
+  }
+
+  public void testOgnlDirectlyToGetSet(@RequestParam String expr) throws Exception {
+    Ognl.getValue(expr, new Object());
+    Ognl.setValue(expr, new Object(), new Object());
+  }
+
+  public void testStruts(@RequestParam String expr) throws Exception {
+    OgnlUtil ognl = new OgnlUtil();
+    ognl.getValue(expr, new HashMap<>(), new Object());
+    ognl.setValue(expr, new HashMap<>(), new Object(), new Object());
+    new OgnlUtil().callMethod(expr, new HashMap<>(), new Object());
+  }
+}
--- a/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.qlref
+++ b/java/ql/test/experimental/query-tests/security/CWE-917/OgnlInjection.qlref
@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-917/OgnlInjection.ql
--- a/java/ql/test/experimental/query-tests/security/CWE-917/options
+++ b/java/ql/test/experimental/query-tests/security/CWE-917/options
@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.2.3:${testdir}/../../../stubs/ognl-3.2.14:${testdir}/../../../stubs/struts2-core-2.5.22
--- a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/JavaSource.java
+++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/JavaSource.java
@@ -0,0 +1,3 @@
+package ognl;
+
+public interface JavaSource {}
--- a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Node.java
+++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Node.java
@@ -0,0 +1,6 @@
+package ognl;
+
+public interface Node extends JavaSource {
+  public Object getValue(OgnlContext context, Object source) throws OgnlException;
+  public void setValue(OgnlContext context, Object target, Object value) throws OgnlException;
+}
--- a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Ognl.java
+++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/Ognl.java
@@ -0,0 +1,26 @@
+package ognl;
+
+import java.util.*;
+
+public abstract class Ognl {
+  public static Object parseExpression(String expression) throws OgnlException {
+    return new Object();
+  }
+
+  public static Object getValue(Object tree, Map context, Object root) throws OgnlException {
+    return new Object();
+  }
+
+  public static void setValue(Object tree, Object root, Object value) throws OgnlException {}
+
+  public static Node compileExpression(OgnlContext context, Object root, String expression)
+            throws Exception {
+    return null;
+  }
+
+  public static Object getValue(String expression, Object root) throws OgnlException {
+    return new Object();
+  }
+
+  public static void setValue(String expression, Object root, Object value) throws OgnlException {}
+}
--- a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlContext.java
+++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlContext.java
@@ -0,0 +1,71 @@
+package ognl;
+
+import java.util.*;
+
+public class OgnlContext extends Object implements Map {
+  @Override
+  public int size() {
+      return 0;
+  }
+
+  @Override
+  public boolean isEmpty() {
+      return false;
+  }
+
+  @Override
+  public boolean containsKey(Object key) {
+      return true;
+  }
+
+  @Override
+  public boolean containsValue(Object value) {
+      return true;
+  }
+
+  @Override
+  public Object get(Object key) {
+    return new Object();
+  }
+
+  @Override
+  public Object put(Object key, Object value) {
+    return new Object();
+  }
+
+  @Override
+  public Object remove(Object key) {
+    return new Object();
+  }
+
+  @Override
+  public void putAll(Map t) { }
+
+  @Override
+  public void clear() {}
+
+  @Override
+  public Set keySet() {
+    return new HashSet();
+  }
+
+  @Override
+  public Collection values() {
+    return new HashSet();
+  }
+
+  @Override
+  public Set entrySet() {
+    return new HashSet();
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    return true;
+  }
+
+  @Override
+  public int hashCode() {
+    return 0;
+  }
+}
--- a/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlException.java
+++ b/java/ql/test/experimental/stubs/ognl-3.2.14/ognl/OgnlException.java
@@ -0,0 +1,3 @@
+package ognl;
+
+public class OgnlException extends Exception {}
--- a/java/ql/test/experimental/stubs/struts2-core-2.5.22/com/opensymphony/xwork2/ognl/OgnlUtil.java
+++ b/java/ql/test/experimental/stubs/struts2-core-2.5.22/com/opensymphony/xwork2/ognl/OgnlUtil.java
@@ -0,0 +1,16 @@
+package com.opensymphony.xwork2.ognl;
+
+import java.util.*;
+import ognl.OgnlException;
+
+public class OgnlUtil {
+  public Object getValue(final String name, final Map<String, Object> context, final Object root) throws OgnlException {
+    return new Object();
+  }
+
+  public void setValue(final String name, final Map<String, Object> context, final Object root, final Object value) throws OgnlException {}
+
+  public Object callMethod(final String name, final Map<String, Object> context, final Object root) throws OgnlException {
+    return new Object();
+  }
+}
				`@@ -0,0 +1 @@`
				`experimental/Security/CWE/CWE-917/OgnlInjection.ql`
				`@@ -0,0 +1 @@`
				`//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.2.3:${testdir}/../../../stubs/ognl-3.2.14:${testdir}/../../../stubs/struts2-core-2.5.22`