Merge branch 'main' into print-global

Author: jketema

cpp/ql/lib/semmle/code/cpp/security/InvalidPointerDereference/AllocationToInvalidPointer.qll

@@ -1,6 +1,54 @@
 /**
  * This file provides the first phase of the `cpp/invalid-pointer-deref` query that identifies flow
  * from an allocation to a pointer-arithmetic instruction that constructs a pointer that is out of bounds.
+ *
+ * Consider the following snippet:
+ * ```cpp
+ * 1. char* base = (char*)malloc(size);
+ * 2. char* end = base + size;
+ * 3. for(int *p = base; p <= end; p++) {
+ * 4.   use(*p); // BUG: Should have been bounded by `p < end`.
+ * 5. }
+ * ```
+ * this file identifies the flow from `new int[size]` to `base + size`.
+ *
+ * This is done using the product-flow library. The configuration tracks flow from the pair
+ * `(allocation, size of allocation)` to a pair `(a, b)` where there exists a pointer-arithmetic instruction
+ * `pai = a + r` such that `b` is a dataflow node where `b <= r`. Because there will be a dataflow-path from
+ * `allocation` to `a` this means that the `pai` will compute a pointer that is some number of elements beyond
+ * the end position of the allocation. See `pointerAddInstructionHasBounds` for the implementation of this.
+ *
+ * In the above example, the pair `(a, b)` is `(base, size)` with `base` and `size` coming from the expression
+ * `base + size` on line 2, which is also the pointer-arithmetic instruction. In general, the pair does not necessarily
+ * correspond directly to the operands of the pointer-arithmetic instruction.
+ * In the following example, the pair is again `(base, size)`, but with `base` coming from line 3 and `size` from line 2,
+ * and the pointer-arithmetic instruction being `base + n` on line 3:
+ *  ```cpp
+ *  1. int* base = new int[size];
+ *  2. if(n <= size) {
+ *  3.   int* end = base + n;
+ *  4.   for(int* p = base; p <= end; ++p) {
+ *  5.     *p = 0; // BUG: Should have been bounded by `p < end`.
+ *  6.   }
+ *  7. }
+ *  ```
+ *
+ * Handling false positives:
+ *
+ * Consider a snippet such as:
+ * ```cpp
+ * 1. int* base = new int[size];
+ * 2. int n = condition() ? size : 0;
+ * 3. if(n >= size) return;
+ * 4. int* end = base + n;
+ * 5. for(int* p = base; p <= end; ++p) {
+ * 6.   *p = 0; // This is fine since `end < base + size`
+ * 7. }
+ * ```
+ * In order to remove this false positive we define a barrier (see `SizeBarrier::SizeBarrierConfig`) that finds the
+ * possible guards that compares a value to the size of the allocation. In the above example, this is the `(n >= size)`
+ * guard on line 3. `SizeBarrier::getABarrierNode` then defines any node that is guarded by such a guard as a barrier
+ * in the dataflow configuration.
  */
 
 private import cpp
@@ -42,59 +90,57 @@ predicate hasSize(HeuristicAllocationExpr alloc, DataFlow::Node n, int state) {
  * ```
  * In this case, the sink pair identified by the product flow library (without any additional barriers)
  * would be `(p, n)` (where `n` is the `n` in `p[n]`), because there exists a pointer-arithmetic
- * instruction `pai` such that:
- * 1. The left-hand of `pai` flows from the allocation, and
- * 2. The right-hand of `pai` is non-strictly upper bounded by `n` (where `n` is the `n` in `p[n]`)
+ * instruction `pai = a + b` such that:
+ * 1. the allocation flows to `a`, and
+ * 2. `b <= n` where `n` is the `n` in `p[n]`
  * but because there's a strict comparison that compares `n` against the size of the allocation this
  * snippet is fine.
  */
-module Barrier2 {
-  private class FlowState2 = int;
-
-  private module BarrierConfig2 implements DataFlow::ConfigSig {
+module SizeBarrier {
+  private module SizeBarrierConfig implements DataFlow::ConfigSig {
     predicate isSource(DataFlow::Node source) {
       // The sources is the same as in the sources for the second
       // projection in the `AllocToInvalidPointerConfig` module.
       hasSize(_, source, _)
     }
 
     additional predicate isSink(
-      DataFlow::Node left, DataFlow::Node right, IRGuardCondition g, FlowState2 state,
-      boolean testIsTrue
+      DataFlow::Node left, DataFlow::Node right, IRGuardCondition g, int k, boolean testIsTrue
     ) {
-      // The sink is any "large" side of a relational comparison.
-      g.comparesLt(left.asOperand(), right.asOperand(), state, true, testIsTrue)
+      // The sink is any "large" side of a relational comparison. i.e., the `right` expression
+      // in a guard such as `left < right + k`.
+      g.comparesLt(left.asOperand(), right.asOperand(), k, true, testIsTrue)
     }
 
     predicate isSink(DataFlow::Node sink) { isSink(_, sink, _, _, _) }
   }
 
-  private import DataFlow::Global<BarrierConfig2>
+  private import DataFlow::Global<SizeBarrierConfig>
 
-  private FlowState2 getAFlowStateForNode(DataFlow::Node node) {
+  private int getAFlowStateForNode(DataFlow::Node node) {
     exists(DataFlow::Node source |
       flow(source, node) and
       hasSize(_, source, result)
     )
   }
 
   private predicate operandGuardChecks(
-    IRGuardCondition g, Operand left, Operand right, FlowState2 state, boolean edge
+    IRGuardCondition g, Operand left, Operand right, int state, boolean edge
   ) {
-    exists(DataFlow::Node nLeft, DataFlow::Node nRight, FlowState2 state0 |
+    exists(DataFlow::Node nLeft, DataFlow::Node nRight, int k |
       nRight.asOperand() = right and
       nLeft.asOperand() = left and
-      BarrierConfig2::isSink(nLeft, nRight, g, state0, edge) and
+      SizeBarrierConfig::isSink(nLeft, nRight, g, k, edge) and
       state = getAFlowStateForNode(nRight) and
-      state0 <= state
+      k <= state
     )
   }
 
   /**
    * Gets an instruction that is guarded by a guard condition which ensures that
    * the value of the instruction is upper-bounded by size of some allocation.
    */
-  Instruction getABarrierInstruction(FlowState2 state) {
+  Instruction getABarrierInstruction(int state) {
     exists(IRGuardCondition g, ValueNumber value, Operand use, boolean edge |
       use = value.getAUse() and
       operandGuardChecks(pragma[only_bind_into](g), pragma[only_bind_into](use), _,
@@ -108,17 +154,15 @@ module Barrier2 {
    * Gets a `DataFlow::Node` that is guarded by a guard condition which ensures that
    * the value of the node is upper-bounded by size of some allocation.
    */
-  DataFlow::Node getABarrierNode(FlowState2 state) {
+  DataFlow::Node getABarrierNode(int state) {
     result.asOperand() = getABarrierInstruction(state).getAUse()
   }
 
   /**
    * Gets the block of a node that is guarded (see `getABarrierInstruction` or
    * `getABarrierNode` for the definition of what it means to be guarded).
    */
-  IRBlock getABarrierBlock(FlowState2 state) {
-    result.getAnInstruction() = getABarrierInstruction(state)
-  }
+  IRBlock getABarrierBlock(int state) { result.getAnInstruction() = getABarrierInstruction(state) }
 }
 
 private module InterestingPointerAddInstruction {
@@ -151,54 +195,38 @@ private module InterestingPointerAddInstruction {
 }
 
 /**
- * A product-flow configuration for flow from an (allocation, size) pair to a
- * pointer-arithmetic operation that is non-strictly upper-bounded by `allocation + size`.
- *
- * The goal of this query is to find patterns such as:
- * ```cpp
- * 1. char* begin = (char*)malloc(size);
- * 2. char* end = begin + size;
- * 3. for(int *p = begin; p <= end; p++) {
- * 4.   use(*p);
- * 5. }
- * ```
- *
- * We do this by splitting the task up into two configurations:
- * 1. `AllocToInvalidPointerConfig` find flow from `malloc(size)` to `begin + size`, and
- * 2. `InvalidPointerToDerefConfig` finds flow from `begin + size` to an `end` (on line 3).
- *
- * Finally, the range-analysis library will find a load from (or store to) an address that
- * is non-strictly upper-bounded by `end` (which in this case is `*p`).
+ * A product-flow configuration for flow from an `(allocation, size)` pair to a
+ * pointer-arithmetic operation `pai` such that `pai <= allocation + size`.
  */
 private module Config implements ProductFlow::StateConfigSig {
   class FlowState1 = Unit;
 
   class FlowState2 = int;
 
   predicate isSourcePair(
-    DataFlow::Node source1, FlowState1 state1, DataFlow::Node source2, FlowState2 state2
+    DataFlow::Node allocSource, FlowState1 unit, DataFlow::Node sizeSource, FlowState2 sizeAddend
   ) {
     // In the case of an allocation like
     // ```cpp
     // malloc(size + 1);
     // ```
     // we use `state2` to remember that there was an offset (in this case an offset of `1`) added
     // to the size of the allocation. This state is then checked in `isSinkPair`.
-    exists(state1) and
-    hasSize(source1.asConvertedExpr(), source2, state2)
+    exists(unit) and
+    hasSize(allocSource.asConvertedExpr(), sizeSource, sizeAddend)
   }
 
   predicate isSinkPair(
-    DataFlow::Node sink1, FlowState1 state1, DataFlow::Node sink2, FlowState2 state2
+    DataFlow::Node allocSink, FlowState1 unit, DataFlow::Node sizeSink, FlowState2 sizeAddend
   ) {
-    exists(state1) and
+    exists(unit) and
     // We check that the delta computed by the range analysis matches the
     // state value that we set in `isSourcePair`.
-    pointerAddInstructionHasBounds0(_, sink1, sink2, state2)
+    pointerAddInstructionHasBounds0(_, allocSink, sizeSink, sizeAddend)
   }
 
   predicate isBarrier2(DataFlow::Node node, FlowState2 state) {
-    node = Barrier2::getABarrierNode(state)
+    node = SizeBarrier::getABarrierNode(state)
   }
 
   predicate isBarrierIn1(DataFlow::Node node) { isSourcePair(node, _, _, _) }
@@ -211,7 +239,7 @@ private module Config implements ProductFlow::StateConfigSig {
 private module AllocToInvalidPointerFlow = ProductFlow::GlobalWithState<Config>;
 
 /**
- * Holds if `pai` is non-strictly upper bounded by `sink2 + delta` and `sink1` is the
+ * Holds if `pai` is non-strictly upper bounded by `sizeSink + delta` and `allocSink` is the
  * left operand of the pointer-arithmetic operation.
  *
  * For example in,
@@ -220,37 +248,37 @@ private module AllocToInvalidPointerFlow = ProductFlow::GlobalWithState<Config>;
  * ```
  * We will have:
  * - `pai` is `p + (size + 1)`,
- * - `sink1` is `p`
- * - `sink2` is `size`
+ * - `allocSink` is `p`
+ * - `sizeSink` is `size`
  * - `delta` is `1`.
  */
 pragma[nomagic]
 private predicate pointerAddInstructionHasBounds0(
-  PointerAddInstruction pai, DataFlow::Node sink1, DataFlow::Node sink2, int delta
+  PointerAddInstruction pai, DataFlow::Node allocSink, DataFlow::Node sizeSink, int delta
 ) {
   InterestingPointerAddInstruction::isInteresting(pragma[only_bind_into](pai)) and
-  exists(Instruction right, Instruction instr2 |
+  exists(Instruction right, Instruction sizeInstr |
     pai.getRight() = right and
-    pai.getLeft() = sink1.asInstruction() and
-    instr2 = sink2.asInstruction() and
-    // pai.getRight() <= sink2 + delta
-    bounded1(right, instr2, delta) and
-    not right = Barrier2::getABarrierInstruction(delta) and
-    not instr2 = Barrier2::getABarrierInstruction(delta)
+    pai.getLeft() = allocSink.asInstruction() and
+    sizeInstr = sizeSink.asInstruction() and
+    // pai.getRight() <= sizeSink + delta
+    bounded1(right, sizeInstr, delta) and
+    not right = SizeBarrier::getABarrierInstruction(delta) and
+    not sizeInstr = SizeBarrier::getABarrierInstruction(delta)
   )
 }
 
 /**
- * Holds if `allocation` flows to `sink1` and `sink1` represents the left-hand
- * side of the pointer-arithmetic instruction `pai`, and the right-hand side of `pai`
- * is non-strictly upper bounded by the size of `alllocation` + `delta`.
+ * Holds if `allocation` flows to `allocSink` and `allocSink` represents the left operand
+ * of the pointer-arithmetic instruction `pai = a + b` (i.e., `allocSink = a`), and
+ * `b <= allocation + delta`.
  */
 pragma[nomagic]
 predicate pointerAddInstructionHasBounds(
-  DataFlow::Node allocation, PointerAddInstruction pai, DataFlow::Node sink1, int delta
+  DataFlow::Node allocation, PointerAddInstruction pai, DataFlow::Node allocSink, int delta
 ) {
-  exists(DataFlow::Node sink2 |
-    AllocToInvalidPointerFlow::flow(allocation, _, sink1, sink2) and
-    pointerAddInstructionHasBounds0(pai, sink1, sink2, delta)
+  exists(DataFlow::Node sizeSink |
+    AllocToInvalidPointerFlow::flow(allocation, _, allocSink, sizeSink) and
+    pointerAddInstructionHasBounds0(pai, allocSink, sizeSink, delta)
   )
 }