C++: Relax the restrictions on when '%' is a barrier and accept test changes.

Author: MathiasVP

cpp/ql/src/Security/CWE/CWE-190/Bounded.qll

@@ -15,16 +15,11 @@ pragma[inline]
 private predicate boundedDiv(Expr e, Expr left) { e = left }
 
 /**
- * An operand `e` of a remainder expression `rem` (i.e., `rem` is either a `RemExpr` or
- * an `AssignRemExpr`) with left-hand side `left` and right-ahnd side `right` is bounded
- * when `e` is `left` and `right` is upper bounded by some number that is less than the maximum integer
- * allowed by the result type of `rem`.
+ * An operand `e` of a remainder expression (i.e., `e` is an operand of either a `RemExpr` or
+ * a `AssignRemExpr`) is bounded when `e` is the left-hand side of the remainder.
  */
 pragma[inline]
-private predicate boundedRem(Expr e, Expr rem, Expr left, Expr right) {
-  e = left and
-  upperBound(right.getFullyConverted()) < exprMaxVal(rem.getFullyConverted())
-}
+private predicate boundedRem(Expr e, Expr left) { e = left }
 
 /**
  * An operand `e` of a bitwise and expression `andExpr` (i.e., `andExpr` is either an `BitwiseAndExpr`
@@ -50,19 +45,9 @@ predicate bounded(Expr e) {
   ) and
   not convertedExprMightOverflow(e)
   or
-  //  For `%` and `&` we require that `e` is bounded by a value that is strictly smaller than the
-  //  maximum possible value of the result type of the operation.
-  //  For example, the function call `rand()` is considered bounded in the following program:
-  //  ```
-  //  int i = rand() % (UINT8_MAX + 1);
-  //  ```
-  //  but not in:
-  //  ```
-  //  unsigned char uc = rand() % (UINT8_MAX + 1);
-  //  ```
-  exists(RemExpr rem | boundedRem(e, rem, rem.getLeftOperand(), rem.getRightOperand()))
+  boundedRem(e, any(RemExpr rem).getLeftOperand())
   or
-  exists(AssignRemExpr rem | boundedRem(e, rem, rem.getLValue(), rem.getRValue()))
+  boundedRem(e, any(AssignRemExpr rem).getLValue())
   or
   exists(BitwiseAndExpr andExpr |
     boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand())

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

@@ -7,8 +7,6 @@ edges
 | test.c:81:14:81:17 | call to rand | test.c:83:9:83:9 | r |
 | 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:115:12:115:15 | call to rand | test.c:116:3:116:4 | r1 |
-| test.c:118:13:118:16 | call to rand | test.c:119:3:119:4 | r2 |
 | 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 [[]] |
@@ -35,10 +33,6 @@ nodes
 | test.c:83:9:83:9 | r | semmle.label | r |
 | test.c:99:14:99:19 | call to rand | semmle.label | call to rand |
 | test.c:100:5:100:5 | r | semmle.label | r |
-| test.c:115:12:115:15 | call to rand | semmle.label | call to rand |
-| test.c:116:3:116:4 | r1 | semmle.label | r1 |
-| test.c:118:13:118:16 | call to rand | semmle.label | call to rand |
-| test.c:119:3:119:4 | r2 | semmle.label | r2 |
 | 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 [[]] |
@@ -62,8 +56,6 @@ nodes
 | test.c:83:9:83:9 | r | test.c:81:14:81:17 | call to rand | test.c:83:9:83:9 | r | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:81:14:81:17 | call to rand | Uncontrolled value |
 | 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 underflow. | 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 underflow. | test.c:99:14:99:19 | call to rand | Uncontrolled value |
-| test.c:116:3:116:4 | r1 | test.c:115:12:115:15 | call to rand | test.c:116:3:116:4 | r1 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:115:12:115:15 | call to rand | Uncontrolled value |
-| test.c:119:3:119:4 | r2 | test.c:118:13:118:16 | call to rand | test.c:119:3:119:4 | r2 | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:118:13:118:16 | 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

@@ -113,9 +113,9 @@ void add_100(int r) {
 
 void randomTester2(int bound, int min, int max) {
   int r1 = rand() % bound;
-  r1 += 100; // GOOD [FALSE POSITIVE] (`bound` may possibly be MAX_INT in which case this could
-             //                        still overflow, but it's most likely fine)
+  r1 += 100; // GOOD (`bound` may possibly be MAX_INT in which case this could
+             //       still overflow, but it's most likely fine)
 
   int r2 = (rand() % (max - min + 1)) + min;
-  r2 += 100; // GOOD [FALSE POSITIVE] (This is a common way to clamp the random value between [min, max])
+  r2 += 100; // GOOD (This is a common way to clamp the random value between [min, max])
 }