Merge pull request #21036 from paldepind/rust/prioritize-manual-summaries

Rust: Don't apply generated models for functions that have a manual model

Author: paldepind

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

@@ -123,12 +123,15 @@ private class SummarizedCallableFromModel extends SummarizedCallable::Range {
     summaryModel(path, _, _, _, provenance, _)
   }
 
+  private predicate hasManualModel() { summaryModel(path, _, _, _, "manual", _) }
+
   override predicate propagatesFlow(
     string input, string output, boolean preservesValue, string model
   ) {
-    exists(string kind, QlBuiltins::ExtensionId madId |
-      summaryModel(path, input, output, kind, _, madId) and
-      model = "MaD:" + madId.toString()
+    exists(string kind, string provenance, QlBuiltins::ExtensionId madId |
+      summaryModel(path, input, output, kind, provenance, madId) and
+      model = "MaD:" + madId.toString() and
+      (provenance = "manual" or not this.hasManualModel())
     |
       kind = "value" and
       preservesValue = true

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

@@ -96,6 +96,8 @@ extensions:
       - ["<_ as core::iter::traits::iterator::Iterator>::take", "Argument[self]", "ReturnValue", "taint", "manual"]
       # Pin
       - ["<core::pin::Pin>::new", "Argument[0]", "ReturnValue.Field[core::pin::Pin::pointer]", "value", "manual"]
+      # This model is not precise, but helps in cases where a `Pin` is implicitly dereferenced.
+      - ["<core::pin::Pin>::new", "Argument[0].Reference", "ReturnValue", "value", "manual"]
       - ["<core::pin::Pin>::new_unchecked", "Argument[0]", "ReturnValue.Field[core::pin::Pin::pointer]", "value", "manual"]
       - ["<core::pin::Pin>::into_inner", "Argument[0].Field[core::pin::Pin::pointer]", "ReturnValue", "value", "manual"]
       - ["<core::pin::Pin>::into_inner_unchecked", "Argument[0].Field[core::pin::Pin::pointer]", "ReturnValue", "value", "manual"]

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

@@ -58,7 +58,7 @@ extensions:
     data:
       - ["std::fs::canonicalize", "Argument[0].OptionalStep[normalize-path]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
       - ["std::fs::canonicalize", "Argument[0].OptionalBarrier[normalize-path]", "ReturnValue.Field[core::result::Result::Ok(0)]", "taint", "manual"]
-      - ["<std::path::PathBuf>::as_path", "Argument[Self]", "ReturnValue.Reference", "value", "manual"]
+      - ["<std::path::PathBuf>::as_path", "Argument[self].Reference", "ReturnValue.Reference", "value", "manual"]
       - ["<std::path::PathBuf>::as_mut_os_string", "Argument[Self].Reference", "ReturnValue.Reference", "value", "manual"]
       - ["<std::path::PathBuf>::into_os_string", "Argument[Self]", "ReturnValue", "value", "manual"]
       - ["<std::path::PathBuf>::into_boxed_path", "Argument[Self]", "ReturnValue.Reference", "value", "manual"]

rust/ql/test/library-tests/dataflow/models/main.rs

@@ -31,6 +31,20 @@ enum MyPosEnum {
     B(i64),
 }
 
+// has a manual flow model with flow from second argument to the return value
+// and a wrong generated model with flow from first argument to the return value
+fn snd(a: i64, b: i64) -> i64 {
+    0
+}
+
+fn test_snd() {
+    let s1 = source(99);
+    sink(snd(0, s1)); // $ hasValueFlow=99
+
+    let s2 = source(88);
+    sink(snd(s2, 0));
+}
+
 // has a flow model
 fn get_var_pos(e: MyPosEnum) -> i64 {
     0

rust/ql/test/library-tests/dataflow/models/models.expected

@@ -20,6 +20,9 @@ extensions:
       extensible: summaryModel
     data:
       - ["main::coerce", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["main::snd", "Argument[1]", "ReturnValue", "value", "manual"]
+      # Wrong generated model which should not take effect due to the manual model above
+      - ["main::snd", "Argument[0]", "ReturnValue", "value", "dfc-generated"]
       - ["main::get_var_pos", "Argument[0].Field[main::MyPosEnum::A(0)]", "ReturnValue", "value", "manual"]
       - ["main::set_var_pos", "Argument[0]", "ReturnValue.Field[main::MyPosEnum::B(0)]", "value", "manual"]
       - ["main::get_var_field", "Argument[0].Field[main::MyFieldEnum::C::field_c]", "ReturnValue", "value", "manual"]

rust/ql/test/library-tests/dataflow/models/models.ext.yml

@@ -35,7 +35,7 @@ models
 | 34 | Summary: <_ as tokio::io::util::async_read_ext::AsyncReadExt>::read_to_string; Argument[self].Reference; Argument[0].Reference; taint |
 | 35 | Summary: <_ as tokio::io::util::async_read_ext::AsyncReadExt>::read_u8; Argument[self].Reference; ReturnValue.Future.Field[core::result::Result::Ok(0)]; taint |
 | 36 | Summary: <core::result::Result>::unwrap; Argument[self].Field[core::result::Result::Ok(0)]; ReturnValue; value |
-| 37 | Summary: <std::path::PathBuf>::as_path; Argument[self]; ReturnValue; value |
+| 37 | Summary: <std::path::PathBuf>::as_path; Argument[self].Reference; ReturnValue.Reference; value |
 edges
 | test.rs:12:13:12:18 | buffer | test.rs:13:14:13:19 | buffer | provenance |  |
 | test.rs:12:31:12:43 | ...::read | test.rs:12:31:12:55 | ...::read(...) [Ok] | provenance | Src:MaD:11  |