C++: Correct hasUpperBoundsCheck.

Author: geoffw0

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -130,7 +130,17 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
 }
 
 /**
- * A variable that has any kind of upper-bound check anywhere in the program
+ * A variable that has any kind of upper-bound check anywhere in the program.  This is
+ * biased towards being inclusive because there are a lot of valid ways of doing an
+ * upper bounds checks if we don't consider where it occurs, for example:
+ * ```
+ *   if (x < 10) { sink(x); }
+ * 
+ *   if (10 > y) { sink(y); }
+ *
+ *   if (z > 10) { z = 10; }
+ *   sink(z);
+ * ```
  */
 // TODO: This coarse overapproximation, ported from the old taint tracking
 // library, could be replaced with an actual semantic check that a particular
@@ -139,10 +149,10 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
 // previously suppressed by this predicate by coincidence.
 private predicate hasUpperBoundsCheck(Variable var) {
   exists(RelationalOperation oper, VariableAccess access |
-    oper.getLeftOperand() = access and
+    oper.getAnOperand() = access and
     access.getTarget() = var and
     // Comparing to 0 is not an upper bound check
-    not oper.getRightOperand().getValue() = "0"
+    not oper.getAnOperand().getValue() = "0"
   )
 }
 

cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll

@@ -328,14 +328,24 @@ GlobalOrNamespaceVariable globalVarFromId(string id) {
 }
 
 /**
- * A variable that has any kind of upper-bound check anywhere in the program
+ * A variable that has any kind of upper-bound check anywhere in the program.  This is
+ * biased towards being inclusive because there are a lot of valid ways of doing an
+ * upper bounds checks if we don't consider where it occurs, for example:
+ * ```
+ *   if (x < 10) { sink(x); }
+ * 
+ *   if (10 > y) { sink(y); }
+ *
+ *   if (z > 10) { z = 10; }
+ *   sink(z);
+ * ```
  */
 private predicate hasUpperBoundsCheck(Variable var) {
   exists(RelationalOperation oper, VariableAccess access |
-    oper.getLeftOperand() = access and
+    oper.getAnOperand() = access and
     access.getTarget() = var and
     // Comparing to 0 is not an upper bound check
-    not oper.getRightOperand().getValue() = "0"
+    not oper.getAnOperand().getValue() = "0"
   )
 }
 

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected

@@ -12,5 +12,3 @@
 | test.cpp:134:10:134:27 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:132:19:132:24 | call to getenv | user input (getenv) |
 | test.cpp:142:4:142:9 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |
 | test.cpp:142:11:142:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:138:19:138:24 | call to getenv | user input (getenv) |
-| test.cpp:169:4:169:9 | call to malloc | This allocation size is derived from $@ and might overflow | test.cpp:165:19:165:24 | call to getenv | user input (getenv) |
-| test.cpp:169:11:169:28 | ... * ... | This allocation size is derived from $@ and might overflow | test.cpp:165:19:165:24 | call to getenv | user input (getenv) |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/test.cpp

@@ -166,7 +166,7 @@ void more_bounded_tests() {
 
 		if ((100 > size) && (0 < size))
 		{
-			malloc(size * sizeof(int)); // GOOD [FALSE POSITIVE]
+			malloc(size * sizeof(int)); // GOOD
 		}
 	}