Swift: Query for Use of an inappropriate cryptographic hashing algorithm on passwords

[geoffw0]

New query for Use of an inappropriate cryptographic hashing algorithm on passwords.

This query is designed to be used alongside the existing swift/weak-sensitive-data-hashing query, and I've made some adjustments to that query to avoid duplication and to make sure results are reported by the most appropriate query. Previously:

swift/weak-sensitive-data-hashing: password, credential or private info -> MD5 or SHA1 hash

Now:

swift/weak-sensitive-data-hashing: non-password credential or private info -> MD5 or SHA1 hash
swift/weak-password-hashing: password -> MD5, SHA1, SHA2 (including variants such as SHA-512), SHA3 hash

To do this I had to make passwords an explicit kind of sensitive data in Swift (SensitivePassword), and I added MAD syntax for it (sensitive-password). I also fixed the sensitive data regular expressions for mobile phone numbers, added a couple of missing sinks for sensitive data hashing, and made some class names consistent while I was working in this area.

TODO:

• team review
• docs review
• DCA run

[github-actions]

QHelp previews:

swift/ql/src/queries/Security/CWE-328/WeakPasswordHashing.qhelp

Use of an inappropriate cryptographic hashing algorithm on passwords

Hash functions that are not sufficiently computationally hard can leave data vulnerable. You should not use such functions for password hashing.

A strong cryptographic hash function should be resistant to:

• Pre-image attacks. If you know a hash value h(x), you should not be able to easily find the input x.
• Collision attacks. If you know a hash value h(x), you should not be able to easily find a different input y with the same hash value h(x) = h(y).
• Brute force. If you know a hash value h(x), you should not be able to find an input y that computes to that hash value using brute force attacks without significant computational effort.
  All of MD5, SHA-1, SHA-2 and SHA-3 are weak against offline brute forcing, since they are not sufficiently computationally hard. This includes SHA-224, SHA-256, SHA-384 and SHA-512, which are in the SHA-2 family.

Password hashing algorithms should be slow and/or memory intensive to compute, to make brute force attacks more difficult.

Recommendation

For password storage, you should use a sufficiently computationally hard cryptographic hash function, such as one of the following:

• Argon2
• scrypt
• bcrypt
• PBKDF2

Example

The following examples show two versions of the same function. In both cases, a password is hashed using a cryptographic hashing algorithm. In the first case, the SHA-512 hashing algorithm is used. It is vulnerable to offline brute force attacks:

let passwordData = Data(passwordString.utf8)
let passwordHash = Crypto.SHA512.hash(data: passwordData) // BAD: SHA-512 is not suitable for password hashing.

// ...

if Crypto.SHA512.hash(data: Data(passwordString.utf8)) == passwordHash {
    // ...
}

Here is the same function using Argon2, which is suitable for password hashing:

import Argon2Swift

let salt = Salt.newSalt()
let result = try! Argon2Swift.hashPasswordString(password: passwordString, salt: salt) // GOOD: Argon2 is suitable for password hashing.
let passwordHash = result.encodedString()

// ...

if try! Argon2Swift.verifyHashString(password: passwordString, hash: passwordHash) {
    // ...
}

References

• OWASP: Password Storage Cheat Sheet
• GitHub: CryptoSwift README - Password-Based Key Derivation Function
• libsodium: libsodium bindings for other languages
• GitHub: Argon2Swift
• Common Weakness Enumeration: CWE-327.
• Common Weakness Enumeration: CWE-328.
• Common Weakness Enumeration: CWE-916.

[geoffw0]

DCA shows 4 result changes:

• two new results for swift/cleartext-transmission, in SwissCovid__swisscovid-app-ios and signalapp__Signal-iOS, caused by phoneNumber sources (TP).
• two lost results for swift/weak-sensitive-data-hashing in DanielZSY__RxCommonKit, inside a PBKDF1 library implementation (FP, I think, but I'm very much open to opinions on these).

The run also shows some "interesting" rows for stage timings, which look like they might be an unintended result of changes to the DCA rules (i.e. they shouldn't be flagged?). Overall analysis time is fine. I will look into this next year and most likely do another DCA run to confirm.

[geoffw0]

Everything looks good on the repeat DCA run:

• same result changes as before ✔️
  • and I'm happy with them; in particular, I believe PBKDF1 uses MD5 / SHA1 internally in a safe way as part of a carefully designed algorithm, and even if that were not true and it was a broken design, we would ideally want to flag uses of PBKDF1 rather than the implementation.
• no noticeable change in analysis time ✔️
• nothing looks amiss in the stage timings ✔️

[subatoi]

Looks good! Only some minor thoughts and one or two small typos.

[geoffw0]

Thanks for the docs review @subatoi . All points addressed, but please don't hesitate to make further suggestions if anything isn't quite right.

[subatoi]

Looks good @geoffw0, thank you!

[geoffw0]

@rdmarsh2 could I have a final 👍 from you (or request more changes).