Rust: Restrict the scope of DereferenceSink to dereferences of raw pointers

Author: paldepind

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

@@ -10,6 +10,8 @@ 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,16 +49,22 @@ 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
+        // We are only interested in dereferences of raw pointers, as other uses
+        // of `*` are safe.
+        (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") }
   }

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

@@ -26,18 +26,18 @@ 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) {
     node instanceof AccessAfterLifetime::Sink and
-    // exclude cases with sinks in macros, since these results are difficult to interpret
+    // Exclude cases with sinks in macros, since these results are difficult to interpret
     not node.asExpr().isFromMacroExpansion() and
-    // include only results inside `unsafe` blocks, as other results tend to be false positives
-    (
-      node.asExpr().getEnclosingBlock*().isUnsafe() or
-      node.asExpr().getEnclosingCallable().(Function).isUnsafe()
-    )
+    // TODO: Remove this condition if it can be done without negatively
+    // impacting performance. This condition only include nodes with
+    // corresponding to an expression. This excludes sinks from models-as-data.
+    exists(node.asExpr())
   }
 
   predicate isBarrier(DataFlow::Node barrier) { barrier instanceof AccessAfterLifetime::Barrier }