Merge branch 'main' into amammad-python-WebAppsConstatntSecretKeys

Author: RasmusWL

cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/BuildScripts.cs

@@ -327,7 +327,7 @@ public void TestCppAutobuilderSuccess()
         {
             Actions.RunProcess[@"cmd.exe /C nuget restore C:\Project\test.sln -DisableParallelProcessing"] = 1;
             Actions.RunProcess[@"cmd.exe /C C:\Project\.nuget\nuget.exe restore C:\Project\test.sln -DisableParallelProcessing"] = 0;
-            Actions.RunProcess[@"cmd.exe /C CALL ^""C:\Program Files ^(x86^)\Microsoft Visual Studio 14.0\VC\vcvarsall.bat^"" && set Platform=&& type NUL && msbuild C:\Project\test.sln /t:rebuild /p:Platform=""x86"" /p:Configuration=""Release"""] = 0;
+            Actions.RunProcess[@"cmd.exe /C CALL ^""C:\Program^ Files^ ^(x86^)\Microsoft^ Visual^ Studio^ 14.0\VC\vcvarsall.bat^"" && set Platform=&& type NUL && msbuild C:\Project\test.sln /t:rebuild /p:Platform=""x86"" /p:Configuration=""Release"""] = 0;
             Actions.RunProcessOut[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationPath"] = "";
             Actions.RunProcess[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationPath"] = 1;
             Actions.RunProcess[@"C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe -prerelease -legacy -property installationVersion"] = 0;

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

@@ -1520,6 +1520,25 @@ private module Cached {
     )
   }
 
+  /**
+   * Holds if `operand.getDef() = instr`, but there exists a `StoreInstruction` that
+   * writes to an address that is equivalent to the value computed by `instr` in
+   * between `instr` and `operand`, and therefore there should not be flow from `*instr`
+   * to `*operand`.
+   */
+  pragma[nomagic]
+  private predicate isStoredToBetween(Instruction instr, Operand operand) {
+    simpleOperandLocalFlowStep(pragma[only_bind_into](instr), pragma[only_bind_into](operand)) and
+    exists(StoreInstruction store, IRBlock block, int storeIndex, int instrIndex, int operandIndex |
+      store.getDestinationAddress() = instr and
+      block.getInstruction(storeIndex) = store and
+      block.getInstruction(instrIndex) = instr and
+      block.getInstruction(operandIndex) = operand.getUse() and
+      instrIndex < storeIndex and
+      storeIndex < operandIndex
+    )
+  }
+
   private predicate indirectionInstructionFlow(
     RawIndirectInstruction nodeFrom, IndirectOperand nodeTo
   ) {
@@ -1529,7 +1548,8 @@ private module Cached {
       simpleOperandLocalFlowStep(pragma[only_bind_into](instr), pragma[only_bind_into](operand))
     |
       hasOperandAndIndex(nodeTo, operand, pragma[only_bind_into](indirectionIndex)) and
-      hasInstructionAndIndex(nodeFrom, instr, pragma[only_bind_into](indirectionIndex))
+      hasInstructionAndIndex(nodeFrom, instr, pragma[only_bind_into](indirectionIndex)) and
+      not isStoredToBetween(instr, operand)
     )
   }
 

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll

@@ -6,6 +6,7 @@ private import DataFlowImplCommon as DataFlowImplCommon
 private import DataFlowUtil
 private import semmle.code.cpp.models.interfaces.PointerWrapper
 private import DataFlowPrivate
+private import semmle.code.cpp.ir.ValueNumbering
 
 /**
  * Holds if `operand` is an operand that is not used by the dataflow library.
@@ -864,7 +865,7 @@ private module Cached {
    * to a specific address.
    */
   private predicate isCertainAddress(Operand operand) {
-    operand.getDef() instanceof VariableAddressInstruction
+    valueNumberOfOperand(operand).getAnInstruction() instanceof VariableAddressInstruction
     or
     operand.getType() instanceof Cpp::ReferenceType
   }

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/RangeAnalysisStage.qll

@@ -574,16 +574,6 @@ module RangeStage<
     )
   }
 
-  /** Holds if `e >= 1` as determined by sign analysis. */
-  private predicate strictlyPositiveIntegralExpr(SemExpr e) {
-    semStrictlyPositive(e) and getTrackedType(e) instanceof SemIntegerType
-  }
-
-  /** Holds if `e <= -1` as determined by sign analysis. */
-  private predicate strictlyNegativeIntegralExpr(SemExpr e) {
-    semStrictlyNegative(e) and getTrackedType(e) instanceof SemIntegerType
-  }
-
   /**
    * Holds if `e1 + delta` is a valid bound for `e2`.
    * - `upper = true`  : `e2 <= e1 + delta`
@@ -597,27 +587,6 @@ module RangeStage<
     delta = D::fromInt(0) and
     (upper = true or upper = false)
     or
-    exists(SemExpr x, SemSubExpr sub |
-      e2 = sub and
-      sub.getLeftOperand() = e1 and
-      sub.getRightOperand() = x
-    |
-      // `x instanceof ConstantIntegerExpr` is covered by valueFlowStep
-      not x instanceof SemConstantIntegerExpr and
-      if strictlyPositiveIntegralExpr(x)
-      then upper = true and delta = D::fromInt(-1)
-      else
-        if semPositive(x)
-        then upper = true and delta = D::fromInt(0)
-        else
-          if strictlyNegativeIntegralExpr(x)
-          then upper = false and delta = D::fromInt(1)
-          else
-            if semNegative(x)
-            then upper = false and delta = D::fromInt(0)
-            else none()
-    )
-    or
     e2.(SemRemExpr).getRightOperand() = e1 and
     semPositive(e1) and
     delta = D::fromInt(-1) and
@@ -1137,6 +1106,23 @@ module RangeStage<
         b = bRight and origdelta = odRight and reason = rRight and bLeft instanceof SemZeroBound
       )
       or
+      exists(D::Delta dLeft, D::Delta dRight, boolean fbeLeft, boolean fbeRight |
+        boundedSubOperandLeft(e, upper, b, dLeft, fbeLeft, origdelta, reason) and
+        boundedSubOperandRight(e, upper, dRight, fbeRight) and
+        // when `upper` is `true` we have:
+        // left <= b + dLeft
+        // right >= 0 + dRight
+        // left - right <= b + dLeft - (0 + dRight)
+        //               = b + (dLeft - dRight)
+        // and when `upper` is `false` we have:
+        // left >= b + dLeft
+        // right <= 0 + dRight
+        // left - right >= b + dLeft - (0 + dRight)
+        //               = b + (dLeft - dRight)
+        delta = D::fromFloat(D::toFloat(dLeft) - D::toFloat(dRight)) and
+        fromBackEdge = fbeLeft.booleanOr(fbeRight)
+      )
+      or
       exists(
         SemRemExpr rem, D::Delta d_max, D::Delta d1, D::Delta d2, boolean fbe1, boolean fbe2,
         D::Delta od1, D::Delta od2, SemReason r1, SemReason r2
@@ -1201,6 +1187,37 @@ module RangeStage<
     )
   }
 
+  /**
+   * Holds if `sub = left - right` and `left <= b + delta` if `upper` is `true`
+   * and `left >= b + delta` is `upper` is `false`.
+   */
+  pragma[nomagic]
+  private predicate boundedSubOperandLeft(
+    SemSubExpr sub, boolean upper, SemBound b, D::Delta delta, boolean fromBackEdge,
+    D::Delta origdelta, SemReason reason
+  ) {
+    // `semValueFlowStep` already handles the case where one of the operands is a constant.
+    not semValueFlowStep(sub, _, _) and
+    bounded(sub.getLeftOperand(), b, delta, upper, fromBackEdge, origdelta, reason)
+  }
+
+  /**
+   * Holds if `sub = left - right` and `right <= 0 + delta` if `upper` is `false`
+   * and `right >= 0 + delta` is `upper` is `true`.
+   *
+   * Note that the boolean value of `upper` is flipped compared to many other predicates in
+   * this file. This ensures a clean join at the call-site.
+   */
+  pragma[nomagic]
+  private predicate boundedSubOperandRight(
+    SemSubExpr sub, boolean upper, D::Delta delta, boolean fromBackEdge
+  ) {
+    // `semValueFlowStep` already handles the case where one of the operands is a constant.
+    not semValueFlowStep(sub, _, _) and
+    bounded(sub.getRightOperand(), any(SemZeroBound zb), delta, upper.booleanNot(), fromBackEdge, _,
+      _)
+  }
+
   pragma[nomagic]
   private predicate boundedRemExpr(
     SemRemExpr rem, boolean upper, D::Delta delta, boolean fromBackEdge, D::Delta origdelta,

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 {