Swift: detect the use of constant salts

[karimhamdanali]

Constant salts should not be used for password hashing. Data hashed using constant salts are vulnerable to dictionary attacks, enabling attackers to recover the original input.

The rule currently supports all ciphers that the CryptoSwift API provides, but we can always extend it further if more APIs 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-760/ConstantSalt.qhelp

Use of constant salts

Constant salts should not be used for password hashing. Data hashed using constant salts are vulnerable to dictionary attacks, enabling attackers to recover the original input.

Recommendation

Use randomly generated salts to securely hash input data.

Example

The following example shows a few cases of hashing input data. In the 'BAD' cases, the salt is constant, making the generated hashes vulnerable to dictionary attacks. In the 'GOOD' cases, the salt is randomly generated, which protects the hashed data against recovery.

func encrypt(padding : Padding) {
	// ...

	// BAD: Using constant salts for hashing
	let salt: Array<UInt8> = [0x2a, 0x3a, 0x80, 0x05]
	let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
	_ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2)
	_ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
	_ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
	_ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1)

	// GOOD: Using randomly generated salts for hashing
	let salt = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
	let randomArray = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
	_ = try HKDF(password: randomArray, salt: salt, info: randomArray, keyLength: 0, variant: Variant.sha2)
	_ = try PKCS5.PBKDF1(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
	_ = try PKCS5.PBKDF2(password: randomArray, salt: salt, iterations: 120120, keyLength: 0)
	_ = try Scrypt(password: randomArray, salt: salt, dkLen: 64, N: 16384, r: 8, p: 1)

	// ...
}

References

• What are Salted Passwords and Password Hashing?
• Common Weakness Enumeration: CWE-760.

[geoffw0]

This looks like a great start, my only concern is how often salts are specified in the wild using the functions modelled above. I did a quick MRVA query for parameters called "salt" (or similar) and the most common call that appears to be relevant is to a thing called CCKeyDerivationPBKDF(_:_:_:_:_:_:_:_:_:) from CommonCrypto. Do you think we should perhaps create a follow-up issue to add a model of that to this query as well?

[karimhamdanali]

We should add further support for CommonCrytpo, not just for this query, but perhaps other crypto queries too. This might be a good first step in supporting more crypto APIs.

[geoffw0]

Would you mind creating an issue for that, listing the support you think we need to add?

[karimhamdanali]

Done. Just added one with an initial set of tasks along with some useful resources.

[geoffw0]

Possible future expansions aside, I'm happy with this query. Ready for docs review?

[lucascosti]

👋 Docs first responder here! I've put this on our review board for a writer to review

[karimhamdanali]

Thanks @lucascosti for adding this PR to your review board. I was wondering though if a team member will be able to pick it up this week for docs review?

[mchammer01]

@karimhamdanali 👋🏻 - I've picked this up for editorial review and will review this for you today 😃

[mchammer01]

@karimhamdanali 👋🏻 - this LGTM ✨
Left a few comments for your consideration.

[mchammer01]

LGTM