Rust: Improve handling of implicit derefs/borrows in data flow

Author: hvitved

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowConsistency.qll

@@ -21,7 +21,7 @@ private module Input implements InputSig<Location, RustDataFlow> {
     or
     // We allow flow into post-update node for receiver expressions (from the
     // synthetic post receiever node).
-    n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() = any(Node::ReceiverNode r).getReceiver()
+    n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() = any(Node::DerefBorrowNode r).getNode()
     or
     n.(Node::PostUpdateNode).getPreUpdateNode().asExpr() = getPostUpdateReverseStep(_, _)
     or

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

@@ -12,7 +12,6 @@ private import codeql.rust.elements.Call
 private import SsaImpl as SsaImpl
 private import codeql.rust.controlflow.internal.Scope as Scope
 private import codeql.rust.internal.PathResolution
-private import codeql.rust.internal.TypeInference as TypeInference
 private import codeql.rust.controlflow.ControlFlowGraph
 private import codeql.rust.dataflow.Ssa
 private import codeql.rust.dataflow.FlowSummary
@@ -135,24 +134,17 @@ final class ArgumentPosition extends ParameterPosition {
   Expr getArgument(Call call) {
     result = call.getPositionalArgument(this.getPosition())
     or
-    result = call.getReceiver() and this.isSelf()
+    this.isSelf() and result = call.getReceiver()
   }
 }
 
 /**
  * Holds if `arg` is an argument of `call` at the position `pos`.
- *
- * Note that this does not hold for the receiever expression of a method call
- * as the synthetic `ReceiverNode` is the argument for the `self` parameter.
  */
-predicate isArgumentForCall(Expr arg, Call call, ParameterPosition pos) {
+predicate isArgumentForCall(Expr arg, Call call, ArgumentPosition pos) {
   // TODO: Handle index expressions as calls in data flow.
   not call instanceof IndexExpr and
-  (
-    call.getPositionalArgument(pos.getPosition()) = arg
-    or
-    call.getReceiver() = arg and pos.isSelf() and not call.receiverImplicitlyBorrowed()
-  )
+  arg = pos.getArgument(call)
 }
 
 /** Provides logic related to SSA. */
@@ -283,14 +275,6 @@ module LocalFlow {
     or
     nodeFrom.asPat().(OrPat).getAPat() = nodeTo.asPat()
     or
-    // Simple value step from receiver expression to receiver node, in case
-    // there is no implicit deref or borrow operation.
-    nodeFrom.asExpr() = nodeTo.(ReceiverNode).getReceiver()
-    or
-    // The dual step of the above, for the post-update nodes.
-    nodeFrom.(PostUpdateNode).getPreUpdateNode().(ReceiverNode).getReceiver() =
-      nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr()
-    or
     nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr() =
       getPostUpdateReverseStep(nodeFrom.(PostUpdateNode).getPreUpdateNode().asExpr(), true)
   }
@@ -380,7 +364,7 @@ module RustDataFlow implements InputSig<Location> {
     node.(FlowSummaryNode).getSummaryNode().isHidden() or
     node instanceof CaptureNode or
     node instanceof ClosureParameterNode or
-    node instanceof ReceiverNode or
+    node instanceof DerefBorrowNode or
     node.asExpr() instanceof ParenExpr or
     nodeIsHidden(node.(PostUpdateNode).getPreUpdateNode())
   }
@@ -520,16 +504,16 @@ module RustDataFlow implements InputSig<Location> {
   }
 
   pragma[nomagic]
-  private predicate implicitDerefToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) {
-    TypeInference::receiverHasImplicitDeref(node1.asExpr()) and
-    node1.asExpr() = node2.getReceiver() and
+  private predicate implicitDeref(Node node1, DerefBorrowNode node2, ReferenceContent c) {
+    not node2.isBorrow() and
+    node1.asExpr() = node2.getNode() and
     exists(c)
   }
 
   pragma[nomagic]
-  private predicate implicitBorrowToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) {
-    TypeInference::receiverHasImplicitBorrow(node1.asExpr()) and
-    node1.asExpr() = node2.getReceiver() and
+  private predicate implicitBorrow(Node node1, DerefBorrowNode node2, ReferenceContent c) {
+    node2.isBorrow() and
+    node1.asExpr() = node2.getNode() and
     exists(c)
   }
 
@@ -539,6 +523,15 @@ module RustDataFlow implements InputSig<Location> {
     exists(c)
   }
 
+  private Node getFieldExprContainerNode(FieldExpr fe) {
+    exists(Expr container | container = fe.getContainer() |
+      not any(DerefBorrowNode n).getNode() = container and
+      result.asExpr() = container
+      or
+      result.(DerefBorrowNode).getNode() = container
+    )
+  }
+
   pragma[nomagic]
   additional predicate readContentStep(Node node1, Content c, Node node2) {
     exists(TupleStructPat pat, int pos |
@@ -563,9 +556,9 @@ module RustDataFlow implements InputSig<Location> {
     node1.asPat().(RefPat).getPat() = node2.asPat()
     or
     exists(FieldExpr access |
-      node1.asExpr() = access.getContainer() and
       node2.asExpr() = access and
-      access = c.(FieldContent).getAnAccess()
+      access = c.(FieldContent).getAnAccess() and
+      node1 = getFieldExprContainerNode(access)
     )
     or
     exists(IndexExpr arr |
@@ -616,12 +609,10 @@ module RustDataFlow implements InputSig<Location> {
     referenceExprToExpr(node2.(PostUpdateNode).getPreUpdateNode(),
       node1.(PostUpdateNode).getPreUpdateNode(), c)
     or
-    // Step from receiver expression to receiver node, in case of an implicit
-    // dereference.
-    implicitDerefToReceiver(node1, node2, c)
+    implicitDeref(node1, node2, c)
     or
     // A read step dual to the store step for implicit borrows.
-    implicitBorrowToReceiver(node2.(PostUpdateNode).getPreUpdateNode(),
+    implicitBorrow(node2.(PostUpdateNode).getPreUpdateNode(),
       node1.(PostUpdateNode).getPreUpdateNode(), c)
     or
     VariableCapture::readStep(node1, c, node2)
@@ -657,7 +648,7 @@ module RustDataFlow implements InputSig<Location> {
     exists(AssignmentExpr assignment, FieldExpr access |
       assignment.getLhs() = access and
       node1.asExpr() = assignment.getRhs() and
-      node2.asExpr() = access.getContainer() and
+      node2 = getFieldExprContainerNode(access) and
       access = c.getAnAccess()
     )
   }
@@ -729,9 +720,11 @@ module RustDataFlow implements InputSig<Location> {
     or
     VariableCapture::storeStep(node1, c, node2)
     or
-    // Step from receiver expression to receiver node, in case of an implicit
-    // borrow.
-    implicitBorrowToReceiver(node1, node2, c)
+    implicitBorrow(node1, node2, c)
+    or
+    // A store step dual to the read step for implicit dereferences.
+    implicitDeref(node2.(PostUpdateNode).getPreUpdateNode(),
+      node1.(PostUpdateNode).getPreUpdateNode(), c)
   }
 
   /**

rust/ql/lib/codeql/rust/dataflow/internal/Node.qll

@@ -14,6 +14,7 @@ private import codeql.rust.controlflow.ControlFlowGraph
 private import codeql.rust.controlflow.CfgNodes
 private import codeql.rust.dataflow.Ssa
 private import codeql.rust.dataflow.FlowSummary
+private import codeql.rust.internal.TypeInference as TypeInference
 private import Node as Node
 private import DataFlowImpl
 private import FlowSummaryImpl as FlowSummaryImpl
@@ -226,35 +227,53 @@ final class ExprArgumentNode extends ArgumentNode, ExprNode {
   private Call call_;
   private RustDataFlow::ArgumentPosition pos_;
 
-  ExprArgumentNode() { isArgumentForCall(n, call_, pos_) }
+  ExprArgumentNode() {
+    isArgumentForCall(n, call_, pos_) and
+    not TypeInference::implicitDeref(n) and
+    not TypeInference::implicitBorrow(n)
+  }
 
   override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
     call.asCall() = call_ and pos = pos_
   }
 }
 
 /**
- * The receiver of a method call _after_ any implicit borrow or dereferencing
- * has taken place.
+ * A node that represents the value of an expression after implicit dereference
+ * or borrow.
  */
-final class ReceiverNode extends ArgumentNode, TReceiverNode {
-  private Call n;
+class DerefBorrowNode extends Node, TDerefBorrowNode {
+  AstNode n;
+  boolean isBorrow;
 
-  ReceiverNode() { this = TReceiverNode(n, false) }
+  DerefBorrowNode() { this = TDerefBorrowNode(n, isBorrow, false) }
 
-  Expr getReceiver() { result = n.getReceiver() }
+  AstNode getNode() { result = n }
 
-  MethodCallExpr getMethodCall() { result = n }
+  predicate isBorrow() { isBorrow = true }
 
-  override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
-    call.asCall() = n and pos = TSelfParameterPosition()
+  override CfgScope getCfgScope() { result = n.getEnclosingCfgScope() }
+
+  override Location getLocation() { result = n.getLocation() }
+
+  override string toString() {
+    if isBorrow = true then result = n + " [borrowed]" else result = n + " [dereferenced]"
   }
+}
 
-  override CfgScope getCfgScope() { result = n.getEnclosingCfgScope() }
+/**
+ * The argument of a call _after_ any implicit borrow or dereferencing
+ * has taken place.
+ */
+final class DerefBorrowArgNode extends DerefBorrowNode, ArgumentNode {
+  private DataFlowCall call_;
+  private RustDataFlow::ArgumentPosition pos_;
 
-  override Location getLocation() { result = this.getReceiver().getLocation() }
+  DerefBorrowArgNode() { isArgumentForCall(n, call_.asCall(), pos_) }
 
-  override string toString() { result = "receiver for " + this.getReceiver() }
+  override predicate isArgumentOf(DataFlowCall call, RustDataFlow::ArgumentPosition pos) {
+    call = call_ and pos = pos_
+  }
 }
 
 final class SummaryArgumentNode extends FlowSummaryNode, ArgumentNode {
@@ -402,16 +421,17 @@ final class ExprPostUpdateNode extends PostUpdateNode, TExprPostUpdateNode {
   override Location getLocation() { result = e.getLocation() }
 }
 
-final class ReceiverPostUpdateNode extends PostUpdateNode, TReceiverNode {
-  private Call call;
+final class DerefBorrowPostUpdateNode extends PostUpdateNode, TDerefBorrowNode {
+  private Expr arg;
+  private boolean isBorrow;
 
-  ReceiverPostUpdateNode() { this = TReceiverNode(call, true) }
+  DerefBorrowPostUpdateNode() { this = TDerefBorrowNode(arg, isBorrow, true) }
 
-  override Node getPreUpdateNode() { result = TReceiverNode(call, false) }
+  override DerefBorrowNode getPreUpdateNode() { result = TDerefBorrowNode(arg, isBorrow, false) }
 
-  override CfgScope getCfgScope() { result = call.getEnclosingCfgScope() }
+  override CfgScope getCfgScope() { result = arg.getEnclosingCfgScope() }
 
-  override Location getLocation() { result = call.getReceiver().getLocation() }
+  override Location getLocation() { result = arg.getLocation() }
 }
 
 final class SummaryPostUpdateNode extends FlowSummaryNode, PostUpdateNode {
@@ -467,18 +487,17 @@ newtype TNode =
           any(IndexExpr i).getBase(), //
           any(FieldExpr access).getContainer(), //
           any(TryExpr try).getExpr(), //
-          any(PrefixExpr pe | pe.getOperatorName() = "*").getExpr(), //
           any(AwaitExpr a).getExpr(), //
-          any(MethodCallExpr mc).getReceiver(), //
           getPostUpdateReverseStep(any(PostUpdateNode n).getPreUpdateNode().asExpr(), _)
         ]
     )
   } or
-  TReceiverNode(Call call, Boolean isPost) {
-    call.hasEnclosingCfgScope() and
-    call.receiverImplicitlyBorrowed() and
-    // TODO: Handle index expressions as calls in data flow.
-    not call instanceof IndexExpr
+  TDerefBorrowNode(AstNode n, boolean borrow, Boolean isPost) {
+    TypeInference::implicitDeref(n) and
+    borrow = false
+    or
+    TypeInference::implicitBorrow(n) and
+    borrow = true
   } or
   TSsaNode(SsaImpl::DataFlowIntegration::SsaNode node) or
   TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) {

rust/ql/lib/codeql/rust/elements/internal/FieldExprImpl.qll

@@ -23,10 +23,10 @@ module Impl {
    */
   class FieldExpr extends Generated::FieldExpr {
     /** Gets the record field that this access references, if any. */
-    StructField getStructField() { result = TypeInference::resolveStructFieldExpr(this) }
+    StructField getStructField() { result = TypeInference::resolveStructFieldExpr(this, _) }
 
     /** Gets the tuple field that this access references, if any. */
-    TupleField getTupleField() { result = TypeInference::resolveTupleFieldExpr(this) }
+    TupleField getTupleField() { result = TypeInference::resolveTupleFieldExpr(this, _) }
 
     override string toStringImpl() {
       exists(string abbr, string name |

rust/ql/lib/codeql/rust/frameworks/futures.model.yml

@@ -7,13 +7,13 @@ extensions:
       - ["<futures_util::io::buf_reader::BufReader>::new", "Argument[0]", "ReturnValue", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncReadExt>::read", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncReadExt>::read", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncReadExt>::read_to_end", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
+      #- ["<_ as futures_util::io::AsyncReadExt>::read_to_end", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncReadExt>::read_to_end", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncBufReadExt>::read_line", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
+      #- ["<_ as futures_util::io::AsyncBufReadExt>::read_line", "Argument[self]", "Argument[0].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncBufReadExt>::read_line", "Argument[self].Reference", "Argument[0].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncBufReadExt>::read_until", "Argument[self]", "Argument[1].Reference", "taint", "manual"]
+      #- ["<_ as futures_util::io::AsyncBufReadExt>::read_until", "Argument[self]", "Argument[1].Reference", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncBufReadExt>::read_until", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"]
-      - ["<_ as futures_util::io::AsyncBufReadExt>::fill_buf", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
+      - ["<_ as futures_util::io::AsyncBufReadExt>::fill_buf", "Argument[self].Reference", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<_ as futures_util::io::AsyncBufReadExt>::lines", "Argument[self]", "ReturnValue", "taint", "manual"]
       - ["<_ as futures_io::if_std::AsyncBufRead>::poll_fill_buf", "Argument[self].Reference", "ReturnValue.Field[core::task::poll::Poll::Ready(0)].Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<_ as futures_io::if_std::AsyncRead>::poll_read", "Argument[self].Reference", "Argument[1].Reference", "taint", "manual"]

rust/ql/lib/codeql/rust/frameworks/reqwest.model.yml

@@ -18,10 +18,10 @@ extensions:
       - ["<reqwest::response::Response>::text", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::response::Response>::text_with_charset", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::response::Response>::bytes", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
-      - ["<reqwest::response::Response>::chunk", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]
+      - ["<reqwest::response::Response>::chunk", "Argument[self].Reference", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]
       - ["<reqwest::blocking::response::Response>::text", "Argument[self]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::blocking::response::Response>::text_with_charset", "Argument[self]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::blocking::response::Response>::bytes", "Argument[self]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::async_impl::response::Response>::text", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["<reqwest::async_impl::response::Response>::bytes", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)]", "taint", "manual"]
-      - ["<reqwest::async_impl::response::Response>::chunk", "Argument[self]", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]
+      - ["<reqwest::async_impl::response::Response>::chunk", "Argument[self].Reference", "ReturnValue.Future.Field[core::result::Result::Ok(0)].Field[core::option::Option::Some(0)]", "taint", "manual"]

rust/ql/lib/codeql/rust/frameworks/rustcrypto/RustCrypto.qll

@@ -24,13 +24,13 @@ class StreamCipherInit extends Cryptography::CryptographicOperation::Range {
   StreamCipherInit() {
     // a call to `cipher::KeyInit::new`, `cipher::KeyInit::new_from_slice`,
     // `cipher::KeyIvInit::new`, `cipher::KeyIvInit::new_from_slices`, `rc2::Rc2::new_with_eff_key_len` or similar.
-    exists(CallExprBase ce, string rawAlgorithmName |
-      ce = this.asExpr() and
-      ce.getStaticTarget().(Function).getName().getText() =
+    exists(Call call, string rawAlgorithmName |
+      call = this.asExpr() and
+      call.getStaticTarget().(Function).getName().getText() =
         ["new", "new_from_slice", "new_with_eff_key_len", "new_from_slices"] and
       // extract the algorithm name from the type of `ce` or its receiver.
       exists(Type t, TypePath tp |
-        t = inferType([ce, ce.(MethodCallExpr).getReceiver()], tp) and
+        t = inferType([call, call.getReceiver()], tp) and
         rawAlgorithmName = t.(StructType).getStruct().(Addressable).getCanonicalPath().splitAt("::")
       ) and
       algorithmName = simplifyAlgorithmName(rawAlgorithmName) and