Skip to content

Swift: Add RNCryptor sinks to swift/hardcoded-key #11911

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

Merged
merged 5 commits into from
Jan 25, 2023
Merged

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jan 17, 2023

Add RNCryptor sinks to swift/hardcoded-key. This PR is very similar to #11905 but for the methods where an encryption key is specified, rather than a password.

I have upgraded the query from dataflow to taint tracking so that it can track flow through conversions such as Data("string"). In principle this means we could get false positive results where a constant string is "mixed" with enough random data to make it a sufficiently random key overall. In practice swift/constant-password already uses taint tracking so I don't expect this to be a problem, and if we do see it when our testing scales up (e.g. with MRVA), we can add sanitizers or otherwise restrict flow to find the right balance.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Jan 17, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner January 17, 2023 16:11
Copy link
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! Do we want to run a DCA on this?

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jan 25, 2023

DCA LGTM.

@geoffw0 geoffw0 merged commit e92a5eb into github:main Jan 25, 2023
@geoffw0 geoffw0 deleted the rncrypt2 branch March 6, 2023 13:34
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