Non-initializing writes should target post-update nodes

Author: owen-mc

go/ql/lib/semmle/go/controlflow/ControlFlowGraph.qll

@@ -118,6 +118,8 @@ module ControlFlow {
     /** Gets the left-hand side of this write. */
     IR::WriteTarget getLhs() { result = super.getLhs() }
 
+    private predicate isInitialization() { super.isInitialization() }
+
     /** Gets the right-hand side of this write. */
     DataFlow::Node getRhs() { super.getRhs() = result.asInstruction() }
 
@@ -134,13 +136,20 @@ module ControlFlow {
      * Holds if this node sets the value of field `f` on `base` (or its implicit dereference) to
      * `rhs`.
      *
-     * For example, for the assignment `x.width = newWidth`, `base` is either the data-flow node
-     * corresponding to `x` or (if `x` is a pointer) the data-flow node corresponding to the
-     * implicit dereference `*x`, `f` is the field referenced by `width`, and `rhs` is the data-flow
-     * node corresponding to `newWidth`.
+     * For example, for the assignment `x.width = newWidth`, `base` is the post-update node of
+     * either the data-flow node corresponding to `x` or (if `x` is a pointer) the data-flow node
+     * corresponding to the implicit dereference `*x`, `f` is the field referenced by `width`, and
+     * `rhs` is the data-flow node corresponding to `newWidth`. If this `WriteNode` is a struct
+     * initialization then there is no need for a post-update node and `base` is the struct literal
+     * being initialized.
      */
     predicate writesField(DataFlow::Node base, Field f, DataFlow::Node rhs) {
-      this.writesFieldInsn(base.asInstruction(), f, rhs.asInstruction())
+      exists(DataFlow::Node b | this.writesFieldInsn(b.asInstruction(), f, rhs.asInstruction()) |
+        this.isInitialization() and base = b
+        or
+        not this.isInitialization() and
+        b = base.(DataFlow::PostUpdateNode).getPreUpdateNode()
+      )
     }
 
     private predicate writesFieldInsn(IR::Instruction base, Field f, IR::Instruction rhs) {
@@ -158,13 +167,22 @@ module ControlFlow {
      * Holds if this node sets the value of element `index` on `base` (or its implicit dereference)
      * to `rhs`.
      *
-     * For example, for the assignment `xs[i] = v`, `base` is either the data-flow node
-     * corresponding to `xs` or (if `xs` is a pointer) the data-flow node corresponding to the
-     * implicit dereference `*xs`, `index` is the data-flow node corresponding to `i`, and `rhs`
-     * is the data-flow node corresponding to `base`.
+     * For example, for the assignment `xs[i] = v`, `base` is the post-update node of the data-flow
+     * node corresponding to `xs` or (if `xs` is a pointer) the implicit dereference `*xs`, `index`
+     * is the data-flow node corresponding to `i`, and `rhs` is the data-flow node corresponding to
+     * `base`. If this `WriteNode` corresponds to the initialization of an array/slice/map then
+     * there is no need for a post-update node and `base` is the array/slice/map literal being
+     * initialized.
      */
     predicate writesElement(DataFlow::Node base, DataFlow::Node index, DataFlow::Node rhs) {
-      this.writesElementInsn(base.asInstruction(), index.asInstruction(), rhs.asInstruction())
+      exists(DataFlow::Node b |
+        this.writesElementInsn(b.asInstruction(), index.asInstruction(), rhs.asInstruction())
+      |
+        this.isInitialization() and base = b
+        or
+        not this.isInitialization() and
+        b = base.(DataFlow::PostUpdateNode).getPreUpdateNode()
+      )
     }
 
     private predicate writesElementInsn(
@@ -184,7 +202,7 @@ module ControlFlow {
      * Holds if this node sets any field or element of `base` to `rhs`.
      */
     predicate writesComponent(DataFlow::Node base, DataFlow::Node rhs) {
-      this.writesComponentInstruction(base.asInstruction(), rhs.asInstruction())
+      this.writesElement(base, _, rhs) or this.writesField(base, _, rhs)
     }
 
     /**

go/ql/lib/semmle/go/controlflow/IR.qll

@@ -430,18 +430,24 @@ module IR {
    */
   class WriteInstruction extends Instruction {
     WriteTarget lhs;
+    Boolean initialization;
 
     WriteInstruction() {
-      lhs = MkLhs(this, _)
-      or
-      lhs = MkLiteralElementTarget(this)
+      (
+        lhs = MkLhs(this, _)
+        or
+        lhs = MkResultWriteTarget(this)
+      ) and
+      initialization = false
       or
-      lhs = MkResultWriteTarget(this)
+      lhs = MkLiteralElementTarget(this) and initialization = true
     }
 
     /** Gets the target to which this instruction writes. */
     WriteTarget getLhs() { result = lhs }
 
+    predicate isInitialization() { initialization = true }
+
     /** Gets the instruction computing the value this instruction writes. */
     Instruction getRhs() { none() }
 

go/ql/lib/semmle/go/dataflow/internal/ContainerFlow.qll

@@ -22,7 +22,7 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
         t instanceof SliceType
       ) and
       (
-        exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
+        exists(Write w | w.writesElement(node2, _, node1))
         or
         node1 = node2.(ImplicitVarargsSlice).getCallNode().getAnImplicitVarargsArgument()
         or
@@ -44,11 +44,11 @@ predicate containerStoreStep(Node node1, Node node2, Content c) {
     or
     c instanceof MapKeyContent and
     t instanceof MapType and
-    exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), node1, _))
+    exists(Write w | w.writesElement(node2, node1, _))
     or
     c instanceof MapValueContent and
     t instanceof MapType and
-    exists(Write w | w.writesElement(node2.(PostUpdateNode).getPreUpdateNode(), _, node1))
+    exists(Write w | w.writesElement(node2, _, node1))
   )
 }
 

go/ql/lib/semmle/go/dataflow/internal/DataFlowPrivate.qll

@@ -156,7 +156,7 @@ predicate storeStep(Node node1, ContentSet cs, Node node2) {
     // which in turn flows into the pointer content of `p`
     exists(Write w, Field f, DataFlow::Node base, DataFlow::Node rhs | w.writesField(base, f, rhs) |
       node1 = rhs and
-      node2.(PostUpdateNode).getPreUpdateNode() = base and
+      node2 = base and
       c = any(DataFlow::FieldContent fc | fc.getField() = f)
       or
       node1 = base and

go/ql/lib/semmle/go/dataflow/internal/FlowSummaryImpl.qll

@@ -435,13 +435,20 @@ module SourceSinkInterpretationInput implements
       mid.asCallable() = getNodeEnclosingCallable(ret)
     )
     or
-    exists(SourceOrSinkElement e, DataFlow::Write fw, DataFlow::Node base, Field f |
+    exists(
+      SourceOrSinkElement e, DataFlow::Write fw, DataFlow::Node base, DataFlow::Node qual, Field f
+    |
       e = mid.asElement() and
       f = e.asFieldEntity()
     |
       c = "" and
       fw.writesField(base, f, node.asNode()) and
-      pragma[only_bind_into](e) = getElementWithQualifier(f, base)
+      pragma[only_bind_into](e) = getElementWithQualifier(f, qual) and
+      (
+        qual = base.(PostUpdateNode).getPreUpdateNode()
+        or
+        not base instanceof PostUpdateNode and qual = base
+      )
     )
     or
     // A package-scope (or universe-scope) variable

go/ql/lib/semmle/go/dataflow/internal/TaintTrackingUtil.qll

@@ -144,7 +144,7 @@ predicate referenceStep(DataFlow::Node pred, DataFlow::Node succ) {
  * `succ`.
  */
 predicate elementWriteStep(DataFlow::Node pred, DataFlow::Node succ) {
-  any(DataFlow::Write w).writesElement(succ.(DataFlow::PostUpdateNode).getPreUpdateNode(), _, pred)
+  any(DataFlow::Write w).writesElement(succ, _, pred)
   or
   FlowSummaryImpl::Private::Steps::summaryStoreStep(pred.(DataFlowPrivate::FlowSummaryNode)
         .getSummaryNode(), any(DataFlow::ArrayContent ac).asContentSet(),

go/ql/lib/semmle/go/frameworks/GinCors.qll

@@ -25,10 +25,15 @@ module GinCors {
     DataFlow::Node base;
 
     AllowCredentialsWrite() {
-      exists(Field f, Write w |
+      exists(Field f, Write w, DataFlow::Node n |
         f.hasQualifiedName(packagePath(), "Config", "AllowCredentials") and
-        w.writesField(base, f, this) and
-        this.getType() instanceof BoolType
+        w.writesField(n, f, this) and
+        this.getType() instanceof BoolType and
+        (
+          base = n.(DataFlow::PostUpdateNode).getPreUpdateNode()
+          or
+          not n instanceof DataFlow::PostUpdateNode and base = n
+        )
       )
     }
 
@@ -59,10 +64,15 @@ module GinCors {
     DataFlow::Node base;
 
     AllowOriginsWrite() {
-      exists(Field f, Write w |
+      exists(Field f, Write w, DataFlow::Node n |
         f.hasQualifiedName(packagePath(), "Config", "AllowOrigins") and
-        w.writesField(base, f, this) and
-        this.asExpr() instanceof SliceLit
+        w.writesField(n, f, this) and
+        this.asExpr() instanceof SliceLit and
+        (
+          base = n.(DataFlow::PostUpdateNode).getPreUpdateNode()
+          or
+          not n instanceof DataFlow::PostUpdateNode and base = n
+        )
       )
     }
 
@@ -93,10 +103,15 @@ module GinCors {
     DataFlow::Node base;
 
     AllowAllOriginsWrite() {
-      exists(Field f, Write w |
+      exists(Field f, Write w, DataFlow::Node n |
         f.hasQualifiedName(packagePath(), "Config", "AllowAllOrigins") and
-        w.writesField(base, f, this) and
-        this.getType() instanceof BoolType
+        w.writesField(n, f, this) and
+        this.getType() instanceof BoolType and
+        (
+          base = n.(DataFlow::PostUpdateNode).getPreUpdateNode()
+          or
+          not n instanceof DataFlow::PostUpdateNode and base = n
+        )
       )
     }
 
@@ -109,14 +124,9 @@ module GinCors {
      * Get config variable holding header values
      */
     override GinConfig getConfig() {
-      exists(GinConfig gc |
-        (
-          gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
-            base.asInstruction() or
-          gc.getV().getAUse() = base
-        ) and
-        result = gc
-      )
+      result.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
+        base.asInstruction() or
+      result.getV().getAUse() = base
     }
   }
 

go/ql/lib/semmle/go/frameworks/NoSQL.qll

@@ -38,9 +38,8 @@ module NoSql {
    */
   predicate isAdditionalMongoTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
     // Taint an entry if the `Value` is tainted
-    exists(Write w, DataFlow::Node base, Field f | w.writesField(base, f, pred) |
-      base = succ.(DataFlow::PostUpdateNode).getPreUpdateNode() and
-      base.getType().hasQualifiedName(package("go.mongodb.org/mongo-driver", "bson/primitive"), "E") and
+    exists(Write w, Field f | w.writesField(succ, f, pred) |
+      succ.getType().hasQualifiedName(package("go.mongodb.org/mongo-driver", "bson/primitive"), "E") and
       f.getName() = "Value"
     )
   }

go/ql/lib/semmle/go/frameworks/Protobuf.qll

@@ -64,11 +64,10 @@ module Protobuf {
    */
   private class MarshalStateStep extends TaintTracking::AdditionalTaintStep {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(DataFlow::PostUpdateNode marshalInput, DataFlow::CallNode marshalStateCall |
+      exists(DataFlow::Node marshalInput, DataFlow::CallNode marshalStateCall |
         marshalStateCall = marshalStateMethod().getACall() and
         // pred -> marshalInput.Message
-        any(DataFlow::Write w)
-            .writesField(marshalInput.getPreUpdateNode(), inputMessageField(), pred) and
+        any(DataFlow::Write w).writesField(marshalInput, inputMessageField(), pred) and
         // marshalInput -> marshalStateCall
         marshalStateCall.getArgument(0) = globalValueNumber(marshalInput).getANode() and
         // marshalStateCall -> succ
@@ -142,10 +141,13 @@ module Protobuf {
   private class WriteMessageFieldStep extends TaintTracking::AdditionalTaintStep {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       [succ.getType(), succ.getType().getPointerType()] instanceof MessageType and
-      exists(DataFlow::ReadNode base |
+      exists(DataFlow::Node n, DataFlow::ReadNode base |
         succ.(DataFlow::PostUpdateNode).getPreUpdateNode() = getUnderlyingNode(base)
       |
-        any(DataFlow::Write w).writesComponent(base, pred)
+        any(DataFlow::Write w).writesComponent(n, pred) and
+        // The below line only works because `base`'s type, `DataFlow::ReadNode`,
+        // is incompatible with `DataFlow::PostUpdateNode`.
+        base = [n, n.(DataFlow::PostUpdateNode).getPreUpdateNode()]
       )
     }
   }

go/ql/lib/semmle/go/frameworks/RsCors.qll

@@ -52,10 +52,15 @@ module RsCors {
     DataFlow::Node base;
 
     AllowCredentialsWrite() {
-      exists(Field f, Write w |
+      exists(Field f, Write w, DataFlow::Node n |
         f.hasQualifiedName(packagePath(), "Options", "AllowCredentials") and
-        w.writesField(base, f, this) and
-        this.getType() instanceof BoolType
+        w.writesField(n, f, this) and
+        this.getType() instanceof BoolType and
+        (
+          base = n.(DataFlow::PostUpdateNode).getPreUpdateNode()
+          or
+          not n instanceof DataFlow::PostUpdateNode and base = n
+        )
       )
     }
 
@@ -80,10 +85,15 @@ module RsCors {
     DataFlow::Node base;
 
     AllowOriginsWrite() {
-      exists(Field f, Write w |
+      exists(Field f, Write w, DataFlow::Node n |
         f.hasQualifiedName(packagePath(), "Options", "AllowedOrigins") and
-        w.writesField(base, f, this) and
-        this.asExpr() instanceof SliceLit
+        w.writesField(n, f, this) and
+        this.asExpr() instanceof SliceLit and
+        (
+          base = n.(DataFlow::PostUpdateNode).getPreUpdateNode()
+          or
+          not n instanceof DataFlow::PostUpdateNode and base = n
+        )
       )
     }
 
@@ -111,10 +121,15 @@ module RsCors {
     DataFlow::Node base;
 
     AllowAllOriginsWrite() {
-      exists(Field f, Write w |
+      exists(Field f, Write w, DataFlow::Node n |
         f.hasQualifiedName(packagePath(), "Options", "AllowAllOrigins") and
-        w.writesField(base, f, this) and
-        this.getType() instanceof BoolType
+        w.writesField(n, f, this) and
+        this.getType() instanceof BoolType and
+        (
+          base = n.(DataFlow::PostUpdateNode).getPreUpdateNode()
+          or
+          not n instanceof DataFlow::PostUpdateNode and base = n
+        )
       )
     }