Skip to content

Swift: Imprecise Taint Flows#14925

Merged
MathiasVP merged 9 commits into
github:mainfrom
geoffw0:flows
Dec 11, 2023
Merged

Swift: Imprecise Taint Flows#14925
MathiasVP merged 9 commits into
github:mainfrom
geoffw0:flows

Conversation

@geoffw0
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 commented Nov 27, 2023

Adds a small number of imprecise taint flow models. There's a lot more we could potentially do here, but lets start small for now:

  • taint flow through methods that look like an "append" or "insert"; many of these will be on classes derived from Collection (which we can model and I think do model most of), but quite often these patterns are also copied in classes that do not derived from Collection. By using an imprecise model, we can catch the latter as well.
  • taint flow through initializers with a "data" argument, which I'm pretty happy will inherit taint from an argument of that nature.

There's a contents / flow paths issue here - we only have flow path support on MAD flow models, not on QL flow models which these imprecise sinks have to be - so we can't have flow from the contents of the argument to the contents of the qualifier. That's why we don't catch the cases on lines 118 and 119 of the test where taint was introduced in the contents of the taintedArray argument rather than in taintedArray itself. I'm keen to hear thoughts on how we could solve this.

@geoffw0 geoffw0 added the Swift label Nov 27, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner November 27, 2023 20:16
@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Nov 28, 2023

DCA run looks clean.

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, but otherwise this LGTM

Comment thread swift/ql/lib/codeql/swift/frameworks/Heuristic.qll Outdated
@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Dec 5, 2023

DCA looks good to me.

@MathiasVP
Copy link
Copy Markdown
Contributor

DCA looks good to me.

I agree. I think the only remaining blocker is my comment here 🙂.

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Dec 5, 2023

Sorry. I'll get around to that change soon...

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Dec 11, 2023

I think this is ready to merge.

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Thanks for addressing my comments!

@MathiasVP MathiasVP merged commit d8f53e5 into github:main Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants