fix(windows): use cryptographically random IV for AES-CBC encryption#46
fix(windows): use cryptographically random IV for AES-CBC encryption#46kofany wants to merge 1 commit intoChoochmeque:mainfrom
Conversation
The previous implementation derived the IV deterministically from the
domain name using SHA-256("IV_" + domain). This violates AES-CBC security
requirements: IVs must be unpredictable and unique per encryption
operation. Reusing the same IV for the same domain allows pattern
analysis across ciphertext updates.
Replace the deterministic IV with CryptographicBuffer::GenerateRandom(16)
and prepend the IV to the ciphertext before base64 encoding. The stored
format becomes base64(IV || ciphertext).
Decryption detects the format automatically: it first attempts to extract
the IV from the first 16 bytes (new format), and falls back to the legacy
deterministic IV for data encrypted before this change.
There was a problem hiding this comment.
Pull request overview
This PR updates the Windows-specific encryption format used when storing data in the Windows PasswordVault, fixing an insecure deterministic AES-CBC IV by switching to a per-encryption cryptographically random IV while attempting to remain backward compatible for existing stored values.
Changes:
- Replace deterministic IV derivation (
SHA-256("IV_" + domain)) with a random 16-byte IV. - Store encrypted payload as
base64(IV || ciphertext)by prepending the IV to the ciphertext. - Update decryption to try the new format first, then fall back to the legacy deterministic-IV scheme.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Convert to base64 string (format: base64(IV || ciphertext)) | ||
| Ok(CryptographicBuffer::EncodeToBase64String(&combined_buffer)?.to_string()) |
There was a problem hiding this comment.
The new storage format is base64(IV || ciphertext) but it does not include any version/magic marker. In decrypt_data, this makes the format ambiguous with legacy payloads (which are also base64(ciphertext)); legacy ciphertexts that are >= 2 blocks can be misinterpreted as (IV=ciphertext[0..16], ciphertext=ciphertext[16..]) and decrypt “successfully” to truncated/garbled plaintext. Consider adding an explicit prefix (e.g., "v2:" or a short magic header before the IV) so decryption can reliably select the correct format without trial-decrypting.
| // New format: first 16 bytes are the random IV, remainder is ciphertext | ||
| if raw_bytes.len() > 16 { | ||
| let iv = CryptographicBuffer::CreateFromByteArray(&raw_bytes.as_slice()[..16])?; | ||
| let ciphertext = | ||
| CryptographicBuffer::CreateFromByteArray(&raw_bytes.as_slice()[16..])?; |
There was a problem hiding this comment.
This “try new format, then fallback” approach can return incorrect plaintext for legacy values: for a legacy ciphertext with length >= 32 bytes, treating the first 16 bytes as an IV and decrypting the remainder is valid AES-CBC input and will often pass PKCS#7 padding, yielding plaintext missing the first block. Without an unambiguous version marker (or an integrity check like a MAC/AEAD), decrypt-success is not a safe signal for format detection. Add a version/magic header (or switch to an authenticated format) and only attempt the corresponding decode/decrypt path.
Summary
The current Windows implementation derives the AES-CBC IV deterministically from the domain name (
SHA-256("IV_" + domain)). This violates AES-CBC security requirements — IVs must be unpredictable and unique per encryption operation. Reusing the same IV for a given domain enables pattern analysis when data is updated (an attacker with access to the vault can detect whether the stored value has changed).Changes:
CryptographicBuffer::GenerateRandom(16)(CSPRNG)base64(IV || ciphertext)— IV is prepended to ciphertext before encodingNo changes to macOS/iOS/Android implementations (they correctly delegate encryption to OS-level Keychain/KeyStore).
References