Rust: Add barrier for null pointer checks to the query.

Author: geoffw0

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

@@ -9,6 +9,7 @@ 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
 
 /**
  * Provides default sources, sinks and barriers for detecting accesses to
@@ -59,4 +60,10 @@ module AccessInvalidPointer {
   private class ModelsAsDataSink extends Sink {
     ModelsAsDataSink() { sinkNode(this, "pointer-access") }
   }
+
+  /**
+   * A barrier for invalid pointer access vulnerabilities for values found to be `null` in
+   * a comparison.
+   */
+  private class NullCheckBarrier extends Barrier instanceof Barriers::NotNullCheckBarrier { }
 }

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

@@ -8,6 +8,8 @@ private import codeql.rust.dataflow.DataFlow
 private import codeql.rust.internal.TypeInference as TypeInference
 private import codeql.rust.internal.Type
 private import codeql.rust.frameworks.stdlib.Builtins
+private import codeql.rust.controlflow.ControlFlowGraph as Cfg
+private import codeql.rust.controlflow.CfgNodes as CfgNodes
 
 /**
  * A node whose type is a numeric or boolean type, which may be an appropriate
@@ -40,3 +42,25 @@ class IntegralOrBooleanTypeBarrier extends DataFlow::Node {
     )
   }
 }
+
+/**
+ * Holds if guard expression `g` having result `branch` indicates that the
+ * sub-expression `node` is not null. For example when `ptr.is_null()` is
+ * `false`, we have that `ptr` is not null.
+ */
+private predicate notNullCheck(CfgNodes::AstCfgNode g, Cfg::CfgNode node, boolean branch) {
+  exists(MethodCallExpr call |
+    call.getStaticTarget().getName().getText() = "is_null" and
+    g = call.getACfgNode() and
+    node = call.getReceiver().getACfgNode() and
+    branch = false
+  )
+}
+
+/**
+ * A node representing a check that the value is not null, which may be an
+ * appropriate taint flow barrier for some queries.
+ */
+class NotNullCheckBarrier extends DataFlow::Node {
+  NotNullCheckBarrier() { this = DataFlow::BarrierGuard<notNullCheck/3>::getABarrierNode() }
+}

rust/ql/test/query-tests/security/CWE-825/AccessInvalidPointer.expected

@@ -26,7 +26,6 @@
 | deallocation.rs:210:7:210:9 | ptr | deallocation.rs:199:9:199:26 | ...::null_mut | deallocation.rs:210:7:210:9 | ptr | This operation dereferences a pointer that may be $@. | deallocation.rs:199:9:199:26 | ...::null_mut | invalid |
 | deallocation.rs:210:7:210:9 | ptr | deallocation.rs:207:9:207:26 | ...::null_mut | deallocation.rs:210:7:210:9 | ptr | This operation dereferences a pointer that may be $@. | deallocation.rs:207:9:207:26 | ...::null_mut | invalid |
 | deallocation.rs:226:13:226:21 | const_ptr | deallocation.rs:219:15:219:32 | ...::null_mut | deallocation.rs:226:13:226:21 | const_ptr | This operation dereferences a pointer that may be $@. | deallocation.rs:219:15:219:32 | ...::null_mut | invalid |
-| deallocation.rs:229:13:229:21 | const_ptr | deallocation.rs:219:15:219:32 | ...::null_mut | deallocation.rs:229:13:229:21 | const_ptr | This operation dereferences a pointer that may be $@. | deallocation.rs:219:15:219:32 | ...::null_mut | invalid |
 | deallocation.rs:274:15:274:16 | p1 | deallocation.rs:270:3:270:25 | ...::drop_in_place | deallocation.rs:274:15:274:16 | p1 | This operation dereferences a pointer that may be $@. | deallocation.rs:270:3:270:25 | ...::drop_in_place | invalid |
 | deallocation.rs:274:15:274:16 | p1 | deallocation.rs:270:3:270:25 | ...::drop_in_place | deallocation.rs:274:15:274:16 | p1 | This operation dereferences a pointer that may be $@. | deallocation.rs:270:3:270:25 | ...::drop_in_place | invalid |
 | deallocation.rs:342:18:342:20 | ptr | deallocation.rs:336:3:336:25 | ...::drop_in_place | deallocation.rs:342:18:342:20 | ptr | This operation dereferences a pointer that may be $@. | deallocation.rs:336:3:336:25 | ...::drop_in_place | invalid |
@@ -81,7 +80,6 @@ edges
 | deallocation.rs:207:9:207:26 | ...::null_mut | deallocation.rs:207:9:207:28 | ...::null_mut(...) | provenance | Src:MaD:8 MaD:8 |
 | deallocation.rs:207:9:207:28 | ...::null_mut(...) | deallocation.rs:207:3:207:5 | ptr | provenance |  |
 | deallocation.rs:219:3:219:11 | const_ptr | deallocation.rs:226:13:226:21 | const_ptr | provenance |  |
-| deallocation.rs:219:3:219:11 | const_ptr | deallocation.rs:229:13:229:21 | const_ptr | provenance |  |
 | deallocation.rs:219:15:219:32 | ...::null_mut | deallocation.rs:219:15:219:34 | ...::null_mut(...) | provenance | Src:MaD:8 MaD:8 |
 | deallocation.rs:219:15:219:34 | ...::null_mut(...) | deallocation.rs:219:3:219:11 | const_ptr | provenance |  |
 | deallocation.rs:270:3:270:25 | ...::drop_in_place | deallocation.rs:270:27:270:28 | [post] p1 | provenance | Src:MaD:6 MaD:6 |
@@ -161,7 +159,6 @@ nodes
 | deallocation.rs:219:15:219:32 | ...::null_mut | semmle.label | ...::null_mut |
 | deallocation.rs:219:15:219:34 | ...::null_mut(...) | semmle.label | ...::null_mut(...) |
 | deallocation.rs:226:13:226:21 | const_ptr | semmle.label | const_ptr |
-| deallocation.rs:229:13:229:21 | const_ptr | semmle.label | const_ptr |
 | deallocation.rs:270:3:270:25 | ...::drop_in_place | semmle.label | ...::drop_in_place |
 | deallocation.rs:270:3:270:25 | ...::drop_in_place | semmle.label | ...::drop_in_place |
 | deallocation.rs:270:27:270:28 | [post] p1 | semmle.label | [post] p1 |

rust/ql/test/query-tests/security/CWE-825/deallocation.rs

@@ -226,7 +226,7 @@ pub unsafe fn test_ptr_invalid_conditions(mode: i32) {
 		let v = (*const_ptr).value; // $ Alert[rust/access-invalid-pointer]
 		println!("	cond10 v = {v}");
 	} else {
-		let v = (*const_ptr).value; // $ SPURIOUS: Alert[rust/access-invalid-pointer] good - unreachable with null pointer
+		let v = (*const_ptr).value; // $ good - unreachable with null pointer
 		println!("	cond11 v = {v}");
 	}
 }