Merge branch 'main' into weak_crypto

Author: geoffw0

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/change-notes/2021-05-18-static-buffer-overflow.md

@@ -0,0 +1,2 @@
+lgtm
+* The "Static buffer overflow" query (cpp/static-buffer-overflow) has been improved to produce fewer false positives.

cpp/change-notes/2021-12-05-uncontrolled-arithmetic.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* The query "Uncontrolled arithmetic" (`cpp/uncontrolled-arithmetic`) has been improved to produce fewer false positives.

cpp/ql/src/Critical/OverflowStatic.ql

@@ -14,6 +14,7 @@
 
 import cpp
 import semmle.code.cpp.commons.Buffer
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import LoopBounds
 
 private predicate staticBufferBase(VariableAccess access, Variable v) {
@@ -51,6 +52,8 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) {
     loop.getStmt().getAChild*() = bufaccess.getEnclosingStmt() and
     loop.limit() >= bufaccess.bufferSize() and
     loop.counter().getAnAccess() = bufaccess.getArrayOffset() and
+    // Ensure that we don't have an upper bound on the array index that's less than the buffer size.
+    not upperBound(bufaccess.getArrayOffset().getFullyConverted()) < bufaccess.bufferSize() and
     msg =
       "Potential buffer-overflow: counter '" + loop.counter().toString() + "' <= " +
         loop.limit().toString() + " but '" + bufaccess.buffer().getName() + "' has " +
@@ -94,17 +97,22 @@ class CallWithBufferSize extends FunctionCall {
   }
 
   int statedSizeValue() {
-    exists(Expr statedSizeSrc |
-      DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) and
-      result = statedSizeSrc.getValue().toInt()
-    )
+    // `upperBound(e)` defaults to `exprMaxVal(e)` when `e` isn't analyzable. So to get a meaningful
+    // result in this case we pick the minimum value obtainable from dataflow and range analysis.
+    result =
+      upperBound(statedSizeExpr())
+          .minimum(min(Expr statedSizeSrc |
+              DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr())
+            |
+              statedSizeSrc.getValue().toInt()
+            ))
   }
 }
 
 predicate wrongBufferSize(Expr error, string msg) {
   exists(CallWithBufferSize call, int bufsize, Variable buf, int statedSize |
     staticBuffer(call.buffer(), buf, bufsize) and
-    statedSize = min(call.statedSizeValue()) and
+    statedSize = call.statedSizeValue() and
     statedSize > bufsize and
     error = call.statedSizeExpr() and
     msg =

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

@@ -15,34 +15,99 @@ import cpp
 import semmle.code.cpp.security.Overflow
 import semmle.code.cpp.security.Security
 import semmle.code.cpp.security.TaintTracking
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import TaintedWithPath
 
-predicate isRandCall(FunctionCall fc) { fc.getTarget().getName() = "rand" }
+predicate isUnboundedRandCall(FunctionCall fc) {
+  fc.getTarget().getName() = "rand" and not bounded(fc)
+}
+
+/**
+ * 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]
+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]
+predicate boundedRem(Expr e, Expr rem, Expr left, Expr right) {
+  e = left and
+  upperBound(right.getFullyConverted()) < exprMaxVal(rem.getFullyConverted())
+}
 
-predicate isRandCallOrParent(Expr e) {
-  isRandCall(e) or
-  isRandCallOrParent(e.getAChild())
+/**
+ * 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
+ * bounded by some number that is less than the maximum integer allowed by the result type of `andExpr`.
+ */
+pragma[inline]
+predicate boundedBitwiseAnd(Expr e, Expr andExpr, Expr operand1, Expr operand2) {
+  operand1 != operand2 and
+  e = operand1 and
+  upperBound(operand2.getFullyConverted()) < exprMaxVal(andExpr.getFullyConverted())
 }
 
-predicate isRandValue(Expr e) {
-  isRandCall(e)
+/**
+ * Holds if `fc` is a part of the left operand of a binary operation that greatly reduces the range
+ * of possible values.
+ */
+predicate bounded(Expr e) {
+  //  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()))
+  or
+  exists(AssignRemExpr rem | boundedRem(e, rem, rem.getLValue(), rem.getRValue()))
+  or
+  exists(BitwiseAndExpr andExpr |
+    boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand())
+  )
+  or
+  exists(AssignAndExpr andExpr |
+    boundedBitwiseAnd(e, andExpr, andExpr.getAnOperand(), andExpr.getAnOperand())
+  )
+  or
+  // Optimitically assume that a division always yields a much smaller value.
+  boundedDiv(e, any(DivExpr div).getLeftOperand())
+  or
+  boundedDiv(e, any(AssignDivExpr div).getLValue())
+}
+
+predicate isUnboundedRandCallOrParent(Expr e) {
+  isUnboundedRandCall(e)
+  or
+  isUnboundedRandCallOrParent(e.getAChild())
+}
+
+predicate isUnboundedRandValue(Expr e) {
+  isUnboundedRandCall(e)
   or
   exists(MacroInvocation mi |
     e = mi.getExpr() and
-    isRandCallOrParent(e)
+    isUnboundedRandCallOrParent(e)
   )
 }
 
 class SecurityOptionsArith extends SecurityOptions {
   override predicate isUserInput(Expr expr, string cause) {
-    isRandValue(expr) and
-    cause = "rand" and
-    not expr.getParent*() instanceof DivExpr
+    isUnboundedRandValue(expr) and
+    cause = "rand"
   }
 }
 
-predicate isDiv(VariableAccess va) { exists(AssignDivExpr div | div.getLValue() = va) }
-
 predicate missingGuard(VariableAccess va, string effect) {
   exists(Operation op | op.getAnOperand() = va |
     missingGuardAgainstUnderflow(op, va) and effect = "underflow"
@@ -52,29 +117,15 @@ predicate missingGuard(VariableAccess va, string effect) {
 }
 
 class Configuration extends TaintTrackingConfiguration {
-  override predicate isSink(Element e) {
-    isDiv(e)
-    or
-    missingGuard(e, _)
-  }
-}
+  override predicate isSink(Element e) { missingGuard(e, _) }
 
-/**
- * A value that undergoes division is likely to be bounded within a safe
- * range.
- */
-predicate guardedByAssignDiv(Expr origin) {
-  exists(VariableAccess va |
-    taintedWithPath(origin, va, _, _) and
-    isDiv(va)
-  )
+  override predicate isBarrier(Expr e) { super.isBarrier(e) or bounded(e) }
 }
 
 from Expr origin, VariableAccess va, string effect, PathNode sourceNode, PathNode sinkNode
 where
   taintedWithPath(origin, va, sourceNode, sinkNode) and
-  missingGuard(va, effect) and
-  not guardedByAssignDiv(origin)
+  missingGuard(va, effect)
 select va, sourceNode, sinkNode,
   "$@ flows to here and is used in arithmetic, potentially causing an " + effect + ".", origin,
   "Uncontrolled value"

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(

cpp/ql/src/Summary/LinesOfCode.ql

@@ -4,6 +4,7 @@
  * @description The total number of lines of C/C++ code across all files, including system headers, libraries, and auto-generated files. This is a useful metric of the size of a database. For all files that were seen during the build, this query counts the lines of code, excluding whitespace or comments.
  * @kind metric
  * @tags summary
+ *       lines-of-code
  */
 
 import cpp

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -748,16 +748,10 @@ private predicate modelFlow(Operand opFrom, Instruction iTo) {
       )
       or
       exists(int index, ReadSideEffectInstruction read |
-        modelIn.isParameterDeref(index) and
+        modelIn.isParameterDerefOrQualifierObject(index) and
         read = getSideEffectFor(call, index) and
         opFrom = read.getSideEffectOperand()
       )
-      or
-      exists(ReadSideEffectInstruction read |
-        modelIn.isQualifierObject() and
-        read = getSideEffectFor(call, -1) and
-        opFrom = read.getSideEffectOperand()
-      )
     )
   )
 }

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp

@@ -586,6 +586,23 @@ void test21(bool cond)
 	if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1]
 }
 
+void test22(bool b, const char* source) {
+	char buffer[16];
+	int k;
+	for (k = 0; k <= 100; k++) {
+		if(k < 16) {
+			buffer[k] = 'x'; // GOOD
+		}
+	}
+
+	char dest[128];
+	int n = b ? 1024 : 132;
+	if (n >= 128) {
+    return;
+  }
+  memcpy(dest, source, n); // GOOD
+}
+
 int main(int argc, char *argv[])
 {
 	long long arr17[19];
@@ -609,6 +626,7 @@ int main(int argc, char *argv[])
 	test19(argc == 0);
 	test20();
 	test21(argc == 0);
+	test22(argc == 0, argv[0]);
 
 	return 0;
 }