From 59e6e1ffac3ab71a80e2b0427a23b201b224662a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 2 Jun 2021 09:58:30 +0200 Subject: [PATCH 01/16] Moved from experimental --- .../Security/CWE/CWE-094/MvelInjection.qhelp | 0 .../Security/CWE/CWE-094/MvelInjection.ql | 0 .../Security/CWE/CWE-094/MvelInjectionLib.qll | 0 .../CWE/CWE-094/UnsafeMvelExpressionEvaluation.java | 0 .../query-tests/security/CWE-094/MvelInjection.qlref | 1 - .../query-tests/security/CWE-094/options | 2 +- .../security/CWE-094/MvelInjection.expected | 0 .../query-tests/security/CWE-094/MvelInjection.java | 0 .../query-tests/security/CWE-094/MvelInjection.qlref | 1 + java/ql/test/query-tests/security/CWE-094/options | 2 +- .../org/mvel2/jsr223/MvelCompiledScript.java | 12 ++++++++++-- 11 files changed, 13 insertions(+), 5 deletions(-) rename java/ql/src/{experimental => }/Security/CWE/CWE-094/MvelInjection.qhelp (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-094/MvelInjection.ql (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-094/MvelInjectionLib.qll (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java (100%) delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.qlref rename java/ql/test/{experimental => }/query-tests/security/CWE-094/MvelInjection.expected (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-094/MvelInjection.java (100%) create mode 100644 java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjection.qhelp b/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-094/MvelInjection.qhelp rename to java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjection.ql b/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-094/MvelInjection.ql rename to java/ql/src/Security/CWE/CWE-094/MvelInjection.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll b/java/ql/src/Security/CWE/CWE-094/MvelInjectionLib.qll similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-094/MvelInjectionLib.qll rename to java/ql/src/Security/CWE/CWE-094/MvelInjectionLib.qll diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java b/java/ql/src/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java rename to java/ql/src/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java diff --git a/java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.qlref b/java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.qlref deleted file mode 100644 index 13d7cbd2295..00000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-094/MvelInjection.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-094/options b/java/ql/test/experimental/query-tests/security/CWE-094/options index 48cc00e0a17..e45718af8ff 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-094/options +++ b/java/ql/test/experimental/query-tests/security/CWE-094/options @@ -1,2 +1,2 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/springframework-5.2.3:${testdir}/../../../../stubs/mvel2-2.4.7:${testdir}/../../../../stubs/jsr223-api:${testdir}/../../../../stubs/apache-commons-jexl-2.1.1:${testdir}/../../../../stubs/apache-commons-jexl-3.1:${testdir}/../../../../stubs/scriptengine:${testdir}/../../../../stubs/java-ee-el:${testdir}/../../../../stubs/juel-2.2:${testdir}/../../../stubs/groovy-all-3.0.7:${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jython-2.7.2:${testdir}/../../../../experimental/stubs/rhino-1.7.13 +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/springframework-5.2.3:${testdir}/../../../../stubs/jsr223-api:${testdir}/../../../../stubs/apache-commons-jexl-2.1.1:${testdir}/../../../../stubs/apache-commons-jexl-3.1:${testdir}/../../../../stubs/scriptengine:${testdir}/../../../../stubs/java-ee-el:${testdir}/../../../../stubs/juel-2.2:${testdir}/../../../stubs/groovy-all-3.0.7:${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jython-2.7.2:${testdir}/../../../../experimental/stubs/rhino-1.7.13 diff --git a/java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.expected b/java/ql/test/query-tests/security/CWE-094/MvelInjection.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.expected rename to java/ql/test/query-tests/security/CWE-094/MvelInjection.expected diff --git a/java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.java b/java/ql/test/query-tests/security/CWE-094/MvelInjection.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-094/MvelInjection.java rename to java/ql/test/query-tests/security/CWE-094/MvelInjection.java diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref b/java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref new file mode 100644 index 00000000000..07d8b705dc7 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-094/MvelInjection.ql \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-094/options b/java/ql/test/query-tests/security/CWE-094/options index 468d90aeabc..4b0f7ffb1f1 100644 --- a/java/ql/test/query-tests/security/CWE-094/options +++ b/java/ql/test/query-tests/security/CWE-094/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/validation-api-2.0.1.Final +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/validation-api-2.0.1.Final:${testdir}/../../../stubs/mvel2-2.4.7:${testdir}/../../../stubs/scriptengine:${testdir}/../../../stubs/jsr223-api:${testdir} diff --git a/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/jsr223/MvelCompiledScript.java b/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/jsr223/MvelCompiledScript.java index a4be37ada32..771f8b83345 100644 --- a/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/jsr223/MvelCompiledScript.java +++ b/java/ql/test/stubs/mvel2-2.4.7/org/mvel2/jsr223/MvelCompiledScript.java @@ -3,9 +3,17 @@ package org.mvel2.jsr223; import java.io.Serializable; import javax.script.CompiledScript; import javax.script.ScriptContext; +import javax.script.ScriptEngine; import javax.script.ScriptException; public class MvelCompiledScript extends CompiledScript { public MvelCompiledScript(MvelScriptEngine engine, Serializable compiledScript) {} - public Object eval(ScriptContext context) throws ScriptException { return null; } -} \ No newline at end of file + + public Object eval(ScriptContext context) throws ScriptException { + return null; + } + + public ScriptEngine getEngine() { + return null; + } +} From b30c92e69ec7ae1567ff83cd1f148b766dadbe4a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 2 Jun 2021 11:33:01 +0200 Subject: [PATCH 02/16] Refactored into MvelInjection.qll using CSV models --- .../src/Security/CWE/CWE-094/MvelInjection.ql | 26 +- .../Security/CWE/CWE-094/MvelInjectionLib.qll | 367 ------------------ .../code/java/security/MvelInjection.qll | 233 +++++++++++ 3 files changed, 257 insertions(+), 369 deletions(-) delete mode 100644 java/ql/src/Security/CWE/CWE-094/MvelInjectionLib.qll create mode 100644 java/ql/src/semmle/code/java/security/MvelInjection.qll diff --git a/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql b/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql index d32c33c343c..b88b017fc7e 100644 --- a/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql +++ b/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql @@ -11,9 +11,31 @@ */ import java -import MvelInjectionLib +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.MvelInjection import DataFlow::PathGraph -from DataFlow::PathNode source, DataFlow::PathNode sink, MvelInjectionConfig conf +/** + * A taint-tracking configuration for unsafe user input + * that is used to construct and evaluate a MVEL expression. + */ +class MvelInjectionFlowConfig extends TaintTracking::Configuration { + MvelInjectionFlowConfig() { this = "MvelInjectionFlowConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof MvelEvaluationSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof MvelInjectionSanitizer + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + any(MvelInjectionAdditionalTaintStep c).step(node1, node2) + } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, MvelInjectionFlowConfig conf where conf.hasFlowPath(source, sink) select sink.getNode(), source, sink, "MVEL injection from $@.", source.getNode(), "this user input" diff --git a/java/ql/src/Security/CWE/CWE-094/MvelInjectionLib.qll b/java/ql/src/Security/CWE/CWE-094/MvelInjectionLib.qll deleted file mode 100644 index a6cf891330f..00000000000 --- a/java/ql/src/Security/CWE/CWE-094/MvelInjectionLib.qll +++ /dev/null @@ -1,367 +0,0 @@ -import java -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.dataflow.TaintTracking - -/** - * A taint-tracking configuration for unsafe user input - * that is used to construct and evaluate a MVEL expression. - */ -class MvelInjectionConfig extends TaintTracking::Configuration { - MvelInjectionConfig() { this = "MvelInjectionConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof MvelEvaluationSink } - - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - expressionCompilationStep(node1, node2) or - createExpressionCompilerStep(node1, node2) or - expressionCompilerCompileStep(node1, node2) or - createCompiledAccExpressionStep(node1, node2) or - scriptCompileStep(node1, node2) or - createMvelCompiledScriptStep(node1, node2) or - templateCompileStep(node1, node2) or - createTemplateCompilerStep(node1, node2) - } -} - -/** - * A sink for EL injection vulnerabilities via MVEL, - * i.e. methods that run evaluation of a MVEL expression. - */ -class MvelEvaluationSink extends DataFlow::ExprNode { - MvelEvaluationSink() { - exists(StaticMethodAccess ma, Method m | m = ma.getMethod() | - ( - m instanceof MvelEvalMethod or - m instanceof TemplateRuntimeEvaluationMethod - ) and - ma.getArgument(0) = asExpr() - ) - or - exists(MethodAccess ma, Method m | m = ma.getMethod() | - m instanceof MvelScriptEngineEvaluationMethod and - ma.getArgument(0) = asExpr() - ) - or - exists(MethodAccess ma, Method m | m = ma.getMethod() | - ( - m instanceof ExecutableStatementEvaluationMethod or - m instanceof CompiledExpressionEvaluationMethod or - m instanceof CompiledAccExpressionEvaluationMethod or - m instanceof AccessorEvaluationMethod or - m instanceof CompiledScriptEvaluationMethod or - m instanceof MvelCompiledScriptEvaluationMethod - ) and - ma.getQualifier() = asExpr() - ) - or - exists(StaticMethodAccess ma, Method m | m = ma.getMethod() | - m instanceof MvelRuntimeEvaluationMethod and - ma.getArgument(1) = asExpr() - ) - } -} - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression - * by callilng `MVEL.compileExpression(tainted)`. - */ -predicate expressionCompilationStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | - m.getDeclaringType() instanceof MVEL and - m.hasName("compileExpression") and - ma.getAnArgument() = node1.asExpr() and - node2.asExpr() = ma - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step creates `ExpressionCompiler`, - * i.e. `new ExpressionCompiler(tainted)`. - */ -predicate createExpressionCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof ExpressionCompiler and - cc = node2.asExpr() and - cc.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step creates `CompiledAccExpression`, - * i.e. `new CompiledAccExpression(tainted, ...)`. - */ -predicate createCompiledAccExpressionStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof CompiledAccExpression and - cc = node2.asExpr() and - cc.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression - * by calling `ExpressionCompiler.compile()`. - */ -predicate expressionCompilerCompileStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - m.getDeclaringType() instanceof ExpressionCompiler and - m.hasName("compile") and - ma = node2.asExpr() and - ma.getQualifier() = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a script via `MvelScriptEngine`, - * i.e. `engine.compile(tainted)` or `engine.compiledScript(tainted)`. - */ -predicate scriptCompileStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - m instanceof MvelScriptEngineCompilationMethod and - ma = node2.asExpr() and - ma.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step creates `MvelCompiledScript`, - * i.e. `new MvelCompiledScript(engine, tainted)`. - */ -predicate createMvelCompiledScriptStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof MvelCompiledScript and - cc = node2.asExpr() and - cc.getArgument(1) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step creates `TemplateCompiler`, - * i.e. `new TemplateCompiler(tainted)`. - */ -predicate createTemplateCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof TemplateCompiler and - cc = node2.asExpr() and - cc.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a script via `TemplateCompiler`, - * i.e. `compiler.compile()` or `TemplateCompiler.compileTemplate(tainted)`. - */ -predicate templateCompileStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - m instanceof TemplateCompilerCompileMethod and - ma.getQualifier() = node1.asExpr() and - ma = node2.asExpr() - ) - or - exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | - m instanceof TemplateCompilerCompileTemplateMethod and - ma = node2.asExpr() and - ma.getArgument(0) = node1.asExpr() - ) -} - -/** - * Methods in the MVEL class that evaluate a MVEL expression. - */ -class MvelEvalMethod extends Method { - MvelEvalMethod() { - getDeclaringType() instanceof MVEL and - ( - hasName("eval") or - hasName("executeExpression") or - hasName("evalToBoolean") or - hasName("evalToString") or - hasName("executeAllExpression") or - hasName("executeSetExpression") - ) - } -} - -/** - * Methods in `MVEL` class that compile a MVEL expression. - */ -class MvelCompileExpressionMethod extends Method { - MvelCompileExpressionMethod() { - getDeclaringType() instanceof MVEL and - ( - hasName("compileExpression") or - hasName("compileGetExpression") or - hasName("compileSetExpression") - ) - } -} - -/** - * Methods in `ExecutableStatement` that evaluate a MVEL expression. - */ -class ExecutableStatementEvaluationMethod extends Method { - ExecutableStatementEvaluationMethod() { - getDeclaringType() instanceof ExecutableStatement and - hasName("getValue") - } -} - -/** - * Methods in `CompiledExpression` that evaluate a MVEL expression. - */ -class CompiledExpressionEvaluationMethod extends Method { - CompiledExpressionEvaluationMethod() { - getDeclaringType() instanceof CompiledExpression and - hasName("getDirectValue") - } -} - -/** - * Methods in `CompiledAccExpression` that evaluate a MVEL expression. - */ -class CompiledAccExpressionEvaluationMethod extends Method { - CompiledAccExpressionEvaluationMethod() { - getDeclaringType() instanceof CompiledAccExpression and - hasName("getValue") - } -} - -/** - * Methods in `Accessor` that evaluate a MVEL expression. - */ -class AccessorEvaluationMethod extends Method { - AccessorEvaluationMethod() { - getDeclaringType() instanceof Accessor and - hasName("getValue") - } -} - -/** - * Methods in `MvelScriptEngine` that evaluate a MVEL expression. - */ -class MvelScriptEngineEvaluationMethod extends Method { - MvelScriptEngineEvaluationMethod() { - getDeclaringType() instanceof MvelScriptEngine and - (hasName("eval") or hasName("evaluate")) - } -} - -/** - * Methods in `MvelScriptEngine` that compile a MVEL expression. - */ -class MvelScriptEngineCompilationMethod extends Method { - MvelScriptEngineCompilationMethod() { - getDeclaringType() instanceof MvelScriptEngine and - (hasName("compile") or hasName("compiledScript")) - } -} - -/** - * Methods in `CompiledScript` that evaluate a MVEL expression. - */ -class CompiledScriptEvaluationMethod extends Method { - CompiledScriptEvaluationMethod() { - getDeclaringType() instanceof CompiledScript and - hasName("eval") - } -} - -/** - * Methods in `TemplateRuntime` that evaluate a MVEL template. - */ -class TemplateRuntimeEvaluationMethod extends Method { - TemplateRuntimeEvaluationMethod() { - getDeclaringType() instanceof TemplateRuntime and - (hasName("eval") or hasName("execute")) - } -} - -/** - * `TemplateCompiler.compile()` method compiles a MVEL template. - */ -class TemplateCompilerCompileMethod extends Method { - TemplateCompilerCompileMethod() { - getDeclaringType() instanceof TemplateCompiler and - hasName("compile") - } -} - -/** - * `TemplateCompiler.compileTemplate(tainted)` static method compiles a MVEL template. - */ -class TemplateCompilerCompileTemplateMethod extends Method { - TemplateCompilerCompileTemplateMethod() { - getDeclaringType() instanceof TemplateCompiler and - hasName("compileTemplate") - } -} - -/** - * Methods in `MvelCompiledScript` that evaluate a MVEL expression. - */ -class MvelCompiledScriptEvaluationMethod extends Method { - MvelCompiledScriptEvaluationMethod() { - getDeclaringType() instanceof MvelCompiledScript and - hasName("eval") - } -} - -/** - * Methods in `MVELRuntime` that evaluate a MVEL expression. - */ -class MvelRuntimeEvaluationMethod extends Method { - MvelRuntimeEvaluationMethod() { - getDeclaringType() instanceof MVELRuntime and - hasName("execute") - } -} - -class MVEL extends RefType { - MVEL() { hasQualifiedName("org.mvel2", "MVEL") } -} - -class ExpressionCompiler extends RefType { - ExpressionCompiler() { hasQualifiedName("org.mvel2.compiler", "ExpressionCompiler") } -} - -class ExecutableStatement extends RefType { - ExecutableStatement() { hasQualifiedName("org.mvel2.compiler", "ExecutableStatement") } -} - -class CompiledExpression extends RefType { - CompiledExpression() { hasQualifiedName("org.mvel2.compiler", "CompiledExpression") } -} - -class CompiledAccExpression extends RefType { - CompiledAccExpression() { hasQualifiedName("org.mvel2.compiler", "CompiledAccExpression") } -} - -class Accessor extends RefType { - Accessor() { hasQualifiedName("org.mvel2.compiler", "Accessor") } -} - -class CompiledScript extends RefType { - CompiledScript() { hasQualifiedName("javax.script", "CompiledScript") } -} - -class MvelScriptEngine extends RefType { - MvelScriptEngine() { hasQualifiedName("org.mvel2.jsr223", "MvelScriptEngine") } -} - -class MvelCompiledScript extends RefType { - MvelCompiledScript() { hasQualifiedName("org.mvel2.jsr223", "MvelCompiledScript") } -} - -class TemplateRuntime extends RefType { - TemplateRuntime() { hasQualifiedName("org.mvel2.templates", "TemplateRuntime") } -} - -class TemplateCompiler extends RefType { - TemplateCompiler() { hasQualifiedName("org.mvel2.templates", "TemplateCompiler") } -} - -class MVELRuntime extends RefType { - MVELRuntime() { hasQualifiedName("org.mvel2", "MVELRuntime") } -} diff --git a/java/ql/src/semmle/code/java/security/MvelInjection.qll b/java/ql/src/semmle/code/java/security/MvelInjection.qll new file mode 100644 index 00000000000..18bb16c6a65 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/MvelInjection.qll @@ -0,0 +1,233 @@ +/** Provides classes to reason about MVEL injection attacks. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow + +/** A data flow sink for unvalidated user input that is used to construct MVEL expressions. */ +abstract class MvelEvaluationSink extends DataFlow::Node { } + +/** A sanitizer that prevents MVEL injection attacks. */ +abstract class MvelInjectionSanitizer extends DataFlow::Node { } + +/** + * A unit class for adding additional taint steps. + * + * Extend this class to add additional taint steps that should apply to the `MvelInjectionFlowConfig`. + */ +class MvelInjectionAdditionalTaintStep extends Unit { + /** + * Holds if the step from `node1` to `node2` should be considered a taint + * step for the `MvelInjectionFlowConfig` configuration. + */ + abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); +} + +private class DefaulMvelEvaluationSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.script;CompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2;MVEL;false;eval;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToBoolean;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToString;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeAllExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeSetExpression;;;Argument[0];mvel", + "org.mvel2;MVELRuntime;false;execute;;;Argument[1];mvel", + "org.mvel2.templates;TemplateRuntime;false;eval;;;Argument[0];mvel", + "org.mvel2.templates;TemplateRuntime;false;execute;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelScriptEngine;false;eval;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelScriptEngine;false;evaluate;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelCompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2.compiler;ExecutableStatement;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledExpression;false;getDirectValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledAccExpression;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;Accessor;false;getValue;;;Argument[-1];mvel" + ] + } +} + +/** Default sink for MVEL injection vulnerabilities. */ +private class DefaultMvelEvaluationSink extends MvelEvaluationSink { + DefaultMvelEvaluationSink() { sinkNode(this, "mvel") } +} + +/** A default sanitizer that considers numeric and boolean typed data safe for building MVEL expressions */ +private class DefaultMvelInjectionSanitizer extends MvelInjectionSanitizer { + DefaultMvelInjectionSanitizer() { + this.getType() instanceof NumericType or this.getType() instanceof BooleanType + } +} + +/** A set of additional taint steps to consider when taint tracking MVEL related data flows. */ +private class DefaultMvelInjectionAdditionalTaintStep extends MvelInjectionAdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + expressionCompilationStep(node1, node2) or + createExpressionCompilerStep(node1, node2) or + expressionCompilerCompileStep(node1, node2) or + createCompiledAccExpressionStep(node1, node2) or + scriptCompileStep(node1, node2) or + createMvelCompiledScriptStep(node1, node2) or + templateCompileStep(node1, node2) or + createTemplateCompilerStep(node1, node2) + } +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression + * by callilng `MVEL.compileExpression(tainted)`. + */ +private predicate expressionCompilationStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | + m.getDeclaringType() instanceof MVEL and + m.hasName("compileExpression") and + ma.getAnArgument() = node1.asExpr() and + node2.asExpr() = ma + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step creates `ExpressionCompiler`, + * i.e. `new ExpressionCompiler(tainted)`. + */ +private predicate createExpressionCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof ExpressionCompiler and + cc = node2.asExpr() and + cc.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step creates `CompiledAccExpression`, + * i.e. `new CompiledAccExpression(tainted, ...)`. + */ +private predicate createCompiledAccExpressionStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof CompiledAccExpression and + cc = node2.asExpr() and + cc.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression + * by calling `ExpressionCompiler.compile()`. + */ +private predicate expressionCompilerCompileStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + m.getDeclaringType() instanceof ExpressionCompiler and + m.hasName("compile") and + ma = node2.asExpr() and + ma.getQualifier() = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a script via `MvelScriptEngine`, + * i.e. `engine.compile(tainted)` or `engine.compiledScript(tainted)`. + */ +private predicate scriptCompileStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + m instanceof MvelScriptEngineCompilationMethod and + ma = node2.asExpr() and + ma.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step creates `MvelCompiledScript`, + * i.e. `new MvelCompiledScript(engine, tainted)`. + */ +private predicate createMvelCompiledScriptStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof MvelCompiledScript and + cc = node2.asExpr() and + cc.getArgument(1) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step creates `TemplateCompiler`, + * i.e. `new TemplateCompiler(tainted)`. + */ +private predicate createTemplateCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof TemplateCompiler and + cc = node2.asExpr() and + cc.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a script via `TemplateCompiler`, + * i.e. `compiler.compile()` or `TemplateCompiler.compileTemplate(tainted)`. + */ +private predicate templateCompileStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + m instanceof TemplateCompilerCompileMethod and + ma.getQualifier() = node1.asExpr() and + ma = node2.asExpr() + ) + or + exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | + m instanceof TemplateCompilerCompileTemplateMethod and + ma = node2.asExpr() and + ma.getArgument(0) = node1.asExpr() + ) +} + +/** + * Methods in `MvelScriptEngine` that compile a MVEL expression. + */ +private class MvelScriptEngineCompilationMethod extends Method { + MvelScriptEngineCompilationMethod() { + getDeclaringType() instanceof MvelScriptEngine and + (hasName("compile") or hasName("compiledScript")) + } +} + +/** + * `TemplateCompiler.compile()` method that compiles a MVEL template. + */ +private class TemplateCompilerCompileMethod extends Method { + TemplateCompilerCompileMethod() { + getDeclaringType() instanceof TemplateCompiler and + hasName("compile") + } +} + +/** + * `TemplateCompiler.compileTemplate(tainted)` static method that compiles a MVEL template. + */ +private class TemplateCompilerCompileTemplateMethod extends Method { + TemplateCompilerCompileTemplateMethod() { + getDeclaringType() instanceof TemplateCompiler and + hasName("compileTemplate") + } +} + +private class MVEL extends RefType { + MVEL() { hasQualifiedName("org.mvel2", "MVEL") } +} + +private class ExpressionCompiler extends RefType { + ExpressionCompiler() { hasQualifiedName("org.mvel2.compiler", "ExpressionCompiler") } +} + +private class CompiledAccExpression extends RefType { + CompiledAccExpression() { hasQualifiedName("org.mvel2.compiler", "CompiledAccExpression") } +} + +private class MvelScriptEngine extends RefType { + MvelScriptEngine() { hasQualifiedName("org.mvel2.jsr223", "MvelScriptEngine") } +} + +private class MvelCompiledScript extends RefType { + MvelCompiledScript() { hasQualifiedName("org.mvel2.jsr223", "MvelCompiledScript") } +} + +private class TemplateCompiler extends RefType { + TemplateCompiler() { hasQualifiedName("org.mvel2.templates", "TemplateCompiler") } +} From d476459727ba1fe8072ce77b98afa6aad8bc6fcc Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 2 Jun 2021 12:15:26 +0200 Subject: [PATCH 03/16] Use InlineExpectationsTest --- .../security/CWE-094/MvelInjection.expected | 67 ------------------- .../security/CWE-094/MvelInjection.qlref | 1 - .../CWE-094/MvelInjectionTest.expected | 0 ...lInjection.java => MvelInjectionTest.java} | 33 +++++---- .../security/CWE-094/MvelInjectionTest.ql | 36 ++++++++++ 5 files changed, 52 insertions(+), 85 deletions(-) delete mode 100644 java/ql/test/query-tests/security/CWE-094/MvelInjection.expected delete mode 100644 java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref create mode 100644 java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.expected rename java/ql/test/query-tests/security/CWE-094/{MvelInjection.java => MvelInjectionTest.java} (78%) create mode 100644 java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjection.expected b/java/ql/test/query-tests/security/CWE-094/MvelInjection.expected deleted file mode 100644 index 545da956f8e..00000000000 --- a/java/ql/test/query-tests/security/CWE-094/MvelInjection.expected +++ /dev/null @@ -1,67 +0,0 @@ -edges -| MvelInjection.java:29:54:29:65 | read(...) : String | MvelInjection.java:30:28:30:37 | expression | -| MvelInjection.java:34:58:34:69 | read(...) : String | MvelInjection.java:36:5:36:13 | statement | -| MvelInjection.java:34:58:34:69 | read(...) : String | MvelInjection.java:37:5:37:13 | statement | -| MvelInjection.java:41:58:41:69 | read(...) : String | MvelInjection.java:43:5:43:14 | expression | -| MvelInjection.java:48:7:48:18 | read(...) : String | MvelInjection.java:49:5:49:14 | expression | -| MvelInjection.java:53:20:53:31 | read(...) : String | MvelInjection.java:57:5:57:18 | compiledScript | -| MvelInjection.java:53:20:53:31 | read(...) : String | MvelInjection.java:60:21:60:26 | script | -| MvelInjection.java:65:58:65:69 | read(...) : String | MvelInjection.java:68:5:68:10 | script | -| MvelInjection.java:77:40:77:51 | read(...) : String | MvelInjection.java:77:7:77:52 | compileTemplate(...) | -| MvelInjection.java:81:54:81:65 | read(...) : String | MvelInjection.java:82:29:82:46 | compile(...) | -| MvelInjection.java:86:58:86:69 | read(...) : String | MvelInjection.java:88:32:88:41 | expression | -| MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:94:15:94:16 | is : InputStream | -| MvelInjection.java:94:15:94:16 | is : InputStream | MvelInjection.java:94:23:94:27 | bytes [post update] : byte[] | -| MvelInjection.java:94:23:94:27 | bytes [post update] : byte[] | MvelInjection.java:95:14:95:36 | new String(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:25:15:25:26 | read(...) | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:29:54:29:65 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:34:58:34:69 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:41:58:41:69 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:48:7:48:18 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:53:20:53:31 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:65:58:65:69 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:72:26:72:37 | read(...) | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:77:40:77:51 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:81:54:81:65 | read(...) : String | -| MvelInjection.java:95:14:95:36 | new String(...) : String | MvelInjection.java:86:58:86:69 | read(...) : String | -nodes -| MvelInjection.java:25:15:25:26 | read(...) | semmle.label | read(...) | -| MvelInjection.java:29:54:29:65 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:30:28:30:37 | expression | semmle.label | expression | -| MvelInjection.java:34:58:34:69 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:36:5:36:13 | statement | semmle.label | statement | -| MvelInjection.java:37:5:37:13 | statement | semmle.label | statement | -| MvelInjection.java:41:58:41:69 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:43:5:43:14 | expression | semmle.label | expression | -| MvelInjection.java:48:7:48:18 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:49:5:49:14 | expression | semmle.label | expression | -| MvelInjection.java:53:20:53:31 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:57:5:57:18 | compiledScript | semmle.label | compiledScript | -| MvelInjection.java:60:21:60:26 | script | semmle.label | script | -| MvelInjection.java:65:58:65:69 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:68:5:68:10 | script | semmle.label | script | -| MvelInjection.java:72:26:72:37 | read(...) | semmle.label | read(...) | -| MvelInjection.java:77:7:77:52 | compileTemplate(...) | semmle.label | compileTemplate(...) | -| MvelInjection.java:77:40:77:51 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:81:54:81:65 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:82:29:82:46 | compile(...) | semmle.label | compile(...) | -| MvelInjection.java:86:58:86:69 | read(...) : String | semmle.label | read(...) : String | -| MvelInjection.java:88:32:88:41 | expression | semmle.label | expression | -| MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| MvelInjection.java:94:15:94:16 | is : InputStream | semmle.label | is : InputStream | -| MvelInjection.java:94:23:94:27 | bytes [post update] : byte[] | semmle.label | bytes [post update] : byte[] | -| MvelInjection.java:95:14:95:36 | new String(...) : String | semmle.label | new String(...) : String | -#select -| MvelInjection.java:25:15:25:26 | read(...) | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:25:15:25:26 | read(...) | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:30:28:30:37 | expression | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:30:28:30:37 | expression | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:36:5:36:13 | statement | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:36:5:36:13 | statement | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:37:5:37:13 | statement | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:37:5:37:13 | statement | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:43:5:43:14 | expression | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:43:5:43:14 | expression | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:49:5:49:14 | expression | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:49:5:49:14 | expression | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:57:5:57:18 | compiledScript | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:57:5:57:18 | compiledScript | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:60:21:60:26 | script | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:60:21:60:26 | script | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:68:5:68:10 | script | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:68:5:68:10 | script | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:72:26:72:37 | read(...) | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:72:26:72:37 | read(...) | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:77:7:77:52 | compileTemplate(...) | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:77:7:77:52 | compileTemplate(...) | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:82:29:82:46 | compile(...) | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:82:29:82:46 | compile(...) | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | -| MvelInjection.java:88:32:88:41 | expression | MvelInjection.java:92:27:92:49 | getInputStream(...) : InputStream | MvelInjection.java:88:32:88:41 | expression | MVEL injection from $@. | MvelInjection.java:92:27:92:49 | getInputStream(...) | this user input | diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref b/java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref deleted file mode 100644 index 07d8b705dc7..00000000000 --- a/java/ql/test/query-tests/security/CWE-094/MvelInjection.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-094/MvelInjection.ql \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.expected b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjection.java b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java similarity index 78% rename from java/ql/test/query-tests/security/CWE-094/MvelInjection.java rename to java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java index 88f9b861cae..6de9375d595 100644 --- a/java/ql/test/query-tests/security/CWE-094/MvelInjection.java +++ b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java @@ -19,34 +19,34 @@ import org.mvel2.templates.CompiledTemplate; import org.mvel2.templates.TemplateCompiler; import org.mvel2.templates.TemplateRuntime; -public class MvelInjection { +public class MvelInjectionTest { public static void testWithMvelEval(Socket socket) throws IOException { - MVEL.eval(read(socket)); + MVEL.eval(read(socket)); // $hasMvelInjection } public static void testWithMvelCompileAndExecute(Socket socket) throws IOException { Serializable expression = MVEL.compileExpression(read(socket)); - MVEL.executeExpression(expression); + MVEL.executeExpression(expression); // $hasMvelInjection } public static void testWithExpressionCompiler(Socket socket) throws IOException { ExpressionCompiler compiler = new ExpressionCompiler(read(socket)); ExecutableStatement statement = compiler.compile(); - statement.getValue(new Object(), new ImmutableDefaultFactory()); - statement.getValue(new Object(), new Object(), new ImmutableDefaultFactory()); + statement.getValue(new Object(), new ImmutableDefaultFactory()); // $hasMvelInjection + statement.getValue(new Object(), new Object(), new ImmutableDefaultFactory()); // $hasMvelInjection } public static void testWithCompiledExpressionGetDirectValue(Socket socket) throws IOException { ExpressionCompiler compiler = new ExpressionCompiler(read(socket)); CompiledExpression expression = compiler.compile(); - expression.getDirectValue(new Object(), new ImmutableDefaultFactory()); + expression.getDirectValue(new Object(), new ImmutableDefaultFactory()); // $hasMvelInjection } public static void testCompiledAccExpressionGetValue(Socket socket) throws IOException { - CompiledAccExpression expression = new CompiledAccExpression( - read(socket).toCharArray(), Object.class, new ParserContext()); - expression.getValue(new Object(), new ImmutableDefaultFactory()); + CompiledAccExpression expression = + new CompiledAccExpression(read(socket).toCharArray(), Object.class, new ParserContext()); + expression.getValue(new Object(), new ImmutableDefaultFactory()); // $hasMvelInjection } public static void testMvelScriptEngineCompileAndEvaluate(Socket socket) throws Exception { @@ -54,10 +54,10 @@ public class MvelInjection { MvelScriptEngine engine = new MvelScriptEngine(); CompiledScript compiledScript = engine.compile(input); - compiledScript.eval(); + compiledScript.eval(); // $hasMvelInjection Serializable script = engine.compiledScript(input); - engine.evaluate(script, new SimpleScriptContext()); + engine.evaluate(script, new SimpleScriptContext()); // $hasMvelInjection } public static void testMvelCompiledScriptCompileAndEvaluate(Socket socket) throws Exception { @@ -65,27 +65,26 @@ public class MvelInjection { ExpressionCompiler compiler = new ExpressionCompiler(read(socket)); ExecutableStatement statement = compiler.compile(); MvelCompiledScript script = new MvelCompiledScript(engine, statement); - script.eval(new SimpleScriptContext()); + script.eval(new SimpleScriptContext()); // $hasMvelInjection } public static void testTemplateRuntimeEval(Socket socket) throws Exception { - TemplateRuntime.eval(read(socket), new HashMap()); + TemplateRuntime.eval(read(socket), new HashMap()); // $hasMvelInjection } public static void testTemplateRuntimeCompileTemplateAndExecute(Socket socket) throws Exception { - TemplateRuntime.execute( - TemplateCompiler.compileTemplate(read(socket)), new HashMap()); + TemplateRuntime.execute(TemplateCompiler.compileTemplate(read(socket)), new HashMap()); // $hasMvelInjection } public static void testTemplateRuntimeCompileAndExecute(Socket socket) throws Exception { TemplateCompiler compiler = new TemplateCompiler(read(socket)); - TemplateRuntime.execute(compiler.compile(), new HashMap()); + TemplateRuntime.execute(compiler.compile(), new HashMap()); // $hasMvelInjection } public static void testMvelRuntimeExecute(Socket socket) throws Exception { ExpressionCompiler compiler = new ExpressionCompiler(read(socket)); CompiledExpression expression = compiler.compile(); - MVELRuntime.execute(false, expression, new Object(), new ImmutableDefaultFactory()); + MVELRuntime.execute(false, expression, new Object(), new ImmutableDefaultFactory()); // $hasMvelInjection } public static String read(Socket socket) throws IOException { diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql new file mode 100644 index 00000000000..68e0bca118e --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql @@ -0,0 +1,36 @@ +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.security.MvelInjection +import TestUtilities.InlineExpectationsTest + +class Conf extends TaintTracking::Configuration { + Conf() { this = "test:cwe:mvel-injection" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof MvelEvaluationSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof MvelInjectionSanitizer + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + any(MvelInjectionAdditionalTaintStep c).step(node1, node2) + } +} + +class HasMvelInjectionTest extends InlineExpectationsTest { + HasMvelInjectionTest() { this = "HasMvelInjectionTest" } + + override string getARelevantTag() { result = "hasMvelInjection" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasMvelInjection" and + exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) | + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} From ae0a00e30aebc3e4f8c326004c56082416a0dd03 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 3 Jun 2021 10:21:59 +0200 Subject: [PATCH 04/16] Added change note --- java/change-notes/2021-06-02-mvel-injection-query.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 java/change-notes/2021-06-02-mvel-injection-query.md diff --git a/java/change-notes/2021-06-02-mvel-injection-query.md b/java/change-notes/2021-06-02-mvel-injection-query.md new file mode 100644 index 00000000000..53b4634e9fe --- /dev/null +++ b/java/change-notes/2021-06-02-mvel-injection-query.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "Expression language injection (MVEL) (`java/mvel-expression-injection`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @artem-smotrakov](https://github.com/github/codeql/pull/3329) \ No newline at end of file From 56d6fc951cc5457264ac439fd0cbe8f94db6727f Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 3 Jun 2021 10:22:15 +0200 Subject: [PATCH 05/16] Fixed some QLDoc --- .../code/java/security/MvelInjection.qll | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/MvelInjection.qll b/java/ql/src/semmle/code/java/security/MvelInjection.qll index 18bb16c6a65..19f0ac00265 100644 --- a/java/ql/src/semmle/code/java/security/MvelInjection.qll +++ b/java/ql/src/semmle/code/java/security/MvelInjection.qll @@ -88,8 +88,8 @@ private predicate expressionCompilationStep(DataFlow::Node node1, DataFlow::Node } /** - * Holds if `node1` to `node2` is a dataflow step creates `ExpressionCompiler`, - * i.e. `new ExpressionCompiler(tainted)`. + * Holds if `node1` to `node2` is a dataflow step that creates `ExpressionCompiler` + * by calling `new ExpressionCompiler(tainted)`. */ private predicate createExpressionCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { exists(ConstructorCall cc | @@ -100,8 +100,8 @@ private predicate createExpressionCompilerStep(DataFlow::Node node1, DataFlow::N } /** - * Holds if `node1` to `node2` is a dataflow step creates `CompiledAccExpression`, - * i.e. `new CompiledAccExpression(tainted, ...)`. + * Holds if `node1` to `node2` is a dataflow step creates `CompiledAccExpression` + * by calling `new CompiledAccExpression(tainted, ...)`. */ private predicate createCompiledAccExpressionStep(DataFlow::Node node1, DataFlow::Node node2) { exists(ConstructorCall cc | @@ -125,8 +125,8 @@ private predicate expressionCompilerCompileStep(DataFlow::Node node1, DataFlow:: } /** - * Holds if `node1` to `node2` is a dataflow step that compiles a script via `MvelScriptEngine`, - * i.e. `engine.compile(tainted)` or `engine.compiledScript(tainted)`. + * Holds if `node1` to `node2` is a dataflow step that compiles a script via `MvelScriptEngine` + * by calling `engine.compile(tainted)` or `engine.compiledScript(tainted)`. */ private predicate scriptCompileStep(DataFlow::Node node1, DataFlow::Node node2) { exists(MethodAccess ma, Method m | ma.getMethod() = m | @@ -137,8 +137,8 @@ private predicate scriptCompileStep(DataFlow::Node node1, DataFlow::Node node2) } /** - * Holds if `node1` to `node2` is a dataflow step creates `MvelCompiledScript`, - * i.e. `new MvelCompiledScript(engine, tainted)`. + * Holds if `node1` to `node2` is a dataflow step that creates `MvelCompiledScript` + * by calling `new MvelCompiledScript(engine, tainted)`. */ private predicate createMvelCompiledScriptStep(DataFlow::Node node1, DataFlow::Node node2) { exists(ConstructorCall cc | @@ -149,8 +149,8 @@ private predicate createMvelCompiledScriptStep(DataFlow::Node node1, DataFlow::N } /** - * Holds if `node1` to `node2` is a dataflow step creates `TemplateCompiler`, - * i.e. `new TemplateCompiler(tainted)`. + * Holds if `node1` to `node2` is a dataflow step creates `TemplateCompiler` + * by calling `new TemplateCompiler(tainted)`. */ private predicate createTemplateCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { exists(ConstructorCall cc | @@ -161,8 +161,8 @@ private predicate createTemplateCompilerStep(DataFlow::Node node1, DataFlow::Nod } /** - * Holds if `node1` to `node2` is a dataflow step that compiles a script via `TemplateCompiler`, - * i.e. `compiler.compile()` or `TemplateCompiler.compileTemplate(tainted)`. + * Holds if `node1` to `node2` is a dataflow step that compiles a script via `TemplateCompiler` + * by calling `compiler.compile()` or `TemplateCompiler.compileTemplate(tainted)`. */ private predicate templateCompileStep(DataFlow::Node node1, DataFlow::Node node2) { exists(MethodAccess ma, Method m | ma.getMethod() = m | From 9cb0e3371c0831bf427a0541d2f32c8adc0fdd46 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 3 Jun 2021 10:22:42 +0200 Subject: [PATCH 06/16] Bidirectional import in ExternalFlow.qll --- java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 33a146d07fe..e84d146f892 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -82,6 +82,7 @@ private module Frameworks { private import semmle.code.java.security.XSS private import semmle.code.java.security.LdapInjection private import semmle.code.java.security.XPath + private import semmle.code.java.security.MvelInjection } private predicate sourceModelCsv(string row) { From 34a8383c1aeddf6714f854415892b722d9c1752e Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 3 Jun 2021 10:22:53 +0200 Subject: [PATCH 07/16] Unused import --- java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java index 6de9375d595..4013246eecd 100644 --- a/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java +++ b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.java @@ -15,7 +15,6 @@ import org.mvel2.compiler.ExpressionCompiler; import org.mvel2.integration.impl.ImmutableDefaultFactory; import org.mvel2.jsr223.MvelCompiledScript; import org.mvel2.jsr223.MvelScriptEngine; -import org.mvel2.templates.CompiledTemplate; import org.mvel2.templates.TemplateCompiler; import org.mvel2.templates.TemplateRuntime; From 46faf68d646bd5cb78f92bfa39a4e69e484d7738 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 19 Jul 2021 13:50:03 +0200 Subject: [PATCH 08/16] Decouple MvelInjection.qll to reuse the taint tracking configuration --- .../src/Security/CWE/CWE-094/MvelInjection.ql | 24 +--------- .../code/java/dataflow/ExternalFlow.qll | 2 +- ...elInjection.qll => MvelInjectionQuery.qll} | 47 +++++++++---------- .../java/security/MvelInjectionSinkModels.qll | 28 +++++++++++ .../query-tests/security/CWE-094/options | 4 -- .../security/CWE-094/MvelInjectionTest.ql | 22 ++------- 6 files changed, 56 insertions(+), 71 deletions(-) rename java/ql/src/semmle/code/java/security/{MvelInjection.qll => MvelInjectionQuery.qll} (84%) create mode 100644 java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll diff --git a/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql b/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql index b88b017fc7e..9a65bb5e481 100644 --- a/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql +++ b/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql @@ -11,31 +11,9 @@ */ import java -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.security.MvelInjection +import semmle.code.java.security.MvelInjectionQuery import DataFlow::PathGraph -/** - * A taint-tracking configuration for unsafe user input - * that is used to construct and evaluate a MVEL expression. - */ -class MvelInjectionFlowConfig extends TaintTracking::Configuration { - MvelInjectionFlowConfig() { this = "MvelInjectionFlowConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof MvelEvaluationSink } - - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof MvelInjectionSanitizer - } - - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - any(MvelInjectionAdditionalTaintStep c).step(node1, node2) - } -} - from DataFlow::PathNode source, DataFlow::PathNode sink, MvelInjectionFlowConfig conf where conf.hasFlowPath(source, sink) select sink.getNode(), source, sink, "MVEL injection from $@.", source.getNode(), "this user input" diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index 9c8aaa6ce65..ac9a2183658 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -98,7 +98,7 @@ private module Frameworks { private import semmle.code.java.security.InformationLeak private import semmle.code.java.security.JexlInjectionSinkModels private import semmle.code.java.security.LdapInjection - private import semmle.code.java.security.MvelInjection + private import semmle.code.java.security.MvelInjectionSinkModels private import semmle.code.java.security.XPath private import semmle.code.java.frameworks.android.SQLite private import semmle.code.java.frameworks.Jdbc diff --git a/java/ql/src/semmle/code/java/security/MvelInjection.qll b/java/ql/src/semmle/code/java/security/MvelInjectionQuery.qll similarity index 84% rename from java/ql/src/semmle/code/java/security/MvelInjection.qll rename to java/ql/src/semmle/code/java/security/MvelInjectionQuery.qll index 19f0ac00265..0681876c07c 100644 --- a/java/ql/src/semmle/code/java/security/MvelInjection.qll +++ b/java/ql/src/semmle/code/java/security/MvelInjectionQuery.qll @@ -3,6 +3,8 @@ import java import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.ExternalFlow +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.TaintTracking /** A data flow sink for unvalidated user input that is used to construct MVEL expressions. */ abstract class MvelEvaluationSink extends DataFlow::Node { } @@ -23,31 +25,6 @@ class MvelInjectionAdditionalTaintStep extends Unit { abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); } -private class DefaulMvelEvaluationSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.script;CompiledScript;false;eval;;;Argument[-1];mvel", - "org.mvel2;MVEL;false;eval;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeExpression;;;Argument[0];mvel", - "org.mvel2;MVEL;false;evalToBoolean;;;Argument[0];mvel", - "org.mvel2;MVEL;false;evalToString;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeAllExpression;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeSetExpression;;;Argument[0];mvel", - "org.mvel2;MVELRuntime;false;execute;;;Argument[1];mvel", - "org.mvel2.templates;TemplateRuntime;false;eval;;;Argument[0];mvel", - "org.mvel2.templates;TemplateRuntime;false;execute;;;Argument[0];mvel", - "org.mvel2.jsr223;MvelScriptEngine;false;eval;;;Argument[0];mvel", - "org.mvel2.jsr223;MvelScriptEngine;false;evaluate;;;Argument[0];mvel", - "org.mvel2.jsr223;MvelCompiledScript;false;eval;;;Argument[-1];mvel", - "org.mvel2.compiler;ExecutableStatement;false;getValue;;;Argument[-1];mvel", - "org.mvel2.compiler;CompiledExpression;false;getDirectValue;;;Argument[-1];mvel", - "org.mvel2.compiler;CompiledAccExpression;false;getValue;;;Argument[-1];mvel", - "org.mvel2.compiler;Accessor;false;getValue;;;Argument[-1];mvel" - ] - } -} - /** Default sink for MVEL injection vulnerabilities. */ private class DefaultMvelEvaluationSink extends MvelEvaluationSink { DefaultMvelEvaluationSink() { sinkNode(this, "mvel") } @@ -74,6 +51,26 @@ private class DefaultMvelInjectionAdditionalTaintStep extends MvelInjectionAddit } } +/** + * A taint-tracking configuration for unsafe user input + * that is used to construct and evaluate a MVEL expression. + */ +class MvelInjectionFlowConfig extends TaintTracking::Configuration { + MvelInjectionFlowConfig() { this = "MvelInjectionFlowConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof MvelEvaluationSink } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + sanitizer instanceof MvelInjectionSanitizer + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + any(MvelInjectionAdditionalTaintStep c).step(node1, node2) + } +} + /** * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression * by callilng `MVEL.compileExpression(tainted)`. diff --git a/java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll b/java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll new file mode 100644 index 00000000000..397b85ecec6 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll @@ -0,0 +1,28 @@ +/** Provides sink models relating to MVEL injection vulnerabilities. */ + +import semmle.code.java.dataflow.ExternalFlow + +private class DefaulMvelEvaluationSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.script;CompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2;MVEL;false;eval;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToBoolean;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToString;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeAllExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeSetExpression;;;Argument[0];mvel", + "org.mvel2;MVELRuntime;false;execute;;;Argument[1];mvel", + "org.mvel2.templates;TemplateRuntime;false;eval;;;Argument[0];mvel", + "org.mvel2.templates;TemplateRuntime;false;execute;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelScriptEngine;false;eval;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelScriptEngine;false;evaluate;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelCompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2.compiler;ExecutableStatement;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledExpression;false;getDirectValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledAccExpression;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;Accessor;false;getValue;;;Argument[-1];mvel" + ] + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-094/options b/java/ql/test/experimental/query-tests/security/CWE-094/options index 88c8fd2e432..647a038cd2f 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-094/options +++ b/java/ql/test/experimental/query-tests/security/CWE-094/options @@ -1,5 +1 @@ -<<<<<<< HEAD -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/springframework-5.2.3:${testdir}/../../../../stubs/jsr223-api:${testdir}/../../../../stubs/scriptengine:${testdir}/../../../../stubs/java-ee-el:${testdir}/../../../../stubs/juel-2.2:${testdir}/../../../stubs/groovy-all-3.0.7:${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jython-2.7.2:${testdir}/../../../../experimental/stubs/rhino-1.7.13 -======= //semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/springframework-5.3.8:${testdir}/../../../../stubs/jsr223-api:${testdir}/../../../../stubs/scriptengine:${testdir}/../../../../stubs/java-ee-el:${testdir}/../../../../stubs/juel-2.2:${testdir}/../../../stubs/groovy-all-3.0.7:${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jython-2.7.2:${testdir}/../../../../experimental/stubs/rhino-1.7.13:${testdir}/../../../../stubs/bsh-2.0b5:${testdir}/../../../../experimental/stubs/jshell ->>>>>>> main diff --git a/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql index 68e0bca118e..71146acfcf9 100644 --- a/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql +++ b/java/ql/test/query-tests/security/CWE-094/MvelInjectionTest.ql @@ -1,25 +1,9 @@ import java import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources -import semmle.code.java.security.MvelInjection +import semmle.code.java.security.MvelInjectionQuery import TestUtilities.InlineExpectationsTest -class Conf extends TaintTracking::Configuration { - Conf() { this = "test:cwe:mvel-injection" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof MvelEvaluationSink } - - override predicate isSanitizer(DataFlow::Node sanitizer) { - sanitizer instanceof MvelInjectionSanitizer - } - - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - any(MvelInjectionAdditionalTaintStep c).step(node1, node2) - } -} - class HasMvelInjectionTest extends InlineExpectationsTest { HasMvelInjectionTest() { this = "HasMvelInjectionTest" } @@ -27,7 +11,9 @@ class HasMvelInjectionTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasMvelInjection" and - exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) | + exists(DataFlow::Node src, DataFlow::Node sink, MvelInjectionFlowConfig conf | + conf.hasFlow(src, sink) + | sink.getLocation() = location and element = sink.toString() and value = "" From 70081b6a1e8701b62d798f23b2feb9ad0ad159b9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 19 Jul 2021 15:36:35 +0200 Subject: [PATCH 09/16] Refactor MvelInjection.qll --- .../code/java/dataflow/ExternalFlow.qll | 2 +- .../code/java/security/MvelInjection.qll | 233 ++++++++++++++++++ .../code/java/security/MvelInjectionQuery.qll | 208 +--------------- .../java/security/MvelInjectionSinkModels.qll | 28 --- 4 files changed, 236 insertions(+), 235 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/MvelInjection.qll delete mode 100644 java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll diff --git a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll index ac9a2183658..9c8aaa6ce65 100644 --- a/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll @@ -98,7 +98,7 @@ private module Frameworks { private import semmle.code.java.security.InformationLeak private import semmle.code.java.security.JexlInjectionSinkModels private import semmle.code.java.security.LdapInjection - private import semmle.code.java.security.MvelInjectionSinkModels + private import semmle.code.java.security.MvelInjection private import semmle.code.java.security.XPath private import semmle.code.java.frameworks.android.SQLite private import semmle.code.java.frameworks.Jdbc diff --git a/java/ql/src/semmle/code/java/security/MvelInjection.qll b/java/ql/src/semmle/code/java/security/MvelInjection.qll new file mode 100644 index 00000000000..516a647d19b --- /dev/null +++ b/java/ql/src/semmle/code/java/security/MvelInjection.qll @@ -0,0 +1,233 @@ +/** Provides classes to reason about MVEL injection attacks. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow + +/** A data flow sink for unvalidated user input that is used to construct MVEL expressions. */ +abstract class MvelEvaluationSink extends DataFlow::Node { } + +/** A sanitizer that prevents MVEL injection attacks. */ +abstract class MvelInjectionSanitizer extends DataFlow::Node { } + +/** + * A unit class for adding additional taint steps. + * + * Extend this class to add additional taint steps that should apply to the `MvelInjectionFlowConfig`. + */ +class MvelInjectionAdditionalTaintStep extends Unit { + /** + * Holds if the step from `node1` to `node2` should be considered a taint + * step for the `MvelInjectionFlowConfig` configuration. + */ + abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); +} + +/** Default sink for MVEL injection vulnerabilities. */ +private class DefaultMvelEvaluationSink extends MvelEvaluationSink { + DefaultMvelEvaluationSink() { sinkNode(this, "mvel") } +} + +private class DefaulMvelEvaluationSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.script;CompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2;MVEL;false;eval;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToBoolean;;;Argument[0];mvel", + "org.mvel2;MVEL;false;evalToString;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeAllExpression;;;Argument[0];mvel", + "org.mvel2;MVEL;false;executeSetExpression;;;Argument[0];mvel", + "org.mvel2;MVELRuntime;false;execute;;;Argument[1];mvel", + "org.mvel2.templates;TemplateRuntime;false;eval;;;Argument[0];mvel", + "org.mvel2.templates;TemplateRuntime;false;execute;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelScriptEngine;false;eval;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelScriptEngine;false;evaluate;;;Argument[0];mvel", + "org.mvel2.jsr223;MvelCompiledScript;false;eval;;;Argument[-1];mvel", + "org.mvel2.compiler;ExecutableStatement;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledExpression;false;getDirectValue;;;Argument[-1];mvel", + "org.mvel2.compiler;CompiledAccExpression;false;getValue;;;Argument[-1];mvel", + "org.mvel2.compiler;Accessor;false;getValue;;;Argument[-1];mvel" + ] + } +} + +/** A default sanitizer that considers numeric and boolean typed data safe for building MVEL expressions */ +private class DefaultMvelInjectionSanitizer extends MvelInjectionSanitizer { + DefaultMvelInjectionSanitizer() { + this.getType() instanceof NumericType or this.getType() instanceof BooleanType + } +} + +/** A set of additional taint steps to consider when taint tracking MVEL related data flows. */ +private class DefaultMvelInjectionAdditionalTaintStep extends MvelInjectionAdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + expressionCompilationStep(node1, node2) or + createExpressionCompilerStep(node1, node2) or + expressionCompilerCompileStep(node1, node2) or + createCompiledAccExpressionStep(node1, node2) or + scriptCompileStep(node1, node2) or + createMvelCompiledScriptStep(node1, node2) or + templateCompileStep(node1, node2) or + createTemplateCompilerStep(node1, node2) + } +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression + * by callilng `MVEL.compileExpression(tainted)`. + */ +private predicate expressionCompilationStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | + m.getDeclaringType() instanceof MVEL and + m.hasName("compileExpression") and + ma.getAnArgument() = node1.asExpr() and + node2.asExpr() = ma + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that creates `ExpressionCompiler` + * by calling `new ExpressionCompiler(tainted)`. + */ +private predicate createExpressionCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof ExpressionCompiler and + cc = node2.asExpr() and + cc.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step creates `CompiledAccExpression` + * by calling `new CompiledAccExpression(tainted, ...)`. + */ +private predicate createCompiledAccExpressionStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof CompiledAccExpression and + cc = node2.asExpr() and + cc.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression + * by calling `ExpressionCompiler.compile()`. + */ +private predicate expressionCompilerCompileStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + m.getDeclaringType() instanceof ExpressionCompiler and + m.hasName("compile") and + ma = node2.asExpr() and + ma.getQualifier() = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a script via `MvelScriptEngine` + * by calling `engine.compile(tainted)` or `engine.compiledScript(tainted)`. + */ +private predicate scriptCompileStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + m instanceof MvelScriptEngineCompilationMethod and + ma = node2.asExpr() and + ma.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that creates `MvelCompiledScript` + * by calling `new MvelCompiledScript(engine, tainted)`. + */ +private predicate createMvelCompiledScriptStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof MvelCompiledScript and + cc = node2.asExpr() and + cc.getArgument(1) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step creates `TemplateCompiler` + * by calling `new TemplateCompiler(tainted)`. + */ +private predicate createTemplateCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(ConstructorCall cc | + cc.getConstructedType() instanceof TemplateCompiler and + cc = node2.asExpr() and + cc.getArgument(0) = node1.asExpr() + ) +} + +/** + * Holds if `node1` to `node2` is a dataflow step that compiles a script via `TemplateCompiler` + * by calling `compiler.compile()` or `TemplateCompiler.compileTemplate(tainted)`. + */ +private predicate templateCompileStep(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + m instanceof TemplateCompilerCompileMethod and + ma.getQualifier() = node1.asExpr() and + ma = node2.asExpr() + ) + or + exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | + m instanceof TemplateCompilerCompileTemplateMethod and + ma = node2.asExpr() and + ma.getArgument(0) = node1.asExpr() + ) +} + +/** + * Methods in `MvelScriptEngine` that compile a MVEL expression. + */ +private class MvelScriptEngineCompilationMethod extends Method { + MvelScriptEngineCompilationMethod() { + getDeclaringType() instanceof MvelScriptEngine and + (hasName("compile") or hasName("compiledScript")) + } +} + +/** + * `TemplateCompiler.compile()` method that compiles a MVEL template. + */ +private class TemplateCompilerCompileMethod extends Method { + TemplateCompilerCompileMethod() { + getDeclaringType() instanceof TemplateCompiler and + hasName("compile") + } +} + +/** + * `TemplateCompiler.compileTemplate(tainted)` static method that compiles a MVEL template. + */ +private class TemplateCompilerCompileTemplateMethod extends Method { + TemplateCompilerCompileTemplateMethod() { + getDeclaringType() instanceof TemplateCompiler and + hasName("compileTemplate") + } +} + +private class MVEL extends RefType { + MVEL() { hasQualifiedName("org.mvel2", "MVEL") } +} + +private class ExpressionCompiler extends RefType { + ExpressionCompiler() { hasQualifiedName("org.mvel2.compiler", "ExpressionCompiler") } +} + +private class CompiledAccExpression extends RefType { + CompiledAccExpression() { hasQualifiedName("org.mvel2.compiler", "CompiledAccExpression") } +} + +private class MvelScriptEngine extends RefType { + MvelScriptEngine() { hasQualifiedName("org.mvel2.jsr223", "MvelScriptEngine") } +} + +private class MvelCompiledScript extends RefType { + MvelCompiledScript() { hasQualifiedName("org.mvel2.jsr223", "MvelCompiledScript") } +} + +private class TemplateCompiler extends RefType { + TemplateCompiler() { hasQualifiedName("org.mvel2.templates", "TemplateCompiler") } +} diff --git a/java/ql/src/semmle/code/java/security/MvelInjectionQuery.qll b/java/ql/src/semmle/code/java/security/MvelInjectionQuery.qll index 0681876c07c..332ea387e6b 100644 --- a/java/ql/src/semmle/code/java/security/MvelInjectionQuery.qll +++ b/java/ql/src/semmle/code/java/security/MvelInjectionQuery.qll @@ -1,55 +1,9 @@ -/** Provides classes to reason about MVEL injection attacks. */ +/** Provides taint tracking configurations to be used in MVEL injection related queries. */ import java -import semmle.code.java.dataflow.DataFlow -import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking - -/** A data flow sink for unvalidated user input that is used to construct MVEL expressions. */ -abstract class MvelEvaluationSink extends DataFlow::Node { } - -/** A sanitizer that prevents MVEL injection attacks. */ -abstract class MvelInjectionSanitizer extends DataFlow::Node { } - -/** - * A unit class for adding additional taint steps. - * - * Extend this class to add additional taint steps that should apply to the `MvelInjectionFlowConfig`. - */ -class MvelInjectionAdditionalTaintStep extends Unit { - /** - * Holds if the step from `node1` to `node2` should be considered a taint - * step for the `MvelInjectionFlowConfig` configuration. - */ - abstract predicate step(DataFlow::Node n1, DataFlow::Node n2); -} - -/** Default sink for MVEL injection vulnerabilities. */ -private class DefaultMvelEvaluationSink extends MvelEvaluationSink { - DefaultMvelEvaluationSink() { sinkNode(this, "mvel") } -} - -/** A default sanitizer that considers numeric and boolean typed data safe for building MVEL expressions */ -private class DefaultMvelInjectionSanitizer extends MvelInjectionSanitizer { - DefaultMvelInjectionSanitizer() { - this.getType() instanceof NumericType or this.getType() instanceof BooleanType - } -} - -/** A set of additional taint steps to consider when taint tracking MVEL related data flows. */ -private class DefaultMvelInjectionAdditionalTaintStep extends MvelInjectionAdditionalTaintStep { - override predicate step(DataFlow::Node node1, DataFlow::Node node2) { - expressionCompilationStep(node1, node2) or - createExpressionCompilerStep(node1, node2) or - expressionCompilerCompileStep(node1, node2) or - createCompiledAccExpressionStep(node1, node2) or - scriptCompileStep(node1, node2) or - createMvelCompiledScriptStep(node1, node2) or - templateCompileStep(node1, node2) or - createTemplateCompilerStep(node1, node2) - } -} +import semmle.code.java.security.MvelInjection /** * A taint-tracking configuration for unsafe user input @@ -70,161 +24,3 @@ class MvelInjectionFlowConfig extends TaintTracking::Configuration { any(MvelInjectionAdditionalTaintStep c).step(node1, node2) } } - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression - * by callilng `MVEL.compileExpression(tainted)`. - */ -private predicate expressionCompilationStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | - m.getDeclaringType() instanceof MVEL and - m.hasName("compileExpression") and - ma.getAnArgument() = node1.asExpr() and - node2.asExpr() = ma - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that creates `ExpressionCompiler` - * by calling `new ExpressionCompiler(tainted)`. - */ -private predicate createExpressionCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof ExpressionCompiler and - cc = node2.asExpr() and - cc.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step creates `CompiledAccExpression` - * by calling `new CompiledAccExpression(tainted, ...)`. - */ -private predicate createCompiledAccExpressionStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof CompiledAccExpression and - cc = node2.asExpr() and - cc.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a MVEL expression - * by calling `ExpressionCompiler.compile()`. - */ -private predicate expressionCompilerCompileStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - m.getDeclaringType() instanceof ExpressionCompiler and - m.hasName("compile") and - ma = node2.asExpr() and - ma.getQualifier() = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a script via `MvelScriptEngine` - * by calling `engine.compile(tainted)` or `engine.compiledScript(tainted)`. - */ -private predicate scriptCompileStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - m instanceof MvelScriptEngineCompilationMethod and - ma = node2.asExpr() and - ma.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that creates `MvelCompiledScript` - * by calling `new MvelCompiledScript(engine, tainted)`. - */ -private predicate createMvelCompiledScriptStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof MvelCompiledScript and - cc = node2.asExpr() and - cc.getArgument(1) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step creates `TemplateCompiler` - * by calling `new TemplateCompiler(tainted)`. - */ -private predicate createTemplateCompilerStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(ConstructorCall cc | - cc.getConstructedType() instanceof TemplateCompiler and - cc = node2.asExpr() and - cc.getArgument(0) = node1.asExpr() - ) -} - -/** - * Holds if `node1` to `node2` is a dataflow step that compiles a script via `TemplateCompiler` - * by calling `compiler.compile()` or `TemplateCompiler.compileTemplate(tainted)`. - */ -private predicate templateCompileStep(DataFlow::Node node1, DataFlow::Node node2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - m instanceof TemplateCompilerCompileMethod and - ma.getQualifier() = node1.asExpr() and - ma = node2.asExpr() - ) - or - exists(StaticMethodAccess ma, Method m | ma.getMethod() = m | - m instanceof TemplateCompilerCompileTemplateMethod and - ma = node2.asExpr() and - ma.getArgument(0) = node1.asExpr() - ) -} - -/** - * Methods in `MvelScriptEngine` that compile a MVEL expression. - */ -private class MvelScriptEngineCompilationMethod extends Method { - MvelScriptEngineCompilationMethod() { - getDeclaringType() instanceof MvelScriptEngine and - (hasName("compile") or hasName("compiledScript")) - } -} - -/** - * `TemplateCompiler.compile()` method that compiles a MVEL template. - */ -private class TemplateCompilerCompileMethod extends Method { - TemplateCompilerCompileMethod() { - getDeclaringType() instanceof TemplateCompiler and - hasName("compile") - } -} - -/** - * `TemplateCompiler.compileTemplate(tainted)` static method that compiles a MVEL template. - */ -private class TemplateCompilerCompileTemplateMethod extends Method { - TemplateCompilerCompileTemplateMethod() { - getDeclaringType() instanceof TemplateCompiler and - hasName("compileTemplate") - } -} - -private class MVEL extends RefType { - MVEL() { hasQualifiedName("org.mvel2", "MVEL") } -} - -private class ExpressionCompiler extends RefType { - ExpressionCompiler() { hasQualifiedName("org.mvel2.compiler", "ExpressionCompiler") } -} - -private class CompiledAccExpression extends RefType { - CompiledAccExpression() { hasQualifiedName("org.mvel2.compiler", "CompiledAccExpression") } -} - -private class MvelScriptEngine extends RefType { - MvelScriptEngine() { hasQualifiedName("org.mvel2.jsr223", "MvelScriptEngine") } -} - -private class MvelCompiledScript extends RefType { - MvelCompiledScript() { hasQualifiedName("org.mvel2.jsr223", "MvelCompiledScript") } -} - -private class TemplateCompiler extends RefType { - TemplateCompiler() { hasQualifiedName("org.mvel2.templates", "TemplateCompiler") } -} diff --git a/java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll b/java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll deleted file mode 100644 index 397b85ecec6..00000000000 --- a/java/ql/src/semmle/code/java/security/MvelInjectionSinkModels.qll +++ /dev/null @@ -1,28 +0,0 @@ -/** Provides sink models relating to MVEL injection vulnerabilities. */ - -import semmle.code.java.dataflow.ExternalFlow - -private class DefaulMvelEvaluationSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.script;CompiledScript;false;eval;;;Argument[-1];mvel", - "org.mvel2;MVEL;false;eval;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeExpression;;;Argument[0];mvel", - "org.mvel2;MVEL;false;evalToBoolean;;;Argument[0];mvel", - "org.mvel2;MVEL;false;evalToString;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeAllExpression;;;Argument[0];mvel", - "org.mvel2;MVEL;false;executeSetExpression;;;Argument[0];mvel", - "org.mvel2;MVELRuntime;false;execute;;;Argument[1];mvel", - "org.mvel2.templates;TemplateRuntime;false;eval;;;Argument[0];mvel", - "org.mvel2.templates;TemplateRuntime;false;execute;;;Argument[0];mvel", - "org.mvel2.jsr223;MvelScriptEngine;false;eval;;;Argument[0];mvel", - "org.mvel2.jsr223;MvelScriptEngine;false;evaluate;;;Argument[0];mvel", - "org.mvel2.jsr223;MvelCompiledScript;false;eval;;;Argument[-1];mvel", - "org.mvel2.compiler;ExecutableStatement;false;getValue;;;Argument[-1];mvel", - "org.mvel2.compiler;CompiledExpression;false;getDirectValue;;;Argument[-1];mvel", - "org.mvel2.compiler;CompiledAccExpression;false;getValue;;;Argument[-1];mvel", - "org.mvel2.compiler;Accessor;false;getValue;;;Argument[-1];mvel" - ] - } -} From 68df8028d21c289e00cf618862b9368cbffa838d Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 20 Jul 2021 14:47:16 +0200 Subject: [PATCH 10/16] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- java/ql/src/semmle/code/java/security/MvelInjection.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/MvelInjection.qll b/java/ql/src/semmle/code/java/security/MvelInjection.qll index 516a647d19b..984641fbd18 100644 --- a/java/ql/src/semmle/code/java/security/MvelInjection.qll +++ b/java/ql/src/semmle/code/java/security/MvelInjection.qll @@ -1,8 +1,8 @@ /** Provides classes to reason about MVEL injection attacks. */ import java -import semmle.code.java.dataflow.DataFlow -import semmle.code.java.dataflow.ExternalFlow +private import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.ExternalFlow /** A data flow sink for unvalidated user input that is used to construct MVEL expressions. */ abstract class MvelEvaluationSink extends DataFlow::Node { } @@ -184,7 +184,7 @@ private predicate templateCompileStep(DataFlow::Node node1, DataFlow::Node node2 private class MvelScriptEngineCompilationMethod extends Method { MvelScriptEngineCompilationMethod() { getDeclaringType() instanceof MvelScriptEngine and - (hasName("compile") or hasName("compiledScript")) + hasName(["compile", "compiledScript"]) } } From 8f1fc9e893c57ef3091363aa1c8644e3f04c8177 Mon Sep 17 00:00:00 2001 From: mc <42146119+mchammer01@users.noreply.github.com> Date: Thu, 29 Jul 2021 11:30:19 +0100 Subject: [PATCH 11/16] Update MvelInjection.qhelp Minor tweaks --- java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp b/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp index d68d298b5f5..b42ab142fd8 100644 --- a/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp @@ -3,11 +3,11 @@

-MVEL is an expression language based on Java-syntax. -The language offers many features +MVEL is an expression language based on Java-syntax, +which offers many features including invocation of methods available in the JVM. If a MVEL expression is built using attacker-controlled data, -and then evaluated, then it may allow the attacker to run arbitrary code. +and then evaluated, then it may allow attackers to run arbitrary code.

@@ -35,4 +35,4 @@ and then runs it in the default powerfull context. Expression Language Injection. - \ No newline at end of file + From a08217242221eb51757dde3060698ccf78fd731a Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 29 Jul 2021 14:46:32 +0200 Subject: [PATCH 12/16] C++: Add testcase demonstrating missing local flow out of fields that are defined by reference. --- .../dataflow/dataflow-tests/localFlow.expected | 4 ++++ .../library-tests/dataflow/dataflow-tests/test.cpp | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected index 75cdaee69ac..7cf6d412b0e 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected @@ -70,3 +70,7 @@ | test.cpp:391:11:391:13 | tmp | test.cpp:391:10:391:13 | & ... | | test.cpp:391:17:391:23 | source1 | test.cpp:391:10:391:13 | ref arg & ... | | test.cpp:391:17:391:23 | source1 | test.cpp:391:16:391:23 | & ... | +| test.cpp:480:67:480:67 | s | test.cpp:481:21:481:21 | s | +| test.cpp:480:67:480:67 | s | test.cpp:482:20:482:20 | s | +| test.cpp:481:21:481:21 | s [post update] | test.cpp:482:20:482:20 | s | +| test.cpp:482:23:482:29 | content | test.cpp:483:9:483:17 | p_content | diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp index f59552aa2dd..afceddfabd0 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp @@ -470,3 +470,15 @@ void viaOutparam() { intOutparamSource(&x); sink(x); // $ ast,ir } + +void writes_to_content(void*); + +struct MyStruct { + int* content; +}; + +void local_field_flow_def_by_ref_steps_with_local_flow(MyStruct * s) { + writes_to_content(s->content); + int* p_content = s->content; + sink(*p_content); +} \ No newline at end of file From 41d233f086a9e5db31bf4486e6694cd5816064ec Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 29 Jul 2021 14:49:59 +0200 Subject: [PATCH 13/16] C++: Make the 'definition by reference'-node in 'foo(a.b);' a source in the 'FieldConfiguration' configuration. --- .../src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll | 7 ++++++- .../dataflow/dataflow-tests/localFlow.expected | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll index 0d723ac05b6..01338eaeff4 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll @@ -735,7 +735,12 @@ private module FieldFlow { private class FieldConfiguration extends Configuration { FieldConfiguration() { this = "FieldConfiguration" } - override predicate isSource(Node source) { storeStep(source, _, _) } + override predicate isSource(Node source) { + storeStep(source, _, _) + or + // Also mark `foo(a.b);` as a source when `a.b` may be overwritten by `foo`. + readStep(_, _, any(Node node | node.asExpr() = source.asDefiningArgument())) + } override predicate isSink(Node sink) { readStep(_, _, sink) } diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected index 7cf6d412b0e..bb14729a12b 100644 --- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected +++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected @@ -73,4 +73,5 @@ | test.cpp:480:67:480:67 | s | test.cpp:481:21:481:21 | s | | test.cpp:480:67:480:67 | s | test.cpp:482:20:482:20 | s | | test.cpp:481:21:481:21 | s [post update] | test.cpp:482:20:482:20 | s | +| test.cpp:481:24:481:30 | ref arg content | test.cpp:482:23:482:29 | content | | test.cpp:482:23:482:29 | content | test.cpp:483:9:483:17 | p_content | From bbb38fd2aae61292a043aa898b364552372409f4 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 29 Jul 2021 15:49:50 +0200 Subject: [PATCH 14/16] C++: Accept more test changes. --- .../dataflow/taint-tests/localTaint.expected | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index ce939661c92..27cfbb6430a 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -496,9 +496,13 @@ | map.cpp:49:7:49:7 | f [post update] | map.cpp:51:7:51:7 | f | | | map.cpp:49:7:49:7 | f [post update] | map.cpp:53:30:53:30 | f | | | map.cpp:49:7:49:7 | f [post update] | map.cpp:59:6:59:6 | f | | +| map.cpp:49:9:49:13 | ref arg first | map.cpp:54:9:54:13 | first | | +| map.cpp:49:9:49:13 | ref arg first | map.cpp:60:9:60:13 | first | | | map.cpp:50:7:50:7 | f [post update] | map.cpp:51:7:51:7 | f | | | map.cpp:50:7:50:7 | f [post update] | map.cpp:53:30:53:30 | f | | | map.cpp:50:7:50:7 | f [post update] | map.cpp:59:6:59:6 | f | | +| map.cpp:50:9:50:14 | ref arg second | map.cpp:55:9:55:14 | second | | +| map.cpp:50:9:50:14 | ref arg second | map.cpp:61:9:61:14 | second | | | map.cpp:53:30:53:30 | f | map.cpp:54:7:54:7 | g | | | map.cpp:53:30:53:30 | f | map.cpp:55:7:55:7 | g | | | map.cpp:53:30:53:30 | f | map.cpp:56:7:56:7 | g | | @@ -3395,6 +3399,7 @@ | smart_pointer.cpp:125:20:125:20 | call to operator-> [post update] | smart_pointer.cpp:125:18:125:19 | ref arg p1 | TAINT | | smart_pointer.cpp:125:22:125:22 | q | smart_pointer.cpp:125:18:125:22 | call to shared_ptr | | | smart_pointer.cpp:125:22:125:22 | ref arg q | smart_pointer.cpp:125:22:125:22 | q [inner post update] | | +| smart_pointer.cpp:125:22:125:22 | ref arg q | smart_pointer.cpp:126:12:126:12 | q | | | smart_pointer.cpp:126:8:126:9 | p1 | smart_pointer.cpp:126:10:126:10 | call to operator-> | | | smart_pointer.cpp:126:8:126:9 | ref arg p1 | smart_pointer.cpp:124:48:124:49 | p1 | | | smart_pointer.cpp:126:10:126:10 | call to operator-> [post update] | smart_pointer.cpp:126:8:126:9 | ref arg p1 | TAINT | @@ -3432,6 +3437,7 @@ | smart_pointer.cpp:133:23:133:24 | ref arg p1 | smart_pointer.cpp:132:53:132:54 | p1 | | | smart_pointer.cpp:133:23:133:24 | ref arg p1 | smart_pointer.cpp:134:8:134:9 | p1 | | | smart_pointer.cpp:133:25:133:25 | call to operator-> [post update] | smart_pointer.cpp:133:23:133:24 | ref arg p1 | TAINT | +| smart_pointer.cpp:133:27:133:27 | ref arg q | smart_pointer.cpp:134:12:134:12 | q | | | smart_pointer.cpp:134:8:134:9 | p1 | smart_pointer.cpp:134:10:134:10 | call to operator-> | | | smart_pointer.cpp:134:8:134:9 | ref arg p1 | smart_pointer.cpp:132:53:132:54 | p1 | | | smart_pointer.cpp:134:10:134:10 | call to operator-> [post update] | smart_pointer.cpp:134:8:134:9 | ref arg p1 | TAINT | @@ -6435,6 +6441,7 @@ | taint.cpp:669:18:669:18 | s [post update] | taint.cpp:671:7:671:7 | s | | | taint.cpp:669:18:669:18 | s [post update] | taint.cpp:672:7:672:7 | s | | | taint.cpp:669:18:669:18 | s [post update] | taint.cpp:673:7:673:7 | s | | +| taint.cpp:669:20:669:20 | ref arg x | taint.cpp:672:9:672:9 | x | | | taint.cpp:672:7:672:7 | s [post update] | taint.cpp:673:7:673:7 | s | | | vector.cpp:16:43:16:49 | source1 | vector.cpp:17:26:17:32 | source1 | | | vector.cpp:16:43:16:49 | source1 | vector.cpp:31:38:31:44 | source1 | | @@ -7076,14 +7083,20 @@ | vector.cpp:198:3:198:4 | ee [post update] | vector.cpp:200:3:200:4 | ee | | | vector.cpp:198:3:198:4 | ee [post update] | vector.cpp:201:8:201:9 | ee | | | vector.cpp:198:3:198:4 | ee [post update] | vector.cpp:202:2:202:2 | ee | | +| vector.cpp:198:6:198:7 | ref arg vs | vector.cpp:199:11:199:12 | vs | | +| vector.cpp:198:6:198:7 | ref arg vs | vector.cpp:200:6:200:7 | vs | | +| vector.cpp:198:6:198:7 | ref arg vs | vector.cpp:201:11:201:12 | vs | | | vector.cpp:198:19:198:19 | 0 | vector.cpp:198:6:198:7 | ref arg vs | TAINT | | vector.cpp:199:8:199:9 | ee [post update] | vector.cpp:200:3:200:4 | ee | | | vector.cpp:199:8:199:9 | ee [post update] | vector.cpp:201:8:201:9 | ee | | | vector.cpp:199:8:199:9 | ee [post update] | vector.cpp:202:2:202:2 | ee | | +| vector.cpp:199:11:199:12 | ref arg vs | vector.cpp:200:6:200:7 | vs | | +| vector.cpp:199:11:199:12 | ref arg vs | vector.cpp:201:11:201:12 | vs | | | vector.cpp:199:11:199:12 | vs | vector.cpp:199:13:199:13 | call to operator[] | TAINT | | vector.cpp:200:3:200:4 | ee [post update] | vector.cpp:201:8:201:9 | ee | | | vector.cpp:200:3:200:4 | ee [post update] | vector.cpp:202:2:202:2 | ee | | | vector.cpp:200:3:200:21 | ... = ... | vector.cpp:200:8:200:8 | call to operator[] [post update] | | +| vector.cpp:200:6:200:7 | ref arg vs | vector.cpp:201:11:201:12 | vs | | | vector.cpp:200:6:200:7 | vs | vector.cpp:200:8:200:8 | call to operator[] | TAINT | | vector.cpp:200:8:200:8 | call to operator[] [post update] | vector.cpp:200:6:200:7 | ref arg vs | TAINT | | vector.cpp:200:14:200:19 | call to source | vector.cpp:200:3:200:21 | ... = ... | | From 90b5e02b6e7c87bf5fcab4aa7e31ceadb51813b7 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 16:28:10 +0200 Subject: [PATCH 15/16] Improve qhelp --- .../CWE/CWE-094/MvelExpressionEvaluation.java | 25 +++++++++++++++++++ .../Security/CWE/CWE-094/MvelInjection.qhelp | 6 +++-- .../UnsafeMvelExpressionEvaluation.java | 8 ------ 3 files changed, 29 insertions(+), 10 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-094/MvelExpressionEvaluation.java delete mode 100644 java/ql/src/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java diff --git a/java/ql/src/Security/CWE/CWE-094/MvelExpressionEvaluation.java b/java/ql/src/Security/CWE/CWE-094/MvelExpressionEvaluation.java new file mode 100644 index 00000000000..b8ec0a4db4d --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-094/MvelExpressionEvaluation.java @@ -0,0 +1,25 @@ +public void evaluate(Socket socket) throws IOException { + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(socket.getInputStream()))) { + + String expression = reader.readLine(); + // BAD: the user-provided expression is directly evaluated + MVEL.eval(expression); + } +} + +public void safeEvaluate(Socket socket) throws IOException { + try (BufferedReader reader = new BufferedReader( + new InputStreamReader(socket.getInputStream()))) { + + String expression = reader.readLine(); + // GOOD: the user-provided expression is validated before evaluation + validateExpression(expression); + MVEL.eval(expression); + } +} + +private void validateExpression(String expression) { + // Validate that the expression does not contain unexpected code. + // For instance, this can be done with allow-lists or deny-lists of code patterns. +} \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp b/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp index b42ab142fd8..f5354699b6b 100644 --- a/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp @@ -19,8 +19,10 @@ Including user input in a MVEL expression should be avoided.

-The following example uses untrusted data to build a MVEL expression -and then runs it in the default powerfull context. +In the following sample, the first example uses untrusted data to build a MVEL expression +and then runs it in the default context. In the second example, the untrusted data is +validated with a custom method that checks that the expression does not contain unexpected code +before evaluating it.

diff --git a/java/ql/src/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java b/java/ql/src/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java deleted file mode 100644 index 4942bee79f6..00000000000 --- a/java/ql/src/Security/CWE/CWE-094/UnsafeMvelExpressionEvaluation.java +++ /dev/null @@ -1,8 +0,0 @@ -public void evaluate(Socket socket) throws IOException { - try (BufferedReader reader = new BufferedReader( - new InputStreamReader(socket.getInputStream()))) { - - String expression = reader.readLine(); - MVEL.eval(expression); - } -} \ No newline at end of file From 9fadb26325beef6f4453cc8f802fcbf9d9cfebe9 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 2 Aug 2021 10:00:59 +0200 Subject: [PATCH 16/16] Fix qhelp sample --- java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp b/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp index f5354699b6b..ad303285920 100644 --- a/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/MvelInjection.qhelp @@ -24,7 +24,7 @@ and then runs it in the default context. In the second example, the untrusted da validated with a custom method that checks that the expression does not contain unexpected code before evaluating it.

- +