From a2879dc754a0c7e3abb945644519ccdefabd5258 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Thu, 16 Jan 2020 10:46:47 +0000
Subject: [PATCH] Model implicit dereferences in data flow.

---
 .../go/dataflow/internal/DataFlowPrivate.qll  |  9 +++-
 .../go/dataflow/internal/DataFlowUtil.qll     | 49 ++++++++++++++-----
 .../Security/CWE-022/ZipSlip.expected         |  2 +
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll b/ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll
index cdcfa9a975e..9fa2e6819b2 100644
--- a/ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll
+++ b/ql/src/semmle/go/dataflow/internal/DataFlowPrivate.qll
@@ -149,9 +149,14 @@ private class PointerContent extends Content, TPointerContent {
  * value of `node1`.
  */
 predicate storeStep(Node node1, Content c, PostUpdateNode node2) {
-  exists(Write w, Field f |
-    w.writesField(node2.getPreUpdateNode(), f, node1) and
+  exists(Write w, Field f, DataFlow::Node base, DataFlow::Node rhs | w.writesField(base, f, rhs) |
+    node1 = rhs and
+    node2.getPreUpdateNode() = base and
     c = TFieldContent(f)
+    or
+    node1 = base and
+    node2.getPreUpdateNode() = node1.(PointerDereferenceNode).getOperand() and
+    c = TPointerContent(node2.getType())
   )
   or
   node1 = node2.(AddressOperationNode).getOperand() and
diff --git a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
index 93a15af564c..eb9c733c9a5 100644
--- a/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
+++ b/ql/src/semmle/go/dataflow/internal/DataFlowUtil.qll
@@ -378,8 +378,13 @@ class PostUpdateNode extends Node {
 
   PostUpdateNode() {
     (
-      preupd instanceof AddressOperationNode or
-      any(Write w).writesField(preupd, _, _)
+      preupd instanceof AddressOperationNode
+      or
+      exists(Write w, DataFlow::Node base | w.writesField(base, _, _) |
+        preupd = base
+        or
+        preupd = base.(PointerDereferenceNode).getOperand()
+      )
     ) and
     (
       preupd = this.(SsaNode).getAUse()
@@ -571,26 +576,40 @@ class BinaryOperationNode extends Node {
 /**
  * An IR instruction corresponding to an expression with a unary operator.
  */
-class UnaryOperationNode extends ExprNode {
+class UnaryOperationNode extends InstructionNode {
   UnaryOperationNode() {
-    expr instanceof UnaryExpr or
-    expr instanceof StarExpr
+    asExpr() instanceof UnaryExpr
+    or
+    asExpr() instanceof StarExpr
+    or
+    insn instanceof IR::EvalImplicitDerefInstruction
   }
 
   /** Holds if this operation may have observable side effects. */
-  predicate mayHaveSideEffects() { expr.mayHaveOwnSideEffects() }
+  predicate mayHaveSideEffects() {
+    asExpr().mayHaveOwnSideEffects()
+    or
+    insn instanceof IR::EvalImplicitDerefInstruction
+  }
 
   /** Gets the operand of this operation. */
   Node getOperand() {
-    result = exprNode(expr.(UnaryExpr).getOperand()) or
-    result = exprNode(expr.(StarExpr).getBase())
+    result = exprNode(asExpr().(UnaryExpr).getOperand())
+    or
+    result = exprNode(asExpr().(StarExpr).getBase())
+    or
+    result = exprNode(insn.(IR::EvalImplicitDerefInstruction).getOperand())
   }
 
   /** Gets the operator of this operation. */
   string getOperator() {
-    result = expr.(UnaryExpr).getOperator()
+    result = asExpr().(UnaryExpr).getOperator()
     or
-    expr instanceof StarExpr and result = "*"
+    asExpr() instanceof StarExpr and
+    result = "*"
+    or
+    insn instanceof IR::EvalImplicitDerefInstruction and
+    result = "*"
   }
 }
 
@@ -598,13 +617,19 @@ class UnaryOperationNode extends ExprNode {
  * A pointer-dereference instruction.
  */
 class PointerDereferenceNode extends UnaryOperationNode {
-  PointerDereferenceNode() { expr instanceof StarExpr or expr instanceof DerefExpr }
+  PointerDereferenceNode() {
+    asExpr() instanceof StarExpr
+    or
+    asExpr() instanceof DerefExpr
+    or
+    insn instanceof IR::EvalImplicitDerefInstruction
+  }
 }
 
 /**
  * An address-of instruction.
  */
-class AddressOperationNode extends UnaryOperationNode {
+class AddressOperationNode extends UnaryOperationNode, ExprNode {
   override AddressExpr expr;
 }
 
diff --git a/ql/test/query-tests/Security/CWE-022/ZipSlip.expected b/ql/test/query-tests/Security/CWE-022/ZipSlip.expected
index 81c345ca909..a58613d40ee 100644
--- a/ql/test/query-tests/Security/CWE-022/ZipSlip.expected
+++ b/ql/test/query-tests/Security/CWE-022/ZipSlip.expected
@@ -6,6 +6,8 @@ nodes
 | ZipSlip.go:14:20:14:20 | p | semmle.label | p |
 | tst.go:15:11:15:16 | selection of Name : string | semmle.label | selection of Name : string |
 | tst.go:20:20:20:23 | path | semmle.label | path |
+| tst.go:32:21:32:26 | selection of Name | semmle.label | selection of Name |
 #select
 | ZipSlip.go:12:24:12:29 | selection of Name | ZipSlip.go:12:24:12:29 | selection of Name : string | ZipSlip.go:14:20:14:20 | p | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.go:14:20:14:20 | p | file system operation |
 | tst.go:15:11:15:16 | selection of Name | tst.go:15:11:15:16 | selection of Name : string | tst.go:20:20:20:23 | path | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:20:20:20:23 | path | file system operation |
+| tst.go:32:21:32:26 | selection of Name | tst.go:32:21:32:26 | selection of Name | tst.go:32:21:32:26 | selection of Name | Unsanitized archive entry, which may contain '..', is used in a $@. | tst.go:32:21:32:26 | selection of Name | file system operation |