From 30cbc91092d3206b8a518bcb0fad612d5c4be75f Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Wed, 8 Mar 2023 14:57:44 +0100 Subject: [PATCH] C++: Update XXE XML query with `DataFlow::ConfigSig` --- cpp/ql/src/Security/CWE/CWE-611/Libxml2.qll | 8 +-- cpp/ql/src/Security/CWE/CWE-611/XML.qll | 24 +++++-- cpp/ql/src/Security/CWE/CWE-611/XXE.ql | 22 +++--- cpp/ql/src/Security/CWE/CWE-611/Xerces.qll | 76 ++++++++++----------- 4 files changed, 68 insertions(+), 62 deletions(-) diff --git a/cpp/ql/src/Security/CWE/CWE-611/Libxml2.qll b/cpp/ql/src/Security/CWE/CWE-611/Libxml2.qll index c20849fcd9e..69fc9369cd8 100644 --- a/cpp/ql/src/Security/CWE/CWE-611/Libxml2.qll +++ b/cpp/ql/src/Security/CWE/CWE-611/Libxml2.qll @@ -48,7 +48,7 @@ class Libxml2BadOption extends EnumConstant { class LibXml2Library extends XmlLibrary { LibXml2Library() { this = "LibXml2Library" } - override predicate configurationSource(DataFlow::Node node, string flowstate) { + override predicate configurationSource(DataFlow::Node node, TXxeFlowState flowstate) { // source is an `options` argument on a libxml2 parse call that specifies // at least one unsafe option. // @@ -59,7 +59,7 @@ class LibXml2Library extends XmlLibrary { exists(Libxml2ParseCall call, Expr options | options = call.getOptions() and node.asExpr() = options and - flowstate = "libxml2" and + flowstate instanceof TLibXml2FlowState and exists(Libxml2BadOption opt | globalValueNumber(options).getAnExpr().getValue().toInt().bitAnd(opt.getValue().toInt()) != 0 @@ -67,12 +67,12 @@ class LibXml2Library extends XmlLibrary { ) } - override predicate configurationSink(DataFlow::Node node, string flowstate) { + override predicate configurationSink(DataFlow::Node node, TXxeFlowState flowstate) { // sink is the `options` argument on a `libxml2` parse call. exists(Libxml2ParseCall call, Expr options | options = call.getOptions() and node.asExpr() = options and - flowstate = "libxml2" + flowstate instanceof TLibXml2FlowState ) } } diff --git a/cpp/ql/src/Security/CWE/CWE-611/XML.qll b/cpp/ql/src/Security/CWE/CWE-611/XML.qll index 5d071ed7654..824e786151a 100644 --- a/cpp/ql/src/Security/CWE/CWE-611/XML.qll +++ b/cpp/ql/src/Security/CWE/CWE-611/XML.qll @@ -11,10 +11,20 @@ import Libxml2 /** * A flow state representing a possible configuration of an XML object. */ -abstract class XxeFlowState extends DataFlow::FlowState { - bindingset[this] - XxeFlowState() { any() } // required characteristic predicate -} +newtype TXxeFlowState = + /** + * Flow state for `AbstractDOMParser` or `SAXParser`, where: + * - `disabledDefaultEntityResolution` is 1 if `setDisableDefaultEntityResolution` + * is `true`, 0 otherwise. + * - `createEntityReferenceNodes` is 1 if `setCreateEntityReferenceNodes` + * is `true`, 0 otherwise. + */ + TXercesFlowState(int disabledDefaultEntityResolution, int createEntityReferenceNodes) { + disabledDefaultEntityResolution in [0, 1] and + createEntityReferenceNodes in [0, 1] + } or + /** Flow state for libxml2 */ + TLibXml2FlowState() /** * An XML library or interface. @@ -28,13 +38,13 @@ abstract class XmlLibrary extends string { * object for this XML library, along with `flowstate` representing its * initial state. */ - abstract predicate configurationSource(DataFlow::Node node, string flowstate); + abstract predicate configurationSource(DataFlow::Node node, TXxeFlowState flowstate); /** * Holds if `node` is the sink node where an unsafe configuration object is * used to interpret XML. */ - abstract predicate configurationSink(DataFlow::Node node, string flowstate); + abstract predicate configurationSink(DataFlow::Node node, TXxeFlowState flowstate); } /** @@ -51,5 +61,5 @@ abstract class XxeFlowStateTransformer extends Expr { * transform(transform(x)) = transform(x) * ``` */ - abstract XxeFlowState transform(XxeFlowState flowstate); + abstract TXxeFlowState transform(TXxeFlowState flowstate); } diff --git a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql index 10e2b8571bd..0ec8ffd3b0a 100644 --- a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql +++ b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql @@ -14,38 +14,40 @@ import cpp import XML -import DataFlow::PathGraph +import XxeFlow::PathGraph /** * A configuration for tracking XML objects and their states. */ -class XxeConfiguration extends DataFlow::Configuration { - XxeConfiguration() { this = "XXEConfiguration" } +module XxeConfiguration implements DataFlow::StateConfigSig { + class FlowState = TXxeFlowState; - override predicate isSource(DataFlow::Node node, string flowstate) { + predicate isSource(DataFlow::Node node, FlowState flowstate) { any(XmlLibrary l).configurationSource(node, flowstate) } - override predicate isSink(DataFlow::Node node, string flowstate) { + predicate isSink(DataFlow::Node node, FlowState flowstate) { any(XmlLibrary l).configurationSink(node, flowstate) } - override predicate isAdditionalFlowStep( - DataFlow::Node node1, string state1, DataFlow::Node node2, string state2 + predicate isAdditionalFlowStep( + DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 ) { // create additional flow steps for `XxeFlowStateTransformer`s state2 = node2.asIndirectExpr().(XxeFlowStateTransformer).transform(state1) and DataFlow::simpleLocalFlowStep(node1, node2) } - override predicate isBarrier(DataFlow::Node node, string flowstate) { + predicate isBarrier(DataFlow::Node node, FlowState flowstate) { // when the flowstate is transformed at a call node, block the original // flowstate value. node.asIndirectExpr().(XxeFlowStateTransformer).transform(flowstate) != flowstate } } -from XxeConfiguration conf, DataFlow::PathNode source, DataFlow::PathNode sink -where conf.hasFlowPath(source, sink) +module XxeFlow = DataFlow::MakeWithState; + +from XxeFlow::PathNode source, XxeFlow::PathNode sink +where XxeFlow::hasFlowPath(source, sink) select sink, source, sink, "This $@ is not configured to prevent an XML external entity (XXE) attack.", source, "XML parser" diff --git a/cpp/ql/src/Security/CWE/CWE-611/Xerces.qll b/cpp/ql/src/Security/CWE/CWE-611/Xerces.qll index 96f1631476b..f84cd6d48eb 100644 --- a/cpp/ql/src/Security/CWE/CWE-611/Xerces.qll +++ b/cpp/ql/src/Security/CWE/CWE-611/Xerces.qll @@ -7,39 +7,33 @@ import XML import semmle.code.cpp.valuenumbering.GlobalValueNumbering import semmle.code.cpp.ir.IR -/** - * Gets a valid flow state for `AbstractDOMParser` or `SAXParser` flow. - * - * These flow states take the form `Xerces-A-B`, where: - * - A is 1 if `setDisableDefaultEntityResolution` is `true`, 0 otherwise. - * - B is 1 if `setCreateEntityReferenceNodes` is `true`, 0 otherwise. - */ -predicate encodeXercesFlowState( - string flowstate, int disabledDefaultEntityResolution, int createEntityReferenceNodes -) { - flowstate = "Xerces-0-0" and - disabledDefaultEntityResolution = 0 and - createEntityReferenceNodes = 0 - or - flowstate = "Xerces-0-1" and - disabledDefaultEntityResolution = 0 and - createEntityReferenceNodes = 1 - or - flowstate = "Xerces-1-0" and - disabledDefaultEntityResolution = 1 and - createEntityReferenceNodes = 0 - or - flowstate = "Xerces-1-1" and - disabledDefaultEntityResolution = 1 and - createEntityReferenceNodes = 1 -} - /** * A flow state representing the configuration of an `AbstractDOMParser` or * `SAXParser` object. */ -class XercesFlowState extends XxeFlowState { - XercesFlowState() { encodeXercesFlowState(this, _, _) } +class XercesFlowState extends TXxeFlowState { + int disabledDefaultEntityResolution; + int createEntityReferenceNodes; + + XercesFlowState() { + this = TXercesFlowState(disabledDefaultEntityResolution, createEntityReferenceNodes) + } + + int getDisabledDefaultEntityResolution() { result = disabledDefaultEntityResolution } + + int getCreateEntityReferenceNodes() { result = createEntityReferenceNodes } + + string toString() { result = "XercesFlowState" } +} + +/** + * Gets a valid flow state for `AbstractDOMParser` or `SAXParser` flow. + */ +predicate encodeXercesFlowState( + XercesFlowState flowstate, int disabledDefaultEntityResolution, int createEntityReferenceNodes +) { + flowstate.getDisabledDefaultEntityResolution() = disabledDefaultEntityResolution and + flowstate.getCreateEntityReferenceNodes() = createEntityReferenceNodes } /** @@ -62,7 +56,7 @@ class XercesDomParserClass extends Class { class XercesDomParserLibrary extends XmlLibrary { XercesDomParserLibrary() { this = "XercesDomParserLibrary" } - override predicate configurationSource(DataFlow::Node node, string flowstate) { + override predicate configurationSource(DataFlow::Node node, TXxeFlowState flowstate) { // source is the write on `this` of a call to the `XercesDOMParser` // constructor. exists(Call call | @@ -72,7 +66,7 @@ class XercesDomParserLibrary extends XmlLibrary { ) } - override predicate configurationSink(DataFlow::Node node, string flowstate) { + override predicate configurationSink(DataFlow::Node node, TXxeFlowState flowstate) { // sink is the read of the qualifier of a call to `AbstractDOMParser.parse`. exists(Call call | call.getTarget().getClassAndName("parse") instanceof AbstractDomParserClass and @@ -107,7 +101,7 @@ class CreateLSParser extends Function { class CreateLSParserLibrary extends XmlLibrary { CreateLSParserLibrary() { this = "CreateLSParserLibrary" } - override predicate configurationSource(DataFlow::Node node, string flowstate) { + override predicate configurationSource(DataFlow::Node node, TXxeFlowState flowstate) { // source is the result of a call to `createLSParser`. exists(Call call | call.getTarget() instanceof CreateLSParser and @@ -116,7 +110,7 @@ class CreateLSParserLibrary extends XmlLibrary { ) } - override predicate configurationSink(DataFlow::Node node, string flowstate) { + override predicate configurationSink(DataFlow::Node node, TXxeFlowState flowstate) { // sink is the read of the qualifier of a call to `DOMLSParserClass.parse`. exists(Call call | call.getTarget().getClassAndName("parse") instanceof DomLSParserClass and @@ -147,7 +141,7 @@ class Sax2XmlReader extends Class { class SaxParserLibrary extends XmlLibrary { SaxParserLibrary() { this = "SaxParserLibrary" } - override predicate configurationSource(DataFlow::Node node, string flowstate) { + override predicate configurationSource(DataFlow::Node node, TXxeFlowState flowstate) { // source is the write on `this` of a call to the `SAXParser` // constructor. exists(Call call | @@ -157,7 +151,7 @@ class SaxParserLibrary extends XmlLibrary { ) } - override predicate configurationSink(DataFlow::Node node, string flowstate) { + override predicate configurationSink(DataFlow::Node node, TXxeFlowState flowstate) { // sink is the read of the qualifier of a call to `SAXParser.parse`. exists(Call call | call.getTarget().getClassAndName("parse") instanceof SaxParserClass and @@ -185,7 +179,7 @@ class CreateXmlReader extends Function { class Sax2XmlReaderLibrary extends XmlLibrary { Sax2XmlReaderLibrary() { this = "Sax2XmlReaderLibrary" } - override predicate configurationSource(DataFlow::Node node, string flowstate) { + override predicate configurationSource(DataFlow::Node node, TXxeFlowState flowstate) { // source is the result of a call to `createXMLReader`. exists(Call call | call.getTarget() instanceof CreateXmlReader and @@ -194,7 +188,7 @@ class Sax2XmlReaderLibrary extends XmlLibrary { ) } - override predicate configurationSink(DataFlow::Node node, string flowstate) { + override predicate configurationSink(DataFlow::Node node, TXxeFlowState flowstate) { // sink is the read of the qualifier of a call to `SAX2XMLReader.parse`. exists(Call call | call.getTarget().getClassAndName("parse") instanceof Sax2XmlReader and @@ -227,7 +221,7 @@ class DisableDefaultEntityResolutionTransformer extends XxeFlowStateTransformer ) } - final override XxeFlowState transform(XxeFlowState flowstate) { + final override TXxeFlowState transform(TXxeFlowState flowstate) { exists(int createEntityReferenceNodes | encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and ( @@ -258,7 +252,7 @@ class CreateEntityReferenceNodesTransformer extends XxeFlowStateTransformer { ) } - final override XxeFlowState transform(XxeFlowState flowstate) { + final override TXxeFlowState transform(TXxeFlowState flowstate) { exists(int disabledDefaultEntityResolution | encodeXercesFlowState(flowstate, disabledDefaultEntityResolution, _) and ( @@ -301,7 +295,7 @@ class SetFeatureTransformer extends XxeFlowStateTransformer { ) } - final override XxeFlowState transform(XxeFlowState flowstate) { + final override TXxeFlowState transform(TXxeFlowState flowstate) { exists(int createEntityReferenceNodes | encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and ( @@ -359,7 +353,7 @@ class DomConfigurationSetParameterTransformer extends XxeFlowStateTransformer { ) } - final override XxeFlowState transform(XxeFlowState flowstate) { + final override TXxeFlowState transform(TXxeFlowState flowstate) { exists(int createEntityReferenceNodes | encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and (