Merge pull request github#5678 from MathiasVP/sound-expr-might-overflow-predicate

C++: Make exprMightOverflowPositively sound for unanalyzable expressions

Author: geoffw0

cpp/change-notes/2021-26-04-more-sound-expr-might-overflow.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The `exprMightOverflowPositively` and `exprMightOverflowNegatively` predicates from the `SimpleRangeAnalysis` library now recognize more expressions that might overflow.

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

@@ -28,6 +28,7 @@ predicate outOfBoundsExpr(Expr expr, string kind) {
 
 from Expr use, Expr origin, string kind
 where
+  not use.getUnspecifiedType() instanceof PointerType and
   outOfBoundsExpr(use, kind) and
   tainted(origin, use) and
   origin != use and

cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

@@ -1617,6 +1617,18 @@ private module SimpleRangeAnalysisCached {
     defMightOverflowPositively(def, v)
   }
 
+  /**
+   * Holds if `e` is an expression where the concept of overflow makes sense.
+   * This predicate is used to filter out some of the unanalyzable expressions
+   * from `exprMightOverflowPositively` and `exprMightOverflowNegatively`.
+   */
+  pragma[inline]
+  private predicate exprThatCanOverflow(Expr e) {
+    e instanceof UnaryArithmeticOperation or
+    e instanceof BinaryArithmeticOperation or
+    e instanceof LShiftExpr
+  }
+
   /**
    * Holds if the expression might overflow negatively. This predicate
    * does not consider the possibility that the expression might overflow
@@ -1630,6 +1642,11 @@ private module SimpleRangeAnalysisCached {
     // bound of `x`, so the standard logic (above) does not work for
     // detecting whether it might overflow.
     getLowerBoundsImpl(expr.(PostfixDecrExpr)) = exprMinVal(expr)
+    or
+    // We can't conclude that any unanalyzable expression might overflow. This
+    // is because there are many expressions that the range analysis doesn't
+    // handle, but where the concept of overflow doesn't make sense.
+    exprThatCanOverflow(expr) and not analyzableExpr(expr)
   }
 
   /**
@@ -1657,6 +1674,11 @@ private module SimpleRangeAnalysisCached {
     // bound of `x`, so the standard logic (above) does not work for
     // detecting whether it might overflow.
     getUpperBoundsImpl(expr.(PostfixIncrExpr)) = exprMaxVal(expr)
+    or
+    // We can't conclude that any unanalyzable expression might overflow. This
+    // is because there are many expressions that the range analysis doesn't
+    // handle, but where the concept of overflow doesn't make sense.
+    exprThatCanOverflow(expr) and not analyzableExpr(expr)
   }
 
   /**