C++: Add a cpp/invalid-pointer-deref query to experimental

cpp/ql/lib/experimental/semmle/code/cpp/dataflow/ProductFlow.qll

@@ -1,5 +1,5 @@
-import semmle.code.cpp.ir.dataflow.DataFlow
-import semmle.code.cpp.ir.dataflow.DataFlow2
+import experimental.semmle.code.cpp.ir.dataflow.DataFlow
+import experimental.semmle.code.cpp.ir.dataflow.DataFlow2
 
 module ProductFlow {
   abstract class Configuration extends string {
@@ -11,14 +11,43 @@ module ProductFlow {
      *
      * `source1` and `source2` must belong to the same callable.
      */
-    abstract predicate isSourcePair(DataFlow::Node source1, DataFlow::Node source2);
+    predicate isSourcePair(DataFlow::Node source1, DataFlow::Node source2) { none() }
+
+    /**
+     * Holds if `(source1, source2)` is a relevant data flow source with initial states `state1`
+     * and `state2`, respectively.
+     *
+     * `source1` and `source2` must belong to the same callable.
+     */
+    predicate isSourcePair(
+      DataFlow::Node source1, string state1, DataFlow::Node source2, string state2
+    ) {
+      state1 = "" and
+      state2 = "" and
+      this.isSourcePair(source1, source2)
+    }
 
     /**
      * Holds if `(sink1, sink2)` is a relevant data flow sink.
      *
      * `sink1` and `sink2` must belong to the same callable.
      */
-    abstract predicate isSinkPair(DataFlow::Node sink1, DataFlow::Node sink2);
+    predicate isSinkPair(DataFlow::Node sink1, DataFlow::Node sink2) { none() }
+
+    /**
+     * Holds if `(sink1, sink2)` is a relevant data flow sink with final states `state1`
+     * and `state2`, respectively.
+     *
+     * `sink1` and `sink2` must belong to the same callable.
+     */
+    predicate isSinkPair(
+      DataFlow::Node sink1, DataFlow::FlowState state1, DataFlow::Node sink2,
+      DataFlow::FlowState state2
+    ) {
+      state1 = "" and
+      state2 = "" and
+      this.isSinkPair(sink1, sink2)
+    }
 
     predicate hasFlowPath(
       DataFlow::PathNode source1, DataFlow2::PathNode source2, DataFlow::PathNode sink1,
@@ -34,28 +63,28 @@ module ProductFlow {
     class Conf1 extends DataFlow::Configuration {
       Conf1() { this = "Conf1" }
 
-      override predicate isSource(DataFlow::Node source) {
-        exists(Configuration conf | conf.isSourcePair(source, _))
+      override predicate isSource(DataFlow::Node source, string state) {
+        exists(Configuration conf | conf.isSourcePair(source, state, _, _))
       }
 
-      override predicate isSink(DataFlow::Node sink) {
-        exists(Configuration conf | conf.isSinkPair(sink, _))
+      override predicate isSink(DataFlow::Node sink, string state) {
+        exists(Configuration conf | conf.isSinkPair(sink, state, _, _))
       }
     }
 
     class Conf2 extends DataFlow2::Configuration {
       Conf2() { this = "Conf2" }
 
-      override predicate isSource(DataFlow::Node source) {
+      override predicate isSource(DataFlow::Node source, string state) {
         exists(Configuration conf, DataFlow::Node source1 |
-          conf.isSourcePair(source1, source) and
+          conf.isSourcePair(source1, _, source, state) and
           any(Conf1 c).hasFlow(source1, _)
         )
       }
 
-      override predicate isSink(DataFlow::Node sink) {
+      override predicate isSink(DataFlow::Node sink, string state) {
         exists(Configuration conf, DataFlow::Node sink1 |
-          conf.isSinkPair(sink1, sink) and any(Conf1 c).hasFlow(_, sink1)
+          conf.isSinkPair(sink1, _, sink, state) and any(Conf1 c).hasFlow(_, sink1)
         )
       }
     }
@@ -65,7 +94,7 @@ module ProductFlow {
     Configuration conf, DataFlow::PathNode source1, DataFlow2::PathNode source2,
     DataFlow::PathNode node1, DataFlow2::PathNode node2
   ) {
-    conf.isSourcePair(node1.getNode(), node2.getNode()) and
+    conf.isSourcePair(node1.getNode(), _, node2.getNode(), _) and
     node1 = source1 and
     node2 = source2
     or
@@ -128,7 +157,7 @@ module ProductFlow {
   ) {
     exists(DataFlow::PathNode mid1, DataFlow2::PathNode mid2 |
       reachableInterprocEntry(conf, source1, source2, mid1, mid2) and
-      conf.isSinkPair(sink1.getNode(), sink2.getNode()) and
+      conf.isSinkPair(sink1.getNode(), _, sink2.getNode(), _) and
       localPathStep1*(mid1, sink1) and
       localPathStep2*(mid2, sink2)
     )

cpp/ql/lib/experimental/semmle/code/cpp/rangeanalysis/Bound.qll

@@ -28,6 +28,10 @@ private newtype TBound =
       i.(LoadInstruction).getSourceAddress() instanceof FieldAddressInstruction
       or
       i.getAUse() instanceof ArgumentOperand
+      or
+      i instanceof PointerArithmeticInstruction
+      or
+      i.getAUse() instanceof AddressOperand
     )
   }
 

cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExpr.qll

@@ -178,11 +178,11 @@ class SemRelationalExpr extends SemBinaryExpr {
 }
 
 class SemAddExpr extends SemBinaryExpr {
-  SemAddExpr() { opcode instanceof Opcode::Add }
+  SemAddExpr() { opcode instanceof Opcode::Add or opcode instanceof Opcode::PointerAdd }
 }
 
 class SemSubExpr extends SemBinaryExpr {
-  SemSubExpr() { opcode instanceof Opcode::Sub }
+  SemSubExpr() { opcode instanceof Opcode::Sub or opcode instanceof Opcode::PointerSub }
 }
 
 class SemMulExpr extends SemBinaryExpr {

cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticOpcode.qll

@@ -65,10 +65,18 @@ module Opcode {
     override string toString() { result = "Add" }
   }
 
+  class PointerAdd extends Opcode, TPointerAdd {
+    override string toString() { result = "PointerAdd" }
+  }
+
   class Sub extends Opcode, TSub {
     override string toString() { result = "Sub" }
   }
 
+  class PointerSub extends Opcode, TPointerSub {
+    override string toString() { result = "PointerSub" }
+  }
+
   class Mul extends Opcode, TMul {
     override string toString() { result = "Mul" }
   }

cpp/ql/lib/experimental/semmle/code/cpp/semantic/analysis/RangeAnalysis.qll

@@ -223,7 +223,9 @@ private SemGuard boundFlowCond(
       else resultIsStrict = testIsTrue.booleanNot()
     ) and
     (
-      if getTrackedTypeForSsaVariable(v) instanceof SemIntegerType
+      if
+        getTrackedTypeForSsaVariable(v) instanceof SemIntegerType or
+        getTrackedTypeForSsaVariable(v) instanceof SemAddressType
       then
         upper = true and strengthen = -1
         or

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

@@ -0,0 +1,26 @@
+void *malloc(unsigned);
+unsigned get_size();
+void write_data(const unsigned char*, const unsigned char*);
+
+int main(int argc, char* argv[]) {
+  unsigned size = get_size();
+
+  {
+    unsigned char *begin = (unsigned char*)malloc(size);
+    if(!begin) return -1;
+
+    unsigned char* end = begin + size;
+    write_data(begin, end);
+    *end = '\0'; // BAD: Out-of-bounds write
+  }
+
+  {
+    unsigned char *begin = (unsigned char*)malloc(size);
+    if(!begin) return -1;
+
+    unsigned char* end = begin + size;
+    write_data(begin, end);
+    *(end - 1) = '\0'; // GOOD: writing to the last byte
+  }
+
+}

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

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The program performs an out-of-bounds read or write operation. In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that pointer dereferences are properly guarded to ensure that they cannot be used to read or write past the end of the allocation.</p>
+
+</recommendation>
+<example>
+<p>The first example allocates a buffer of size <code>size</code> and creates a local variable that stores the location that is one byte past the end of the allocation.
+This local variable is then dereferenced which results in an out-of-bounds write.
+The second example subtracts one from the <code>end</code> variable before dereferencing it. This subtraction ensures that the write correctly updates the final byte of the allocation.</p>
+<sample src="InvalidPointerDeref.cpp" />
+
+</example>
+<references>
+
+<li>CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/ARR30-C.+Do+not+form+or+use+out-of-bounds+pointers+or+array+subscripts">ARR30-C. Do not form or use out-of-bounds pointers or array subscripts</a>.</li>
+<li>
+OWASP:
+<a href="https://owasp.org/www-community/vulnerabilities/Buffer_Overflow">Buffer Overflow</a>.
+</li>
+
+</references>
+</qhelp>