diff --git a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql b/cpp/ql/src/Security/CWE/CWE-611/XXE.ql index 78cd914872df..805f3a61277d 100644 --- a/cpp/ql/src/Security/CWE/CWE-611/XXE.ql +++ b/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( diff --git a/cpp/ql/src/change-notes/2022-04-28-external-entity-expansion.md b/cpp/ql/src/change-notes/2022-04-28-external-entity-expansion.md new file mode 100644 index 000000000000..911cbd7e54cf --- /dev/null +++ b/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. diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-611/XXE.expected index 7f05e6d41b85..25f1ad8e1ab5 100644 --- 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,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 | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp index 237951e7fea2..76ceb7b75568 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.cpp @@ -4,8 +4,8 @@ // --- -class SecurityManager; -class InputSource; + + class AbstractDOMParser { public: diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h index ba9fa168d2a6..aa4c539bbd97 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests.h +++ b/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; diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp new file mode 100644 index 000000000000..758cb57b26e4 --- /dev/null +++ b/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 +}