Java: Added Accessor sink for MVEL injections

artem-smotrakov · artem-smotrakov · commit 6a6c805048fd · 2020-06-05T17:13:24.000+03:00
diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll b/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll
@@ -37,7 +37,8 @@ class MvelEvaluationSink extends DataFlow::ExprNode {
       (
         m instanceof ExecutableStatementEvaluationMethod or
         m instanceof CompiledExpressionEvaluationMethod or
-        m instanceof CompiledAccExpressionEvaluationMethod
+        m instanceof CompiledAccExpressionEvaluationMethod or
+        m instanceof AccessorEvaluationMethod
       ) and
       (ma = asExpr() or ma.getQualifier() = asExpr())
     )
@@ -159,6 +160,16 @@ class CompiledAccExpressionEvaluationMethod extends Method {
   }
 }
 
+/**
+ * Methods in `Accessor` that trigger evaluating a MVEL expression.
+ */
+class AccessorEvaluationMethod extends Method {
+  AccessorEvaluationMethod() {
+    getDeclaringType() instanceof Accessor and
+    hasName("getValue")
+  }
+}
+
 class MVEL extends RefType {
   MVEL() { hasQualifiedName("org.mvel2", "MVEL") }
 }
@@ -178,3 +189,7 @@ class CompiledExpression extends RefType {
 class CompiledAccExpression extends RefType {
   CompiledAccExpression() { hasQualifiedName("org.mvel2.compiler", "CompiledAccExpression") }
 }
+
+class Accessor extends RefType {
+  Accessor() { hasQualifiedName("org.mvel2.compiler", "Accessor") }
+}
diff --git a/java/ql/test/experimental/Security/CWE/CWE-094/MvelInjection.expected b/java/ql/test/experimental/Security/CWE/CWE-094/MvelInjection.expected
@@ -2,22 +2,25 @@ edges
 | MvelInjection.java:16:27:16:49 | getInputStream(...) : InputStream | MvelInjection.java:20:17:20:21 | input |
 | MvelInjection.java:25:27:25:49 | getInputStream(...) : InputStream | MvelInjection.java:30:30:30:39 | expression |
 | MvelInjection.java:35:27:35:49 | getInputStream(...) : InputStream | MvelInjection.java:41:7:41:15 | statement |
-| MvelInjection.java:46:27:46:49 | getInputStream(...) : InputStream | MvelInjection.java:52:7:52:16 | expression |
-| MvelInjection.java:57:27:57:49 | getInputStream(...) : InputStream | MvelInjection.java:62:7:62:16 | expression |
+| MvelInjection.java:35:27:35:49 | getInputStream(...) : InputStream | MvelInjection.java:42:7:42:15 | statement |
+| MvelInjection.java:47:27:47:49 | getInputStream(...) : InputStream | MvelInjection.java:53:7:53:16 | expression |
+| MvelInjection.java:58:27:58:49 | getInputStream(...) : InputStream | MvelInjection.java:63:7:63:16 | expression |
 nodes
 | MvelInjection.java:16:27:16:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
 | MvelInjection.java:20:17:20:21 | input | semmle.label | input |
 | MvelInjection.java:25:27:25:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
 | MvelInjection.java:30:30:30:39 | expression | semmle.label | expression |
 | MvelInjection.java:35:27:35:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
 | MvelInjection.java:41:7:41:15 | statement | semmle.label | statement |
-| MvelInjection.java:46:27:46:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
-| MvelInjection.java:52:7:52:16 | expression | semmle.label | expression |
-| MvelInjection.java:57:27:57:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
-| MvelInjection.java:62:7:62:16 | expression | semmle.label | expression |
+| MvelInjection.java:42:7:42:15 | statement | semmle.label | statement |
+| MvelInjection.java:47:27:47:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| MvelInjection.java:53:7:53:16 | expression | semmle.label | expression |
+| MvelInjection.java:58:27:58:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| MvelInjection.java:63:7:63:16 | expression | semmle.label | expression |
 #select
 | MvelInjection.java:20:17:20:21 | input | MvelInjection.java:16:27:16:49 | getInputStream(...) : InputStream | MvelInjection.java:20:17:20:21 | input | MVEL injection from $@. | MvelInjection.java:16:27:16:49 | getInputStream(...) | this user input |
 | MvelInjection.java:30:30:30:39 | expression | MvelInjection.java:25:27:25:49 | getInputStream(...) : InputStream | MvelInjection.java:30:30:30:39 | expression | MVEL injection from $@. | MvelInjection.java:25:27:25:49 | getInputStream(...) | this user input |
 | MvelInjection.java:41:7:41:15 | statement | MvelInjection.java:35:27:35:49 | getInputStream(...) : InputStream | MvelInjection.java:41:7:41:15 | statement | MVEL injection from $@. | MvelInjection.java:35:27:35:49 | getInputStream(...) | this user input |
-| MvelInjection.java:52:7:52:16 | expression | MvelInjection.java:46:27:46:49 | getInputStream(...) : InputStream | MvelInjection.java:52:7:52:16 | expression | MVEL injection from $@. | MvelInjection.java:46:27:46:49 | getInputStream(...) | this user input |
-| MvelInjection.java:62:7:62:16 | expression | MvelInjection.java:57:27:57:49 | getInputStream(...) : InputStream | MvelInjection.java:62:7:62:16 | expression | MVEL injection from $@. | MvelInjection.java:57:27:57:49 | getInputStream(...) | this user input |
+| MvelInjection.java:42:7:42:15 | statement | MvelInjection.java:35:27:35:49 | getInputStream(...) : InputStream | MvelInjection.java:42:7:42:15 | statement | MVEL injection from $@. | MvelInjection.java:35:27:35:49 | getInputStream(...) | this user input |
+| MvelInjection.java:53:7:53:16 | expression | MvelInjection.java:47:27:47:49 | getInputStream(...) : InputStream | MvelInjection.java:53:7:53:16 | expression | MVEL injection from $@. | MvelInjection.java:47:27:47:49 | getInputStream(...) | this user input |
+| MvelInjection.java:63:7:63:16 | expression | MvelInjection.java:58:27:58:49 | getInputStream(...) : InputStream | MvelInjection.java:63:7:63:16 | expression | MVEL injection from $@. | MvelInjection.java:58:27:58:49 | getInputStream(...) | this user input |
diff --git a/java/ql/test/experimental/Security/CWE/CWE-094/MvelInjection.java b/java/ql/test/experimental/Security/CWE/CWE-094/MvelInjection.java
@@ -39,6 +39,7 @@ public static void testWithExpressionCompiler(Socket socket) throws IOException
       ExpressionCompiler compiler = new ExpressionCompiler(input);
       ExecutableStatement statement = compiler.compile();
       statement.getValue(new Object(), new ImmutableDefaultFactory());
+      statement.getValue(new Object(), new Object(), new ImmutableDefaultFactory());
     }
   }
 
diff --git a/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/compiler/Accessor.java b/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/compiler/Accessor.java
@@ -0,0 +1,7 @@
+package org.mvel2.compiler;
+
+import org.mvel2.integration.VariableResolverFactory;
+
+public interface Accessor {
+  public Object getValue(Object ctx, Object elCtx, VariableResolverFactory factory);
+}
diff --git a/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/compiler/CompiledExpression.java b/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/compiler/CompiledExpression.java
@@ -5,4 +5,5 @@
 public class CompiledExpression implements ExecutableStatement {
   public Object getDirectValue(Object staticContext, VariableResolverFactory factory) { return null; }
   public Object getValue(Object staticContext, VariableResolverFactory factory) { return null; }
+  public Object getValue(Object ctx, Object elCtx, VariableResolverFactory factory) { return null; }
 }
diff --git a/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/compiler/ExecutableStatement.java b/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/compiler/ExecutableStatement.java
@@ -2,6 +2,6 @@
 
 import org.mvel2.integration.VariableResolverFactory;
 
-public interface ExecutableStatement {
+public interface ExecutableStatement extends Accessor {
   public Object getValue(Object staticContext, VariableResolverFactory factory);
 }

Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,8 @@ class MvelEvaluationSink extends DataFlow::ExprNode {`
`37`	`37`	`(`
`38`	`38`	`m instanceof ExecutableStatementEvaluationMethod or`
`39`	`39`	`m instanceof CompiledExpressionEvaluationMethod or`
`40`		`- m instanceof CompiledAccExpressionEvaluationMethod`
	`40`	`+ m instanceof CompiledAccExpressionEvaluationMethod or`
	`41`	`+ m instanceof AccessorEvaluationMethod`
`41`	`42`	`) and`
`42`	`43`	`(ma = asExpr() or ma.getQualifier() = asExpr())`
`43`	`44`	`)`
`@@ -159,6 +160,16 @@ class CompiledAccExpressionEvaluationMethod extends Method {`
`159`	`160`	`}`
`160`	`161`	`}`
`161`	`162`
	`163`	`+/**`
	`164`	+ * Methods in `Accessor` that trigger evaluating a MVEL expression.
	`165`	`+ */`
	`166`	`+class AccessorEvaluationMethod extends Method {`
	`167`	`+ AccessorEvaluationMethod() {`
	`168`	`+ getDeclaringType() instanceof Accessor and`
	`169`	`+ hasName("getValue")`
	`170`	`+ }`
	`171`	`+}`
	`172`	`+`
`162`	`173`	`class MVEL extends RefType {`
`163`	`174`	`MVEL() { hasQualifiedName("org.mvel2", "MVEL") }`
`164`	`175`	`}`
`@@ -178,3 +189,7 @@ class CompiledExpression extends RefType {`
`178`	`189`	`class CompiledAccExpression extends RefType {`
`179`	`190`	`CompiledAccExpression() { hasQualifiedName("org.mvel2.compiler", "CompiledAccExpression") }`
`180`	`191`	`}`
	`192`	`+`
	`193`	`+class Accessor extends RefType {`
	`194`	`+ Accessor() { hasQualifiedName("org.mvel2.compiler", "Accessor") }`
	`195`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -39,6 +39,7 @@ public static void testWithExpressionCompiler(Socket socket) throws IOException`
`39`	`39`	`ExpressionCompiler compiler = new ExpressionCompiler(input);`
`40`	`40`	`ExecutableStatement statement = compiler.compile();`
`41`	`41`	`statement.getValue(new Object(), new ImmutableDefaultFactory());`
	`42`	`+ statement.getValue(new Object(), new Object(), new ImmutableDefaultFactory());`
`42`	`43`	`}`
`43`	`44`	`}`
`44`	`45`
Original file line number	Diff line number	Diff line change
`@@ -5,4 +5,5 @@`
`5`	`5`	`public class CompiledExpression implements ExecutableStatement {`
`6`	`6`	`public Object getDirectValue(Object staticContext, VariableResolverFactory factory) { return null; }`
`7`	`7`	`public Object getValue(Object staticContext, VariableResolverFactory factory) { return null; }`
	`8`	`+ public Object getValue(Object ctx, Object elCtx, VariableResolverFactory factory) { return null; }`
`8`	`9`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,6 @@`
`2`	`2`
`3`	`3`	`import org.mvel2.integration.VariableResolverFactory;`
`4`	`4`
`5`		`-public interface ExecutableStatement {`
	`5`	`+public interface ExecutableStatement extends Accessor {`
`6`	`6`	`public Object getValue(Object staticContext, VariableResolverFactory factory);`
`7`	`7`	`}`