Swift: detect hash functions with low # of iterations

[karimhamdanali]

Using hash functions with less than 1,000 iterations is not secure. That scheme is vulnerable to password cracking attacks due to having an insufficient level of computational effort.

The rule currently supports all ciphers that the CryptoSwift API provides, but we can always extend it further if more cophers are added.

I'd appreciate a review of the query itself, the accompanying tests, and the associated documentation.

[github-actions]

QHelp previews:

swift/ql/src/queries/Security/CWE-916/InsufficientHashIterations.qhelp

Insufficient hash iterations

Storing cryptographic hashes of passwords is standard security practice, but it is equally important to select the right hashing scheme. If an attacker obtains the hashed passwords of an application, the password hashing scheme should still prevent the attacker from easily obtaining the original cleartext passwords.

A good password hashing scheme requires a computation that cannot be done efficiently. Hashing schemes with low number of iterations are efficiently computable, and are therefore not suitable for password hashing.

Recommendation

Use the OWASP recommendation for sufficient number of iterations (currently, that is greater than or equal to 120,000) for password hashing schemes.

Example

The following example shows a few cases where a password hashing scheme is instantiated. In the 'BAD' cases, the scheme is initialized with insufficient iterations, making it susceptible to password cracking attacks. In the 'GOOD' cases, the scheme is initialized with at least 120,000 iterations, which protects the hashed data against recovery.

func hash() {
	// ...

	// BAD: Using insufficient (that is, < 120,000) iterations for password hashing
	_ = try PKCS5.PBKDF1(password: getRandomArray(), salt: getRandomArray(), iterations: 90000, keyLength: 0)
	_ = try PKCS5.PBKDF2(password: getRandomArray(), salt: getRandomArray(), iterations: 90000, keyLength: 0)

	// GOOD: Using sufficient (that is, >= 120,000) iterations for password hashing
	_ = try PKCS5.PBKDF1(password: getRandomArray(), salt: getRandomArray(), iterations: 120120, keyLength: 0)
	_ = try PKCS5.PBKDF2(password: getRandomArray(), salt: getRandomArray(), iterations: 310000, keyLength: 0)

	// ...
}

References

• Password-Based Cryptography Specification Version 2.0. 2000.RFC2898.
• OWASP Password Storage Cheat Sheet.
• Common Weakness Enumeration: CWE-916.

[martin389]

👋 Hi from docs, I've added this PR to the team board for a writer to review.

[felicitymay]

Hi @karimhamdanali - thanks for asking for a docs team review of your pull request. 👋🏻

Martin's followed our first responder notes and added this to our review board. Normally we expect a new query to have a technical review either complete, or nearly complete, before we start a docs review. This avoids duplicated effort because a technical review may change the focus of a query, so the user-facing content will change as a result of the review.

I'm going to remove the label and remove this pull request from our review board for now. Please do re-add the label when you're confident that the technical review is nearly complete.

[karimhamdanali]

Thanks @felicitymay for the clarification. I actually didn't know that! Will re-add the label once the PR is through the technical review.

[geoffw0]

Looks great, a few minor suggestions...

[karimhamdanali]

PR is now ready for docs review.

[geoffw0]

I'm happy for this to be merged after the docs review. 👍

[martin389]

Hi again from docs -- Adding this PR to the team board for a writer to review ✨

[felicitymay]

Hi @karimhamdanali 👋🏻

Many thanks for including query help, references and an example for this new query ✨

I've added a few suggestions and comments with the intention of making the information clearer for users. I'm hopeful that my suggestions don't change the intended meaning, but am aware that I have little technical context. Very happy to discuss any suggestions or comments.

[felicitymay]

This query seems as if it's fairly closely related to the JavaScript query Use of password hash with insufficient computational effort.

I think that it might be worth reusing some of the explanation of why this is important.

[karimhamdanali]

Agreed. I've updated the help text here with the relevant information from the JavaScript query. Thanks for the pointer to that query @felicitymay !

[felicitymay]

We don't say how we arrived at this number. Is it taken from an OWASP recommendation, or somewhere else that we could refer to? I'm assuming that recommended minimums will gradually rise, so referring to a source where the current best practice is detailed will help this information stay up to date.

[karimhamdanali]

Agreed. I've updated the text to state that we should follow the OWASP recommendation, with a note to the current recommendation of at least 120,000 iterations.

[felicitymay]

Minor text suggestion:

Suggested change

	<p>The following example shows a few cases of instantiating a password-based key. In the 'BAD' cases, the key is initialized with insufficient iterations, making it susceptible to password cracking attacks. In the 'GOOD' cases, the key is initialized with at least 120000 iterations, which protects the encrypted data against recovery.</p>
	<p>The following example shows a few cases where a password-based key is instantiated. In the 'BAD' cases, the key is initialized with insufficient iterations, making it susceptible to password cracking attacks. In the 'GOOD' cases, the key is initialized with at least 120,000 iterations, which protects the encrypted data against recovery.</p>

[karimhamdanali]

fixed as suggested

[felicitymay]

We write out "i.e." in full English for clarity:

Suggested change

	// BAD: Using insufficient (i.e., < 120,000) hash iterations keys for encryption
	// BAD: Using insufficient (that is, < 120,000) hash iterations keys for encryption

[karimhamdanali]

fixed as suggested

[felicitymay]

Suggested change

	// GOOD: Using sufficient (i.e., >= 120,000) hash iterations keys for encryption
	// GOOD: Using sufficient (that is, >= 120,000) hash iterations keys for encryption

[karimhamdanali]

fixed as suggested

[felicitymay]

Judging by the OWASP reference, what is an adequate number of iterations seems to vary according to the algorithm you're using. It's not entirely clear to me looking at this example, whether or not this is adequate for the algorithm because I don't see which hashing algorithm is used.

This may be entirely unimportant - I'm a writer, not a developer 🙂

[geoffw0]

I think this is a good point. We don't implement the different recommended values for different algorithms, rather we just took the lowest recommendation and check the value used is at least that. In practice this design is likely to catch the vast majority of problems and keeps the query simple and reliable - but there's no reason our examples shouldn't set a good example by being fully OWASP compliant.

[karimhamdanali]

Agreed. I've updated the test to be fully OWASP compliant :)

[felicitymay]

Suggest:

Suggested change

	* @description Using hash functions with less than 120,000 iterations is not secure, because that scheme leads to password cracking attacks due to having an insufficient level of computational effort.
	* @description Using hash functions with fewer than 120,000 iterations is insufficient to protect passwords because a cracking attack will require a low level of computational effort.

[karimhamdanali]

fixed as suggested

[karimhamdanali]

Thanks @felicitymay for the docs review! I think I've addressed all your comments so far. Please let me know if you have any further comments or suggestions before we can merge this PR. Thanks :)

[felicitymay]

Many thanks for the updates to the query help 💖

[karimhamdanali]

Awesome! Thanks @felicitymay. @geoffw0, I think this one is ready to get merged then :) Thanks all for reviewing this PR.