Rust: Lift content reads as taint steps

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

@@ -11,6 +11,20 @@ private import codeql.rust.dataflow.FlowSummary
 private import codeql.rust.dataflow.Ssa
 private import Content
 
+predicate encodeContentTupleField(TupleFieldContent c, string arg) {
+  exists(Addressable a, int pos, string prefix |
+    arg = prefix + "(" + pos + ")" and prefix = a.getCanonicalPath()
+  |
+    c.isStructField(a, pos) or c.isVariantField(a, pos)
+  )
+}
+
+predicate encodeContentStructField(StructFieldContent c, string arg) {
+  exists(Addressable a, string field | arg = a.getCanonicalPath() + "::" + field |
+    c.isStructField(a, field) or c.isVariantField(a, field)
+  )
+}
+
 module Input implements InputSig<Location, RustDataFlow> {
   private import codeql.rust.elements.internal.CallExprBaseImpl::Impl as CallExprBaseImpl
   private import codeql.rust.frameworks.stdlib.Stdlib
@@ -61,24 +75,11 @@ module Input implements InputSig<Location, RustDataFlow> {
     exists(Content c | cs = TSingletonContentSet(c) |
       result = "Field" and
       (
-        exists(Addressable a, int pos, string prefix |
-          arg = prefix + "(" + pos + ")" and prefix = a.getCanonicalPath()
-        |
-          c.(TupleFieldContent).isStructField(a, pos)
-          or
-          c.(TupleFieldContent).isVariantField(a, pos)
-        )
+        encodeContentTupleField(c, arg)
         or
-        exists(Addressable a, string field | arg = a.getCanonicalPath() + "::" + field |
-          c.(StructFieldContent).isStructField(a, field)
-          or
-          c.(StructFieldContent).isVariantField(a, field)
-        )
+        encodeContentStructField(c, arg)
         or
-        exists(int pos |
-          c = TTuplePositionContent(pos) and
-          arg = pos.toString()
-        )
+        exists(int pos | c = TTuplePositionContent(pos) and arg = pos.toString())
       )
       or
       result = "Reference" and

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

@@ -7,6 +7,27 @@ private import Node as Node
 private import Content
 private import FlowSummaryImpl as FlowSummaryImpl
 private import codeql.rust.internal.CachedStages
+private import codeql.rust.internal.TypeInference as TypeInference
+private import codeql.rust.internal.Type as Type
+private import codeql.rust.frameworks.stdlib.Builtins as Builtins
+
+/**
+ * Holds if the field `field` should, by default, be excluded from taint steps
+ * from the containing type to reads of the field. The models-as-data syntax
+ * used to denote the field is the same as for `Field[]` access path elements.
+ */
+extensible predicate excludeFieldTaintStep(string field);
+
+/**
+ * Holds if the content `c` corresponds to a field that has explicitly been
+ * excluded as a taint step.
+ */
+private predicate excludedTaintStepContent(Content c) {
+  exists(string arg | excludeFieldTaintStep(arg) |
+    FlowSummaryImpl::encodeContentStructField(c, arg) or
+    FlowSummaryImpl::encodeContentTupleField(c, arg)
+  )
+}
 
 module RustTaintTracking implements InputSig<Location, RustDataFlow> {
   predicate defaultTaintSanitizer(DataFlow::Node node) { none() }
@@ -40,11 +61,17 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
         succ.asExpr() = index
       )
       or
-      // Although data flow through collections and references is modeled using
-      // stores/reads, we also allow taint to flow out of a tainted collection
-      // or reference.
-      // This is needed in order to support taint-tracking configurations where
-      // the source is a collection or reference.
+      // Read steps give rise to taint steps. This has the effect that if `foo`
+      // is tainted and an operation reads from `foo` (e.g., `foo.bar`) then
+      // taint is propagated.
+      exists(Content c |
+        RustDataFlow::readContentStep(pred, c, succ) and
+        not excludedTaintStepContent(c)
+      )
+      or
+      // In addition to the above, for element and reference content we let
+      // _all_ read steps (including those from flow summaries and those that
+      // result in small primitive types) give rise to taint steps.
       exists(SingletonContentSet cs | RustDataFlow::readStep(pred, cs, succ) |
         cs.getContent() instanceof ElementContent
         or

rust/ql/lib/codeql/rust/frameworks/actix-web.model.yml

@@ -15,9 +15,4 @@ extensions:
       pack: codeql/rust-all
       extensible: summaryModel
     data:
-        - ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue", "taint", "manual"]
-        - ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[0]", "taint", "manual"]
-        - ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[1]", "taint", "manual"]
-        - ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[2]", "taint", "manual"]
-        - ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[3]", "taint", "manual"]
-        - ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue.Field[4]", "taint", "manual"]
+        - ["<actix_web::types::path::Path>::into_inner", "Argument[self]", "ReturnValue", "taint", "manual"]

rust/ql/lib/codeql/rust/frameworks/stdlib/core.model.yml

@@ -74,3 +74,9 @@ extensions:
       - ["core::ptr::write_bytes", "Argument[0]", "pointer-access", "manual"]
       - ["core::ptr::write_unaligned", "Argument[0]", "pointer-access", "manual"]
       - ["core::ptr::write_volatile", "Argument[0]", "pointer-access", "manual"]
+  - addsTo:
+      pack: codeql/rust-all
+      extensible: excludeFieldTaintStep
+    data:
+      - ["core::ops::range::RangeInclusive::start"]
+      - ["core::ops::range::RangeInclusive::end"]

rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll

@@ -4,7 +4,9 @@
  */
 
 import rust
+private import codeql.rust.Concepts
 private import codeql.rust.dataflow.DataFlow
+private import codeql.rust.dataflow.FlowSink
 private import codeql.rust.security.AccessInvalidPointerExtensions
 private import codeql.rust.internal.Type
 private import codeql.rust.internal.TypeInference as TypeInference
@@ -34,6 +36,13 @@ module AccessAfterLifetime {
    */
   class Sink = AccessInvalidPointer::Sink;
 
+  // /**
+  //  * A data flow sink for accesses to a pointer after its lifetime has ended,
+  //  * that is, a dereference.
+  //  */
+  // abstract class Sink extends QuerySink::Range {
+  //   override string getSinkType() { result = "AccessAfterLifetime" }
+  // }
   /**
    * A barrier for accesses to a pointer after its lifetime has ended.
    */
@@ -112,7 +121,10 @@ module AccessAfterLifetime {
   private class RefExprSource extends Source {
     Expr targetValue;
 
-    RefExprSource() { this.asExpr().(RefExpr).getExpr() = targetValue }
+    RefExprSource() {
+      this.asExpr().(RefExpr).getExpr() = targetValue and
+      this.asExpr().(RefExpr).isRaw()
+    }
 
     override Expr getTarget() { result = targetValue }
   }

rust/ql/lib/codeql/rust/security/AccessInvalidPointerExtensions.qll

@@ -4,12 +4,15 @@
  */
 
 import rust
+private import codeql.rust.elements.Call
 private import codeql.rust.dataflow.DataFlow
 private import codeql.rust.dataflow.FlowSource
 private import codeql.rust.dataflow.FlowSink
 private import codeql.rust.Concepts
 private import codeql.rust.dataflow.internal.Node
 private import codeql.rust.security.Barriers as Barriers
+private import codeql.rust.internal.TypeInference as TypeInference
+private import codeql.rust.internal.Type
 
 /**
  * Provides default sources, sinks and barriers for detecting accesses to
@@ -47,20 +50,58 @@ module AccessInvalidPointer {
     ModelsAsDataSource() { sourceNode(this, "pointer-invalidate") }
   }
 
-  /**
-   * A pointer access using the unary `*` operator.
-   */
+  /** A raw pointer access using the unary `*` operator. */
   private class DereferenceSink extends Sink {
-    DereferenceSink() { any(DerefExpr p).getExpr() = this.asExpr() }
+    DereferenceSink() {
+      exists(Expr p, DerefExpr d | p = d.getExpr() and p = this.asExpr() |
+        // Dereferencing a raw pointer is an unsafe operation. Hence relevant
+        // dereferences must occur inside code marked as unsafe.
+        // See: https://doc.rust-lang.org/reference/types/pointer.html#r-type.pointer.raw.safety
+        (p.getEnclosingBlock*().isUnsafe() or p.getEnclosingCallable().(Function).isUnsafe()) and
+        (not exists(TypeInference::inferType(p)) or TypeInference::inferType(p) instanceof PtrType)
+      )
+    }
   }
 
-  /**
-   * A pointer access from model data.
-   */
+  /** A pointer access from model data. */
   private class ModelsAsDataSink extends Sink {
     ModelsAsDataSink() { sinkNode(this, "pointer-access") }
   }
 
+  private class BarrierCall extends Barrier {
+    BarrierCall() {
+      exists(Call call, ArgumentPosition pos, string canonicalName |
+        call.getStaticTarget().getCanonicalPath() = canonicalName and
+        this.asExpr() = call.getArgument(pos)
+      |
+        canonicalName = "<core::ptr::non_null::NonNull>::new" and pos.asPosition() = 0
+      )
+    }
+  }
+
+  private class NumericTypeBarrier extends Barrier instanceof Barriers::NumericTypeBarrier { }
+
+  private class BooleanTypeBarrier extends Barrier instanceof Barriers::BooleanTypeBarrier { }
+
+  private class FieldlessEnumTypeBarrier extends Barrier instanceof Barriers::FieldlessEnumTypeBarrier
+  { }
+
+  private class DefaultBarrier extends Barrier {
+    DefaultBarrier() {
+      // A barrier for calls that statically resolve to the `Default::default`
+      // trait function. Such calls are imprecise, and can always resolve to the
+      // implementations for raw pointers that return a null pointer. This
+      // creates many false positives in combination with other inaccuracies
+      // (too many `pointer-access` sinks created by the model generator).
+      //
+      // We could try removing this barrier in the future when either 1/ the
+      // model generator creates fewer spurious sinks or 2/ data flow for calls
+      // to trait functions is more precise.
+      this.asExpr().(Call).getStaticTarget().getCanonicalPath() =
+        "<_ as core::default::Default>::default"
+    }
+  }
+
   /**
    * A barrier for invalid pointer access vulnerabilities for values checked to
    * be non-`null`.

rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql

@@ -26,7 +26,8 @@ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node node) {
     node instanceof AccessAfterLifetime::Source and
     // exclude cases with sources in macros, since these results are difficult to interpret
-    not node.asExpr().isFromMacroExpansion()
+    not node.asExpr().isFromMacroExpansion() and
+    AccessAfterLifetime::sourceValueScope(node, _, _)
   }
 
   predicate isSink(DataFlow::Node node) {
@@ -36,7 +37,8 @@ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig {
     // include only results inside `unsafe` blocks, as other results tend to be false positives
     (
       node.asExpr().getEnclosingBlock*().isUnsafe() or
-      node.asExpr().getEnclosingCallable().(Function).isUnsafe()
+      node.asExpr().getEnclosingCallable().(Function).isUnsafe() or
+      not exists(node.asExpr())
     )
   }
 

rust/ql/src/queries/summary/Stats.qll

@@ -18,6 +18,7 @@ private import codeql.rust.security.SensitiveData
 private import TaintReach
 // import all query extensions files, so that all extensions of `QuerySink` are found
 private import codeql.rust.security.regex.RegexInjectionExtensions
+private import codeql.rust.security.AccessAfterLifetimeExtensions
 private import codeql.rust.security.AccessInvalidPointerExtensions
 private import codeql.rust.security.CleartextLoggingExtensions
 private import codeql.rust.security.CleartextStorageDatabaseExtensions