Merge pull request #10157 from MathiasVP/swift-field-flow-2

Swift: Add field flow

Author: MathiasVP

swift/ql/lib/codeql/swift/dataflow/Ssa.qll

@@ -81,20 +81,6 @@ module Ssa {
         value.(PropertyGetterCfgNode).getRef() = init
       )
     }
-
-    cached
-    predicate isInoutDef(ExprCfgNode argument) {
-      // TODO: This should probably not be only `ExprCfgNode`s.
-      exists(
-        ApplyExpr c, BasicBlock bb, int blockIndex, VarDecl v, InOutExpr argExpr // TODO: use CFG node for assignment expr
-      |
-        this.definesAt(v, bb, blockIndex) and
-        bb.getNode(blockIndex).getNode().asAstNode() = c and
-        [c.getAnArgument().getExpr(), c.getQualifier()] = argExpr and
-        argExpr = argument.getNode().asAstNode() and
-        argExpr.getSubExpr() = v.getAnAccess() // TODO: fields?
-      )
-    }
   }
 
   cached

swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll

@@ -63,17 +63,26 @@ private module Cached {
   newtype TNode =
     TExprNode(CfgNode n, Expr e) { hasExprNode(n, e) } or
     TSsaDefinitionNode(Ssa::Definition def) or
-    TInoutReturnNode(ParamDecl param) { param.isInout() } or
-    TInOutUpdateNode(Argument arg) { arg.getExpr() instanceof InOutExpr } or
-    TSummaryNode(FlowSummary::SummarizedCallable c, FlowSummaryImpl::Private::SummaryNodeState state)
-
-  private predicate hasExprNode(CfgNode n, Expr e) {
-    n.(ExprCfgNode).getExpr() = e
-    or
-    n.(PropertyGetterCfgNode).getRef() = e
-    or
-    n.(PropertySetterCfgNode).getAssignExpr() = e
-  }
+    TInoutReturnNode(ParamDecl param) { modifiableParam(param) } or
+    TSummaryNode(FlowSummary::SummarizedCallable c, FlowSummaryImpl::Private::SummaryNodeState state) or
+    TExprPostUpdateNode(CfgNode n) {
+      // Obviously, the base of setters needs a post-update node
+      n = any(PropertySetterCfgNode setter).getBase()
+      or
+      // The base of getters and observers needs a post-update node to support reverse reads.
+      n = any(PropertyGetterCfgNode getter).getBase()
+      or
+      n = any(PropertyObserverCfgNode getter).getBase()
+      or
+      // Arguments that are `inout` expressions needs a post-update node,
+      // as well as any class-like argument (since a field can be modified).
+      // Finally, qualifiers and bases of member reference need post-update nodes to support reverse reads.
+      hasExprNode(n,
+        [
+          any(Argument arg | modifiable(arg)).getExpr(), any(MemberRefExpr ref).getBase(),
+          any(ApplyExpr apply).getQualifier()
+        ])
+    }
 
   private predicate localSsaFlowStepUseUse(Ssa::Definition def, Node nodeFrom, Node nodeTo) {
     def.adjacentReadPair(nodeFrom.getCfgNode(), nodeTo.getCfgNode()) and
@@ -102,18 +111,12 @@ private module Cached {
       // use-use flow
       localSsaFlowStepUseUse(def, nodeFrom, nodeTo)
       or
-      //localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
-      //or
+      localSsaFlowStepUseUse(def, nodeFrom.(PostUpdateNode).getPreUpdateNode(), nodeTo)
+      or
       // step from previous read to Phi node
       localFlowSsaInput(nodeFrom, def, nodeTo.asDefinition())
     )
     or
-    // flow through writes to inout parameters
-    exists(ParamReturnKind kind, ExprCfgNode arg |
-      arg = nodeFrom.(InOutUpdateNode).getCall(kind).asCall().getArgument(kind.getIndex()) and
-      nodeTo.asDefinition().(Ssa::WriteDefinition).isInoutDef(arg)
-    )
-    or
     // flow through `&` (inout argument)
     nodeFrom.asExpr() = nodeTo.asExpr().(InOutExpr).getSubExpr()
     or
@@ -138,10 +141,36 @@ private module Cached {
   predicate localFlowStepImpl(Node nodeFrom, Node nodeTo) { localFlowStepCommon(nodeFrom, nodeTo) }
 
   cached
-  newtype TContentSet = TODO_TContentSet()
+  newtype TContentSet = TSingletonContent(Content c)
 
   cached
-  newtype TContent = TODO_Content()
+  newtype TContent = TFieldContent(FieldDecl f)
+}
+
+/**
+ * Holds if `arg` can be modified (by overwriting the content completely),
+ * or if any of its fields can be overwritten by a function call.
+ */
+private predicate modifiable(Argument arg) {
+  arg.getExpr() instanceof InOutExpr
+  or
+  arg.getExpr().getType() instanceof NominalType
+}
+
+predicate modifiableParam(ParamDecl param) {
+  param.isInout()
+  or
+  param instanceof SelfParamDecl
+}
+
+private predicate hasExprNode(CfgNode n, Expr e) {
+  n.(ExprCfgNode).getExpr() = e
+  or
+  n.(PropertyGetterCfgNode).getRef() = e
+  or
+  n.(PropertySetterCfgNode).getAssignExpr() = e
+  or
+  n.(PropertyObserverCfgNode).getAssignExpr() = e
 }
 
 import Cached
@@ -341,40 +370,92 @@ private module OutNodes {
     }
   }
 
-  class InOutUpdateNode extends OutNode, TInOutUpdateNode, NodeImpl {
+  class InOutUpdateArgNode extends OutNode, ExprPostUpdateNode {
     Argument arg;
 
-    InOutUpdateNode() { this = TInOutUpdateNode(arg) }
+    InOutUpdateArgNode() {
+      modifiable(arg) and
+      hasExprNode(n, arg.getExpr())
+    }
 
     override DataFlowCall getCall(ReturnKind kind) {
-      result.asCall().getExpr() = arg.getApplyExpr() and
+      result.getAnArgument() = n and
       kind.(ParamReturnKind).getIndex() = arg.getIndex()
     }
+  }
 
-    override DataFlowCallable getEnclosingCallable() {
-      result = this.getCall(_).getEnclosingCallable()
+  class InOutUpdateQualifierNode extends OutNode, ExprPostUpdateNode {
+    InOutUpdateQualifierNode() { hasExprNode(n, any(ApplyExpr apply).getQualifier()) }
+
+    override DataFlowCall getCall(ReturnKind kind) {
+      result.getAnArgument() = n and
+      kind.(ParamReturnKind).getIndex() = -1
     }
+  }
+
+  class PropertySetterOutNode extends OutNode, ExprNodeImpl {
+    PropertySetterCfgNode setter;
 
-    override Location getLocationImpl() { result = arg.getLocation() }
+    PropertySetterOutNode() { setter = this.getCfgNode() }
 
-    override string toStringImpl() { result = arg.toString() }
+    override DataFlowCall getCall(ReturnKind kind) {
+      result.(PropertySetterCall).getSetter() = setter and kind.(ParamReturnKind).getIndex() = -1
+    }
+  }
+
+  class PropertyGetterOutNode extends OutNode, ExprNodeImpl {
+    PropertyGetterCfgNode getter;
+
+    PropertyGetterOutNode() { getter = this.getCfgNode() }
+
+    override DataFlowCall getCall(ReturnKind kind) {
+      result.(PropertyGetterCall).getGetter() = getter and kind instanceof NormalReturnKind
+    }
+  }
+
+  class PropertyObserverOutNode extends OutNode, ExprNodeImpl {
+    PropertyObserverCfgNode observer;
+
+    PropertyObserverOutNode() { observer = this.getCfgNode() }
+
+    override DataFlowCall getCall(ReturnKind kind) {
+      result.(PropertyGetterCall).getGetter() = observer and kind.(ParamReturnKind).getIndex() = -1
+    }
   }
 }
 
 import OutNodes
 
 predicate jumpStep(Node pred, Node succ) { none() }
 
-predicate storeStep(Node node1, ContentSet c, Node node2) { none() }
+predicate storeStep(Node node1, ContentSet c, Node node2) {
+  exists(MemberRefExpr ref, AssignExpr assign |
+    ref = assign.getDest() and
+    node1.asExpr() = assign.getSource() and
+    node2.(PostUpdateNode).getPreUpdateNode().asExpr() = ref.getBase() and
+    c.isSingleton(any(Content::FieldContent ct | ct.getField() = ref.getMember()))
+  )
+}
 
-predicate readStep(Node node1, ContentSet c, Node node2) { none() }
+predicate isLValue(Expr e) { any(AssignExpr assign).getDest() = e }
+
+predicate readStep(Node node1, ContentSet c, Node node2) {
+  exists(MemberRefExpr ref |
+    not isLValue(ref) and
+    node1.asExpr() = ref.getBase() and
+    node2.asExpr() = ref and
+    c.isSingleton(any(Content::FieldContent ct | ct.getField() = ref.getMember()))
+  )
+}
 
 /**
  * Holds if values stored inside content `c` are cleared at node `n`. For example,
  * any value stored inside `f` is cleared at the pre-update node associated with `x`
  * in `x.f = newValue`.
  */
-predicate clearsContent(Node n, ContentSet c) { none() }
+predicate clearsContent(Node n, ContentSet c) {
+  n = any(PostUpdateNode pun | storeStep(_, c, pun)).getPreUpdateNode()
+}
 
 /**
  * Holds if the value that is being tracked is expected to be stored inside content `c`
@@ -408,7 +489,21 @@ abstract class PostUpdateNodeImpl extends Node {
   abstract Node getPreUpdateNode();
 }
 
-private module PostUpdateNodes { }
+private module PostUpdateNodes {
+  class ExprPostUpdateNode extends PostUpdateNodeImpl, NodeImpl, TExprPostUpdateNode {
+    CfgNode n;
+
+    ExprPostUpdateNode() { this = TExprPostUpdateNode(n) }
+
+    override ExprNode getPreUpdateNode() { n = result.getCfgNode() }
+
+    override Location getLocationImpl() { result = n.getLocation() }
+
+    override string toStringImpl() { result = "[post] " + n.toString() }
+
+    override DataFlowCallable getEnclosingCallable() { result = TDataFlowFunc(n.getScope()) }
+  }
+}
 
 private import PostUpdateNodes
 

swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll

@@ -139,18 +139,43 @@ class Content extends TContent {
   Location getLocation() { none() }
 }
 
+module Content {
+  /** A field of an object, for example an instance variable. */
+  class FieldContent extends Content, TFieldContent {
+    private FieldDecl f;
+
+    FieldContent() { this = TFieldContent(f) }
+
+    /** Gets the name of the field. */
+    FieldDecl getField() { result = f }
+
+    override string toString() { result = f.toString() }
+  }
+}
+
 /**
  * An entity that represents a set of `Content`s.
  *
  * The set may be interpreted differently depending on whether it is
  * stored into (`getAStoreContent`) or read from (`getAReadContent`).
  */
-class ContentSet extends Content {
+class ContentSet extends TContentSet {
+  /** Holds if this content set is the singleton `{c}`. */
+  predicate isSingleton(Content c) { this = TSingletonContent(c) }
+
+  /** Gets a textual representation of this content set. */
+  string toString() {
+    exists(Content c |
+      this.isSingleton(c) and
+      result = c.toString()
+    )
+  }
+
   /** Gets a content that may be stored into when storing into this set. */
-  Content getAStoreContent() { result = this }
+  Content getAStoreContent() { this.isSingleton(result) }
 
   /** Gets a content that may be read from when reading from this set. */
-  Content getAReadContent() { result = this }
+  Content getAReadContent() { this.isSingleton(result) }
 }
 
 /**

swift/ql/lib/codeql/swift/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -27,7 +27,7 @@ Node summaryNode(SummarizedCallable c, SummaryNodeState state) { result = TSumma
 SummaryCall summaryDataFlowCall(Node receiver) { receiver = result.getReceiver() }
 
 /** Gets the type of content `c`. */
-DataFlowType getContentType(Content c) { any() }
+DataFlowType getContentType(ContentSet c) { any() }
 
 /** Gets the return type of kind `rk` for callable `c`. */
 bindingset[c]
@@ -109,13 +109,13 @@ SummaryComponent interpretComponentSpecific(AccessPathToken c) {
 }
 
 /** Gets the textual representation of the content in the format used for flow summaries. */
-private string getContentSpecificCsv(Content c) {
+private string getContentSpecificCsv(ContentSet c) {
   none() // TODO once we have field flow
 }
 
 /** Gets the textual representation of a summary component in the format used for flow summaries. */
 string getComponentSpecificCsv(SummaryComponent sc) {
-  exists(Content c | sc = TContentSummaryComponent(c) and result = getContentSpecificCsv(c))
+  exists(ContentSet c | sc = TContentSummaryComponent(c) and result = getContentSpecificCsv(c))
   or
   exists(ReturnKind rk |
     sc = TReturnSummaryComponent(rk) and

swift/ql/lib/codeql/swift/dataflow/internal/SsaImplSpecific.qll

@@ -4,6 +4,7 @@ private import swift
 private import codeql.swift.controlflow.BasicBlocks as BasicBlocks
 private import codeql.swift.controlflow.ControlFlowGraph
 private import codeql.swift.controlflow.CfgNodes
+private import DataFlowPrivate
 
 class BasicBlock = BasicBlocks::BasicBlock;
 
@@ -29,13 +30,6 @@ predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain)
     certain = true
   )
   or
-  exists(ApplyExpr call, InOutExpr expr |
-    expr = [call.getAnArgument().getExpr(), call.getQualifier()] and
-    expr.getSubExpr() = v.getAnAccess() and
-    bb.getNode(i).getNode().asAstNode() = call and
-    certain = false
-  )
-  or
   v instanceof ParamDecl and
   bb.getNode(i).getNode().asAstNode() = v and
   certain = true
@@ -48,8 +42,6 @@ predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain)
   )
 }
 
-private predicate isLValue(DeclRefExpr ref) { any(AssignExpr assign).getDest() = ref }
-
 predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain) {
   exists(DeclRefExpr ref |
     not isLValue(ref) and
@@ -58,10 +50,17 @@ predicate variableRead(BasicBlock bb, int i, SourceVariable v, boolean certain)
     certain = true
   )
   or
+  exists(InOutExpr expr |
+    bb.getNode(i).getNode().asAstNode() = expr and
+    expr.getSubExpr() = v.getAnAccess() and
+    certain = true
+  )
+  or
   exists(ExitNode exit, AbstractFunctionDecl func |
+    func.getAParam() = v or func.getSelfParam() = v
+  |
     bb.getNode(i) = exit and
-    v.(ParamDecl).isInout() and
-    func.getAParam() = v and
+    modifiableParam(v) and
     bb.getScope() = func and
     certain = true
   )

swift/ql/lib/codeql/swift/dataflow/internal/TaintTrackingPrivate.qll

@@ -32,7 +32,7 @@ private module Cached {
       nodeFrom.asExpr() = [apply.getAnArgument().getExpr(), apply.getQualifier()] and
       apply.getStaticTarget().getName() = ["appendLiteral(_:)", "appendInterpolation(_:)"] and
       e.getExpr() = [apply.getAnArgument().getExpr(), apply.getQualifier()] and
-      nodeTo.asDefinition().(Ssa::WriteDefinition).isInoutDef(e)
+      nodeTo.(PostUpdateNodeImpl).getPreUpdateNode().getCfgNode() = e
     )
     or
     // Flow from the computation of the interpolated string literal to the result of the interpolation.

swift/ql/lib/codeql/swift/dataflow/internal/TaintTrackingPublic.qll

@@ -26,4 +26,4 @@ predicate localTaintStep = localTaintStepCached/2;
  * of `c` at sinks and inputs to additional taint steps.
  */
 bindingset[node]
-predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::Content c) { none() }
+predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet c) { none() }