Swift: detect the use of static initialization vectors

[karimhamdanali]

Using a static initialization vector (IV) for encryption is not secure. To maximize encryption and prevent dictionary attacks, IVs should rather be unique and unpredictable (e.g., randomly generated).

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-1204/StaticInitializationVector.qhelp

Static initialization vector for encryption

When a cipher is used in certain modes (such as CBC or GCM), it requires an initialization vector (IV). Under the same secret key, IVs should be unique and ideally unpredictable. If the same IV is used with the same secret key, then the same plaintext results in the same ciphertext. This behavior may enable an attacker to learn if the same data pieces are transferred or stored, or help the attacker run a dictionary attack.

In particular, if the IV is hardcoded or constant, an attacker may just look up potential keys in a dictionary, then concatenate those with the hardcoded or constant IV rather than trying to discover the entire encryption key.

Recommendation

Use a randomly generated IV.

Example

The following example shows a few cases of instantiating a cipher with various encryption keys. In the 'BAD' cases, the IV is hardcoded or constant, making the encrypted data vulnerable to recovery. In the 'GOOD' cases, the IV is randomly generated and not hardcoded, which protects the encrypted data against recovery.

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

	// BAD: Using static IVs for encryption
	let iv: Array<UInt8> = [0x2a, 0x3a, 0x80, 0x05]
	let ivString = "this is a constant string"
	let key = getRandomKey()
	_ = try AES(key: key, iv: ivString)
	_ = try CBC(iv: iv)

	// GOOD: Using randomly generated IVs for encryption
	let iv = (0..<10).map({ _ in UInt8.random(in: 0...UInt8.max) })
	let ivString = String(cString: iv)
	let key = getRandomKey())
	_ = try AES(key: key, iv: ivString)
	_ = try CBC(iv: iv)

	// ...
}

References

• Wikipedia: Initialization vector.
• National Institute of Standards and Technology: Recommendation for Block Cipher Modes of Operation.
• National Institute of Standards and Technology: FIPS 140-2: Security Requirements for Cryptographic Modules.
• Common Weakness Enumeration: CWE-329.
• Common Weakness Enumeration: CWE-1204.

[geoffw0]

A few minor points, otherwise docs, query and tests look great to me!

[karimhamdanali]

@geoffw0 I think I've addressed all comments here. Ready for docs review?

[geoffw0]

Yep, ready for docs review. 👍

[mchammer01]

@karimhamdanali - this looks good from a Docs perspective ✨
I have left a couple of minor comments for your consideration.

[mchammer01]

rather doesn't really add much, and perhaps we shouldn't use the passive voice.
Feel free to ignore if you don't agree (you could just remove rather if you prefer to keep the current phrasing)

Suggested change

	* @description Using a static initialization vector (IV) for encryption is not secure. To maximize encryption and prevent dictionary attacks, IVs should rather be unique and unpredictable.
	* @description Using a static initialization vector (IV) for encryption is not secure. To maximize encryption and prevent dictionary attacks, go for unique and unpredictable IVs.

[karimhamdanali]

Sounds good. I've changed that by just removing rather.

[mchammer01]

lookup is a noun, to look up is the verb, to I'd be tempted to use an attacker may just look up
However, I am not familiar with this particular use (this may be widely used in the developer world) so feel free to ignore this comment if you don't agree.

[karimhamdanali]

Sure. I've changed that to the suggested wording.

[karimhamdanali]

@mchammer01 thanks again for reviewing this. Since I didn't approve one of your comments (the one where I changed the phrasing slightly), I couldn't approve this PR for merging. Could you please re-approve this PR so that I can get it merged 😅?

[mchammer01]

LGTM