From 46faf68d646bd5cb78f92bfa39a4e69e484d7738 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 19 Jul 2021 13:50:03 +0200 Subject: [PATCH] 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 = ""