From c792567904e7e833d62a0be28a025d05f77cf1db Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 14 Jun 2021 15:11:23 +0200 Subject: [PATCH 01/10] Move from experimental --- .../{experimental => }/Security/CWE/CWE-074/XsltInjection.java | 0 .../{experimental => }/Security/CWE/CWE-074/XsltInjection.qhelp | 0 .../{experimental => }/Security/CWE/CWE-074/XsltInjection.ql | 0 .../Security/CWE/CWE-074/XsltInjectionLib.qll | 0 .../query-tests/security/CWE-074/XsltInjection.qlref | 1 - java/ql/test/experimental/query-tests/security/CWE-074/options | 1 - .../query-tests/security/CWE-074/XsltInjection.expected | 0 .../query-tests/security/CWE-074/XsltInjection.java | 0 java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref | 1 + java/ql/test/query-tests/security/CWE-074/options | 2 +- .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/Configuration.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/lib/SourceResolver.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/om/NotationSet.java | 0 .../net/sf/saxon/s9api/AbstractXsltTransformer.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Destination.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Processor.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/QName.java | 0 .../Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiException.java | 0 .../net/sf/saxon/s9api/SaxonApiUncheckedException.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmItem.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmValue.java | 0 .../Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Xslt30Transformer.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltCompiler.java | 0 .../Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltExecutable.java | 0 .../stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltPackage.java | 0 .../Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltTransformer.java | 0 26 files changed, 2 insertions(+), 3 deletions(-) rename java/ql/src/{experimental => }/Security/CWE/CWE-074/XsltInjection.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-074/XsltInjection.qhelp (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-074/XsltInjection.ql (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-074/XsltInjectionLib.qll (100%) delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.qlref delete mode 100644 java/ql/test/experimental/query-tests/security/CWE-074/options rename java/ql/test/{experimental => }/query-tests/security/CWE-074/XsltInjection.expected (100%) rename java/ql/test/{experimental => }/query-tests/security/CWE-074/XsltInjection.java (100%) create mode 100644 java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/Configuration.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/lib/SourceResolver.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/om/NotationSet.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/AbstractXsltTransformer.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Destination.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Processor.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/QName.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiException.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiUncheckedException.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmItem.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmValue.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Xslt30Transformer.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltCompiler.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltExecutable.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltPackage.java (100%) rename java/ql/test/{experimental => }/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltTransformer.java (100%) diff --git a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.java b/java/ql/src/Security/CWE/CWE-074/XsltInjection.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.java rename to java/ql/src/Security/CWE/CWE-074/XsltInjection.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.qhelp b/java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.qhelp rename to java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp diff --git a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.ql b/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-074/XsltInjection.ql rename to java/ql/src/Security/CWE/CWE-074/XsltInjection.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll b/java/ql/src/Security/CWE/CWE-074/XsltInjectionLib.qll similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-074/XsltInjectionLib.qll rename to java/ql/src/Security/CWE/CWE-074/XsltInjectionLib.qll diff --git a/java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.qlref b/java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.qlref deleted file mode 100644 index eb4c280c5df4..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.qlref +++ /dev/null @@ -1 +0,0 @@ -experimental/Security/CWE/CWE-074/XsltInjection.ql diff --git a/java/ql/test/experimental/query-tests/security/CWE-074/options b/java/ql/test/experimental/query-tests/security/CWE-074/options deleted file mode 100644 index 599f2f5f14aa..000000000000 --- a/java/ql/test/experimental/query-tests/security/CWE-074/options +++ /dev/null @@ -1 +0,0 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/springframework-5.3.8:${testdir}/../../../stubs/Saxon-HE-9.9.1-7 diff --git a/java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.expected b/java/ql/test/query-tests/security/CWE-074/XsltInjection.expected similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.expected rename to java/ql/test/query-tests/security/CWE-074/XsltInjection.expected diff --git a/java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.java b/java/ql/test/query-tests/security/CWE-074/XsltInjection.java similarity index 100% rename from java/ql/test/experimental/query-tests/security/CWE-074/XsltInjection.java rename to java/ql/test/query-tests/security/CWE-074/XsltInjection.java diff --git a/java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref b/java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref new file mode 100644 index 000000000000..9d7fb59ac2c2 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-074/XsltInjection.ql diff --git a/java/ql/test/query-tests/security/CWE-074/options b/java/ql/test/query-tests/security/CWE-074/options index efb24fabff15..a015486520ef 100644 --- a/java/ql/test/query-tests/security/CWE-074/options +++ b/java/ql/test/query-tests/security/CWE-074/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8:${testdir}/../../../stubs/shiro-core-1.5.2:${testdir}/../../../stubs/spring-ldap-2.3.2 +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8:${testdir}/../../../stubs/shiro-core-1.5.2:${testdir}/../../../stubs/spring-ldap-2.3.2:${testdir}/../../../stubs/Saxon-HE-9.9.1-7 diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/Configuration.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/Configuration.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/Configuration.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/Configuration.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/lib/SourceResolver.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/lib/SourceResolver.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/lib/SourceResolver.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/lib/SourceResolver.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/om/NotationSet.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/om/NotationSet.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/om/NotationSet.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/om/NotationSet.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/AbstractXsltTransformer.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/AbstractXsltTransformer.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/AbstractXsltTransformer.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/AbstractXsltTransformer.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Destination.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Destination.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Destination.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Destination.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Processor.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Processor.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Processor.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Processor.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/QName.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/QName.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/QName.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/QName.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiException.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiException.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiException.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiException.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiUncheckedException.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiUncheckedException.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiUncheckedException.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/SaxonApiUncheckedException.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmItem.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmItem.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmItem.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmItem.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmValue.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmValue.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmValue.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XdmValue.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Xslt30Transformer.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Xslt30Transformer.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Xslt30Transformer.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/Xslt30Transformer.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltCompiler.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltCompiler.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltCompiler.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltCompiler.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltExecutable.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltExecutable.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltExecutable.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltExecutable.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltPackage.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltPackage.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltPackage.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltPackage.java diff --git a/java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltTransformer.java b/java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltTransformer.java similarity index 100% rename from java/ql/test/experimental/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltTransformer.java rename to java/ql/test/stubs/Saxon-HE-9.9.1-7/net/sf/saxon/s9api/XsltTransformer.java From d8bb5273e71e60790bbf24e345a01058c0313c76 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 14 Jun 2021 15:37:38 +0200 Subject: [PATCH 02/10] Refactor to use CSV sink models --- .../src/Security/CWE/CWE-074/XsltInjection.ql | 21 +- .../code/java/security/XsltInjection.qll} | 230 ++++++++---------- 2 files changed, 118 insertions(+), 133 deletions(-) rename java/ql/src/{Security/CWE/CWE-074/XsltInjectionLib.qll => semmle/code/java/security/XsltInjection.qll} (60%) diff --git a/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql b/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql index 1403573e4b19..cd47d69470fe 100644 --- a/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql +++ b/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql @@ -12,9 +12,28 @@ import java import semmle.code.java.dataflow.FlowSources -import XsltInjectionLib +import semmle.code.java.security.XsltInjection import DataFlow::PathGraph +/** + * A taint-tracking configuration for unvalidated user input that is used in XSLT transformation. + */ +class XsltInjectionFlowConfig extends TaintTracking::Configuration { + XsltInjectionFlowConfig() { this = "XsltInjectionFlowConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof XsltInjectionSink } + + override predicate isSanitizer(DataFlow::Node node) { + node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + any(XsltInjectionAdditionalTaintStep c).step(node1, node2) + } +} + from DataFlow::PathNode source, DataFlow::PathNode sink, XsltInjectionFlowConfig conf where conf.hasFlowPath(source, sink) select sink.getNode(), source, sink, "XSLT transformation might include stylesheet from $@.", diff --git a/java/ql/src/Security/CWE/CWE-074/XsltInjectionLib.qll b/java/ql/src/semmle/code/java/security/XsltInjection.qll similarity index 60% rename from java/ql/src/Security/CWE/CWE-074/XsltInjectionLib.qll rename to java/ql/src/semmle/code/java/security/XsltInjection.qll index 4ba0eb6d0b11..4db9f7afd566 100644 --- a/java/ql/src/Security/CWE/CWE-074/XsltInjectionLib.qll +++ b/java/ql/src/semmle/code/java/security/XsltInjection.qll @@ -1,23 +1,51 @@ +/** Provides classes to reason about XSLT injection vulnerabilities. */ + import java -import semmle.code.java.dataflow.FlowSources +import semmle.code.java.dataflow.ExternalFlow import semmle.code.java.security.XmlParsers -import DataFlow +import semmle.code.java.dataflow.DataFlow /** - * A taint-tracking configuration for unvalidated user input that is used in XSLT transformation. + * A data flow sink for unvalidated user input that is used in XSLT transformation. + * Extend this class to add your own XSLT Injection sinks. */ -class XsltInjectionFlowConfig extends TaintTracking::Configuration { - XsltInjectionFlowConfig() { this = "XsltInjectionFlowConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } +abstract class XsltInjectionSink extends DataFlow::Node { } + +private class DefaultXsltInjectionSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt" + ] + } +} - override predicate isSink(DataFlow::Node sink) { sink instanceof XsltInjectionSink } +/** A default sink representing methods susceptible to XSLT Injection attacks. */ +private class DefaultXsltInjectionSink extends XsltInjectionSink { + DefaultXsltInjectionSink() { sinkNode(this, "xslt") } +} - override predicate isSanitizer(DataFlow::Node node) { - node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType - } +/** + * A unit class for adding additional taint steps. + * + * Extend this class to add additional taint steps that should apply to the `XsltInjectionFlowConfig`. + */ +class XsltInjectionAdditionalTaintStep extends Unit { + /** + * Holds if the step from `node1` to `node2` should be considered a taint + * step for the `XsltInjectionFlowConfig` configuration. + */ + abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); +} - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { +/** A set of additional taint steps to consider when taint tracking XSLT related data flows. */ +private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { xmlStreamReaderStep(node1, node2) or xmlEventReaderStep(node1, node2) or staxSourceStep(node1, node2) or @@ -31,95 +59,11 @@ class XsltInjectionFlowConfig extends TaintTracking::Configuration { } } -/** The class `javax.xml.transform.stax.StAXSource`. */ -class TypeStAXSource extends Class { - TypeStAXSource() { this.hasQualifiedName("javax.xml.transform.stax", "StAXSource") } -} - -/** The class `javax.xml.transform.dom.DOMSource`. */ -class TypeDOMSource extends Class { - TypeDOMSource() { this.hasQualifiedName("javax.xml.transform.dom", "DOMSource") } -} - -/** The interface `javax.xml.transform.Templates`. */ -class TypeTemplates extends Interface { - TypeTemplates() { this.hasQualifiedName("javax.xml.transform", "Templates") } -} - -/** The method `net.sf.saxon.s9api.XsltTransformer.transform`. */ -class XsltTransformerTransformMethod extends Method { - XsltTransformerTransformMethod() { - this.getDeclaringType().hasQualifiedName("net.sf.saxon.s9api", "XsltTransformer") and - this.hasName("transform") - } -} - -/** The method `net.sf.saxon.s9api.Xslt30Transformer.transform`. */ -class Xslt30TransformerTransformMethod extends Method { - Xslt30TransformerTransformMethod() { - this.getDeclaringType().hasQualifiedName("net.sf.saxon.s9api", "Xslt30Transformer") and - this.hasName("transform") - } -} - -/** The method `net.sf.saxon.s9api.Xslt30Transformer.applyTemplates`. */ -class Xslt30TransformerApplyTemplatesMethod extends Method { - Xslt30TransformerApplyTemplatesMethod() { - this.getDeclaringType().hasQualifiedName("net.sf.saxon.s9api", "Xslt30Transformer") and - this.hasName("applyTemplates") - } -} - -/** The method `net.sf.saxon.s9api.Xslt30Transformer.callFunction`. */ -class Xslt30TransformerCallFunctionMethod extends Method { - Xslt30TransformerCallFunctionMethod() { - this.getDeclaringType().hasQualifiedName("net.sf.saxon.s9api", "Xslt30Transformer") and - this.hasName("callFunction") - } -} - -/** The method `net.sf.saxon.s9api.Xslt30Transformer.callTemplate`. */ -class Xslt30TransformerCallTemplateMethod extends Method { - Xslt30TransformerCallTemplateMethod() { - this.getDeclaringType().hasQualifiedName("net.sf.saxon.s9api", "Xslt30Transformer") and - this.hasName("callTemplate") - } -} - -/** The class `net.sf.saxon.s9api.XsltCompiler`. */ -class TypeXsltCompiler extends Class { - TypeXsltCompiler() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltCompiler") } -} - -/** The class `net.sf.saxon.s9api.XsltExecutable`. */ -class TypeXsltExecutable extends Class { - TypeXsltExecutable() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltExecutable") } -} - -/** The class `net.sf.saxon.s9api.XsltPackage`. */ -class TypeXsltPackage extends Class { - TypeXsltPackage() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltPackage") } -} - -/** A data flow sink for unvalidated user input that is used in XSLT transformation. */ -class XsltInjectionSink extends DataFlow::ExprNode { - XsltInjectionSink() { - exists(MethodAccess ma, Method m | m = ma.getMethod() and ma.getQualifier() = this.getExpr() | - ma instanceof TransformerTransform or - m instanceof XsltTransformerTransformMethod or - m instanceof Xslt30TransformerTransformMethod or - m instanceof Xslt30TransformerApplyTemplatesMethod or - m instanceof Xslt30TransformerCallFunctionMethod or - m instanceof Xslt30TransformerCallTemplateMethod - ) - } -} - /** * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and * `XMLStreamReader`, i.e. `XMLInputFactory.createXMLStreamReader(tainted)`. */ -predicate xmlStreamReaderStep(ExprNode n1, ExprNode n2) { +private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) { exists(XmlInputFactoryStreamReader xmlStreamReader | n1.asExpr() = xmlStreamReader.getSink() and n2.asExpr() = xmlStreamReader @@ -130,7 +74,7 @@ predicate xmlStreamReaderStep(ExprNode n1, ExprNode n2) { * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and * `XMLEventReader`, i.e. `XMLInputFactory.createXMLEventReader(tainted)`. */ -predicate xmlEventReaderStep(ExprNode n1, ExprNode n2) { +private predicate xmlEventReaderStep(DataFlow::Node n1, DataFlow::Node n2) { exists(XmlInputFactoryEventReader xmlEventReader | n1.asExpr() = xmlEventReader.getSink() and n2.asExpr() = xmlEventReader @@ -141,7 +85,7 @@ predicate xmlEventReaderStep(ExprNode n1, ExprNode n2) { * Holds if `n1` to `n2` is a dataflow step that converts between `XMLStreamReader` or * `XMLEventReader` and `StAXSource`, i.e. `new StAXSource(tainted)`. */ -predicate staxSourceStep(ExprNode n1, ExprNode n2) { +private predicate staxSourceStep(DataFlow::Node n1, DataFlow::Node n2) { exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeStAXSource | n1.asExpr() = cc.getAnArgument() and n2.asExpr() = cc @@ -152,7 +96,7 @@ predicate staxSourceStep(ExprNode n1, ExprNode n2) { * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` and `Document`, * i.e. `DocumentBuilder.parse(tainted)`. */ -predicate documentBuilderStep(ExprNode n1, ExprNode n2) { +private predicate documentBuilderStep(DataFlow::Node n1, DataFlow::Node n2) { exists(DocumentBuilderParse documentBuilder | n1.asExpr() = documentBuilder.getSink() and n2.asExpr() = documentBuilder @@ -163,13 +107,30 @@ predicate documentBuilderStep(ExprNode n1, ExprNode n2) { * Holds if `n1` to `n2` is a dataflow step that converts between `Document` and `DOMSource`, i.e. * `new DOMSource(tainted)`. */ -predicate domSourceStep(ExprNode n1, ExprNode n2) { +private predicate domSourceStep(DataFlow::Node n1, DataFlow::Node n2) { exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeDOMSource | n1.asExpr() = cc.getAnArgument() and n2.asExpr() = cc ) } +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or + * `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or + * `TransformerFactory.newTemplates(tainted)`. + */ +private predicate newTransformerOrTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + n1.asExpr() = ma.getAnArgument() and + n2.asExpr() = ma and + m.getDeclaringType() instanceof TransformerFactory and + m.hasName(["newTransformer", "newTemplates"]) and + not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf | + conf.hasFlowToExpr(ma.getQualifier()) + ) + ) +} + /** * A data flow configuration for secure processing feature that is enabled on `TransformerFactory`. */ @@ -207,31 +168,11 @@ private class TransformerFactoryFeatureConfig extends ParserConfig { } } -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or - * `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or - * `TransformerFactory.newTemplates(tainted)`. - */ -predicate newTransformerOrTemplatesStep(ExprNode n1, ExprNode n2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - n1.asExpr() = ma.getAnArgument() and - n2.asExpr() = ma and - ( - m.getDeclaringType() instanceof TransformerFactory and m.hasName("newTransformer") - or - m.getDeclaringType() instanceof TransformerFactory and m.hasName("newTemplates") - ) and - not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf | - conf.hasFlowToExpr(ma.getQualifier()) - ) - ) -} - /** * Holds if `n1` to `n2` is a dataflow step that converts between `Templates` and `Transformer`, * i.e. `tainted.newTransformer()`. */ -predicate newTransformerFromTemplatesStep(ExprNode n1, ExprNode n2) { +private predicate newTransformerFromTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess ma, Method m | ma.getMethod() = m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma and @@ -246,17 +187,12 @@ predicate newTransformerFromTemplatesStep(ExprNode n1, ExprNode n2) { * `XsltCompiler.loadExecutablePackage(tainted)` or `XsltCompiler.compilePackage(tainted)` or * `XsltCompiler.loadLibraryPackage(tainted)`. */ -predicate xsltCompilerStep(ExprNode n1, ExprNode n2) { +private predicate xsltCompilerStep(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess ma, Method m | ma.getMethod() = m | n1.asExpr() = ma.getArgument(0) and n2.asExpr() = ma and m.getDeclaringType() instanceof TypeXsltCompiler and - ( - m.hasName("compile") or - m.hasName("loadExecutablePackage") or - m.hasName("compilePackage") or - m.hasName("loadLibraryPackage") - ) + m.hasName(["compile", "loadExecutablePackage", "compilePackage", "loadLibraryPackage"]) ) } @@ -265,12 +201,12 @@ predicate xsltCompilerStep(ExprNode n1, ExprNode n2) { * `XsltTransformer` or `Xslt30Transformer`, i.e. `XsltExecutable.load()` or * `XsltExecutable.load30()`. */ -predicate xsltExecutableStep(ExprNode n1, ExprNode n2) { +private predicate xsltExecutableStep(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess ma, Method m | ma.getMethod() = m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma and m.getDeclaringType() instanceof TypeXsltExecutable and - (m.hasName("load") or m.hasName("load30")) + m.hasName(["load", "load30"]) ) } @@ -278,7 +214,7 @@ predicate xsltExecutableStep(ExprNode n1, ExprNode n2) { * Holds if `n1` to `n2` is a dataflow step that converts between `XsltPackage` and * `XsltExecutable`, i.e. `XsltPackage.link()`. */ -predicate xsltPackageStep(ExprNode n1, ExprNode n2) { +private predicate xsltPackageStep(DataFlow::Node n1, DataFlow::Node n2) { exists(MethodAccess ma, Method m | ma.getMethod() = m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma and @@ -286,3 +222,33 @@ predicate xsltPackageStep(ExprNode n1, ExprNode n2) { m.hasName("link") ) } + +/** The class `javax.xml.transform.stax.StAXSource`. */ +private class TypeStAXSource extends Class { + TypeStAXSource() { this.hasQualifiedName("javax.xml.transform.stax", "StAXSource") } +} + +/** The class `javax.xml.transform.dom.DOMSource`. */ +private class TypeDOMSource extends Class { + TypeDOMSource() { this.hasQualifiedName("javax.xml.transform.dom", "DOMSource") } +} + +/** The interface `javax.xml.transform.Templates`. */ +private class TypeTemplates extends Interface { + TypeTemplates() { this.hasQualifiedName("javax.xml.transform", "Templates") } +} + +/** The class `net.sf.saxon.s9api.XsltCompiler`. */ +private class TypeXsltCompiler extends Class { + TypeXsltCompiler() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltCompiler") } +} + +/** The class `net.sf.saxon.s9api.XsltExecutable`. */ +private class TypeXsltExecutable extends Class { + TypeXsltExecutable() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltExecutable") } +} + +/** The class `net.sf.saxon.s9api.XsltPackage`. */ +private class TypeXsltPackage extends Class { + TypeXsltPackage() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltPackage") } +} From 108118afa36c40603b61c996591977fd9c299de7 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 14 Jun 2021 16:12:47 +0200 Subject: [PATCH 03/10] Use InlineExpectationsTest --- .../security/CWE-074/XsltInjection.expected | 122 ------------------ .../security/CWE-074/XsltInjection.qlref | 1 - .../CWE-074/XsltInjectionTest.expected | 0 ...tInjection.java => XsltInjectionTest.java} | 74 ++++++----- .../security/CWE-074/XsltInjectionTest.ql | 36 ++++++ 5 files changed, 75 insertions(+), 158 deletions(-) delete mode 100644 java/ql/test/query-tests/security/CWE-074/XsltInjection.expected delete mode 100644 java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref create mode 100644 java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.expected rename java/ql/test/query-tests/security/CWE-074/{XsltInjection.java => XsltInjectionTest.java} (68%) create mode 100644 java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql diff --git a/java/ql/test/query-tests/security/CWE-074/XsltInjection.expected b/java/ql/test/query-tests/security/CWE-074/XsltInjection.expected deleted file mode 100644 index 99cde4c3c10e..000000000000 --- a/java/ql/test/query-tests/security/CWE-074/XsltInjection.expected +++ /dev/null @@ -1,122 +0,0 @@ -edges -| XsltInjection.java:30:27:30:67 | new StreamSource(...) : StreamSource | XsltInjection.java:31:5:31:59 | newTransformer(...) | -| XsltInjection.java:30:44:30:66 | getInputStream(...) : InputStream | XsltInjection.java:30:27:30:67 | new StreamSource(...) : StreamSource | -| XsltInjection.java:35:27:35:90 | new StreamSource(...) : StreamSource | XsltInjection.java:36:5:36:74 | newTransformer(...) | -| XsltInjection.java:35:44:35:89 | new InputStreamReader(...) : InputStreamReader | XsltInjection.java:35:27:35:90 | new StreamSource(...) : StreamSource | -| XsltInjection.java:35:66:35:88 | getInputStream(...) : InputStream | XsltInjection.java:35:44:35:89 | new InputStreamReader(...) : InputStreamReader | -| XsltInjection.java:40:45:40:70 | param : String | XsltInjection.java:42:61:42:64 | xslt : String | -| XsltInjection.java:42:27:42:66 | new StreamSource(...) : StreamSource | XsltInjection.java:43:5:43:59 | newTransformer(...) | -| XsltInjection.java:42:44:42:65 | new StringReader(...) : StringReader | XsltInjection.java:42:27:42:66 | new StreamSource(...) : StreamSource | -| XsltInjection.java:42:61:42:64 | xslt : String | XsltInjection.java:42:44:42:65 | new StringReader(...) : StringReader | -| XsltInjection.java:47:24:47:78 | new SAXSource(...) : SAXSource | XsltInjection.java:48:5:48:74 | newTransformer(...) | -| XsltInjection.java:47:38:47:77 | new InputSource(...) : InputSource | XsltInjection.java:47:24:47:78 | new SAXSource(...) : SAXSource | -| XsltInjection.java:47:54:47:76 | getInputStream(...) : InputStream | XsltInjection.java:47:38:47:77 | new InputSource(...) : InputSource | -| XsltInjection.java:52:24:52:107 | new SAXSource(...) : SAXSource | XsltInjection.java:53:5:53:59 | newTransformer(...) | -| XsltInjection.java:52:44:52:106 | new InputSource(...) : InputSource | XsltInjection.java:52:24:52:107 | new SAXSource(...) : SAXSource | -| XsltInjection.java:52:60:52:105 | new InputStreamReader(...) : InputStreamReader | XsltInjection.java:52:44:52:106 | new InputSource(...) : InputSource | -| XsltInjection.java:52:82:52:104 | getInputStream(...) : InputStream | XsltInjection.java:52:60:52:105 | new InputStreamReader(...) : InputStreamReader | -| XsltInjection.java:57:91:57:113 | getInputStream(...) : InputStream | XsltInjection.java:58:5:58:59 | newTransformer(...) | -| XsltInjection.java:62:98:62:143 | new InputStreamReader(...) : InputStreamReader | XsltInjection.java:63:5:63:74 | newTransformer(...) | -| XsltInjection.java:62:120:62:142 | getInputStream(...) : InputStream | XsltInjection.java:62:98:62:143 | new InputStreamReader(...) : InputStreamReader | -| XsltInjection.java:67:102:67:124 | getInputStream(...) : InputStream | XsltInjection.java:68:5:68:59 | newTransformer(...) | -| XsltInjection.java:72:27:72:67 | new StreamSource(...) : StreamSource | XsltInjection.java:76:5:76:34 | newTransformer(...) | -| XsltInjection.java:72:44:72:66 | getInputStream(...) : InputStream | XsltInjection.java:72:27:72:67 | new StreamSource(...) : StreamSource | -| XsltInjection.java:80:27:80:67 | new StreamSource(...) : StreamSource | XsltInjection.java:83:5:83:34 | newTransformer(...) | -| XsltInjection.java:80:44:80:66 | getInputStream(...) : InputStream | XsltInjection.java:80:27:80:67 | new StreamSource(...) : StreamSource | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:90:5:90:35 | load(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:91:5:91:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:92:5:92:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:93:5:93:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:94:5:94:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:95:5:95:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:96:5:96:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:97:5:97:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:98:5:98:37 | load30(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | XsltInjection.java:99:5:99:37 | load30(...) | -| XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | -| XsltInjection.java:103:36:103:61 | param : String | XsltInjection.java:104:23:104:27 | param : String | -| XsltInjection.java:104:15:104:28 | new URI(...) : URI | XsltInjection.java:108:5:108:46 | load(...) | -| XsltInjection.java:104:15:104:28 | new URI(...) : URI | XsltInjection.java:110:5:110:50 | load(...) | -| XsltInjection.java:104:23:104:27 | param : String | XsltInjection.java:104:15:104:28 | new URI(...) : URI | -| XsltInjection.java:105:27:105:67 | new StreamSource(...) : StreamSource | XsltInjection.java:109:5:109:49 | load(...) | -| XsltInjection.java:105:44:105:66 | getInputStream(...) : InputStream | XsltInjection.java:105:27:105:67 | new StreamSource(...) : StreamSource | -nodes -| XsltInjection.java:30:27:30:67 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource | -| XsltInjection.java:30:44:30:66 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:31:5:31:59 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:35:27:35:90 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource | -| XsltInjection.java:35:44:35:89 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader | -| XsltInjection.java:35:66:35:88 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:36:5:36:74 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:40:45:40:70 | param : String | semmle.label | param : String | -| XsltInjection.java:42:27:42:66 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource | -| XsltInjection.java:42:44:42:65 | new StringReader(...) : StringReader | semmle.label | new StringReader(...) : StringReader | -| XsltInjection.java:42:61:42:64 | xslt : String | semmle.label | xslt : String | -| XsltInjection.java:43:5:43:59 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:47:24:47:78 | new SAXSource(...) : SAXSource | semmle.label | new SAXSource(...) : SAXSource | -| XsltInjection.java:47:38:47:77 | new InputSource(...) : InputSource | semmle.label | new InputSource(...) : InputSource | -| XsltInjection.java:47:54:47:76 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:48:5:48:74 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:52:24:52:107 | new SAXSource(...) : SAXSource | semmle.label | new SAXSource(...) : SAXSource | -| XsltInjection.java:52:44:52:106 | new InputSource(...) : InputSource | semmle.label | new InputSource(...) : InputSource | -| XsltInjection.java:52:60:52:105 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader | -| XsltInjection.java:52:82:52:104 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:53:5:53:59 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:57:91:57:113 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:58:5:58:59 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:62:98:62:143 | new InputStreamReader(...) : InputStreamReader | semmle.label | new InputStreamReader(...) : InputStreamReader | -| XsltInjection.java:62:120:62:142 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:63:5:63:74 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:67:102:67:124 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:68:5:68:59 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:72:27:72:67 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource | -| XsltInjection.java:72:44:72:66 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:76:5:76:34 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:80:27:80:67 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource | -| XsltInjection.java:80:44:80:66 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:83:5:83:34 | newTransformer(...) | semmle.label | newTransformer(...) | -| XsltInjection.java:87:27:87:67 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource | -| XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:90:5:90:35 | load(...) | semmle.label | load(...) | -| XsltInjection.java:91:5:91:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:92:5:92:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:93:5:93:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:94:5:94:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:95:5:95:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:96:5:96:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:97:5:97:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:98:5:98:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:99:5:99:37 | load30(...) | semmle.label | load30(...) | -| XsltInjection.java:103:36:103:61 | param : String | semmle.label | param : String | -| XsltInjection.java:104:15:104:28 | new URI(...) : URI | semmle.label | new URI(...) : URI | -| XsltInjection.java:104:23:104:27 | param : String | semmle.label | param : String | -| XsltInjection.java:105:27:105:67 | new StreamSource(...) : StreamSource | semmle.label | new StreamSource(...) : StreamSource | -| XsltInjection.java:105:44:105:66 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream | -| XsltInjection.java:108:5:108:46 | load(...) | semmle.label | load(...) | -| XsltInjection.java:109:5:109:49 | load(...) | semmle.label | load(...) | -| XsltInjection.java:110:5:110:50 | load(...) | semmle.label | load(...) | -subpaths -#select -| XsltInjection.java:31:5:31:59 | newTransformer(...) | XsltInjection.java:30:44:30:66 | getInputStream(...) : InputStream | XsltInjection.java:31:5:31:59 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:30:44:30:66 | getInputStream(...) | this user input | -| XsltInjection.java:36:5:36:74 | newTransformer(...) | XsltInjection.java:35:66:35:88 | getInputStream(...) : InputStream | XsltInjection.java:36:5:36:74 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:35:66:35:88 | getInputStream(...) | this user input | -| XsltInjection.java:43:5:43:59 | newTransformer(...) | XsltInjection.java:40:45:40:70 | param : String | XsltInjection.java:43:5:43:59 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:40:45:40:70 | param | this user input | -| XsltInjection.java:48:5:48:74 | newTransformer(...) | XsltInjection.java:47:54:47:76 | getInputStream(...) : InputStream | XsltInjection.java:48:5:48:74 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:47:54:47:76 | getInputStream(...) | this user input | -| XsltInjection.java:53:5:53:59 | newTransformer(...) | XsltInjection.java:52:82:52:104 | getInputStream(...) : InputStream | XsltInjection.java:53:5:53:59 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:52:82:52:104 | getInputStream(...) | this user input | -| XsltInjection.java:58:5:58:59 | newTransformer(...) | XsltInjection.java:57:91:57:113 | getInputStream(...) : InputStream | XsltInjection.java:58:5:58:59 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:57:91:57:113 | getInputStream(...) | this user input | -| XsltInjection.java:63:5:63:74 | newTransformer(...) | XsltInjection.java:62:120:62:142 | getInputStream(...) : InputStream | XsltInjection.java:63:5:63:74 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:62:120:62:142 | getInputStream(...) | this user input | -| XsltInjection.java:68:5:68:59 | newTransformer(...) | XsltInjection.java:67:102:67:124 | getInputStream(...) : InputStream | XsltInjection.java:68:5:68:59 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:67:102:67:124 | getInputStream(...) | this user input | -| XsltInjection.java:76:5:76:34 | newTransformer(...) | XsltInjection.java:72:44:72:66 | getInputStream(...) : InputStream | XsltInjection.java:76:5:76:34 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:72:44:72:66 | getInputStream(...) | this user input | -| XsltInjection.java:83:5:83:34 | newTransformer(...) | XsltInjection.java:80:44:80:66 | getInputStream(...) : InputStream | XsltInjection.java:83:5:83:34 | newTransformer(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:80:44:80:66 | getInputStream(...) | this user input | -| XsltInjection.java:90:5:90:35 | load(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:90:5:90:35 | load(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:91:5:91:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:91:5:91:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:92:5:92:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:92:5:92:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:93:5:93:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:93:5:93:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:94:5:94:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:94:5:94:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:95:5:95:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:95:5:95:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:96:5:96:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:96:5:96:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:97:5:97:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:97:5:97:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:98:5:98:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:98:5:98:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:99:5:99:37 | load30(...) | XsltInjection.java:87:44:87:66 | getInputStream(...) : InputStream | XsltInjection.java:99:5:99:37 | load30(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:87:44:87:66 | getInputStream(...) | this user input | -| XsltInjection.java:108:5:108:46 | load(...) | XsltInjection.java:103:36:103:61 | param : String | XsltInjection.java:108:5:108:46 | load(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:103:36:103:61 | param | this user input | -| XsltInjection.java:109:5:109:49 | load(...) | XsltInjection.java:105:44:105:66 | getInputStream(...) : InputStream | XsltInjection.java:109:5:109:49 | load(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:105:44:105:66 | getInputStream(...) | this user input | -| XsltInjection.java:110:5:110:50 | load(...) | XsltInjection.java:103:36:103:61 | param : String | XsltInjection.java:110:5:110:50 | load(...) | XSLT transformation might include stylesheet from $@. | XsltInjection.java:103:36:103:61 | param | this user input | diff --git a/java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref b/java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref deleted file mode 100644 index 9d7fb59ac2c2..000000000000 --- a/java/ql/test/query-tests/security/CWE-074/XsltInjection.qlref +++ /dev/null @@ -1 +0,0 @@ -Security/CWE/CWE-074/XsltInjection.ql diff --git a/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.expected b/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/java/ql/test/query-tests/security/CWE-074/XsltInjection.java b/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.java similarity index 68% rename from java/ql/test/query-tests/security/CWE-074/XsltInjection.java rename to java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.java index d1a19217f02e..2bfd02a865c6 100644 --- a/java/ql/test/query-tests/security/CWE-074/XsltInjection.java +++ b/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.java @@ -25,47 +25,51 @@ import net.sf.saxon.s9api.XsltCompiler; @Controller -public class XsltInjection { +public class XsltInjectionTest { public void testStreamSourceInputStream(Socket socket) throws Exception { StreamSource source = new StreamSource(socket.getInputStream()); - TransformerFactory.newInstance().newTransformer(source).transform(null, null); + TransformerFactory.newInstance().newTransformer(source).transform(null, null); // $hasXsltInjection } public void testStreamSourceReader(Socket socket) throws Exception { StreamSource source = new StreamSource(new InputStreamReader(socket.getInputStream())); - TransformerFactory.newInstance().newTemplates(source).newTransformer().transform(null, null); + TransformerFactory.newInstance().newTemplates(source).newTransformer().transform(null, null); // $hasXsltInjection } @RequestMapping public void testStreamSourceInjectedParam(@RequestParam String param) throws Exception { String xslt = ""; StreamSource source = new StreamSource(new StringReader(xslt)); - TransformerFactory.newInstance().newTransformer(source).transform(null, null); + TransformerFactory.newInstance().newTransformer(source).transform(null, null); // $hasXsltInjection } public void testSAXSourceInputStream(Socket socket) throws Exception { SAXSource source = new SAXSource(new InputSource(socket.getInputStream())); - TransformerFactory.newInstance().newTemplates(source).newTransformer().transform(null, null); + TransformerFactory.newInstance().newTemplates(source).newTransformer().transform(null, null); // $hasXsltInjection } public void testSAXSourceReader(Socket socket) throws Exception { - SAXSource source = new SAXSource(null, new InputSource(new InputStreamReader(socket.getInputStream()))); - TransformerFactory.newInstance().newTransformer(source).transform(null, null); + SAXSource source = + new SAXSource(null, new InputSource(new InputStreamReader(socket.getInputStream()))); + TransformerFactory.newInstance().newTransformer(source).transform(null, null); // $hasXsltInjection } public void testStAXSourceEventReader(Socket socket) throws Exception { - StAXSource source = new StAXSource(XMLInputFactory.newInstance().createXMLEventReader(socket.getInputStream())); - TransformerFactory.newInstance().newTransformer(source).transform(null, null); + StAXSource source = + new StAXSource(XMLInputFactory.newInstance().createXMLEventReader(socket.getInputStream())); + TransformerFactory.newInstance().newTransformer(source).transform(null, null); // $hasXsltInjection } public void testStAXSourceEventStream(Socket socket) throws Exception { - StAXSource source = new StAXSource(XMLInputFactory.newInstance().createXMLStreamReader(null, new InputStreamReader(socket.getInputStream()))); - TransformerFactory.newInstance().newTemplates(source).newTransformer().transform(null, null); + StAXSource source = new StAXSource(XMLInputFactory.newInstance().createXMLStreamReader(null, + new InputStreamReader(socket.getInputStream()))); + TransformerFactory.newInstance().newTemplates(source).newTransformer().transform(null, null); // $hasXsltInjection } public void testDOMSource(Socket socket) throws Exception { - DOMSource source = new DOMSource(DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(socket.getInputStream())); - TransformerFactory.newInstance().newTransformer(source).transform(null, null); + DOMSource source = new DOMSource( + DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(socket.getInputStream())); + TransformerFactory.newInstance().newTransformer(source).transform(null, null); // $hasXsltInjection } public void testDisabledXXE(Socket socket) throws Exception { @@ -73,30 +77,30 @@ public void testDisabledXXE(Socket socket) throws Exception { TransformerFactory factory = TransformerFactory.newInstance(); factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); - factory.newTransformer(source).transform(null, null); + factory.newTransformer(source).transform(null, null); // $hasXsltInjection } public void testFeatureSecureProcessingDisabled(Socket socket) throws Exception { StreamSource source = new StreamSource(socket.getInputStream()); TransformerFactory factory = TransformerFactory.newInstance(); factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, false); - factory.newTransformer(source).transform(null, null); + factory.newTransformer(source).transform(null, null); // $hasXsltInjection } public void testSaxon(Socket socket) throws Exception { StreamSource source = new StreamSource(socket.getInputStream()); XsltCompiler compiler = new Processor(true).newXsltCompiler(); - - compiler.compile(source).load().transform(); - compiler.compile(source).load30().transform(null, null); - compiler.compile(source).load30().applyTemplates((Source) null); - compiler.compile(source).load30().applyTemplates((Source) null, null); - compiler.compile(source).load30().applyTemplates((XdmValue) null); - compiler.compile(source).load30().applyTemplates((XdmValue) null, null); - compiler.compile(source).load30().callFunction(null, null); - compiler.compile(source).load30().callFunction(null, null, null); - compiler.compile(source).load30().callTemplate(null); - compiler.compile(source).load30().callTemplate(null, null); + + compiler.compile(source).load().transform(); // $hasXsltInjection + compiler.compile(source).load30().transform(null, null); // $hasXsltInjection + compiler.compile(source).load30().applyTemplates((Source) null); // $hasXsltInjection + compiler.compile(source).load30().applyTemplates((Source) null, null); // $hasXsltInjection + compiler.compile(source).load30().applyTemplates((XdmValue) null); // $hasXsltInjection + compiler.compile(source).load30().applyTemplates((XdmValue) null, null); // $hasXsltInjection + compiler.compile(source).load30().callFunction(null, null); // $hasXsltInjection + compiler.compile(source).load30().callFunction(null, null, null); // $hasXsltInjection + compiler.compile(source).load30().callTemplate(null); // $hasXsltInjection + compiler.compile(source).load30().callTemplate(null, null); // $hasXsltInjection } @RequestMapping @@ -104,24 +108,24 @@ public void testSaxonXsltPackage(@RequestParam String param, Socket socket) thro URI uri = new URI(param); StreamSource source = new StreamSource(socket.getInputStream()); XsltCompiler compiler = new Processor(true).newXsltCompiler(); - - compiler.loadExecutablePackage(uri).load().transform(); - compiler.compilePackage(source).link().load().transform(); - compiler.loadLibraryPackage(uri).link().load().transform(); + + compiler.loadExecutablePackage(uri).load().transform(); // $hasXsltInjection + compiler.compilePackage(source).link().load().transform(); // $hasXsltInjection + compiler.loadLibraryPackage(uri).link().load().transform(); // $hasXsltInjection } public void testOkFeatureSecureProcessing(Socket socket) throws Exception { StreamSource source = new StreamSource(socket.getInputStream()); TransformerFactory factory = TransformerFactory.newInstance(); factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - factory.newTransformer(source).transform(null, null); + factory.newTransformer(source).transform(null, null); // Safe } public void testOkSaxon(Socket socket) throws Exception { StreamSource source = new StreamSource(socket.getInputStream()); XsltCompiler compiler = new Processor(true).newXsltCompiler(); - - compiler.compile(source).load().close(); - compiler.compile((Source) new Object()).load().transform(); + + compiler.compile(source).load().close(); // Safe + compiler.compile((Source) new Object()).load().transform(); // Safe } -} \ No newline at end of file +} diff --git a/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql b/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql new file mode 100644 index 000000000000..1ac2b87d5056 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql @@ -0,0 +1,36 @@ +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.security.XsltInjection +import TestUtilities.InlineExpectationsTest + +class Conf extends TaintTracking::Configuration { + Conf() { this = "test:cwe:xslt-injection" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof XsltInjectionSink } + + override predicate isSanitizer(DataFlow::Node node) { + node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + any(XsltInjectionAdditionalTaintStep c).step(node1, node2) + } +} + +class HasXsltInjectionTest extends InlineExpectationsTest { + HasXsltInjectionTest() { this = "HasXsltInjectionTest" } + + override string getARelevantTag() { result = "hasXsltInjection" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "hasXsltInjection" and + exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) | + sink.getLocation() = location and + element = sink.toString() and + value = "" + ) + } +} From fc58ada92e15d899769757d96afce1c5fd1ae458 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 17 Jun 2021 10:20:40 +0200 Subject: [PATCH 04/10] Add change note --- java/change-notes/2021-06-16-xslt-injection-query.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 java/change-notes/2021-06-16-xslt-injection-query.md diff --git a/java/change-notes/2021-06-16-xslt-injection-query.md b/java/change-notes/2021-06-16-xslt-injection-query.md new file mode 100644 index 000000000000..a6861f304f6a --- /dev/null +++ b/java/change-notes/2021-06-16-xslt-injection-query.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The query "XSLT transformation with user-controlled stylesheet" (`java/xslt-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 @ggolawski](https://github.com/github/codeql/pull/3363) \ No newline at end of file From 6967b06dee236c7534500a8ae64c3c40df3855a7 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 19 Jul 2021 13:16:11 +0200 Subject: [PATCH 05/10] Decouple XsltInjection.qll to reuse the taint tracking configuration --- .../code/java/dataflow/ExternalFlow.qll | 1 + .../src/Security/CWE/CWE-074/XsltInjection.ql | 22 +----------- ...ltInjection.qll => XsltInjectionQuery.qll} | 35 +++++++++++-------- .../java/security/XsltInjectionSinkModels.qll | 17 +++++++++ .../security/CWE-074/XsltInjectionTest.ql | 22 +++--------- 5 files changed, 43 insertions(+), 54 deletions(-) rename java/ql/src/semmle/code/java/security/{XsltInjection.qll => XsltInjectionQuery.qll} (91%) create mode 100644 java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index bce80a3ee08a..f88cdcb15935 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -110,6 +110,7 @@ private module Frameworks { private import semmle.code.java.security.MvelInjection private import semmle.code.java.security.OgnlInjection private import semmle.code.java.security.XPath + private import semmle.code.java.security.XsltInjectionSinkModels private import semmle.code.java.frameworks.android.Android private import semmle.code.java.frameworks.android.SQLite private import semmle.code.java.frameworks.Jdbc diff --git a/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql b/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql index cd47d69470fe..3728ac9f3eeb 100644 --- a/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql +++ b/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql @@ -11,29 +11,9 @@ */ import java -import semmle.code.java.dataflow.FlowSources -import semmle.code.java.security.XsltInjection +import semmle.code.java.security.XsltInjectionQuery import DataFlow::PathGraph -/** - * A taint-tracking configuration for unvalidated user input that is used in XSLT transformation. - */ -class XsltInjectionFlowConfig extends TaintTracking::Configuration { - XsltInjectionFlowConfig() { this = "XsltInjectionFlowConfig" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof XsltInjectionSink } - - override predicate isSanitizer(DataFlow::Node node) { - node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType - } - - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - any(XsltInjectionAdditionalTaintStep c).step(node1, node2) - } -} - from DataFlow::PathNode source, DataFlow::PathNode sink, XsltInjectionFlowConfig conf where conf.hasFlowPath(source, sink) select sink.getNode(), source, sink, "XSLT transformation might include stylesheet from $@.", diff --git a/java/ql/src/semmle/code/java/security/XsltInjection.qll b/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll similarity index 91% rename from java/ql/src/semmle/code/java/security/XsltInjection.qll rename to java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll index 4db9f7afd566..9a23223a997c 100644 --- a/java/ql/src/semmle/code/java/security/XsltInjection.qll +++ b/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll @@ -1,7 +1,7 @@ /** Provides classes to reason about XSLT injection vulnerabilities. */ import java -import semmle.code.java.dataflow.ExternalFlow +import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.XmlParsers import semmle.code.java.dataflow.DataFlow @@ -11,20 +11,6 @@ import semmle.code.java.dataflow.DataFlow */ abstract class XsltInjectionSink extends DataFlow::Node { } -private class DefaultXsltInjectionSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt" - ] - } -} - /** A default sink representing methods susceptible to XSLT Injection attacks. */ private class DefaultXsltInjectionSink extends XsltInjectionSink { DefaultXsltInjectionSink() { sinkNode(this, "xslt") } @@ -59,6 +45,25 @@ private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAddit } } +/** + * A taint-tracking configuration for unvalidated user input that is used in XSLT transformation. + */ +class XsltInjectionFlowConfig extends TaintTracking::Configuration { + XsltInjectionFlowConfig() { this = "XsltInjectionFlowConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { sink instanceof XsltInjectionSink } + + override predicate isSanitizer(DataFlow::Node node) { + node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + any(XsltInjectionAdditionalTaintStep c).step(node1, node2) + } +} + /** * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and * `XMLStreamReader`, i.e. `XMLInputFactory.createXMLStreamReader(tainted)`. diff --git a/java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll b/java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll new file mode 100644 index 000000000000..599e2e963d30 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll @@ -0,0 +1,17 @@ +/** Provides sink models relating to XSLT injection vulnerabilities. */ + +import semmle.code.java.dataflow.ExternalFlow + +private class DefaultXsltInjectionSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt" + ] + } +} diff --git a/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql b/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql index 1ac2b87d5056..c2f02865b2e9 100644 --- a/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql +++ b/java/ql/test/query-tests/security/CWE-074/XsltInjectionTest.ql @@ -1,25 +1,9 @@ import java import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources -import semmle.code.java.security.XsltInjection +import semmle.code.java.security.XsltInjectionQuery import TestUtilities.InlineExpectationsTest -class Conf extends TaintTracking::Configuration { - Conf() { this = "test:cwe:xslt-injection" } - - override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } - - override predicate isSink(DataFlow::Node sink) { sink instanceof XsltInjectionSink } - - override predicate isSanitizer(DataFlow::Node node) { - node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType - } - - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - any(XsltInjectionAdditionalTaintStep c).step(node1, node2) - } -} - class HasXsltInjectionTest extends InlineExpectationsTest { HasXsltInjectionTest() { this = "HasXsltInjectionTest" } @@ -27,7 +11,9 @@ class HasXsltInjectionTest extends InlineExpectationsTest { override predicate hasActualResult(Location location, string element, string tag, string value) { tag = "hasXsltInjection" and - exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) | + exists(DataFlow::Node src, DataFlow::Node sink, XsltInjectionFlowConfig conf | + conf.hasFlow(src, sink) + | sink.getLocation() = location and element = sink.toString() and value = "" From ff21662b23884deec9a842249916dbecc42ab4a0 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 20 Jul 2021 17:55:45 +0200 Subject: [PATCH 06/10] Refactor XsltInjection.qll --- .../2021-06-16-xslt-injection-query.md | 2 +- .../code/java/security/XsltInjection.qll | 254 ++++++++++++++++++ .../code/java/security/XsltInjectionQuery.qll | 240 +---------------- .../java/security/XsltInjectionSinkModels.qll | 17 -- 4 files changed, 258 insertions(+), 255 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/XsltInjection.qll delete mode 100644 java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll diff --git a/java/change-notes/2021-06-16-xslt-injection-query.md b/java/change-notes/2021-06-16-xslt-injection-query.md index a6861f304f6a..d30421607b3c 100644 --- a/java/change-notes/2021-06-16-xslt-injection-query.md +++ b/java/change-notes/2021-06-16-xslt-injection-query.md @@ -1,2 +1,2 @@ lgtm,codescanning -* The query "XSLT transformation with user-controlled stylesheet" (`java/xslt-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 @ggolawski](https://github.com/github/codeql/pull/3363) \ No newline at end of file +* The query "XSLT transformation with user-controlled stylesheet" (`java/xslt-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 @ggolawski](https://github.com/github/codeql/pull/3363). \ No newline at end of file diff --git a/java/ql/src/semmle/code/java/security/XsltInjection.qll b/java/ql/src/semmle/code/java/security/XsltInjection.qll new file mode 100644 index 000000000000..1ebf961e03c4 --- /dev/null +++ b/java/ql/src/semmle/code/java/security/XsltInjection.qll @@ -0,0 +1,254 @@ +/** Provides classes to reason about XSLT injection vulnerabilities. */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.dataflow.ExternalFlow +import semmle.code.java.security.XmlParsers + +/** + * A data flow sink for unvalidated user input that is used in XSLT transformation. + * Extend this class to add your own XSLT Injection sinks. + */ +abstract class XsltInjectionSink extends DataFlow::Node { } + +/** A default sink representing methods susceptible to XSLT Injection attacks. */ +private class DefaultXsltInjectionSink extends XsltInjectionSink { + DefaultXsltInjectionSink() { sinkNode(this, "xslt") } +} + +private class DefaultXsltInjectionSinkModel extends SinkModelCsv { + override predicate row(string row) { + row = + [ + "javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt", + "net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt" + ] + } +} + +/** + * A unit class for adding additional taint steps. + * + * Extend this class to add additional taint steps that should apply to the `XsltInjectionFlowConfig`. + */ +class XsltInjectionAdditionalTaintStep extends Unit { + /** + * Holds if the step from `node1` to `node2` should be considered a taint + * step for the `XsltInjectionFlowConfig` configuration. + */ + abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); +} + +/** A set of additional taint steps to consider when taint tracking XSLT related data flows. */ +private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + xmlStreamReaderStep(node1, node2) or + xmlEventReaderStep(node1, node2) or + staxSourceStep(node1, node2) or + documentBuilderStep(node1, node2) or + domSourceStep(node1, node2) or + newTransformerOrTemplatesStep(node1, node2) or + newTransformerFromTemplatesStep(node1, node2) or + xsltCompilerStep(node1, node2) or + xsltExecutableStep(node1, node2) or + xsltPackageStep(node1, node2) + } +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and + * `XMLStreamReader`, i.e. `XMLInputFactory.createXMLStreamReader(tainted)`. + */ +private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(XmlInputFactoryStreamReader xmlStreamReader | + n1.asExpr() = xmlStreamReader.getSink() and + n2.asExpr() = xmlStreamReader + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and + * `XMLEventReader`, i.e. `XMLInputFactory.createXMLEventReader(tainted)`. + */ +private predicate xmlEventReaderStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(XmlInputFactoryEventReader xmlEventReader | + n1.asExpr() = xmlEventReader.getSink() and + n2.asExpr() = xmlEventReader + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `XMLStreamReader` or + * `XMLEventReader` and `StAXSource`, i.e. `new StAXSource(tainted)`. + */ +private predicate staxSourceStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeStAXSource | + n1.asExpr() = cc.getAnArgument() and + n2.asExpr() = cc + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` and `Document`, + * i.e. `DocumentBuilder.parse(tainted)`. + */ +private predicate documentBuilderStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(DocumentBuilderParse documentBuilder | + n1.asExpr() = documentBuilder.getSink() and + n2.asExpr() = documentBuilder + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `Document` and `DOMSource`, i.e. + * `new DOMSource(tainted)`. + */ +private predicate domSourceStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeDOMSource | + n1.asExpr() = cc.getAnArgument() and + n2.asExpr() = cc + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or + * `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or + * `TransformerFactory.newTemplates(tainted)`. + */ +private predicate newTransformerOrTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + n1.asExpr() = ma.getAnArgument() and + n2.asExpr() = ma and + m.getDeclaringType() instanceof TransformerFactory and + m.hasName(["newTransformer", "newTemplates"]) and + not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf | + conf.hasFlowToExpr(ma.getQualifier()) + ) + ) +} + +/** + * A data flow configuration for secure processing feature that is enabled on `TransformerFactory`. + */ +private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends DataFlow2::Configuration { + TransformerFactoryWithSecureProcessingFeatureFlowConfig() { + this = "TransformerFactoryWithSecureProcessingFeatureFlowConfig" + } + + override predicate isSource(DataFlow::Node src) { + exists(Variable v | v = src.asExpr().(VarAccess).getVariable() | + exists(TransformerFactoryFeatureConfig config | config.getQualifier() = v.getAnAccess() | + config.enables(configSecureProcessing()) + ) + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + sink.asExpr() = ma.getQualifier() and + ma.getMethod().getDeclaringType() instanceof TransformerFactory + ) + } + + override int fieldFlowBranchLimit() { result = 0 } +} + +/** A `ParserConfig` specific to `TransformerFactory`. */ +private class TransformerFactoryFeatureConfig extends ParserConfig { + TransformerFactoryFeatureConfig() { + exists(Method m | + m = this.getMethod() and + m.getDeclaringType() instanceof TransformerFactory and + m.hasName("setFeature") + ) + } +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `Templates` and `Transformer`, + * i.e. `tainted.newTransformer()`. + */ +private predicate newTransformerFromTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + n1.asExpr() = ma.getQualifier() and + n2.asExpr() = ma and + m.getDeclaringType() instanceof TypeTemplates and + m.hasName("newTransformer") + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `Source` or `URI` and + * `XsltExecutable` or `XsltPackage`, i.e. `XsltCompiler.compile(tainted)` or + * `XsltCompiler.loadExecutablePackage(tainted)` or `XsltCompiler.compilePackage(tainted)` or + * `XsltCompiler.loadLibraryPackage(tainted)`. + */ +private predicate xsltCompilerStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + n1.asExpr() = ma.getArgument(0) and + n2.asExpr() = ma and + m.getDeclaringType() instanceof TypeXsltCompiler and + m.hasName(["compile", "loadExecutablePackage", "compilePackage", "loadLibraryPackage"]) + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `XsltExecutable` and + * `XsltTransformer` or `Xslt30Transformer`, i.e. `XsltExecutable.load()` or + * `XsltExecutable.load30()`. + */ +private predicate xsltExecutableStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + n1.asExpr() = ma.getQualifier() and + n2.asExpr() = ma and + m.getDeclaringType() instanceof TypeXsltExecutable and + m.hasName(["load", "load30"]) + ) +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `XsltPackage` and + * `XsltExecutable`, i.e. `XsltPackage.link()`. + */ +private predicate xsltPackageStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + n1.asExpr() = ma.getQualifier() and + n2.asExpr() = ma and + m.getDeclaringType() instanceof TypeXsltPackage and + m.hasName("link") + ) +} + +/** The class `javax.xml.transform.stax.StAXSource`. */ +private class TypeStAXSource extends Class { + TypeStAXSource() { this.hasQualifiedName("javax.xml.transform.stax", "StAXSource") } +} + +/** The class `javax.xml.transform.dom.DOMSource`. */ +private class TypeDOMSource extends Class { + TypeDOMSource() { this.hasQualifiedName("javax.xml.transform.dom", "DOMSource") } +} + +/** The interface `javax.xml.transform.Templates`. */ +private class TypeTemplates extends Interface { + TypeTemplates() { this.hasQualifiedName("javax.xml.transform", "Templates") } +} + +/** The class `net.sf.saxon.s9api.XsltCompiler`. */ +private class TypeXsltCompiler extends Class { + TypeXsltCompiler() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltCompiler") } +} + +/** The class `net.sf.saxon.s9api.XsltExecutable`. */ +private class TypeXsltExecutable extends Class { + TypeXsltExecutable() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltExecutable") } +} + +/** The class `net.sf.saxon.s9api.XsltPackage`. */ +private class TypeXsltPackage extends Class { + TypeXsltPackage() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltPackage") } +} diff --git a/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll b/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll index 9a23223a997c..8beafb22e10b 100644 --- a/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll +++ b/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll @@ -1,49 +1,9 @@ -/** Provides classes to reason about XSLT injection vulnerabilities. */ +/** Provides taint tracking configurations to be used in XSLT injection queries. */ import java import semmle.code.java.dataflow.FlowSources -import semmle.code.java.security.XmlParsers -import semmle.code.java.dataflow.DataFlow - -/** - * A data flow sink for unvalidated user input that is used in XSLT transformation. - * Extend this class to add your own XSLT Injection sinks. - */ -abstract class XsltInjectionSink extends DataFlow::Node { } - -/** A default sink representing methods susceptible to XSLT Injection attacks. */ -private class DefaultXsltInjectionSink extends XsltInjectionSink { - DefaultXsltInjectionSink() { sinkNode(this, "xslt") } -} - -/** - * A unit class for adding additional taint steps. - * - * Extend this class to add additional taint steps that should apply to the `XsltInjectionFlowConfig`. - */ -class XsltInjectionAdditionalTaintStep extends Unit { - /** - * Holds if the step from `node1` to `node2` should be considered a taint - * step for the `XsltInjectionFlowConfig` configuration. - */ - abstract predicate step(DataFlow::Node node1, DataFlow::Node node2); -} - -/** A set of additional taint steps to consider when taint tracking XSLT related data flows. */ -private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAdditionalTaintStep { - override predicate step(DataFlow::Node node1, DataFlow::Node node2) { - xmlStreamReaderStep(node1, node2) or - xmlEventReaderStep(node1, node2) or - staxSourceStep(node1, node2) or - documentBuilderStep(node1, node2) or - domSourceStep(node1, node2) or - newTransformerOrTemplatesStep(node1, node2) or - newTransformerFromTemplatesStep(node1, node2) or - xsltCompilerStep(node1, node2) or - xsltExecutableStep(node1, node2) or - xsltPackageStep(node1, node2) - } -} +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.XsltInjection /** * A taint-tracking configuration for unvalidated user input that is used in XSLT transformation. @@ -63,197 +23,3 @@ class XsltInjectionFlowConfig extends TaintTracking::Configuration { any(XsltInjectionAdditionalTaintStep c).step(node1, node2) } } - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and - * `XMLStreamReader`, i.e. `XMLInputFactory.createXMLStreamReader(tainted)`. - */ -private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(XmlInputFactoryStreamReader xmlStreamReader | - n1.asExpr() = xmlStreamReader.getSink() and - n2.asExpr() = xmlStreamReader - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` or `Reader` and - * `XMLEventReader`, i.e. `XMLInputFactory.createXMLEventReader(tainted)`. - */ -private predicate xmlEventReaderStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(XmlInputFactoryEventReader xmlEventReader | - n1.asExpr() = xmlEventReader.getSink() and - n2.asExpr() = xmlEventReader - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `XMLStreamReader` or - * `XMLEventReader` and `StAXSource`, i.e. `new StAXSource(tainted)`. - */ -private predicate staxSourceStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeStAXSource | - n1.asExpr() = cc.getAnArgument() and - n2.asExpr() = cc - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `InputStream` and `Document`, - * i.e. `DocumentBuilder.parse(tainted)`. - */ -private predicate documentBuilderStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(DocumentBuilderParse documentBuilder | - n1.asExpr() = documentBuilder.getSink() and - n2.asExpr() = documentBuilder - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `Document` and `DOMSource`, i.e. - * `new DOMSource(tainted)`. - */ -private predicate domSourceStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeDOMSource | - n1.asExpr() = cc.getAnArgument() and - n2.asExpr() = cc - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or - * `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or - * `TransformerFactory.newTemplates(tainted)`. - */ -private predicate newTransformerOrTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - n1.asExpr() = ma.getAnArgument() and - n2.asExpr() = ma and - m.getDeclaringType() instanceof TransformerFactory and - m.hasName(["newTransformer", "newTemplates"]) and - not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf | - conf.hasFlowToExpr(ma.getQualifier()) - ) - ) -} - -/** - * A data flow configuration for secure processing feature that is enabled on `TransformerFactory`. - */ -private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends DataFlow2::Configuration { - TransformerFactoryWithSecureProcessingFeatureFlowConfig() { - this = "TransformerFactoryWithSecureProcessingFeatureFlowConfig" - } - - override predicate isSource(DataFlow::Node src) { - exists(Variable v | v = src.asExpr().(VarAccess).getVariable() | - exists(TransformerFactoryFeatureConfig config | config.getQualifier() = v.getAnAccess() | - config.enables(configSecureProcessing()) - ) - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getQualifier() and - ma.getMethod().getDeclaringType() instanceof TransformerFactory - ) - } - - override int fieldFlowBranchLimit() { result = 0 } -} - -/** A `ParserConfig` specific to `TransformerFactory`. */ -private class TransformerFactoryFeatureConfig extends ParserConfig { - TransformerFactoryFeatureConfig() { - exists(Method m | - m = this.getMethod() and - m.getDeclaringType() instanceof TransformerFactory and - m.hasName("setFeature") - ) - } -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `Templates` and `Transformer`, - * i.e. `tainted.newTransformer()`. - */ -private predicate newTransformerFromTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - n1.asExpr() = ma.getQualifier() and - n2.asExpr() = ma and - m.getDeclaringType() instanceof TypeTemplates and - m.hasName("newTransformer") - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `Source` or `URI` and - * `XsltExecutable` or `XsltPackage`, i.e. `XsltCompiler.compile(tainted)` or - * `XsltCompiler.loadExecutablePackage(tainted)` or `XsltCompiler.compilePackage(tainted)` or - * `XsltCompiler.loadLibraryPackage(tainted)`. - */ -private predicate xsltCompilerStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - n1.asExpr() = ma.getArgument(0) and - n2.asExpr() = ma and - m.getDeclaringType() instanceof TypeXsltCompiler and - m.hasName(["compile", "loadExecutablePackage", "compilePackage", "loadLibraryPackage"]) - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `XsltExecutable` and - * `XsltTransformer` or `Xslt30Transformer`, i.e. `XsltExecutable.load()` or - * `XsltExecutable.load30()`. - */ -private predicate xsltExecutableStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - n1.asExpr() = ma.getQualifier() and - n2.asExpr() = ma and - m.getDeclaringType() instanceof TypeXsltExecutable and - m.hasName(["load", "load30"]) - ) -} - -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `XsltPackage` and - * `XsltExecutable`, i.e. `XsltPackage.link()`. - */ -private predicate xsltPackageStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - n1.asExpr() = ma.getQualifier() and - n2.asExpr() = ma and - m.getDeclaringType() instanceof TypeXsltPackage and - m.hasName("link") - ) -} - -/** The class `javax.xml.transform.stax.StAXSource`. */ -private class TypeStAXSource extends Class { - TypeStAXSource() { this.hasQualifiedName("javax.xml.transform.stax", "StAXSource") } -} - -/** The class `javax.xml.transform.dom.DOMSource`. */ -private class TypeDOMSource extends Class { - TypeDOMSource() { this.hasQualifiedName("javax.xml.transform.dom", "DOMSource") } -} - -/** The interface `javax.xml.transform.Templates`. */ -private class TypeTemplates extends Interface { - TypeTemplates() { this.hasQualifiedName("javax.xml.transform", "Templates") } -} - -/** The class `net.sf.saxon.s9api.XsltCompiler`. */ -private class TypeXsltCompiler extends Class { - TypeXsltCompiler() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltCompiler") } -} - -/** The class `net.sf.saxon.s9api.XsltExecutable`. */ -private class TypeXsltExecutable extends Class { - TypeXsltExecutable() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltExecutable") } -} - -/** The class `net.sf.saxon.s9api.XsltPackage`. */ -private class TypeXsltPackage extends Class { - TypeXsltPackage() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltPackage") } -} diff --git a/java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll b/java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll deleted file mode 100644 index 599e2e963d30..000000000000 --- a/java/ql/src/semmle/code/java/security/XsltInjectionSinkModels.qll +++ /dev/null @@ -1,17 +0,0 @@ -/** Provides sink models relating to XSLT injection vulnerabilities. */ - -import semmle.code.java.dataflow.ExternalFlow - -private class DefaultXsltInjectionSinkModel extends SinkModelCsv { - override predicate row(string row) { - row = - [ - "javax.xml.transform;Transformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;XsltTransformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;transform;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;applyTemplates;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;callFunction;;;Argument[-1];xslt", - "net.sf.saxon.s9api;Xslt30Transformer;false;callTemplate;;;Argument[-1];xslt" - ] - } -} From 13417dbf14f73eb86443e6579363bf314890bb3a Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Wed, 21 Jul 2021 11:12:31 +0200 Subject: [PATCH 07/10] Remove DataFlow references from XsltInjection.qll --- .../code/java/security/XsltInjection.qll | 112 +++++++++--------- .../code/java/security/XsltInjectionQuery.qll | 65 ++++++++++ 2 files changed, 118 insertions(+), 59 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/XsltInjection.qll b/java/ql/src/semmle/code/java/security/XsltInjection.qll index 1ebf961e03c4..752884920046 100644 --- a/java/ql/src/semmle/code/java/security/XsltInjection.qll +++ b/java/ql/src/semmle/code/java/security/XsltInjection.qll @@ -3,7 +3,6 @@ import java import semmle.code.java.dataflow.DataFlow import semmle.code.java.dataflow.ExternalFlow -import semmle.code.java.security.XmlParsers /** * A data flow sink for unvalidated user input that is used in XSLT transformation. @@ -51,7 +50,6 @@ private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAddit staxSourceStep(node1, node2) or documentBuilderStep(node1, node2) or domSourceStep(node1, node2) or - newTransformerOrTemplatesStep(node1, node2) or newTransformerFromTemplatesStep(node1, node2) or xsltCompilerStep(node1, node2) or xsltExecutableStep(node1, node2) or @@ -65,7 +63,10 @@ private class DefaultXsltInjectionAdditionalTaintStep extends XsltInjectionAddit */ private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) { exists(XmlInputFactoryStreamReader xmlStreamReader | - n1.asExpr() = xmlStreamReader.getSink() and + if xmlStreamReader.getMethod().getParameterType(0) instanceof TypeString + then n1.asExpr() = xmlStreamReader.getArgument(1) + else n1.asExpr() = xmlStreamReader.getArgument(0) + | n2.asExpr() = xmlStreamReader ) } @@ -76,7 +77,10 @@ private predicate xmlStreamReaderStep(DataFlow::Node n1, DataFlow::Node n2) { */ private predicate xmlEventReaderStep(DataFlow::Node n1, DataFlow::Node n2) { exists(XmlInputFactoryEventReader xmlEventReader | - n1.asExpr() = xmlEventReader.getSink() and + if xmlEventReader.getMethod().getParameterType(0) instanceof TypeString + then n1.asExpr() = xmlEventReader.getArgument(1) + else n1.asExpr() = xmlEventReader.getArgument(0) + | n2.asExpr() = xmlEventReader ) } @@ -98,7 +102,7 @@ private predicate staxSourceStep(DataFlow::Node n1, DataFlow::Node n2) { */ private predicate documentBuilderStep(DataFlow::Node n1, DataFlow::Node n2) { exists(DocumentBuilderParse documentBuilder | - n1.asExpr() = documentBuilder.getSink() and + n1.asExpr() = documentBuilder.getArgument(0) and n2.asExpr() = documentBuilder ) } @@ -114,60 +118,6 @@ private predicate domSourceStep(DataFlow::Node n1, DataFlow::Node n2) { ) } -/** - * Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or - * `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or - * `TransformerFactory.newTemplates(tainted)`. - */ -private predicate newTransformerOrTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { - exists(MethodAccess ma, Method m | ma.getMethod() = m | - n1.asExpr() = ma.getAnArgument() and - n2.asExpr() = ma and - m.getDeclaringType() instanceof TransformerFactory and - m.hasName(["newTransformer", "newTemplates"]) and - not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf | - conf.hasFlowToExpr(ma.getQualifier()) - ) - ) -} - -/** - * A data flow configuration for secure processing feature that is enabled on `TransformerFactory`. - */ -private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends DataFlow2::Configuration { - TransformerFactoryWithSecureProcessingFeatureFlowConfig() { - this = "TransformerFactoryWithSecureProcessingFeatureFlowConfig" - } - - override predicate isSource(DataFlow::Node src) { - exists(Variable v | v = src.asExpr().(VarAccess).getVariable() | - exists(TransformerFactoryFeatureConfig config | config.getQualifier() = v.getAnAccess() | - config.enables(configSecureProcessing()) - ) - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - sink.asExpr() = ma.getQualifier() and - ma.getMethod().getDeclaringType() instanceof TransformerFactory - ) - } - - override int fieldFlowBranchLimit() { result = 0 } -} - -/** A `ParserConfig` specific to `TransformerFactory`. */ -private class TransformerFactoryFeatureConfig extends ParserConfig { - TransformerFactoryFeatureConfig() { - exists(Method m | - m = this.getMethod() and - m.getDeclaringType() instanceof TransformerFactory and - m.hasName("setFeature") - ) - } -} - /** * Holds if `n1` to `n2` is a dataflow step that converts between `Templates` and `Transformer`, * i.e. `tainted.newTransformer()`. @@ -252,3 +202,47 @@ private class TypeXsltExecutable extends Class { private class TypeXsltPackage extends Class { TypeXsltPackage() { this.hasQualifiedName("net.sf.saxon.s9api", "XsltPackage") } } + +// XmlParsers classes +/** A call to `DocumentBuilder.parse`. */ +private class DocumentBuilderParse extends MethodAccess { + DocumentBuilderParse() { + exists(Method m | + this.getMethod() = m and + m.getDeclaringType() instanceof DocumentBuilder and + m.hasName("parse") + ) + } +} + +/** The class `javax.xml.parsers.DocumentBuilder`. */ +private class DocumentBuilder extends RefType { + DocumentBuilder() { this.hasQualifiedName("javax.xml.parsers", "DocumentBuilder") } +} + +/** A call to `XMLInputFactory.createXMLStreamReader`. */ +private class XmlInputFactoryStreamReader extends MethodAccess { + XmlInputFactoryStreamReader() { + exists(Method m | + this.getMethod() = m and + m.getDeclaringType() instanceof XmlInputFactory and + m.hasName("createXMLStreamReader") + ) + } +} + +/** A call to `XMLInputFactory.createEventReader`. */ +private class XmlInputFactoryEventReader extends MethodAccess { + XmlInputFactoryEventReader() { + exists(Method m | + this.getMethod() = m and + m.getDeclaringType() instanceof XmlInputFactory and + m.hasName("createXMLEventReader") + ) + } +} + +/** The class `javax.xml.stream.XMLInputFactory`. */ +private class XmlInputFactory extends RefType { + XmlInputFactory() { this.hasQualifiedName("javax.xml.stream", "XMLInputFactory") } +} diff --git a/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll b/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll index 8beafb22e10b..34e533c00405 100644 --- a/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll +++ b/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll @@ -3,6 +3,7 @@ import java import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.security.XmlParsers import semmle.code.java.security.XsltInjection /** @@ -23,3 +24,67 @@ class XsltInjectionFlowConfig extends TaintTracking::Configuration { any(XsltInjectionAdditionalTaintStep c).step(node1, node2) } } + +/** + * A set of additional taint steps to consider when taint tracking XSLT related data flows. + * These steps use data flow logic themselves. + */ +private class DataFlowXsltInjectionAdditionalTaintStep extends XsltInjectionAdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + newTransformerOrTemplatesStep(node1, node2) + } +} + +/** + * Holds if `n1` to `n2` is a dataflow step that converts between `Source` and `Transformer` or + * `Templates`, i.e. `TransformerFactory.newTransformer(tainted)` or + * `TransformerFactory.newTemplates(tainted)`. + */ +private predicate newTransformerOrTemplatesStep(DataFlow::Node n1, DataFlow::Node n2) { + exists(MethodAccess ma, Method m | ma.getMethod() = m | + n1.asExpr() = ma.getAnArgument() and + n2.asExpr() = ma and + m.getDeclaringType() instanceof TransformerFactory and + m.hasName(["newTransformer", "newTemplates"]) and + not exists(TransformerFactoryWithSecureProcessingFeatureFlowConfig conf | + conf.hasFlowToExpr(ma.getQualifier()) + ) + ) +} + +/** + * A data flow configuration for secure processing feature that is enabled on `TransformerFactory`. + */ +private class TransformerFactoryWithSecureProcessingFeatureFlowConfig extends DataFlow2::Configuration { + TransformerFactoryWithSecureProcessingFeatureFlowConfig() { + this = "TransformerFactoryWithSecureProcessingFeatureFlowConfig" + } + + override predicate isSource(DataFlow::Node src) { + exists(Variable v | v = src.asExpr().(VarAccess).getVariable() | + exists(TransformerFactoryFeatureConfig config | config.getQualifier() = v.getAnAccess() | + config.enables(configSecureProcessing()) + ) + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + sink.asExpr() = ma.getQualifier() and + ma.getMethod().getDeclaringType() instanceof TransformerFactory + ) + } + + override int fieldFlowBranchLimit() { result = 0 } +} + +/** A `ParserConfig` specific to `TransformerFactory`. */ +private class TransformerFactoryFeatureConfig extends ParserConfig { + TransformerFactoryFeatureConfig() { + exists(Method m | + m = this.getMethod() and + m.getDeclaringType() instanceof TransformerFactory and + m.hasName("setFeature") + ) + } +} From 95751fcc21cfe82e7aac3418c3046ec34496135c Mon Sep 17 00:00:00 2001 From: mc <42146119+mchammer01@users.noreply.github.com> Date: Thu, 29 Jul 2021 11:17:39 +0100 Subject: [PATCH 08/10] Update XsltInjection.qhelp Made a few minor tweaks during editorial review --- java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp b/java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp index 4392550d33fb..402f4f7f5db1 100644 --- a/java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp +++ b/java/ql/src/Security/CWE/CWE-074/XsltInjection.qhelp @@ -4,12 +4,12 @@

XSLT (Extensible Stylesheet Language Transformations) is a language for transforming XML -documents into other XML documents or other formats. Processing of unvalidated XSLT stylesheet can -let attacker to read arbitrary files from the filesystem or to execute arbitrary code.

+documents into other XML documents or other formats. Processing unvalidated XSLT stylesheets can +allow attackers to read arbitrary files from the filesystem or to execute arbitrary code.

 -

The general recommendation is to not process untrusted XSLT stylesheets. If user provided +

The general recommendation is to not process untrusted XSLT stylesheets. If user-provided stylesheets must be processed, enable the secure processing mode.

 @@ -17,7 +17,7 @@ stylesheets must be processed, enable the secure processing mode.

In the following examples, the code accepts an XSLT stylesheet from the user and processes it.

-

In the first example, the user provided XSLT stylesheet is parsed and processed.

+

In the first example, the user-provided XSLT stylesheet is parsed and processed.

In the second example, secure processing mode is enabled.

From ad08ccb50b40c6eef2f8b99d06ad317bd071fd03 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 29 Jul 2021 16:57:04 +0200 Subject: [PATCH 09/10] Apply suggestion from code review --- java/ql/src/Security/CWE/CWE-074/XsltInjection.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql b/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql index 3728ac9f3eeb..aeec7441cb4c 100644 --- a/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql +++ b/java/ql/src/Security/CWE/CWE-074/XsltInjection.ql @@ -1,6 +1,6 @@ /** * @name XSLT transformation with user-controlled stylesheet - * @description Doing an XSLT transformation with user-controlled stylesheet can lead to + * @description Performing an XSLT transformation with user-controlled stylesheets can lead to * information disclosure or execution of arbitrary code. * @kind path-problem * @problem.severity error From 78c12dc505b6732498a7c4cca7d6af141ccd9773 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Mon, 27 Sep 2021 12:04:14 +0200 Subject: [PATCH 10/10] Move to lib --- java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll | 2 +- .../ql/{src => lib}/semmle/code/java/security/XsltInjection.qll | 0 .../semmle/code/java/security/XsltInjectionQuery.qll | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename java/ql/{src => lib}/semmle/code/java/security/XsltInjection.qll (100%) rename java/ql/{src => lib}/semmle/code/java/security/XsltInjectionQuery.qll (100%) diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index f88cdcb15935..7f46f2d79b60 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -110,7 +110,7 @@ private module Frameworks { private import semmle.code.java.security.MvelInjection private import semmle.code.java.security.OgnlInjection private import semmle.code.java.security.XPath - private import semmle.code.java.security.XsltInjectionSinkModels + private import semmle.code.java.security.XsltInjection private import semmle.code.java.frameworks.android.Android 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/XsltInjection.qll b/java/ql/lib/semmle/code/java/security/XsltInjection.qll similarity index 100% rename from java/ql/src/semmle/code/java/security/XsltInjection.qll rename to java/ql/lib/semmle/code/java/security/XsltInjection.qll diff --git a/java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll b/java/ql/lib/semmle/code/java/security/XsltInjectionQuery.qll similarity index 100% rename from java/ql/src/semmle/code/java/security/XsltInjectionQuery.qll rename to java/ql/lib/semmle/code/java/security/XsltInjectionQuery.qll