-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust: Data flow improvements to unlock flow in sqlx test #18291
Changes from 8 commits
df03751
aab3428
defbbb2
cad4f39
402d4e1
ee87d4c
d8c301a
c1e2197
049fab4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
extensions: | ||
- addsTo: | ||
pack: codeql/rust-all | ||
extensible: summaryModel | ||
data: | ||
- ["repo:https://github.com/seanmonstar/reqwest:reqwest", "<crate::blocking::response::Response>::text", "Argument[self]", "ReturnValue.Variant[crate::result::Result::Ok(0)]", "taint", "manual"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,4 +3,16 @@ extensions: | |
pack: codeql/rust-all | ||
extensible: summaryModel | ||
data: | ||
# Option | ||
- ["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"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've moved one of them. But some of our sources specify taint on the entire There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
- ["lang:core", "<crate::option::Option>::unwrap_or", "Argument[self].Variant[crate::option::Option::Some(0)]", "ReturnValue", "value", "manual"] | ||
- ["lang:core", "<crate::option::Option>::unwrap_or", "Argument[0]", "ReturnValue", "value", "manual"] | ||
# Result | ||
- ["lang:core", "<crate::result::Result>::unwrap", "Argument[self].Variant[crate::result::Result::Ok(0)]", "ReturnValue", "value", "manual"] | ||
- ["lang:core", "<crate::result::Result>::unwrap", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
- ["lang:core", "<crate::result::Result>::unwrap_or", "Argument[self].Variant[crate::result::Result::Ok(0)]", "ReturnValue", "value", "manual"] | ||
- ["lang:core", "<crate::result::Result>::unwrap_or", "Argument[0]", "ReturnValue", "value", "manual"] | ||
- ["lang:core", "<crate::result::Result>::unwrap_or", "Argument[self]", "ReturnValue", "taint", "manual"] | ||
# String | ||
- ["lang:alloc", "<crate::string::String>::as_str", "Argument[self]", "ReturnValue", "taint", "manual"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
identityLocalStep | ||
| main.rs:404:7:404:18 | phi(default_name) | Node steps to itself | | ||
| main.rs:394:7:394:18 | phi(default_name) | Node steps to itself | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking my understanding: when you take a reference
&foo
you get data flow fromf
to theReferenceContent
of&f
and you get taint flow fromf
to&f
without content?What sorts of cases do we need the contentless taint flow for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is right. I added the taint flow to support this line in the SQL injection test:
Here
remote_string
is tainted, and the extra taint step makesunsafe_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 readReferenceContent
as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intuition is that having
+
read theReferenceContent
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 popsReferenceContent
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the best way to do that for a built-in operator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think we should revert this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re. this, we also do that right now for arrays (which was inspired by Ruby). Do we want to remove that as well (later)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we only add taint steps for reads out of arrays, and not for stores into arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right. Got it, taint steps for read steps are fine, but taint steps for store steps are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍