Swift: Add CollectionContent to defaultImplicitTaintRead #14521
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MathiasVP
approved these changes
Oct 17, 2023
| // We often expect taint to reach a sink inside `CollectionContent`, for example an array element | ||
| // or pointer contents. It is convenient to have a default implicit read step for these cases rather | ||
| // than implementing this step in a lot of separate `allowImplicitRead`s. | ||
| cs.getAReadContent() instanceof DataFlow::Content::CollectionContent |
There was a problem hiding this comment.
If we ever run into performance problems here a natural thing to do would be to add a conjunct that further restricts node (for example: it's of a type that can actually carry CollectionContent). But since we're not observing any performance issues here yet I don't think we need to go down that rabbit hole for now.
There was a problem hiding this comment.
Yeah, indeed Java already does this. My worry is that we miss out some nodes that could hold collection content and thus end up missing results that we could have got for less effort. Absolutely an option if we see any problems though.
The DCA run looks fine, aside from a bit of wobble.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's been observed that quite a lot of Swift queries have an
allowImplicitReadrule for reading out ofCollectionContent; and it's been further pointed out that we could put that rule indefaultImplicitTaintReadso that it applies to all queries. This behaviour is not expected to ever be undesirable in the context of taint flow queries, so having it there simplifies queries and helps avoid mistakes*. This PR does this change.*- it turns out we do indeed find some new good results in some of the encryption queries as a result of this change - though new barriers were also required to prevent result duplication.Performance is a slight concern, though we've seen no problems in the queries that were already doing this via
allowImplicitRead. I want to call out that we're matchingCollectionContentwithout restricting the node, whereas the Java equivalent case makes some effort to narrow the DataFlow nodes it applies to. I believe matching all nodes is what we want - quite a broad range of nodes could containCollectionContentand I would not want to miss some of them out. There is a restriction to (roughly) sink nodes that comes intodefaultImplicitTaintReadthrough thebindingset[node]annotation, so we're not really matching all nodes anyway. There's also only oneCollectionContenttype in Swift, so a cartesian product is impossible. In any case I've briefly tested performance locally (with no clear result), and DCA should confirm no slowdown.Note to self: there's another case of
allowImplicitReadin #14383 . I can remove it after this PR is merged.