Merge pull request github#6159 from MathiasVP/more-effective-barriers-in-bounded-predicate

C++: More effective barriers in the `bounded` predicate for CWE-190

Author: rdmarsh2

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

@@ -7,25 +7,6 @@ private import cpp
 private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 
-/**
- * An operand `e` of a division expression (i.e., `e` is an operand of either a `DivExpr` or
- * a `AssignDivExpr`) is bounded when `e` is the left-hand side of the division.
- */
-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`.
- */
-pragma[inline]
-private predicate boundedRem(Expr e, Expr rem, Expr left, Expr right) {
-  e = left and
-  upperBound(right.getFullyConverted()) < exprMaxVal(rem.getFullyConverted())
-}
-
 /**
  * An operand `e` of a bitwise and expression `andExpr` (i.e., `andExpr` is either an `BitwiseAndExpr`
  * or an `AssignAndExpr`) with operands `operand1` and `operand2` is the operand that is not `e` is upper
@@ -50,19 +31,10 @@ 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()))
+  // Optimitically assume that a remainder expression always yields a much smaller value.
+  e = any(RemExpr rem).getLeftOperand()
   or
-  exists(AssignRemExpr rem | boundedRem(e, rem, rem.getLValue(), rem.getRValue()))
+  e = any(AssignRemExpr rem).getLValue()
   or
   exists(BitwiseAndExpr andExpr |
     boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand())
@@ -73,11 +45,11 @@ predicate bounded(Expr e) {
   )
   or
   // Optimitically assume that a division always yields a much smaller value.
-  boundedDiv(e, any(DivExpr div).getLeftOperand())
+  e = any(DivExpr div).getLeftOperand()
   or
-  boundedDiv(e, any(AssignDivExpr div).getLValue())
+  e = any(AssignDivExpr div).getLValue()
   or
-  boundedDiv(e, any(RShiftExpr shift).getLeftOperand())
+  e = any(RShiftExpr shift).getLeftOperand()
   or
-  boundedDiv(e, any(AssignRShiftExpr div).getLValue())
+  e = any(AssignRShiftExpr div).getLValue()
 }

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

@@ -109,4 +109,13 @@ void randomTester() {
 
 void add_100(int r) {
   r += 100; // GOOD
-}
+}
+
+void randomTester2(int bound, int min, int max) {
+  int r1 = rand() % bound;
+  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 (This is a common way to clamp the random value between [min, max])
+}