Skip to content

Swift: Taint through arithmetic#12266

Merged
geoffw0 merged 2 commits into
github:mainfrom
geoffw0:taintplusequals
Feb 21, 2023
Merged

Swift: Taint through arithmetic#12266
geoffw0 merged 2 commits into
github:mainfrom
geoffw0:taintplusequals

Conversation

@geoffw0
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 commented Feb 20, 2023

Adds tests and taint flow through arithmetic. Before this PR we had flow through + on Strings only, a rather conservative position. We want taint through other types such as Integers, and through other arithmetic operators such as -.

After this PR I will create a follow-up for taint through += and friends, which builds on the changes here but is a separate chunk of work with a bit of its own complexity. More of the tests will pass after that.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Feb 20, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner February 20, 2023 18:29
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.

LGTM!

I remember for C/C++ we only taint the result of an arithmetic operation when one of the operands is predictable (which pretty much translates to constant). I fully support the decision to not go this route for Swift (since no other CodeQL-support language does it this way), and just provide taint through all arithmetic operations.

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Feb 21, 2023

Yes, I'm not sure why that was. Perhaps there was a problematic pattern involving subtraction???

@MathiasVP
Copy link
Copy Markdown
Contributor

Yes, I'm not sure why that was. Perhaps there was a problematic pattern involving subtraction???

I'm not sure of the history either. What I am sure of is that C/C++ probably has a lot more queries that involve arithmetic operations (and thus a lot more flow through arithmetic operations), so the potential for FPs were probably just a lot more relevant.

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Feb 21, 2023

We are paying a lot of attention to new results (at least at the DCA scale) and have plans to do a wide review of query false positives later in the year. So we don't have to worry about non-obvious FPs too much at this early stage.

Merging.

@geoffw0 geoffw0 merged commit c462e01 into github:main Feb 21, 2023
@geoffw0 geoffw0 deleted the taintplusequals branch March 31, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Swift

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants