C++: Resurrect underflow detection, but only on unsigned types.

Author: geoffw0

cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql

@@ -72,8 +72,19 @@ private class RandS extends RandomFunction {
   override FunctionOutput getFunctionOutput() { result.isParameterDeref(0) }
 }
 
-predicate missingGuard(VariableAccess va) {
-  exists(Operation op | op.getAnOperand() = va | missingGuardAgainstOverflow(op, va))
+predicate missingGuard(VariableAccess va, string effect) {
+  exists(Operation op | op.getAnOperand() = va |
+    // underflow - random numbers are usually non-negative, so underflow is
+    // only likely if the type is unsigned. Multiplication is also unlikely to
+    // cause underflow of a non-negative number.
+    missingGuardAgainstUnderflow(op, va) and
+    effect = "underflow" and
+    op.getUnspecifiedType().(IntegralType).isUnsigned() and
+    not op instanceof MulExpr
+    or
+    // overflow
+    missingGuardAgainstOverflow(op, va) and effect = "overflow"
+  )
 }
 
 class UncontrolledArithConfiguration extends TaintTracking::Configuration {
@@ -91,7 +102,7 @@ class UncontrolledArithConfiguration extends TaintTracking::Configuration {
     )
   }
 
-  override predicate isSink(DataFlow::Node sink) { missingGuard(sink.asExpr()) }
+  override predicate isSink(DataFlow::Node sink) { missingGuard(sink.asExpr(), _) }
 
   override predicate isSanitizer(DataFlow::Node node) {
     bounded(node.asExpr())
@@ -115,11 +126,11 @@ Expr getExpr(DataFlow::Node node) { result = [node.asExpr(), node.asDefiningArgu
 
 from
   UncontrolledArithConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink,
-  VariableAccess va
+  VariableAccess va, string effect
 where
   config.hasFlowPath(source, sink) and
   sink.getNode().asExpr() = va and
-  missingGuard(va)
+  missingGuard(va, effect)
 select sink.getNode(), source, sink,
-  "$@ flows to here and is used in arithmetic, potentially causing an overflow.",
+  "$@ flows to here and is used in arithmetic, potentially causing an " + effect + ".",
   getExpr(source.getNode()), "Uncontrolled value"

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

@@ -8,6 +8,8 @@ edges
 | test.c:81:23:81:26 | call to rand | test.c:83:9:83:9 | r |
 | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r |
 | test.c:125:13:125:16 | call to rand | test.c:127:9:127:9 | r |
+| test.c:148:22:148:25 | call to rand | test.c:150:9:150:9 | r |
+| test.c:148:22:148:27 | (unsigned int)... | test.c:150:9:150:9 | r |
 | test.cpp:8:9:8:12 | Store | test.cpp:24:11:24:18 | call to get_rand |
 | test.cpp:8:9:8:12 | call to rand | test.cpp:8:9:8:12 | Store |
 | test.cpp:13:2:13:15 | Chi [[]] | test.cpp:30:13:30:14 | get_rand2 output argument [[]] |
@@ -41,6 +43,9 @@ nodes
 | test.c:100:5:100:5 | r | semmle.label | r |
 | test.c:125:13:125:16 | call to rand | semmle.label | call to rand |
 | test.c:127:9:127:9 | r | semmle.label | r |
+| test.c:148:22:148:25 | call to rand | semmle.label | call to rand |
+| test.c:148:22:148:27 | (unsigned int)... | semmle.label | (unsigned int)... |
+| test.c:150:9:150:9 | r | semmle.label | r |
 | test.cpp:8:9:8:12 | Store | semmle.label | Store |
 | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand |
 | test.cpp:13:2:13:15 | Chi [[]] | semmle.label | Chi [[]] |
@@ -74,6 +79,8 @@ nodes
 | test.c:83:9:83:9 | r | test.c:81:23:81:26 | call to rand | test.c:83:9:83:9 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:81:23:81:26 | call to rand | Uncontrolled value |
 | test.c:100:5:100:5 | r | test.c:99:14:99:19 | call to rand | test.c:100:5:100:5 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:99:14:99:19 | call to rand | Uncontrolled value |
 | test.c:127:9:127:9 | r | test.c:125:13:125:16 | call to rand | test.c:127:9:127:9 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:125:13:125:16 | call to rand | Uncontrolled value |
+| test.c:150:9:150:9 | r | test.c:148:22:148:25 | call to rand | test.c:150:9:150:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:148:22:148:25 | call to rand | Uncontrolled value |
+| test.c:150:9:150:9 | r | test.c:148:22:148:27 | (unsigned int)... | test.c:150:9:150:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:148:22:148:25 | call to rand | Uncontrolled value |
 | test.cpp:25:7:25:7 | r | test.cpp:8:9:8:12 | call to rand | test.cpp:25:7:25:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:8:9:8:12 | call to rand | Uncontrolled value |
 | test.cpp:31:7:31:7 | r | test.cpp:13:10:13:13 | call to rand | test.cpp:31:7:31:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:13:10:13:13 | call to rand | Uncontrolled value |
 | test.cpp:37:7:37:7 | r | test.cpp:18:9:18:12 | call to rand | test.cpp:37:7:37:7 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:18:9:18:12 | call to rand | Uncontrolled value |

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

@@ -147,6 +147,6 @@ void moreTests() {
   {
     unsigned int r = rand();
 
-    r = r - 100; // BAD [NOT DETECTED]
+    r = r - 100; // BAD
   }
 }