Skip to content

Swift: Add CryptoSwift sinks in swift/weak-sensitive-data-hashing#12824

Merged
geoffw0 merged 5 commits intogithub:mainfrom
geoffw0:modernsec4
Apr 14, 2023
Merged

Swift: Add CryptoSwift sinks in swift/weak-sensitive-data-hashing#12824
geoffw0 merged 5 commits intogithub:mainfrom
geoffw0:modernsec4

Conversation

@geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Apr 13, 2023

This was fairly straightforward now that we have CSV sinks in the query. I think it's quite a big win for the coverage of this query.

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note Swift labels Apr 13, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner April 13, 2023 19:45
@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 13, 2023

I think the QLDoc check warnings are wrong (due to lacking support for modules):

Warning: Missing QLdoc for classless-predicate WeakSensitiveDataHashingQuery::WeakHashingConfig::isBarrierIn/1
Warning: Missing QLdoc for classless-predicate WeakSensitiveDataHashingQuery::WeakHashingConfig::isBarrierOut/1

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! Remembering the amount of trouble the Ruby team had when they started to raise alerts on uses of md5: Should we maybe run this on MRVA before we merge this and have a quick look at the results? (IIRC, the Ruby query didn't have a concept of "sensitive data", so we're in a bit of a better position than they are with the query).

@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 14, 2023

Yes, I believe the issue was that it was flagging more-or-less any use of MD5 with the idea that the algorithm is insecure - but in practice it is widely used for non-cryptographic purposes. The restriction to sensitive data should exclude the vast majority of such non-cryptographic uses.

Also note that we already flag the Insecure.MD5 CryptoKit implementation, this change only extends coverage to CryptoSwift's MD5 as well.

Nevertheless I will do a MRVA run later today out of an abundance of caution.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Apr 14, 2023

In the top 1000 MRVA projects (actually 808 projects) I found 4 new results, all in one project. Two cases in a function hashing a password + salt with .sha1 by default (configurable by parameter). Two cases in a function hashing a password only with .sha256 by default (which is OK, but configurable with .sha1 and .md5 available options). I'm very happy with the first two result and happy enough with the second two, as they appear to be an incident waiting to happen. There is definitely not an issue of noisy results here.

@geoffw0 geoffw0 merged commit d94ed1b into github:main Apr 14, 2023
@github github deleted a comment from THEWZRD23 Apr 15, 2023
@github github deleted a comment from THEWZRD23 Apr 15, 2023
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