Merge pull request #4125 from geoffw0/oparray2

C++: Model operator[]

Author: jbj

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

@@ -484,6 +484,17 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   // Expr -> Expr
   exprToExprStep_nocfg(nodeFrom.asExpr(), nodeTo.asExpr())
   or
+  // Assignment -> LValue post-update node
+  //
+  // This is used for assignments whose left-hand side is not a variable
+  // assignment or a storeStep but is still modeled by other means. It could be
+  // a call to `operator*` or `operator[]` where taint should flow to the
+  // post-update node of the qualifier.
+  exists(AssignExpr assign |
+    nodeFrom.asExpr() = assign and
+    nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr() = assign.getLValue()
+  )
+  or
   // Node -> FlowVar -> VariableAccess
   exists(FlowVar var |
     (

cpp/ql/src/semmle/code/cpp/dataflow/internal/TaintTrackingUtil.qll

@@ -82,6 +82,19 @@ predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeT
   exprToDefinitionByReferenceStep(nodeFrom.asExpr(), nodeTo.asDefiningArgument())
   or
   exprToPartialDefinitionStep(nodeFrom.asExpr(), nodeTo.asPartialDefinition())
+  or
+  // Reverse taint: taint that flows from the post-update node of a reference
+  // returned by a function call, back into the qualifier of that function.
+  // This allows taint to flow 'in' through references returned by a modeled
+  // function such as `operator[]`.
+  exists(TaintFunction f, Call call, FunctionInput inModel, FunctionOutput outModel |
+    call.getTarget() = f and
+    inModel.isReturnValueDeref() and
+    outModel.isQualifierObject() and
+    f.hasTaintFlow(inModel, outModel) and
+    nodeFrom.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = call and
+    nodeTo.asDefiningArgument() = call.getQualifier()
+  )
 }
 
 /**

cpp/ql/src/semmle/code/cpp/models/implementations/StdContainer.qll

@@ -14,10 +14,7 @@ import semmle.code.cpp.models.interfaces.Taint
  */
 class StdSequenceContainerConstructor extends Constructor, TaintFunction {
   StdSequenceContainerConstructor() {
-    this.getDeclaringType().hasQualifiedName("std", "vector") or
-    this.getDeclaringType().hasQualifiedName("std", "deque") or
-    this.getDeclaringType().hasQualifiedName("std", "list") or
-    this.getDeclaringType().hasQualifiedName("std", "forward_list")
+    this.getDeclaringType().hasQualifiedName("std", ["vector", "deque", "list", "forward_list"])
   }
 
   /**
@@ -42,10 +39,8 @@ class StdSequenceContainerConstructor extends Constructor, TaintFunction {
 class StdSequenceContainerPush extends TaintFunction {
   StdSequenceContainerPush() {
     this.hasQualifiedName("std", "vector", "push_back") or
-    this.hasQualifiedName("std", "deque", "push_back") or
-    this.hasQualifiedName("std", "deque", "push_front") or
-    this.hasQualifiedName("std", "list", "push_back") or
-    this.hasQualifiedName("std", "list", "push_front") or
+    this.hasQualifiedName("std", "deque", ["push_back", "push_front"]) or
+    this.hasQualifiedName("std", "list", ["push_back", "push_front"]) or
     this.hasQualifiedName("std", "forward_list", "push_front")
   }
 
@@ -61,14 +56,10 @@ class StdSequenceContainerPush extends TaintFunction {
  */
 class StdSequenceContainerFrontBack extends TaintFunction {
   StdSequenceContainerFrontBack() {
-    this.hasQualifiedName("std", "array", "front") or
-    this.hasQualifiedName("std", "array", "back") or
-    this.hasQualifiedName("std", "vector", "front") or
-    this.hasQualifiedName("std", "vector", "back") or
-    this.hasQualifiedName("std", "deque", "front") or
-    this.hasQualifiedName("std", "deque", "back") or
-    this.hasQualifiedName("std", "list", "front") or
-    this.hasQualifiedName("std", "list", "back") or
+    this.hasQualifiedName("std", "array", ["front", "back"]) or
+    this.hasQualifiedName("std", "vector", ["front", "back"]) or
+    this.hasQualifiedName("std", "deque", ["front", "back"]) or
+    this.hasQualifiedName("std", "list", ["front", "back"]) or
     this.hasQualifiedName("std", "forward_list", "front")
   }
 
@@ -84,11 +75,7 @@ class StdSequenceContainerFrontBack extends TaintFunction {
  */
 class StdSequenceContainerSwap extends TaintFunction {
   StdSequenceContainerSwap() {
-    this.hasQualifiedName("std", "array", "swap") or
-    this.hasQualifiedName("std", "vector", "swap") or
-    this.hasQualifiedName("std", "deque", "swap") or
-    this.hasQualifiedName("std", "list", "swap") or
-    this.hasQualifiedName("std", "forward_list", "swap")
+    this.hasQualifiedName("std", ["array", "vector", "deque", "list", "forward_list"], "swap")
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
@@ -100,3 +87,22 @@ class StdSequenceContainerSwap extends TaintFunction {
     output.isQualifierObject()
   }
 }
+
+/**
+ * The standard container functions `at` and `operator[]`.
+ */
+class StdSequenceContainerAt extends TaintFunction {
+  StdSequenceContainerAt() {
+    this.hasQualifiedName("std", ["vector", "array", "deque"], ["at", "operator[]"])
+  }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from qualifier to referenced return value
+    input.isQualifierObject() and
+    output.isReturnValueDeref()
+    or
+    // reverse flow from returned reference to the qualifier
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
+  }
+}

cpp/ql/src/semmle/code/cpp/models/implementations/StdString.qll

@@ -11,10 +11,7 @@ class StdBasicString extends TemplateClass {
  * The `std::string` functions `c_str` and  `data`.
  */
 class StdStringCStr extends TaintFunction {
-  StdStringCStr() {
-    this.hasQualifiedName("std", "basic_string", "c_str") or
-    this.hasQualifiedName("std", "basic_string", "data")
-  }
+  StdStringCStr() { this.hasQualifiedName("std", "basic_string", ["c_str", "data"]) }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // flow from string itself (qualifier) to return value
@@ -49,10 +46,7 @@ class StdStringPlus extends TaintFunction {
  */
 class StdStringAppend extends TaintFunction {
   StdStringAppend() {
-    this.hasQualifiedName("std", "basic_string", "operator+=") or
-    this.hasQualifiedName("std", "basic_string", "append") or
-    this.hasQualifiedName("std", "basic_string", "insert") or
-    this.hasQualifiedName("std", "basic_string", "replace")
+    this.hasQualifiedName("std", "basic_string", ["operator+=", "append", "insert", "replace"])
   }
 
   /**
@@ -145,3 +139,20 @@ class StdStringSwap extends TaintFunction {
     output.isQualifierObject()
   }
 }
+
+/**
+ * The `std::string` functions `at` and `operator[]`.
+ */
+class StdStringAt extends TaintFunction {
+  StdStringAt() { this.hasQualifiedName("std", "basic_string", ["at", "operator[]"]) }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from qualifier to referenced return value
+    input.isQualifierObject() and
+    output.isReturnValueDeref()
+    or
+    // reverse flow from returned reference to the qualifier
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
+  }
+}

cpp/ql/src/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll

@@ -10,7 +10,8 @@ private newtype TFunctionInput =
   TInParameter(ParameterIndex i) or
   TInParameterDeref(ParameterIndex i) or
   TInQualifierObject() or
-  TInQualifierAddress()
+  TInQualifierAddress() or
+  TInReturnValueDeref()
 
 /**
  * An input to a function. This can be:
@@ -106,6 +107,31 @@ class FunctionInput extends TFunctionInput {
    *   (with type `C const *`) on entry to the function.
    */
   predicate isQualifierAddress() { none() }
+
+  /**
+   * Holds if this is the input value pointed to by the return value of a
+   * function, if the function returns a pointer, or the input value referred
+   * to by the return value of a function, if the function returns a reference.
+   *
+   * Example:
+   * ```
+   * char* getPointer();
+   * float& getReference();
+   * int getInt();
+   * ```
+   * - `isReturnValueDeref()` holds for the `FunctionInput` that represents the
+   *   value of `*getPointer()` (with type `char`).
+   * - `isReturnValueDeref()` holds for the `FunctionInput` that represents the
+   *   value of `getReference()` (with type `float`).
+   * - There is no `FunctionInput` of `getInt()` for which
+   *   `isReturnValueDeref()` holds because the return type of `getInt()` is
+   *   neither a pointer nor a reference.
+   *
+   * Note that data flows in through function return values are relatively
+   * rare, but they do occur when a function returns a reference to itself,
+   * part of itself, or one of its other inputs.
+   */
+  predicate isReturnValueDeref() { none() }
 }
 
 /**
@@ -199,6 +225,34 @@ class InQualifierAddress extends FunctionInput, TInQualifierAddress {
   override predicate isQualifierAddress() { any() }
 }
 
+/**
+ * The input value pointed to by the return value of a function, if the
+ * function returns a pointer, or the input value referred to by the return
+ * value of a function, if the function returns a reference.
+ *
+ * Example:
+ * ```
+ * char* getPointer();
+ * float& getReference();
+ * int getInt();
+ * ```
+ * - `InReturnValueDeref` represents the value of `*getPointer()` (with type
+ *   `char`).
+ * - `InReturnValueDeref` represents the value of `getReference()` (with type
+ *   `float`).
+ * - `InReturnValueDeref` does not represent the return value of `getInt()`
+ *   because the return type of `getInt()` is neither a pointer nor a reference.
+ *
+ * Note that data flows in through function return values are relatively
+ * rare, but they do occur when a function returns a reference to itself,
+ * part of itself, or one of its other inputs.
+ */
+class InReturnValueDeref extends FunctionInput, TInReturnValueDeref {
+  override string toString() { result = "InReturnValueDeref" }
+
+  override predicate isReturnValueDeref() { any() }
+}
+
 private newtype TFunctionOutput =
   TOutParameterDeref(ParameterIndex i) or
   TOutQualifierObject() or

cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected

@@ -7,11 +7,14 @@
 | example.c:17:19:17:22 | {...} | example.c:26:19:26:24 | coords |
 | example.c:24:2:24:7 | coords [post update] | example.c:26:2:26:7 | coords |
 | example.c:24:2:24:7 | coords [post update] | example.c:26:19:26:24 | coords |
+| example.c:24:2:24:30 | ... = ... | example.c:24:9:24:9 | x [post update] |
 | example.c:24:13:24:18 | coords [post update] | example.c:24:2:24:7 | coords |
 | example.c:24:13:24:18 | coords [post update] | example.c:26:2:26:7 | coords |
 | example.c:24:13:24:18 | coords [post update] | example.c:26:19:26:24 | coords |
 | example.c:24:13:24:30 | ... = ... | example.c:24:2:24:30 | ... = ... |
+| example.c:24:13:24:30 | ... = ... | example.c:24:20:24:20 | y [post update] |
 | example.c:24:24:24:30 | ... + ... | example.c:24:13:24:30 | ... = ... |
+| example.c:26:2:26:25 | ... = ... | example.c:26:9:26:9 | x [post update] |
 | example.c:26:13:26:16 | call to getX | example.c:26:2:26:25 | ... = ... |
 | example.c:26:18:26:24 | ref arg & ... | example.c:26:2:26:7 | coords |
 | example.c:26:18:26:24 | ref arg & ... | example.c:26:19:26:24 | coords [inner post update] |