Rust: Data flow improvements to unlock flow in sqlx test #18291

paldepind · 2024-12-16T10:49:33Z

Adds some more data flow tests.
Add a content type for references, models & as stores, and * as reads.
Adds a few MaD models to unlock flow in the sqlx SQL injection test.

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

+    1000 + i
+}
+
+fn sink(s: i64) {


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

@@ -0,0 +1,81 @@
+// Taint tests for strings
+
+fn source(i: i64) -> String {


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

+    "source"
+}
+
+fn sink_slice(s: &str) {


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

+    println!("{}", s);
+}
+
+fn sink(s: String) {


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

geoffw0

Looks valuable, a few points to discuss.

geoffw0 · 2024-12-16T12:35:56Z

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

@@ -46,6 +46,8 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
        RustDataFlow::readStep(pred, cs, succ) and
        cs.getContent() instanceof ArrayElementContent
      )
+      or
+      pred.asExpr() = succ.asExpr().(RefExprCfgNode).getExpr()


Checking my understanding: when you take a reference &foo you get data flow from f to the ReferenceContent of &f and you get taint flow from f to &f without content?

What sorts of cases do we need the contentless taint flow for?

Yes, that is right. I added the taint flow to support this line in the SQL injection test:

let unsafe_query_1 = String::from("SELECT * FROM people WHERE firstname='") + &remote_string + "'";

Here remote_string is tainted, and the extra taint step makes unsafe_query_1 tainted at well. One could argue that the reference itself isn't really tainted, but on the other hand the only thing it can be used for is access tainted data and it seemed like a simple way to unlock some additional flow. Alternatively, we could also extend the handling of + to read ReferenceContent as well?

My intuition is that having + read the ReferenceContent is more accurate but ... I'm worried this will be a can of worms if we got this way. So I guess we should probably leave it the way it is.

@hvitved do you have an opinion on this?

Modelling store steps as also taint steps has proven bad in the past, so I think it would be better to provide a taint flow summary for + which pops ReferenceContent.

What would be the best way to do that for a built-in operator?

I still think we should revert this.

Modelling store steps as also taint steps has proven bad in the past

Re. this, we also do that right now for arrays (which was inspired by Ruby). Do we want to remove that as well (later)?

Hopefully we only add taint steps for reads out of arrays, and not for stores into arrays?

Yes, that's right. Got it, taint steps for read steps are fine, but taint steps for store steps are not.

I still think we should revert this.

Done 👍

geoffw0 · 2024-12-16T12:59:48Z

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

+    let b = &mut a;
+    sink(*b);
+    *b = source(37);
+    sink(*b); // $ MISSING: hasValueFlow=37


write_through_borrow was a tough test, but I'm surprised we don't get this one. Do you know what's missing? Is it that for * we have a readStep but this case is storing into it? In Swift I think there was some magic that made this kind of thing work on the left side of assignment.

Is it that for * we have a readStep but this case is storing into it?

Yes, I think that's it. We'll need to add a case for assignment statements with a * on the left hand side.

Yep, and it should be robust enough to handle stuff like (*foo).bar = source() (or whatever the correct syntax would be).

geoffw0 · 2024-12-16T13:01:04Z

rust/ql/test/library-tests/dataflow/sources/test.rs

@@ -61,7 +61,7 @@ async fn test_reqwest() -> Result<(), reqwest::Error> {
    sink(remote_string1); // $ MISSING: hasTaintFlow

    let remote_string2 = reqwest::blocking::get("http://example.com/").unwrap().text().unwrap(); // $ Alert[rust/summary/taint-sources]
-    sink(remote_string2); // $ MISSING: hasTaintFlow
+    sink(remote_string2); // $ hasTaintFlow


geoffw0 · 2024-12-16T13:05:37Z

rust/ql/test/library-tests/variables/variables.rs

    x = 2; // $ write_access=x
-    print_i64_ref(&x); // $ access=x
+    print_i64_ref(&x); // $ read_access=x


Hmm, if print_i64_ref took a mutable reference and wrote to it, would we still label it a read_access?

Yes. It's sort of inaccurate. But I think that in order for read steps from foo to &foo to work the SSA library needs to treat &foo as a read. At least, from what I can see, it seems like the simplest and most straightforward way to handle & and *.

OK. I'd like to hear @hvitved 's opinion on this point as well, I'm not really sure what other languages do for this and why.

See my earlier comment.

geoffw0 · 2024-12-16T13:06:26Z

rust/ql/test/library-tests/variables/variables.rs

@@ -341,14 +341,14 @@ fn add_assign() {
    let mut a = 0; // a
    a += 1; // $ access=a
    print_i64(a); // $ read_access=a
-    (&mut a).add_assign(10); // $ access=a
+    (&mut a).add_assign(10); // $ read_access=a


Seems like this is writing to a. Same for a few of the other cases.

rust/ql/test/query-tests/security/CWE-089/sqlx.rs

geoffw0 · 2024-12-16T19:31:19Z

Since I'm away for Christmas I'd better say: I'll be happy for this to be merged once the open conversations have been concluded (and I consider "we'll deal with this later" an acceptable conclusion). Don't wait for my final 👍 if someone else wants to approve.

hvitved · 2024-12-17T08:36:36Z

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

@@ -712,6 +712,11 @@ private class CapturedVariableContent extends Content, TCapturedVariableContent
  override string toString() { result = "captured " + v }
 }

+/** A value refered to by a reference. */


hvitved · 2024-12-17T08:41:32Z

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

@@ -46,6 +46,8 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
        RustDataFlow::readStep(pred, cs, succ) and
        cs.getContent() instanceof ArrayElementContent
      )
+      or
+      pred.asExpr() = succ.asExpr().(RefExprCfgNode).getExpr()


Modelling store steps as also taint steps has proven bad in the past, so I think it would be better to provide a taint flow summary for + which pops ReferenceContent.

hvitved · 2024-12-17T08:51:17Z

rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll

@@ -484,7 +484,6 @@ module Impl {
  class VariableReadAccess extends VariableAccess {
    VariableReadAccess() {
      not this instanceof VariableWriteAccess and
-      not this = any(RefExpr re).getExpr() and


I think it may be better to only consider these reads for the SSA library. Should be enough to change certain = false to certain = true here.

Done. I had to also handle RefExpr in variableReadActual.

hvitved · 2024-12-17T08:52:31Z

rust/ql/test/library-tests/variables/variables.rs

    x = 2; // $ write_access=x
-    print_i64_ref(&x); // $ access=x
+    print_i64_ref(&x); // $ read_access=x


See my earlier comment.

hvitved · 2024-12-17T08:57:10Z

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

+      pack: codeql/rust-all
+      extensible: summaryModel
+    data:
+      - ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::response::Response>::text", "Argument[self]", "ReturnValue", "taint", "manual"]


I believe it should be ReturnValue.Variant[crate::result::Result::Ok(0)].

hvitved · 2024-12-17T08:57:52Z

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

      - ["lang:core", "<crate::option::Option>::unwrap", "Argument[self].Variant[crate::option::Option::Some(0)]", "ReturnValue", "value", "manual"]
+      - ["lang:core", "<crate::option::Option>::unwrap", "Argument[self]", "ReturnValue", "taint", "manual"]


All these taint models should not be needed after altering the summary above.

I've moved one of them. But some of our sources specify taint on the entire Result, so I think I'd be fine to keep the others until that is no longer the case.

I'd rather that we remove these lines, and not have flow for now, we should soon be able to have it once #18298 lands. Otherwise I fear we forget to remove these lines.

paldepind · 2024-12-18T07:55:05Z

Except from the RefExprCfgNode taint step/+ modeling, all the comments should now be addressed :)

hvitved · 2024-12-18T10:04:05Z

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

@@ -46,6 +46,8 @@ module RustTaintTracking implements InputSig<Location, RustDataFlow> {
        RustDataFlow::readStep(pred, cs, succ) and
        cs.getContent() instanceof ArrayElementContent
      )
+      or
+      pred.asExpr() = succ.asExpr().(RefExprCfgNode).getExpr()


I still think we should revert this.

hvitved · 2024-12-18T10:05:57Z

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

      - ["lang:core", "<crate::option::Option>::unwrap", "Argument[self].Variant[crate::option::Option::Some(0)]", "ReturnValue", "value", "manual"]
+      - ["lang:core", "<crate::option::Option>::unwrap", "Argument[self]", "ReturnValue", "taint", "manual"]


I'd rather that we remove these lines, and not have flow for now, we should soon be able to have it once #18298 lands. Otherwise I fear we forget to remove these lines.

paldepind added 3 commits December 16, 2024 11:09

Rust: Add data flow tests

df03751

Rust: Model address-of and dereference as stores and loads

aab3428

Rust: Add additional models for stdlib and sqlx

defbbb2

github-actions bot added the Rust Pull requests that update Rust code label Dec 16, 2024

github-advanced-security bot found potential problems Dec 16, 2024

View reviewed changes

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll Fixed Show fixed Hide fixed

paldepind marked this pull request as ready for review December 16, 2024 11:35

paldepind requested review from hvitved and geoffw0 December 16, 2024 11:35

Rust: Database name capitalization

cad4f39

geoffw0 reviewed Dec 16, 2024

View reviewed changes

Rust: Re-add inline expectations query tags

402d4e1

hvitved requested changes Dec 17, 2024

View reviewed changes

paldepind added 2 commits December 17, 2024 13:12

Merge branch 'main' into rust-data-flow-models

ee87d4c

Merge branch 'main' into rust-data-flow-models

d8c301a

paldepind force-pushed the rust-data-flow-models branch from dfa3b82 to dc68260 Compare December 17, 2024 15:20

Rust: Address review comments

c1e2197

paldepind force-pushed the rust-data-flow-models branch from dc68260 to c1e2197 Compare December 17, 2024 16:25

paldepind requested a review from hvitved December 18, 2024 07:55

hvitved requested changes Dec 18, 2024

View reviewed changes

Rust: Remove taint steps

049fab4

hvitved approved these changes Dec 18, 2024

View reviewed changes

paldepind merged commit 87b9e60 into github:main Dec 18, 2024
15 checks passed

paldepind deleted the rust-data-flow-models branch December 18, 2024 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: Data flow improvements to unlock flow in sqlx test #18291

Rust: Data flow improvements to unlock flow in sqlx test #18291

paldepind commented Dec 16, 2024

geoffw0 left a comment

geoffw0 Dec 16, 2024

paldepind Dec 16, 2024

geoffw0 Dec 16, 2024

hvitved Dec 17, 2024

paldepind Dec 17, 2024

hvitved Dec 18, 2024

paldepind Dec 18, 2024

hvitved Dec 18, 2024 •

edited

Loading

paldepind Dec 18, 2024

paldepind Dec 18, 2024

geoffw0 Dec 16, 2024

paldepind Dec 16, 2024

geoffw0 Dec 16, 2024

geoffw0 Dec 16, 2024

geoffw0 Dec 16, 2024

paldepind Dec 16, 2024

geoffw0 Dec 16, 2024

hvitved Dec 17, 2024

geoffw0 Dec 16, 2024

geoffw0 commented Dec 16, 2024

hvitved Dec 17, 2024

hvitved Dec 17, 2024

hvitved Dec 17, 2024

paldepind Dec 17, 2024

hvitved Dec 17, 2024

hvitved Dec 17, 2024

hvitved Dec 17, 2024

paldepind Dec 17, 2024

hvitved Dec 18, 2024

paldepind commented Dec 18, 2024

hvitved Dec 18, 2024

hvitved Dec 18, 2024

		@@ -0,0 +1,81 @@
		// Taint tests for strings

		fn source(i: i64) -> String {

		- ["lang:core", "<crate::option::Option>::unwrap", "Argument[self].Variant[crate::option::Option::Some(0)]", "ReturnValue", "value", "manual"]
		- ["lang:core", "<crate::option::Option>::unwrap", "Argument[self]", "ReturnValue", "taint", "manual"]

Rust: Data flow improvements to unlock flow in sqlx test #18291

Rust: Data flow improvements to unlock flow in sqlx test #18291

Conversation

paldepind commented Dec 16, 2024

geoffw0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffw0 commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paldepind commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved Dec 18, 2024 •

edited

Loading