Merge pull request github#13725 from MathiasVP/fix-barriers-in-invalid-pointer-deref

C++: Fix barriers in invalid pointer deref

Author: MathiasVP

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

@@ -96,57 +96,83 @@ predicate hasSize(HeuristicAllocationExpr alloc, DataFlow::Node n, int state) {
  * but because there's a strict comparison that compares `n` against the size of the allocation this
  * snippet is fine.
  */
-module SizeBarrier {
+private 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, _)
     }
 
+    /**
+     * Holds if `small <= large + k` holds if `g` evaluates to `testIsTrue`.
+     */
     additional predicate isSink(
-      DataFlow::Node left, DataFlow::Node right, IRGuardCondition g, int k, boolean testIsTrue
+      DataFlow::Node small, DataFlow::Node large, IRGuardCondition g, int k, boolean 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)
+      // The sink is any "large" side of a relational comparison. i.e., the `large` expression
+      // in a guard such as `small <= large + k`.
+      g.comparesLt(small.asOperand(), large.asOperand(), k + 1, true, testIsTrue)
     }
 
     predicate isSink(DataFlow::Node sink) { isSink(_, sink, _, _, _) }
   }
 
-  private import DataFlow::Global<SizeBarrierConfig>
+  module SizeBarrierFlow = DataFlow::Global<SizeBarrierConfig>;
 
-  private int getAFlowStateForNode(DataFlow::Node node) {
+  private int getASizeAddend(DataFlow::Node node) {
     exists(DataFlow::Node source |
-      flow(source, node) and
+      SizeBarrierFlow::flow(source, node) and
       hasSize(_, source, result)
     )
   }
 
+  /**
+   * Holds if `small <= large + k` holds if `g` evaluates to `edge`.
+   */
   private predicate operandGuardChecks(
-    IRGuardCondition g, Operand left, Operand right, int state, boolean edge
+    IRGuardCondition g, Operand small, DataFlow::Node large, int k, boolean edge
   ) {
-    exists(DataFlow::Node nLeft, DataFlow::Node nRight, int k |
-      nRight.asOperand() = right and
-      nLeft.asOperand() = left and
-      SizeBarrierConfig::isSink(nLeft, nRight, g, k, edge) and
-      state = getAFlowStateForNode(nRight) and
-      k <= state
+    SizeBarrierFlow::flowTo(large) and
+    SizeBarrierConfig::isSink(DataFlow::operandNode(small), large, g, k, edge)
+  }
+
+  /**
+   * Gets an instruction `instr` that is guarded by a check such as `instr <= small + delta` where
+   * `small <= _ + k` and `small` is the "small side" of of a relational comparison that checks
+   * whether `small <= size` where `size` is the size of an allocation.
+   */
+  Instruction getABarrierInstruction0(int delta, int k) {
+    exists(
+      IRGuardCondition g, ValueNumber value, Operand small, boolean edge, DataFlow::Node large
+    |
+      // We know:
+      // 1. result <= value + delta (by `bounded`)
+      // 2. value <= large + k (by `operandGuardChecks`).
+      // So:
+      // result <= value + delta (by 1.)
+      //        <= large + k + delta (by 2.)
+      small = value.getAUse() and
+      operandGuardChecks(pragma[only_bind_into](g), pragma[only_bind_into](small), large,
+        pragma[only_bind_into](k), pragma[only_bind_into](edge)) and
+      bounded(result, value.getAnInstruction(), delta) and
+      g.controls(result.getBlock(), edge) and
+      k < getASizeAddend(large)
     )
   }
 
   /**
    * 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.
    */
+  bindingset[state]
+  pragma[inline_late]
   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), _,
-        pragma[only_bind_into](state), pragma[only_bind_into](edge)) and
-      result = value.getAnInstruction() and
-      g.controls(result.getBlock(), edge)
+    exists(int delta, int k |
+      state > k + delta and
+      // result <= "size of allocation" + delta + k
+      //        < "size of allocation" + state
+      result = getABarrierInstruction0(delta, k)
     )
   }
 
@@ -155,14 +181,16 @@ module SizeBarrier {
    * the value of the node is upper-bounded by size of some allocation.
    */
   DataFlow::Node getABarrierNode(int state) {
-    result.asOperand() = getABarrierInstruction(state).getAUse()
+    exists(DataFlow::Node source, int delta, int k |
+      SizeBarrierFlow::flow(source, result) and
+      hasSize(_, source, state) and
+      result.asInstruction() = SizeBarrier::getABarrierInstruction0(delta, k) and
+      state > k + delta
+      // so now we have:
+      // result <= "size of allocation" + delta + k
+      //        < "size of allocation" + state
+    )
   }
-
-  /**
-   * 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(int state) { result.getAnInstruction() = getABarrierInstruction(state) }
 }
 
 private module InterestingPointerAddInstruction {

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

@@ -66,11 +66,14 @@
  * module. Since the node we are tracking is not necessarily _equal_ to the pointer-arithmetic instruction, but rather satisfies
  * `node.asInstruction() <= pai + deltaDerefSourceAndPai`, we need to account for the delta when checking if a guard is sufficiently
  * strong to infer that a future dereference is safe. To do this, we check that the guard guarantees that a node `n` satisfies
- * `n < node + k` where `node` is a node we know is equal to the value of the dereference source (i.e., it satisfies
- * `node.asInstruction() <= pai + deltaDerefSourceAndPai`) and `k <= deltaDerefSourceAndPai`. Combining this we have
- * `n < node + k <= node + deltaDerefSourceAndPai <= pai + 2*deltaDerefSourceAndPai` (TODO: Oops. This math doesn't quite work out.
- * I think this is because we need to redefine the `BarrierConfig` to start flow at the pointer-arithmetic instruction instead of
- * at the dereference source. When combined with TODO above it's easy to show that this guard ensures that the dereference is safe).
+ * `n < node + k` where `node` is a node such that `node <= pai`. Thus, we know that any node `m` such that `m <= n + delta` where
+ * `delta + k <= 0` will be safe because:
+ * ```
+ * m <= n + delta
+ *   <  node + k + delta
+ *   <= pai + k + delta
+ *   <= pai
+ * ```
  */
 
 private import cpp
@@ -82,76 +85,102 @@ private import RangeAnalysisUtil
 
 private module InvalidPointerToDerefBarrier {
   private module BarrierConfig implements DataFlow::ConfigSig {
-    predicate isSource(DataFlow::Node source) {
-      // The sources is the same as in the sources for `InvalidPointerToDerefConfig`.
-      invalidPointerToDerefSource(_, _, source, _)
+    additional predicate isSource(DataFlow::Node source, PointerArithmeticInstruction pai) {
+      invalidPointerToDerefSource(_, pai, _, _) and
+      // source <= pai
+      bounded2(source.asInstruction(), pai, any(int d | d <= 0))
     }
 
+    predicate isSource(DataFlow::Node source) { isSource(source, _) }
+
     additional predicate isSink(
-      DataFlow::Node left, DataFlow::Node right, IRGuardCondition g, int k, boolean testIsTrue
+      DataFlow::Node small, DataFlow::Node large, IRGuardCondition g, int k, boolean testIsTrue
     ) {
       // The sink is any "large" side of a relational comparison.
-      g.comparesLt(left.asOperand(), right.asOperand(), k, true, testIsTrue)
+      g.comparesLt(small.asOperand(), large.asOperand(), k, true, testIsTrue)
     }
 
     predicate isSink(DataFlow::Node sink) { isSink(_, sink, _, _, _) }
   }
 
   private module BarrierFlow = DataFlow::Global<BarrierConfig>;
 
-  private int getInvalidPointerToDerefSourceDelta(DataFlow::Node node) {
-    exists(DataFlow::Node source |
-      BarrierFlow::flow(source, node) and
-      invalidPointerToDerefSource(_, _, source, result)
-    )
-  }
-
+  /**
+   * Holds if `g` ensures that `small < large + k` if `g` evaluates to `edge`.
+   *
+   * Additionally, it also holds that `large <= pai`. Thus, when `g` evaluates to `edge`
+   * it holds that `small < pai + k`.
+   */
   private predicate operandGuardChecks(
-    IRGuardCondition g, Operand left, Operand right, int state, boolean edge
+    PointerArithmeticInstruction pai, IRGuardCondition g, Operand small, int k, boolean edge
   ) {
-    exists(DataFlow::Node nLeft, DataFlow::Node nRight, int k |
-      nRight.asOperand() = right and
-      nLeft.asOperand() = left and
-      BarrierConfig::isSink(nLeft, nRight, g, k, edge) and
-      state = getInvalidPointerToDerefSourceDelta(nRight) and
-      k <= state
+    exists(DataFlow::Node source, DataFlow::Node nSmall, DataFlow::Node nLarge |
+      nSmall.asOperand() = small and
+      BarrierConfig::isSource(source, pai) and
+      BarrierFlow::flow(source, nLarge) and
+      BarrierConfig::isSink(nSmall, nLarge, g, k, edge)
     )
   }
 
-  Instruction getABarrierInstruction(int state) {
-    exists(IRGuardCondition g, ValueNumber value, Operand use, boolean edge |
+  /**
+   * Gets an instruction `instr` such that `instr < pai`.
+   */
+  Instruction getABarrierInstruction(PointerArithmeticInstruction pai) {
+    exists(IRGuardCondition g, ValueNumber value, Operand use, boolean edge, int delta, int k |
       use = value.getAUse() and
-      operandGuardChecks(pragma[only_bind_into](g), pragma[only_bind_into](use), _, state,
-        pragma[only_bind_into](edge)) and
-      result = value.getAnInstruction() and
-      g.controls(result.getBlock(), edge)
+      // value < pai + k
+      operandGuardChecks(pai, pragma[only_bind_into](g), pragma[only_bind_into](use),
+        pragma[only_bind_into](k), pragma[only_bind_into](edge)) and
+      // result <= value + delta
+      bounded(result, value.getAnInstruction(), delta) and
+      g.controls(result.getBlock(), edge) and
+      delta + k <= 0
+      // combining the above we have: result < pai + k + delta <= pai
     )
   }
 
-  DataFlow::Node getABarrierNode() { result.asOperand() = getABarrierInstruction(_).getAUse() }
+  DataFlow::Node getABarrierNode(PointerArithmeticInstruction pai) {
+    result.asOperand() = getABarrierInstruction(pai).getAUse()
+  }
 
-  pragma[nomagic]
-  IRBlock getABarrierBlock(int state) { result.getAnInstruction() = getABarrierInstruction(state) }
+  /**
+   * Gets an address operand whose definition `instr` satisfies `instr < pai`.
+   */
+  AddressOperand getABarrierAddressOperand(PointerArithmeticInstruction pai) {
+    result.getDef() = getABarrierInstruction(pai)
+  }
 }
 
 /**
  * A configuration to track flow from a pointer-arithmetic operation found
  * by `AllocToInvalidPointerConfig` to a dereference of the pointer.
  */
-private module InvalidPointerToDerefConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { invalidPointerToDerefSource(_, _, source, _) }
+private module InvalidPointerToDerefConfig implements DataFlow::StateConfigSig {
+  class FlowState extends PointerArithmeticInstruction {
+    FlowState() { invalidPointerToDerefSource(_, this, _, _) }
+  }
+
+  predicate isSource(DataFlow::Node source, FlowState pai) {
+    invalidPointerToDerefSource(_, pai, source, _)
+  }
 
   pragma[inline]
-  predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink(sink, _, _, _) }
+  predicate isSink(DataFlow::Node sink) { isInvalidPointerDerefSink(sink, _, _, _, _) }
+
+  predicate isSink(DataFlow::Node sink, FlowState pai) { none() }
 
   predicate isBarrier(DataFlow::Node node) {
     node = any(DataFlow::SsaPhiNode phi | not phi.isPhiRead()).getAnInput(true)
-    or
-    node = InvalidPointerToDerefBarrier::getABarrierNode()
+  }
+
+  predicate isBarrier(DataFlow::Node node, FlowState pai) {
+    // `node = getABarrierNode(pai)` ensures that node < pai, so this node is safe to dereference.
+    // Note that this is the only place where the `FlowState` is used in this configuration.
+    node = InvalidPointerToDerefBarrier::getABarrierNode(pai)
   }
 }
 
-private import DataFlow::Global<InvalidPointerToDerefConfig>
+private import DataFlow::GlobalWithState<InvalidPointerToDerefConfig>
 
 /**
  * Holds if `allocSource` is dataflow node that represents an allocation that flows to the
@@ -165,19 +194,14 @@ private predicate invalidPointerToDerefSource(
   DataFlow::Node allocSource, PointerArithmeticInstruction pai, DataFlow::Node derefSource,
   int deltaDerefSourceAndPai
 ) {
-  exists(int rhsSizeDelta |
-    // Note that `deltaDerefSourceAndPai` is not necessarily equal to `rhsSizeDelta`:
-    // `rhsSizeDelta` is the constant offset added to the size of the allocation, and
-    // `deltaDerefSourceAndPai` is the constant difference between the pointer-arithmetic instruction
-    // and the instruction computing the address for which we will search for a dereference.
-    AllocToInvalidPointer::pointerAddInstructionHasBounds(allocSource, pai, _, rhsSizeDelta) and
-    bounded2(derefSource.asInstruction(), pai, deltaDerefSourceAndPai) and
-    deltaDerefSourceAndPai >= 0 and
-    // TODO: This condition will go away once #13725 is merged, and then we can make `SizeBarrier`
-    // private to `AllocationToInvalidPointer.qll`.
-    not derefSource.getBasicBlock() =
-      AllocToInvalidPointer::SizeBarrier::getABarrierBlock(rhsSizeDelta)
-  )
+  // Note that `deltaDerefSourceAndPai` is not necessarily equal to `rhsSizeDelta`:
+  // `rhsSizeDelta` is the constant offset added to the size of the allocation, and
+  // `deltaDerefSourceAndPai` is the constant difference between the pointer-arithmetic instruction
+  // and the instruction computing the address for which we will search for a dereference.
+  AllocToInvalidPointer::pointerAddInstructionHasBounds(allocSource, pai, _, _) and
+  // derefSource <= pai + deltaDerefSourceAndPai
+  bounded2(derefSource.asInstruction(), pai, deltaDerefSourceAndPai) and
+  deltaDerefSourceAndPai >= 0
 }
 
 /**
@@ -187,15 +211,14 @@ private predicate invalidPointerToDerefSource(
  */
 pragma[inline]
 private predicate isInvalidPointerDerefSink(
-  DataFlow::Node sink, Instruction i, string operation, int deltaDerefSinkAndDerefAddress
+  DataFlow::Node sink, AddressOperand addr, Instruction i, string operation,
+  int deltaDerefSinkAndDerefAddress
 ) {
-  exists(AddressOperand addr, Instruction s, IRBlock b |
+  exists(Instruction s |
     s = sink.asInstruction() and
     bounded(addr.getDef(), s, deltaDerefSinkAndDerefAddress) and
     deltaDerefSinkAndDerefAddress >= 0 and
-    i.getAnOperand() = addr and
-    b = i.getBlock() and
-    not b = InvalidPointerToDerefBarrier::getABarrierBlock(deltaDerefSinkAndDerefAddress)
+    i.getAnOperand() = addr
   |
     i instanceof StoreInstruction and
     operation = "write"
@@ -221,9 +244,11 @@ private Instruction getASuccessor(Instruction instr) {
   instr.getBlock().getASuccessor+() = result.getBlock()
 }
 
-private predicate paiForDereferenceSink(PointerArithmeticInstruction pai, DataFlow::Node derefSink) {
+private predicate paiForDereferenceSink(
+  PointerArithmeticInstruction pai, DataFlow::Node derefSink, int deltaDerefSourceAndPai
+) {
   exists(DataFlow::Node derefSource |
-    invalidPointerToDerefSource(_, pai, derefSource, _) and
+    invalidPointerToDerefSource(_, pai, derefSource, deltaDerefSourceAndPai) and
     flow(derefSource, derefSink)
   )
 }
@@ -235,13 +260,15 @@ private predicate paiForDereferenceSink(PointerArithmeticInstruction pai, DataFl
  */
 private predicate derefSinkToOperation(
   DataFlow::Node derefSink, PointerArithmeticInstruction pai, DataFlow::Node operation,
-  string description, int deltaDerefSinkAndDerefAddress
+  string description, int deltaDerefSourceAndPai, int deltaDerefSinkAndDerefAddress
 ) {
-  exists(Instruction operationInstr |
-    paiForDereferenceSink(pai, pragma[only_bind_into](derefSink)) and
-    isInvalidPointerDerefSink(derefSink, operationInstr, description, deltaDerefSinkAndDerefAddress) and
+  exists(Instruction operationInstr, AddressOperand addr |
+    paiForDereferenceSink(pai, pragma[only_bind_into](derefSink), deltaDerefSourceAndPai) and
+    isInvalidPointerDerefSink(derefSink, addr, operationInstr, description,
+      deltaDerefSinkAndDerefAddress) and
     operationInstr = getASuccessor(derefSink.asInstruction()) and
-    operation.asInstruction() = operationInstr
+    operation.asInstruction() = operationInstr and
+    not addr = InvalidPointerToDerefBarrier::getABarrierAddressOperand(pai)
   )
 }
 
@@ -260,7 +287,8 @@ predicate operationIsOffBy(
   exists(int deltaDerefSourceAndPai, int deltaDerefSinkAndDerefAddress |
     invalidPointerToDerefSource(allocation, pai, derefSource, deltaDerefSourceAndPai) and
     flow(derefSource, derefSink) and
-    derefSinkToOperation(derefSink, pai, operation, description, deltaDerefSinkAndDerefAddress) and
+    derefSinkToOperation(derefSink, pai, operation, description, deltaDerefSourceAndPai,
+      deltaDerefSinkAndDerefAddress) and
     delta = deltaDerefSourceAndPai + deltaDerefSinkAndDerefAddress
   )
 }