Merge pull request #19080 from geoffw0/deallocation

Rust: Query for dereferencing an invalid pointer

Author: geoffw0

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

@@ -0,0 +1,7 @@
+extensions:
+  - addsTo:
+      pack: codeql/rust-all
+      extensible: sourceModel
+    data:
+      # Alloc
+      - ["repo:https://github.com/rust-lang/libc:libc", "::free", "Argument[0]", "pointer-invalidate", "manual"]

rust/ql/lib/codeql/rust/frameworks/stdlib/lang-alloc.model.yml

@@ -0,0 +1,17 @@
+extensions:
+  - addsTo:
+      pack: codeql/rust-all
+      extensible: summaryModel
+    data:
+      # Fmt
+      - ["lang:alloc", "crate::fmt::format", "Argument[0]", "ReturnValue", "taint", "manual"]
+      # String
+      - ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
+      - ["lang:alloc", "<crate::string::String>::as_bytes", "Argument[self]", "ReturnValue", "taint", "manual"]
+      - ["lang:alloc", "<_ as crate::string::ToString>::to_string", "Argument[self]", "ReturnValue", "taint", "manual"]
+  - addsTo:
+      pack: codeql/rust-all
+      extensible: sourceModel
+    data:
+      # Alloc
+      - ["lang:alloc", "crate::alloc::dealloc", "Argument[0]", "pointer-invalidate", "manual"]

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

@@ -3,8 +3,6 @@ extensions:
       pack: codeql/rust-all
       extensible: summaryModel
     data:
-      # Fmt
-      - ["lang:alloc", "crate::fmt::format", "Argument[0]", "ReturnValue", "taint", "manual"]
       # Iterator
       - ["lang:core", "<[_]>::iter", "Argument[Self].Element", "ReturnValue.Element", "value", "manual"]
       - ["lang:core", "<[_]>::iter_mut", "Argument[Self].Element", "ReturnValue.Element", "value", "manual"]
@@ -19,7 +17,7 @@ extensions:
       - ["lang:core", "<crate::slice::iter::Iter as crate::iter::traits::iterator::Iterator>::collect", "Argument[self].Element", "ReturnValue.Element", "value", "manual"]
       - ["lang:core", "<crate::slice::iter::Iter as crate::iter::traits::iterator::Iterator>::map", "Argument[self].Element", "Argument[0].Parameter[0]", "value", "manual"]
       - ["lang:core", "<crate::slice::iter::Iter as crate::iter::traits::iterator::Iterator>::for_each", "Argument[self].Element", "Argument[0].Parameter[0]", "value", "manual"]
-      # ptr
+      # Ptr
       - ["lang:core", "crate::ptr::read", "Argument[0].Reference", "ReturnValue", "value", "manual"]
       - ["lang:core", "crate::ptr::read_unaligned", "Argument[0].Reference", "ReturnValue", "value", "manual"]
       - ["lang:core", "crate::ptr::read_volatile", "Argument[0].Reference", "ReturnValue", "value", "manual"]
@@ -28,7 +26,24 @@ extensions:
       - ["lang:core", "crate::ptr::write_volatile", "Argument[1]", "Argument[0].Reference", "value", "manual"]
       # Str
       - ["lang:core", "<str>::parse", "Argument[self]", "ReturnValue.Field[crate::result::Result::Ok(0)]", "taint", "manual"]
-      # String
-      - ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"]
-      - ["lang:alloc", "<crate::string::String>::as_bytes", "Argument[self]", "ReturnValue", "taint", "manual"]
-      - ["lang:alloc", "<_ as crate::string::ToString>::to_string", "Argument[self]", "ReturnValue", "taint", "manual"]
+  - addsTo:
+      pack: codeql/rust-all
+      extensible: sourceModel
+    data:
+      # Ptr
+      - ["lang:core", "crate::ptr::drop_in_place", "Argument[0]", "pointer-invalidate", "manual"]
+      - ["lang:core", "crate::ptr::dangling", "ReturnValue", "pointer-invalidate", "manual"]
+      - ["lang:core", "crate::ptr::dangling_mut", "ReturnValue", "pointer-invalidate", "manual"]
+      - ["lang:core", "crate::ptr::null", "ReturnValue", "pointer-invalidate", "manual"]
+  - addsTo:
+      pack: codeql/rust-all
+      extensible: sinkModel
+    data:
+      # Ptr
+      - ["lang:core", "crate::ptr::read", "Argument[0]", "pointer-access", "manual"]
+      - ["lang:core", "crate::ptr::read_unaligned", "Argument[0]", "pointer-access", "manual"]
+      - ["lang:core", "crate::ptr::read_volatile", "Argument[0]", "pointer-access", "manual"]
+      - ["lang:core", "crate::ptr::write", "Argument[0]", "pointer-access", "manual"]
+      - ["lang:core", "crate::ptr::write_bytes", "Argument[0]", "pointer-access", "manual"]
+      - ["lang:core", "crate::ptr::write_unaligned", "Argument[0]", "pointer-access", "manual"]
+      - ["lang:core", "crate::ptr::write_volatile", "Argument[0]", "pointer-access", "manual"]

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

@@ -0,0 +1,64 @@
+/**
+ * Provides classes and predicates for reasoning about accesses to invalid
+ * pointers.
+ */
+
+import rust
+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
+
+/**
+ * Provides default sources, sinks and barriers for detecting accesses to
+ * invalid pointers, as well as extension points for adding your own.
+ */
+module AccessInvalidPointer {
+  /**
+   * A data flow source for invalid pointer accesses, that is, an operation
+   * where a pointer becomes invalid.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for invalid pointer accesses, that is, a pointer
+   * dereference.
+   */
+  abstract class Sink extends QuerySink::Range {
+    override string getSinkType() { result = "AccessInvalidPointer" }
+  }
+
+  /**
+   * A barrier for invalid pointer accesses.
+   */
+  abstract class Barrier extends DataFlow::Node { }
+
+  /**
+   * A pointer invalidation from model data.
+   *
+   * Note: we don't currently support invalidation via the object itself rather than via a pointer, such as:
+   * ```
+   * drop(obj)
+   * ```
+   */
+  private class ModelsAsDataSource extends Source {
+    ModelsAsDataSource() { sourceNode(this, "pointer-invalidate") }
+  }
+
+  /**
+   * A pointer access using the unary `*` operator.
+   */
+  private class DereferenceSink extends Sink {
+    DereferenceSink() {
+      exists(PrefixExpr p | p.getOperatorName() = "*" and p.getExpr() = this.asExpr().getExpr())
+    }
+  }
+
+  /**
+   * A pointer access from model data.
+   */
+  private class ModelsAsDataSink extends Sink {
+    ModelsAsDataSink() { sinkNode(this, "pointer-access") }
+  }
+}

rust/ql/src/queries/security/CWE-825/AccessInvalidPointer.qhelp

@@ -0,0 +1,49 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+
+<p>
+Dereferencing an invalid or dangling pointer may cause undefined behavior. Memory may be corrupted
+causing the program to crash or behave incorrectly, in some cases exposing the program to
+potential attacks.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+When dereferencing a pointer in <code>unsafe</code> code, take care that the pointer is valid and
+points to the intended data. Code may need to be rearranged or additional checks added to ensure
+safety in all circumstances. If possible, rewrite the code using safe Rust types to avoid this
+kind of problem altogether.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example, <code>std::ptr::drop_in_place</code> is used to execute the destructor
+of an object. However, a pointer to that object is dereferenced later in the program, causing
+undefined behavior:
+</p>
+
+<sample src="AccessInvalidPointerBad.rs" />
+
+<p>
+In this case, undefined behavior can be avoided by rearranging the code so that the dereferencing
+comes before the call to <code>std::ptr::drop_in_place</code>:
+</p>
+
+<sample src="AccessInvalidPointerGood.rs" />
+
+</example>
+<references>
+
+<li>Rust Documentation: <a href="https://doc.rust-lang.org/reference/behavior-considered-undefined.html#dangling-pointers">Behavior considered undefined &gt;&gt; Dangling pointers</a>.</li>
+<li>Rust Documentation: <a href="https://doc.rust-lang.org/std/ptr/index.html#safety">Module ptr - Safety</a>.</li>
+<li>Massachusetts Institute of Technology: <a href="https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/second-edition/ch19-01-unsafe-rust.html#dereferencing-a-raw-pointer">Unsafe Rust - Dereferencing a Raw Pointer</a>.</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,42 @@
+/**
+ * @name Access of invalid pointer
+ * @description Dereferencing an invalid or dangling pointer causes undefined behavior and may result in memory corruption.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @id rust/access-invalid-pointer
+ * @tags reliability
+ *       security
+ *       external/cwe/cwe-476
+ *       external/cwe/cwe-825
+ */
+
+import rust
+import codeql.rust.dataflow.DataFlow
+import codeql.rust.dataflow.TaintTracking
+import codeql.rust.security.AccessInvalidPointerExtensions
+import AccessInvalidPointerFlow::PathGraph
+
+/**
+ * A data flow configuration for accesses to invalid pointers.
+ */
+module AccessInvalidPointerConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof AccessInvalidPointer::Source }
+
+  predicate isSink(DataFlow::Node node) { node instanceof AccessInvalidPointer::Sink }
+
+  predicate isBarrier(DataFlow::Node barrier) { barrier instanceof AccessInvalidPointer::Barrier }
+
+  predicate isBarrierOut(DataFlow::Node node) {
+    // make sinks barriers so that we only report the closest instance
+    isSink(node)
+  }
+}
+
+module AccessInvalidPointerFlow = TaintTracking::Global<AccessInvalidPointerConfig>;
+
+from AccessInvalidPointerFlow::PathNode sourceNode, AccessInvalidPointerFlow::PathNode sinkNode
+where AccessInvalidPointerFlow::flowPath(sourceNode, sinkNode)
+select sinkNode.getNode(), sourceNode, sinkNode,
+  "This operation dereferences a pointer that may be $@.", sourceNode.getNode(), "invalid"

rust/ql/src/queries/security/CWE-825/AccessInvalidPointerBad.rs

@@ -0,0 +1,10 @@
+
+unsafe {
+    std::ptr::drop_in_place(ptr); // executes the destructor of `*ptr`
+}
+
+// ...
+
+unsafe {
+    do_something(&*ptr); // BAD: dereferences `ptr`
+}

rust/ql/src/queries/security/CWE-825/AccessInvalidPointerGood.rs

@@ -0,0 +1,10 @@
+
+unsafe {
+    do_something(&*ptr); // GOOD: dereferences `ptr` while it is still valid
+}
+
+// ...
+
+{
+    std::ptr::drop_in_place(ptr); // executes the destructor of `*ptr`
+}

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

@@ -15,6 +15,7 @@ private import codeql.rust.Diagnostics
 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.AccessInvalidPointerExtensions
 private import codeql.rust.security.CleartextLoggingExtensions
 private import codeql.rust.security.SqlInjectionExtensions
 private import codeql.rust.security.WeakSensitiveDataHashingExtensions

rust/ql/test/query-tests/security/CWE-020/RegexInjection.expected

@@ -2,15 +2,15 @@
 | main.rs:6:25:6:30 | &regex | main.rs:4:20:4:32 | ...::var | main.rs:6:25:6:30 | &regex | This regular expression is constructed from a $@. | main.rs:4:20:4:32 | ...::var | user-provided value |
 edges
 | main.rs:4:9:4:16 | username | main.rs:5:25:5:44 | MacroExpr | provenance |  |
-| main.rs:4:20:4:32 | ...::var | main.rs:4:20:4:40 | ...::var(...) [Ok] | provenance | Src:MaD:64  |
-| main.rs:4:20:4:40 | ...::var(...) [Ok] | main.rs:4:20:4:66 | ... .unwrap_or(...) | provenance | MaD:1627 |
+| main.rs:4:20:4:32 | ...::var | main.rs:4:20:4:40 | ...::var(...) [Ok] | provenance | Src:MaD:65  |
+| main.rs:4:20:4:40 | ...::var(...) [Ok] | main.rs:4:20:4:66 | ... .unwrap_or(...) | provenance | MaD:1640 |
 | main.rs:4:20:4:66 | ... .unwrap_or(...) | main.rs:4:9:4:16 | username | provenance |  |
 | main.rs:5:9:5:13 | regex | main.rs:6:26:6:30 | regex | provenance |  |
 | main.rs:5:17:5:45 | res | main.rs:5:25:5:44 | { ... } | provenance |  |
 | main.rs:5:25:5:44 | ...::format(...) | main.rs:5:17:5:45 | res | provenance |  |
 | main.rs:5:25:5:44 | ...::must_use(...) | main.rs:5:9:5:13 | regex | provenance |  |
-| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:100 |
-| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:3050 |
+| main.rs:5:25:5:44 | MacroExpr | main.rs:5:25:5:44 | ...::format(...) | provenance | MaD:101 |
+| main.rs:5:25:5:44 | { ... } | main.rs:5:25:5:44 | ...::must_use(...) | provenance | MaD:3063 |
 | main.rs:6:26:6:30 | regex | main.rs:6:25:6:30 | &regex | provenance |  |
 nodes
 | main.rs:4:9:4:16 | username | semmle.label | username |