Swift: Add summaries for tainted URL fields#10774
Conversation
2eaaa78 to
8525db5
Compare
geoffw0
left a comment
There was a problem hiding this comment.
LGTM, two very minor suggestions.
| ";URL;true;init(string:relativeTo:);(String,URL?);;Argument[0,1];ReturnValue.Field[standardized];taint", | ||
| ";URL;true;init(string:relativeTo:);(String,URL?);;Argument[0,1];ReturnValue.Field[standardizedFileURL];taint", | ||
| ";URL;true;init(string:relativeTo:);(String,URL?);;Argument[0,1];ReturnValue.Field[user];taint", | ||
| ";URL;true;init(string:relativeTo:);(String,URL?);;Argument[0,1];ReturnValue.Field[password];taint", |
There was a problem hiding this comment.
It would be good to have a couple of comments explaining what blocks of these rows do.
Something like your PR description:
If a URL is tainted, so are its fields.
There was a problem hiding this comment.
Good suggestion, applied in 1e48500, thanks!
| sink(arg: URL(string: tainted, relativeTo: nil)!) // $ tainted=57 | ||
| sink(arg: URL(string: clean, relativeTo: urlClean)!) | ||
| sink(arg: URL(string: clean, relativeTo: urlTainted)!) // $ tainted=39 | ||
| sink(arg: URL(string: clean, relativeTo: urlTainted)!) // $ tainted=57 |
There was a problem hiding this comment.
I think we could do with at least one test for a field after init(string:relativeTo:).
There was a problem hiding this comment.
This actually made me think more detailedly about this constructor, and after testing it, the conclusion is:
- if the
string:argument is an absolute URL, basically it acts as the 1-argument constructor andrelativeTo:is ignored (butbaseURLis tainted by it). - if the
string:argument is a relative path:- the path, query, and fragment related fields are tainted by it.
- all the other fields are tainted by
relativeTo:.
I adjusted the CSV rows to model this behavior as closely as possible, but of course the conditional logic is not handled. If we wanted more precise models we would need to switch to regular CodeQL instead, and apply some heuristics to the first argument to determine whether it's an absolute URL or a relative path.
What's your opinion about that?
0f2469a to
e0346b0
Compare
|
@geoffw0 I decided to handle this with |
e0346b0 to
d7c75d5
Compare
d7c75d5 to
0eeaf71
Compare
geoffw0
left a comment
There was a problem hiding this comment.
I decided to handle this with TaintInheritingContent in the end: it introduces some spurious flow (marked as such in the tests)
This seems fine to me. I can't really imagine someone constructing a part-tainted, part-clean URL and then depending on that for correct safe behaviour afterwards.
| sink(arg: urlTainted) // $ tainted=57 | ||
| sink(arg: urlTainted.absoluteURL) // $ tainted=57 | ||
| sink(arg: urlTainted.baseURL) // $ tainted=57 | ||
| sink(arg: urlTainted.baseURL) // $ Safe |
There was a problem hiding this comment.
Ah that's interesting, so if I'm understanding correctly URL("http://example.com/abc") and URL("abc", URL("http://example.com/")) are not two ways of constructing exactly the same thing - because the latter has a baseURL (amongst other visible differences).
There was a problem hiding this comment.
Correct. It seems that if the first argument is a relative path it only overrides the path, query, and fragment parts of the relativeTo URL, but then the original is saved as baseURL. With the one-argument constructor baseURL is apparently never populated.
If a URL is tainted, so are its fields.