C++: Use GVN on the values passed into set* functions.

geoffw0 · geoffw0 · commit 7fb1069d69b0 · 2022-04-29T10:09:52.000+01:00
diff --git 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.
@@ -124,10 +125,10 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {
     exists(int createEntityReferenceNodes |
       encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and
       (
-        newValue.getValue().toInt() = 1 and // true
+        globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
         encodeXercesFlowState(result, 1, createEntityReferenceNodes)
         or
-        not newValue.getValue().toInt() = 1 and // false or unknown
+        not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
         encodeXercesFlowState(result, 0, createEntityReferenceNodes)
       )
     )
@@ -156,10 +157,10 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {
     exists(int disabledDefaultEntityResolution |
       encodeXercesFlowState(flowstate, disabledDefaultEntityResolution, _) and
       (
-        newValue.getValue().toInt() = 1 and // true
+        globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true
         encodeXercesFlowState(result, disabledDefaultEntityResolution, 1)
         or
-        not newValue.getValue().toInt() = 1 and // false or unknown
+        not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown
         encodeXercesFlowState(result, disabledDefaultEntityResolution, 0)
       )
     )
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
@@ -1,7 +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 |
-| tests2.cpp:41:17:41:31 | SAXParser output argument | tests2.cpp:45:2:45: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 |
@@ -34,8 +33,6 @@ 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 |
-| tests2.cpp:41:17:41:31 | SAXParser output argument | semmle.label | SAXParser output argument |
-| tests2.cpp:45:2:45: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 |
@@ -77,7 +74,6 @@ 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 |
-| tests2.cpp:45:2:45:2 | p | tests2.cpp:41:17:41:31 | SAXParser output argument | tests2.cpp:45:2:45:2 | p | This $@ is not configured to prevent an XML external entity (XXE) attack. | tests2.cpp:41:17:41: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/tests2.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-611/tests2.cpp
@@ -42,5 +42,5 @@ void test2_4(InputSource &data) {
 	bool v = true;
 
 	p->setDisableDefaultEntityResolution(v);
-	p->parse(data); // GOOD [FALSE POSITIVE]
+	p->parse(data); // GOOD
 }

Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,7 @@ import cpp`
`16`	`16`	`import semmle.code.cpp.ir.dataflow.DataFlow`
`17`	`17`	`import DataFlow::PathGraph`
`18`	`18`	`import semmle.code.cpp.ir.IR`
	`19`	`+import semmle.code.cpp.valuenumbering.GlobalValueNumbering`
`19`	`20`
`20`	`21`	`/**`
`21`	`22`	`* A flow state representing a possible configuration of an XML object.`
`@@ -124,10 +125,10 @@ class DisableDefaultEntityResolutionTranformer extends XXEFlowStateTranformer {`
`124`	`125`	`exists(int createEntityReferenceNodes \|`
`125`	`126`	`encodeXercesFlowState(flowstate, _, createEntityReferenceNodes) and`
`126`	`127`	`(`
`127`		`- newValue.getValue().toInt() = 1 and // true`
	`128`	`+ globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true`
`128`	`129`	`encodeXercesFlowState(result, 1, createEntityReferenceNodes)`
`129`	`130`	`or`
`130`		`- not newValue.getValue().toInt() = 1 and // false or unknown`
	`131`	`+ not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown`
`131`	`132`	`encodeXercesFlowState(result, 0, createEntityReferenceNodes)`
`132`	`133`	`)`
`133`	`134`	`)`
`@@ -156,10 +157,10 @@ class CreateEntityReferenceNodesTranformer extends XXEFlowStateTranformer {`
`156`	`157`	`exists(int disabledDefaultEntityResolution \|`
`157`	`158`	`encodeXercesFlowState(flowstate, disabledDefaultEntityResolution, _) and`
`158`	`159`	`(`
`159`		`- newValue.getValue().toInt() = 1 and // true`
	`160`	`+ globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // true`
`160`	`161`	`encodeXercesFlowState(result, disabledDefaultEntityResolution, 1)`
`161`	`162`	`or`
`162`		`- not newValue.getValue().toInt() = 1 and // false or unknown`
	`163`	`+ not globalValueNumber(newValue).getAnExpr().getValue().toInt() = 1 and // false or unknown`
`163`	`164`	`encodeXercesFlowState(result, disabledDefaultEntityResolution, 0)`
`164`	`165`	`)`
`165`	`166`	`)`
Original file line number	Diff line number	Diff line change
`@@ -42,5 +42,5 @@ void test2_4(InputSource &data) {`
`42`	`42`	`bool v = true;`
`43`	`43`
`44`	`44`	`p->setDisableDefaultEntityResolution(v);`
`45`		`- p->parse(data); // GOOD [FALSE POSITIVE]`
	`45`	`+ p->parse(data); // GOOD`
`46`	`46`	`}`