From db732918af20bad35f6ad474eb9f99725ea871cf Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Thu, 13 May 2021 13:15:51 +0200
Subject: [PATCH] Add taint step for setExpression

---
 .../code/java/security/OgnlInjection.qll      | 27 +++++++++++++++++--
 .../security/CWE-917/OgnlInjection.java       | 12 +++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/java/ql/src/semmle/code/java/security/OgnlInjection.qll b/java/ql/src/semmle/code/java/security/OgnlInjection.qll
index 64fa9fffb87..9bb5a56eb16 100644
--- a/java/ql/src/semmle/code/java/security/OgnlInjection.qll
+++ b/java/ql/src/semmle/code/java/security/OgnlInjection.qll
@@ -61,6 +61,13 @@ private class TypeNode extends Interface {
   }
 }
 
+private class TypeExpressionAccessor extends Interface {
+  TypeExpressionAccessor() {
+    this.hasQualifiedName("org.apache.commons.ognl.enhance", "ExpressionAccessor") or
+    this.hasQualifiedName("ognl.enhance", "ExpressionAccessor")
+  }
+}
+
 /**
  * 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)`.
@@ -87,15 +94,31 @@ private predicate getAccessorStep(DataFlow::Node n1, DataFlow::Node n2) {
     n1.asExpr() = ma.getQualifier() and
     n2.asExpr() = ma and
     ma.getMethod() = m and
-    m.getDeclaringType() instanceof TypeNode
+    m.getDeclaringType().getASupertype*() instanceof TypeNode
   |
     m.hasName("getAccessor")
   )
 }
 
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `Node` and `Accessor`
+ * in a `setExpression` call, i.e. `accessor.setExpression(tainted)`
+ */
+private predicate setExpressionStep(DataFlow::Node n1, DataFlow::Node n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getArgument(0) and
+    n2.asExpr() = ma.getQualifier() and
+    ma.getMethod() = m and
+    m.getDeclaringType().getASupertype*() instanceof TypeExpressionAccessor
+  |
+    m.hasName("setExpression")
+  )
+}
+
 private class DefaultOgnlInjectionAdditionalTaintStep extends OgnlInjectionAdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
     parseCompileExpressionStep(node1, node2) or
-    getAccessorStep(node1, node2)
+    getAccessorStep(node1, node2) or
+    setExpressionStep(node1, node2)
   }
 }
diff --git a/java/ql/test/query-tests/security/CWE-917/OgnlInjection.java b/java/ql/test/query-tests/security/CWE-917/OgnlInjection.java
index feac2a72418..777bbcb06aa 100644
--- a/java/ql/test/query-tests/security/CWE-917/OgnlInjection.java
+++ b/java/ql/test/query-tests/security/CWE-917/OgnlInjection.java
@@ -56,6 +56,18 @@ public class OgnlInjection {
 
     Ognl.getValue(accessor, null, new Object()); // $hasOgnlInjection
     Ognl.setValue(accessor, null, new Object()); // $hasOgnlInjection
+  }
 
+  @RequestMapping
+  public void testExpressionAccessorSetExpression(@RequestParam String expr) throws Exception {
+    Node tree = Ognl.compileExpression(null, new Object(), "\"some safe expression\".toString()");
+    ExpressionAccessor accessor = tree.getAccessor();
+    Node taintedTree = Ognl.compileExpression(null, new Object(), expr);
+    accessor.setExpression(taintedTree);
+    accessor.get(null, new Object()); // $hasOgnlInjection
+    accessor.set(null, new Object(), new Object()); // $hasOgnlInjection
+
+    Ognl.getValue(accessor, null, new Object()); // $hasOgnlInjection
+    Ognl.setValue(accessor, null, new Object()); // $hasOgnlInjection
   }
 }