Merge branch 'main' into implement-is-unreachable-in-call-2

Author: MathiasVP

.github/workflows/check-implicit-this.yml

@@ -1,6 +1,13 @@
 name: "Check implicit this warnings"
 
-on: workflow_dispatch
+on:
+  workflow_dispatch:
+  pull_request:
+    paths:
+      - "**qlpack.yml"
+    branches:
+      - main
+      - "rc/*"
 
 jobs:
   check:
@@ -15,7 +22,7 @@ jobs:
           for pack_file in ${packs}; do
             option="$(yq '.warnOnImplicitThis' ${pack_file})"
             if [ "${option}" != "true" ]; then
-              echo "warnOnImplicitThis property must be set to 'true' for pack ${pack_file}"
+              echo "::error file=${pack_file}::warnOnImplicitThis property must be set to 'true' for pack ${pack_file}"
               EXIT_CODE=1
             fi
           done

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

@@ -321,9 +321,11 @@ private class PrimaryArgumentNode extends ArgumentNode, OperandNode {
 
 private class SideEffectArgumentNode extends ArgumentNode, SideEffectOperandNode {
   override predicate argumentOf(DataFlowCall dfCall, ArgumentPosition pos) {
-    this.getCallInstruction() = dfCall and
-    pos.(IndirectionPosition).getArgumentIndex() = this.getArgumentIndex() and
-    super.hasAddressOperandAndIndirectionIndex(_, pos.(IndirectionPosition).getIndirectionIndex())
+    exists(int indirectionIndex |
+      pos = TIndirectionPosition(argumentIndex, pragma[only_bind_into](indirectionIndex)) and
+      this.getCallInstruction() = dfCall and
+      super.hasAddressOperandAndIndirectionIndex(_, pragma[only_bind_into](indirectionIndex))
+    )
   }
 }
 

cpp/ql/src/experimental/Security/CWE/CWE-193/InvalidPointerDeref.ql

@@ -312,11 +312,13 @@ Instruction getASuccessor(Instruction instr) {
  */
 pragma[inline]
 predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string operation, int delta) {
-  exists(AddressOperand addr, Instruction s |
+  exists(AddressOperand addr, Instruction s, IRBlock b |
     s = sink.asInstruction() and
-    bounded1(addr.getDef(), s, delta) and
+    boundedImpl(addr.getDef(), s, delta) and
     delta >= 0 and
-    i.getAnOperand() = addr
+    i.getAnOperand() = addr and
+    b = i.getBlock() and
+    not b = InvalidPointerToDerefBarrier::getABarrierBlock(delta)
   |
     i instanceof StoreInstruction and
     operation = "write"
@@ -326,6 +328,60 @@ predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string o
   )
 }
 
+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 isSink(
+      DataFlow::Node left, DataFlow::Node right, IRGuardCondition g, int state, boolean testIsTrue
+    ) {
+      // The sink is any "large" side of a relational comparison.
+      g.comparesLt(left.asOperand(), right.asOperand(), state, true, testIsTrue)
+    }
+
+    predicate isSink(DataFlow::Node sink) { isSink(_, sink, _, _, _) }
+  }
+
+  private import DataFlow::Global<BarrierConfig>
+
+  private int getInvalidPointerToDerefSourceDelta(DataFlow::Node node) {
+    exists(DataFlow::Node source |
+      flow(source, node) and
+      invalidPointerToDerefSource(_, source, result)
+    )
+  }
+
+  private predicate operandGuardChecks(
+    IRGuardCondition g, Operand left, Operand right, int state, boolean edge
+  ) {
+    exists(DataFlow::Node nLeft, DataFlow::Node nRight, int state0 |
+      nRight.asOperand() = right and
+      nLeft.asOperand() = left and
+      BarrierConfig::isSink(nLeft, nRight, g, state0, edge) and
+      state = getInvalidPointerToDerefSourceDelta(nRight) and
+      state0 <= 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), _, state,
+        pragma[only_bind_into](edge)) and
+      result = value.getAnInstruction() and
+      g.controls(result.getBlock(), edge)
+    )
+  }
+
+  DataFlow::Node getABarrierNode() { result.asOperand() = getABarrierInstruction(_).getAUse() }
+
+  pragma[nomagic]
+  IRBlock getABarrierBlock(int state) { result.getAnInstruction() = getABarrierInstruction(state) }
+}
+
 /**
  * A configuration to track flow from a pointer-arithmetic operation found
  * by `AllocToInvalidPointerConfig` to a dereference of the pointer.
@@ -338,6 +394,8 @@ module InvalidPointerToDerefConfig implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) {
     node = any(DataFlow::SsaPhiNode phi | not phi.isPhiRead()).getAnInput(true)
+    or
+    node = InvalidPointerToDerefBarrier::getABarrierNode()
   }
 }
 
@@ -382,7 +440,7 @@ newtype TMergedPathNode =
   // pointer, but we want to raise an alert at the dereference.
   TPathNodeSink(Instruction i) {
     exists(DataFlow::Node n |
-      InvalidPointerToDerefFlow::flowTo(n) and
+      InvalidPointerToDerefFlow::flowTo(pragma[only_bind_into](n)) and
       isInvalidPointerDerefSink(n, i, _, _) and
       i = getASuccessor(n.asInstruction())
     )

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/ConstantSizeArrayOffByOne.expected

@@ -39,6 +39,21 @@ edges
 | test.cpp:148:23:148:28 | buffer | test.cpp:151:5:151:11 | access to array |
 | test.cpp:159:25:159:29 | array | test.cpp:161:5:161:10 | access to array |
 | test.cpp:159:25:159:29 | array | test.cpp:162:5:162:10 | access to array |
+| test.cpp:175:30:175:30 | p | test.cpp:191:27:191:30 | access to array |
+| test.cpp:198:14:198:20 | buffer1 | test.cpp:175:30:175:30 | p |
+| test.cpp:198:14:198:20 | buffer1 | test.cpp:198:14:198:20 | buffer1 |
+| test.cpp:201:14:201:20 | buffer2 | test.cpp:175:30:175:30 | p |
+| test.cpp:201:14:201:20 | buffer2 | test.cpp:201:14:201:20 | buffer2 |
+| test.cpp:204:14:204:20 | buffer3 | test.cpp:175:30:175:30 | p |
+| test.cpp:204:14:204:20 | buffer3 | test.cpp:204:14:204:20 | buffer3 |
+| test.cpp:207:35:207:35 | p | test.cpp:208:14:208:14 | p |
+| test.cpp:208:14:208:14 | p | test.cpp:175:30:175:30 | p |
+| test.cpp:213:19:213:25 | buffer1 | test.cpp:207:35:207:35 | p |
+| test.cpp:213:19:213:25 | buffer1 | test.cpp:213:19:213:25 | buffer1 |
+| test.cpp:216:19:216:25 | buffer2 | test.cpp:207:35:207:35 | p |
+| test.cpp:216:19:216:25 | buffer2 | test.cpp:216:19:216:25 | buffer2 |
+| test.cpp:219:19:219:25 | buffer3 | test.cpp:207:35:207:35 | p |
+| test.cpp:219:19:219:25 | buffer3 | test.cpp:219:19:219:25 | buffer3 |
 nodes
 | test.cpp:34:5:34:24 | access to array | semmle.label | access to array |
 | test.cpp:34:10:34:12 | buf | semmle.label | buf |
@@ -97,6 +112,22 @@ nodes
 | test.cpp:159:25:159:29 | array | semmle.label | array |
 | test.cpp:161:5:161:10 | access to array | semmle.label | access to array |
 | test.cpp:162:5:162:10 | access to array | semmle.label | access to array |
+| test.cpp:175:30:175:30 | p | semmle.label | p |
+| test.cpp:191:27:191:30 | access to array | semmle.label | access to array |
+| test.cpp:198:14:198:20 | buffer1 | semmle.label | buffer1 |
+| test.cpp:198:14:198:20 | buffer1 | semmle.label | buffer1 |
+| test.cpp:201:14:201:20 | buffer2 | semmle.label | buffer2 |
+| test.cpp:201:14:201:20 | buffer2 | semmle.label | buffer2 |
+| test.cpp:204:14:204:20 | buffer3 | semmle.label | buffer3 |
+| test.cpp:204:14:204:20 | buffer3 | semmle.label | buffer3 |
+| test.cpp:207:35:207:35 | p | semmle.label | p |
+| test.cpp:208:14:208:14 | p | semmle.label | p |
+| test.cpp:213:19:213:25 | buffer1 | semmle.label | buffer1 |
+| test.cpp:213:19:213:25 | buffer1 | semmle.label | buffer1 |
+| test.cpp:216:19:216:25 | buffer2 | semmle.label | buffer2 |
+| test.cpp:216:19:216:25 | buffer2 | semmle.label | buffer2 |
+| test.cpp:219:19:219:25 | buffer3 | semmle.label | buffer3 |
+| test.cpp:219:19:219:25 | buffer3 | semmle.label | buffer3 |
 subpaths
 #select
 | test.cpp:35:5:35:22 | PointerAdd: access to array | test.cpp:35:10:35:12 | buf | test.cpp:35:5:35:22 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:15:9:15:11 | buf | buf | test.cpp:35:5:35:26 | Store: ... = ... | write |
@@ -113,3 +144,5 @@ subpaths
 | test.cpp:136:9:136:16 | PointerAdd: ... += ... | test.cpp:143:18:143:21 | asdf | test.cpp:138:13:138:15 | arr | This pointer arithmetic may have an off-by-2 error allowing it to overrun $@ at this $@. | test.cpp:142:10:142:13 | asdf | asdf | test.cpp:138:12:138:15 | Load: * ... | read |
 | test.cpp:151:5:151:11 | PointerAdd: access to array | test.cpp:148:23:148:28 | buffer | test.cpp:151:5:151:11 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:147:19:147:24 | buffer | buffer | test.cpp:151:5:151:15 | Store: ... = ... | write |
 | test.cpp:162:5:162:10 | PointerAdd: access to array | test.cpp:159:25:159:29 | array | test.cpp:162:5:162:10 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:158:10:158:14 | array | array | test.cpp:162:5:162:19 | Store: ... = ... | write |
+| test.cpp:191:27:191:30 | PointerAdd: access to array | test.cpp:201:14:201:20 | buffer2 | test.cpp:191:27:191:30 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:200:19:200:25 | buffer2 | buffer2 | test.cpp:191:27:191:30 | Load: access to array | read |
+| test.cpp:191:27:191:30 | PointerAdd: access to array | test.cpp:216:19:216:25 | buffer2 | test.cpp:191:27:191:30 | access to array | This pointer arithmetic may have an off-by-1 error allowing it to overrun $@ at this $@. | test.cpp:215:19:215:25 | buffer2 | buffer2 | test.cpp:191:27:191:30 | Load: access to array | read |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/constant-size/test.cpp

@@ -169,3 +169,52 @@ void pointer_size_larger_than_array_element_size_and_does_not_divide_it() {
     ptr[0] = vec3{}; // GOOD: writes ints 0, 1, 2
     ptr[1] = vec3{}; // BAD: writes ints 3, 4, 5 [NOT DETECTED]
 }
+
+void use(...);
+
+void call_use(unsigned char* p, int n) {
+    if(n == 0) {
+        return;
+    }
+    if(n == 1) {
+        unsigned char x = p[0];
+        use(x);
+    }
+    if(n == 2) {
+        unsigned char x = p[0];
+        unsigned char y = p[1];
+        use(x, y);
+    }
+    if(n == 3) {
+        unsigned char x = p[0];
+        unsigned char y = p[1];
+        unsigned char z = p[2]; // GOOD [FALSE POSITIVE]: `call_use(buffer2, 2)` won't reach this point.
+        use(x, y, z);
+    }
+}
+
+void test_call_use() {
+    unsigned char buffer1[1];
+    call_use(buffer1,1);
+
+    unsigned char buffer2[2];
+    call_use(buffer2,2);
+
+    unsigned char buffer3[3];
+    call_use(buffer3,3);
+}
+
+void call_call_use(unsigned char* p, int n) {
+    call_use(p, n);
+}
+
+void test_call_use2() {
+    unsigned char buffer1[1];
+    call_call_use(buffer1,1);
+
+    unsigned char buffer2[2];
+    call_call_use(buffer2,2);
+
+    unsigned char buffer3[3];
+    call_call_use(buffer3,3);
+}