Merge pull request #8948 from geoffw0/xxe3

C++: Add support for SAXParser to the CWE-611 XXE query.

cpp/ql/src/Security/CWE/CWE-611/XXE.ql

@@ -16,6 +16,7 @@ import cpp
 import semmle.code.cpp.ir.dataflow.DataFlow
 import DataFlow::PathGraph
 import semmle.code.cpp.ir.IR
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
 /**
  * A flow state representing a possible configuration of an XML object.
@@ -57,42 +58,51 @@ class XercesDOMParserClass extends Class {
 }
 
 /**
- * Gets a valid flow state for `XercesDOMParser` flow.
+ * The `SAXParser` class.
+ */
+class SAXParserClass extends Class {
+  SAXParserClass() { this.hasName("SAXParser") }
+}
+
+/**
+ * Gets a valid flow state for `AbstractDOMParser` or `SAXParser` flow.
  *
- * These flow states take the form `XercesDOM-A-B`, where:
+ * 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 encodeXercesDOMFlowState(
+predicate encodeXercesFlowState(
   string flowstate, int disabledDefaultEntityResolution, int createEntityReferenceNodes
 ) {
-  flowstate = "XercesDOM-0-0" and
+  flowstate = "Xerces-0-0" and
   disabledDefaultEntityResolution = 0 and
   createEntityReferenceNodes = 0
   or
-  flowstate = "XercesDOM-0-1" and
+  flowstate = "Xerces-0-1" and
   disabledDefaultEntityResolution = 0 and
   createEntityReferenceNodes = 1
   or
-  flowstate = "XercesDOM-1-0" and
+  flowstate = "Xerces-1-0" and
   disabledDefaultEntityResolution = 1 and
   createEntityReferenceNodes = 0
   or
-  flowstate = "XercesDOM-1-1" and
+  flowstate = "Xerces-1-1" and
   disabledDefaultEntityResolution = 1 and
   createEntityReferenceNodes = 1
 }
 
 /**
- * A flow state representing the configuration of a `XercesDOMParser` object.
+ * A flow state representing the configuration of an `AbstractDOMParser` or
+ * `SAXParser` object.
  */
-class XercesDOMParserFlowState extends XXEFlowState {
-  XercesDOMParserFlowState() { encodeXercesDOMFlowState(this, _, _) }
+class XercesFlowState extends XXEFlowState {
+  XercesFlowState() { encodeXercesFlowState(this, _, _) }
 }
 
 /**
  * A flow state transformer for a call to
- * `AbstractDOMParser.setDisableDefaultEntityResolution`. Transforms the flow
+ * `AbstractDOMParser.setDisableDefaultEntityResolution` or
+ * `SAXParser.setDisableDefaultEntityResolution`. Transforms the flow
  * state through the qualifier according to the setting in the parameter.
  */
 class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
@@ -101,7 +111,10 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
   DisableDefaultEntityResolutionTranformer() {
     exists(Call call, Function f |
       call.getTarget() = f and
-      f.getDeclaringType() instanceof AbstractDOMParserClass and
+      (
+        f.getDeclaringType() instanceof AbstractDOMParserClass or
+        f.getDeclaringType() instanceof SAXParserClass
+      ) and
       f.hasName("setDisableDefaultEntityResolution") and
       this = call.getQualifier() and
       newValue = call.getArgument(0)
@@ -110,13 +123,13 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
 
   final override XXEFlowState transform(XXEFlowState flowstate) {
     exists(int createEntityReferenceNodes |
-      encodeXercesDOMFlowState(flowstate, _, createEntityReferenceNodes) and
+      encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and
       (
-        newValue.getValue().toInt() = 1 and // true
-        encodeXercesDOMFlowState(result, 1, createEntityReferenceNodes)
+        globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
+        encodeXercesFlowState(result, 1, createEntityReferenceNodes)
         or
-        not newValue.getValue().toInt() = 1 and // false or unknown
-        encodeXercesDOMFlowState(result, 0, createEntityReferenceNodes)
+        not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
+        encodeXercesFlowState(result, 0, createEntityReferenceNodes)
       )
     )
   }
@@ -142,27 +155,31 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {
 
   final override XXEFlowState transform(XXEFlowState flowstate) {
     exists(int disabledDefaultEntityResolution |
-      encodeXercesDOMFlowState(flowstate, disabledDefaultEntityResolution, _) and
+      encodeXercesFlowState(flowstate, disabledDefaultEntityResolution, _) and
       (
-        newValue.getValue().toInt() = 1 and // true
-        encodeXercesDOMFlowState(result, disabledDefaultEntityResolution, 1)
+        globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
+        encodeXercesFlowState(result, disabledDefaultEntityResolution, 1)
         or
-        not newValue.getValue().toInt() = 1 and // false or unknown
-        encodeXercesDOMFlowState(result, disabledDefaultEntityResolution, 0)
+        not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
+        encodeXercesFlowState(result, disabledDefaultEntityResolution, 0)
       )
     )
   }
 }
 
 /**
- * The `AbstractDOMParser.parse` method.
+ * The `AbstractDOMParser.parse` or `SAXParser.parse` method.
  */
 class ParseFunction extends Function {
-  ParseFunction() { this.getClassAndName("parse") instanceof AbstractDOMParserClass }
+  ParseFunction() {
+    this.getClassAndName("parse") instanceof AbstractDOMParserClass or
+    this.getClassAndName("parse") instanceof SAXParserClass
+  }
 }
 
 /**
- * The `createLSParser` function that returns a newly created `LSParser` object.
+ * The `createLSParser` function that returns a newly created `DOMLSParser`
+ * object.
  */
 class CreateLSParser extends Function {
   CreateLSParser() {
@@ -184,14 +201,23 @@ class XXEConfiguration extends DataFlow::Configuration {
       call.getStaticCallTarget() = any(XercesDOMParserClass c).getAConstructor() and
       node.asInstruction().(WriteSideEffectInstruction).getDestinationAddress() =
         call.getThisArgument() and
-      encodeXercesDOMFlowState(flowstate, 0, 1) // default configuration
+      encodeXercesFlowState(flowstate, 0, 1) // default configuration
     )
     or
     // source is the result of a call to `createLSParser`.
     exists(Call call |
       call.getTarget() instanceof CreateLSParser and
       call = node.asExpr() and
-      encodeXercesDOMFlowState(flowstate, 0, 1) // default configuration
+      encodeXercesFlowState(flowstate, 0, 1) // default configuration
+    )
+    or
+    // source is the write on `this` of a call to the `SAXParser`
+    // constructor.
+    exists(CallInstruction call |
+      call.getStaticCallTarget() = any(SAXParserClass c).getAConstructor() and
+      node.asInstruction().(WriteSideEffectInstruction).getDestinationAddress() =
+        call.getThisArgument() and
+      encodeXercesFlowState(flowstate, 0, 1) // default configuration
     )
   }
 
@@ -201,8 +227,8 @@ class XXEConfiguration extends DataFlow::Configuration {
       call.getTarget() instanceof ParseFunction and
       call.getQualifier() = node.asConvertedExpr()
     ) and
-    flowstate instanceof XercesDOMParserFlowState and
-    not encodeXercesDOMFlowState(flowstate, 1, 1) // safe configuration
+    flowstate instanceof XercesFlowState and
+    not encodeXercesFlowState(flowstate, 1, 1) // safe configuration
   }
 
   override predicate isAdditionalFlowStep(

cpp/ql/src/change-notes/2022-04-28-external-entity-expansion.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "XML external entity expansion" (`cpp/external-entity-expansion`) query has been extended to support a broader selection of XML libraries and interfaces.

cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected

@@ -1,4 +1,6 @@
 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 |
 | tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p |
 | tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p |
 | tests.cpp:53:19:53:19 | VariableAddress [post update] | tests.cpp:55:2:55:2 | p |
@@ -27,6 +29,10 @@ edges
 | tests.cpp:146:18:146:18 | q | tests.cpp:134:39:134:39 | p |
 | tests.cpp:150:19:150:32 | call to createLSParser | tests.cpp:152:2:152:2 | p |
 nodes
+| tests2.cpp:20:17:20:31 | SAXParser output argument | semmle.label | SAXParser output argument |
+| 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 |
 | tests.cpp:33:23:33:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
 | tests.cpp:35:2:35:2 | p | semmle.label | p |
 | tests.cpp:46:23:46:43 | XercesDOMParser output argument | semmle.label | XercesDOMParser output argument |
@@ -66,6 +72,8 @@ nodes
 | tests.cpp:152:2:152:2 | p | semmle.label | p |
 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 |
 | tests.cpp:35:2:35:2 | p | tests.cpp:33:23:33:43 | XercesDOMParser output argument | tests.cpp:35:2:35:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:33:23:33:43 | XercesDOMParser output argument | XML parser |
 | tests.cpp:49:2:49:2 | p | tests.cpp:46:23:46:43 | XercesDOMParser output argument | tests.cpp:49:2:49:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:46:23:46:43 | XercesDOMParser output argument | XML parser |
 | tests.cpp:57:2:57:2 | p | tests.cpp:53:23:53:43 | XercesDOMParser output argument | tests.cpp:57:2:57:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests.cpp:53:23:53:43 | XercesDOMParser output argument | XML parser |

cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp

@@ -4,8 +4,8 @@
 
 // ---
 
-class SecurityManager;
-class InputSource;
+
+
 
 class AbstractDOMParser {
 public:

cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h

@@ -1,2 +1,4 @@
-// library functions for rule CWE-611
+// library/common functions for rule CWE-611
 
+class SecurityManager;
+class InputSource;

cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp

@@ -0,0 +1,46 @@
+// test cases for rule CWE-611
+
+#include "tests.h"
+
+// ---
+
+class SAXParser
+{
+public:
+	SAXParser();
+
+	void setDisableDefaultEntityResolution(bool); // default is false
+	void setSecurityManager(SecurityManager *const manager);
+	void parse(const InputSource &data);
+};
+
+// ---
+
+void test2_1(InputSource &data) {
+	SAXParser *p = new SAXParser();
+
+	p->parse(data); // BAD (parser not correctly configured)
+}
+
+void test2_2(InputSource &data) {
+	SAXParser *p = new SAXParser();
+
+	p->setDisableDefaultEntityResolution(true);
+	p->parse(data); // GOOD
+}
+
+void test2_3(InputSource &data) {
+	SAXParser *p = new SAXParser();
+	bool v = false;
+
+	p->setDisableDefaultEntityResolution(v);
+	p->parse(data); // BAD (parser not correctly configured)
+}
+
+void test2_4(InputSource &data) {
+	SAXParser *p = new SAXParser();
+	bool v = true;
+
+	p->setDisableDefaultEntityResolution(v);
+	p->parse(data); // GOOD
+}