Merge pull request #5903 from MathiasVP/tainted-allocation-size-barrier

C++: Add barriers to `cpp/uncontrolled-allocation-size`

Author: MathiasVP

cpp/change-notes/2021-05-14-uncontrolled-allocation-size.md

@@ -0,0 +1,2 @@
+lgtm
+* The "Tainted allocation size" query (cpp/uncontrolled-allocation-size) has been improved to produce fewer false positives.

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

@@ -12,6 +12,7 @@
  */
 
 import cpp
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import semmle.code.cpp.security.TaintTracking
 import TaintedWithPath
 
@@ -27,6 +28,27 @@ predicate allocSink(Expr alloc, Expr tainted) {
 
 class TaintedAllocationSizeConfiguration extends TaintTrackingConfiguration {
   override predicate isSink(Element tainted) { allocSink(_, tainted) }
+
+  override predicate isBarrier(Expr e) {
+    super.isBarrier(e)
+    or
+    // There can be two separate reasons for `convertedExprMightOverflow` not holding:
+    // 1. `e` really cannot overflow.
+    // 2. `e` isn't analyzable.
+    // If we didn't rule out case 2 we would place barriers on anything that isn't analyzable.
+    (
+      e instanceof UnaryArithmeticOperation or
+      e instanceof BinaryArithmeticOperation or
+      e instanceof AssignArithmeticOperation
+    ) and
+    not convertedExprMightOverflow(e)
+    or
+    // Subtracting two pointers is either well-defined (and the result will likely be small), or
+    // terribly undefined and dangerous. Here, we assume that the programmer has ensured that the
+    // result is well-defined (i.e., the two pointers point to the same object), and thus the result
+    // will likely be small.
+    e = any(PointerDiffExpr diff).getAnOperand()
+  }
 }
 
 predicate taintedAllocSize(