Merge pull request #9047 from geoffw0/xxe6

C++: Add support for SAX2XMLReader in the CWE-611 XXE query.
2026-05-02 12:15:17 +02:00 · 2022-05-10 19:07:35 +01:00
parent 1d10f14629 85cc9b8901
commit a7fe37a631
4 changed files with 115 additions and 8 deletions
--- a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
+++ b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql
@@ -60,8 +60,15 @@ class XercesDOMParserClass extends Class {
 /**
 * The `SAXParser` class.
 */
-class SAXParserClass extends Class {
-  SAXParserClass() { this.hasName("SAXParser") }
+class SaxParserClass extends Class {
+  SaxParserClass() { this.hasName("SAXParser") }
+}
+
+/**
+ * The `SAX2XMLReader` class.
+ */
+class Sax2XmlReader extends Class {
+  Sax2XmlReader() { this.hasName("SAX2XMLReader") }
 }

 /**
@@ -113,7 +120,7 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
      call.getTarget() = f and
      (
        f.getDeclaringType() instanceof AbstractDOMParserClass or
-        f.getDeclaringType() instanceof SAXParserClass
+        f.getDeclaringType() instanceof SaxParserClass
      ) and
      f.hasName("setDisableDefaultEntityResolution") and
      this = call.getQualifier() and
@@ -146,8 +153,7 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {
  CreateEntityReferenceNodesTranformer() {
    exists(Call call, Function f |
      call.getTarget() = f and
-      f.getDeclaringType() instanceof AbstractDOMParserClass and
-      f.hasName("setCreateEntityReferenceNodes") and
+      f.getClassAndName("setCreateEntityReferenceNodes") instanceof AbstractDOMParserClass and
      this = call.getQualifier() and
      newValue = call.getArgument(0)
    )
@@ -168,12 +174,57 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {
 }

 /**
- * The `AbstractDOMParser.parse` or `SAXParser.parse` method.
+ * The `XMLUni.fgXercesDisableDefaultEntityResolution` constant.
+ */
+class FeatureDisableDefaultEntityResolution extends Variable {
+  FeatureDisableDefaultEntityResolution() {
+    this.getName() = "fgXercesDisableDefaultEntityResolution" and
+    this.getDeclaringType().getName() = "XMLUni"
+  }
+}
+
+/**
+ * A flow state transformer for a call to `SAX2XMLReader.setFeature`
+ * specifying the feature `XMLUni::fgXercesDisableDefaultEntityResolution`.
+ * Transforms the flow state through the qualifier according to this setting.
+ */
+class SetFeatureTranformer extends XXEFlowStateTranformer {
+  Expr newValue;
+
+  SetFeatureTranformer() {
+    exists(Call call, Function f |
+      call.getTarget() = f and
+      f.getClassAndName("setFeature") instanceof Sax2XmlReader and
+      this = call.getQualifier() and
+      globalValueNumber(call.getArgument(0)).getAnExpr().(VariableAccess).getTarget() instanceof
+        FeatureDisableDefaultEntityResolution and
+      newValue = call.getArgument(1)
+    )
+  }
+
+  final override XXEFlowState transform(XXEFlowState flowstate) {
+    exists(int createEntityReferenceNodes |
+      encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and
+      (
+        globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
+        encodeXercesFlowState(result, 1, createEntityReferenceNodes)
+        or
+        not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
+        encodeXercesFlowState(result, 0, createEntityReferenceNodes)
+      )
+    )
+  }
+}
+
+/**
+ * The `AbstractDOMParser.parse`, `SAXParser.parse` or `SAX2XMLReader.parse`
+ * method.
 */
 class ParseFunction extends Function {
  ParseFunction() {
    this.getClassAndName("parse") instanceof AbstractDOMParserClass or
-    this.getClassAndName("parse") instanceof SAXParserClass
+    this.getClassAndName("parse") instanceof SaxParserClass or
+    this.getClassAndName("parse") instanceof Sax2XmlReader
  }
 }

@@ -188,6 +239,17 @@ class CreateLSParser extends Function {
  }
 }

+/**
+ * The `createXMLReader` function that returns a newly created `SAX2XMLReader`
+ * object.
+ */
+class CreateXmlReader extends Function {
+  CreateXmlReader() {
+    this.hasName("createXMLReader") and
+    this.getUnspecifiedType().(PointerType).getBaseType() instanceof Sax2XmlReader // returns a `SAX2XMLReader *`.
+  }
+}
+
 /**
 * A call to a `libxml2` function that parses XML.
 */
@@ -250,12 +312,19 @@ class XXEConfiguration extends DataFlow::Configuration {
    // source is the write on `this` of a call to the `SAXParser`
    // constructor.
    exists(CallInstruction call |
-      call.getStaticCallTarget() = any(SAXParserClass c).getAConstructor() and
+      call.getStaticCallTarget() = any(SaxParserClass c).getAConstructor() and
      node.asInstruction().(WriteSideEffectInstruction).getDestinationAddress() =
        call.getThisArgument() and
      encodeXercesFlowState(flowstate, 0, 1) // default configuration
    )
    or
+    // source is the result of a call to `createXMLReader`.
+    exists(Call call |
+      call.getTarget() instanceof CreateXmlReader and
+      call = node.asExpr() and
+      encodeXercesFlowState(flowstate, 0, 1) // default configuration
+    )
+    or
    // source is an `options` argument on a `libxml2` parse call that specifies
    // at least one unsafe option.
    //
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected
@@ -1,6 +1,9 @@
 edges
 | tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p |
 | tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p |
+| tests3.cpp:23:21:23:53 | call to createXMLReader | tests3.cpp:25:2:25:2 | p |
+| tests3.cpp:60:21:60:53 | call to createXMLReader | tests3.cpp:63:2:63:2 | p |
+| tests3.cpp:67:21:67:53 | call to createXMLReader | tests3.cpp:70:2:70:2 | p |
 | tests.cpp:15:23:15:43 | XercesDOMParser output argument | tests.cpp:17:2:17:2 | p |
 | tests.cpp:28:23:28:43 | XercesDOMParser output argument | tests.cpp:31:2:31:2 | p |
 | tests.cpp:35:19:35:19 | VariableAddress [post update] | tests.cpp:37:2:37:2 | p |
@@ -32,6 +35,12 @@ nodes
 | tests2.cpp:22:2:22:2 | p | semmle.label | p |
 | tests2.cpp:33:17:33:31 | SAXParser output argument | semmle.label | SAXParser output argument |
 | tests2.cpp:37:2:37:2 | p | semmle.label | p |
+| tests3.cpp:23:21:23:53 | call to createXMLReader | semmle.label | call to createXMLReader |
+| tests3.cpp:25:2:25:2 | p | semmle.label | p |
+| tests3.cpp:60:21:60:53 | call to createXMLReader | semmle.label | call to createXMLReader |
+| tests3.cpp:63:2:63:2 | p | semmle.label | p |
+| tests3.cpp:67:21:67:53 | call to createXMLReader | semmle.label | call to createXMLReader |
+| tests3.cpp:70:2:70:2 | p | semmle.label | p |
 | tests4.cpp:26:34:26:48 | (int)... | semmle.label | (int)... |
 | tests4.cpp:36:34:36:50 | (int)... | semmle.label | (int)... |
 | tests4.cpp:46:34:46:68 | ... \| ... | semmle.label | ... \| ... |
@@ -76,6 +85,9 @@ subpaths
 #select
 | tests2.cpp:22:2:22:2 | p | tests2.cpp:20:17:20:31 | SAXParser output argument | tests2.cpp:22:2:22:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:20:17:20:31 | SAXParser output argument | XML parser |
 | tests2.cpp:37:2:37:2 | p | tests2.cpp:33:17:33:31 | SAXParser output argument | tests2.cpp:37:2:37:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:33:17:33:31 | SAXParser output argument | XML parser |
+| tests3.cpp:25:2:25:2 | p | tests3.cpp:23:21:23:53 | call to createXMLReader | tests3.cpp:25:2:25:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests3.cpp:23:21:23:53 | call to createXMLReader | XML parser |
+| tests3.cpp:63:2:63:2 | p | tests3.cpp:60:21:60:53 | call to createXMLReader | tests3.cpp:63:2:63:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests3.cpp:60:21:60:53 | call to createXMLReader | XML parser |
+| tests3.cpp:70:2:70:2 | p | tests3.cpp:67:21:67:53 | call to createXMLReader | tests3.cpp:70:2:70:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests3.cpp:67:21:67:53 | call to createXMLReader | XML parser |
 | tests4.cpp:26:34:26:48 | (int)... | tests4.cpp:26:34:26:48 | (int)... | tests4.cpp:26:34:26:48 | (int)... | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests4.cpp:26:34:26:48 | (int)... | XML parser |
 | tests4.cpp:36:34:36:50 | (int)... | tests4.cpp:36:34:36:50 | (int)... | tests4.cpp:36:34:36:50 | (int)... | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests4.cpp:36:34:36:50 | (int)... | XML parser |
 | tests4.cpp:46:34:46:68 | ... \| ... | tests4.cpp:46:34:46:68 | ... \| ... | tests4.cpp:46:34:46:68 | ... \| ... | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests4.cpp:46:34:46:68 | ... \| ... | XML parser |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h
@@ -21,5 +21,6 @@ class XMLUni
 {
 public:
 	static const XMLCh fgXercesDisableDefaultEntityResolution[];
+	static const XMLCh fgXercesHarmlessOption[];
 };

--- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests3.cpp
@@ -55,3 +55,28 @@ void test3_5(InputSource &data) {
 	test3_5_init();
 	p_3_5->parse(data); // GOOD
 }
+
+void test3_6(InputSource &data) {
+	SAX2XMLReader *p = XMLReaderFactory::createXMLReader();
+
+	p->setFeature(XMLUni::fgXercesDisableDefaultEntityResolution, false);
+	p->parse(data); // BAD (parser not correctly configured)
+}
+
+void test3_7(InputSource &data) {
+	SAX2XMLReader *p = XMLReaderFactory::createXMLReader();
+
+	p->setFeature(XMLUni::fgXercesHarmlessOption, true);
+	p->parse(data); // BAD (parser not correctly configured)
+}
+
+void test3_8(InputSource &data) {
+	SAX2XMLReader *p = XMLReaderFactory::createXMLReader();
+	const XMLCh *feature = XMLUni::fgXercesDisableDefaultEntityResolution;
+
+	p->setFeature(feature, true);
+	p->parse(data); // GOOD
+}
+
+
+