Cipher seed MAC key is derived incorrectly #4859
Assignees
Labels
A-security
Area - Security related
A-wallet
Area - related to the wallet
C-bug
Category - fixes a bug, typically associated with an issue.
Milestone
Comments
stringhandler
added
C-bug
stringhandler
pushed a commit
that referenced
this issue
Nov 7, 2022
Description --- Significantly refactors `CipherSeed` for clarity, zeroizes internal secrets, fixes key derivation, and verifies MACs in constant time. Does _not_ zeroize external passphrases, which should be done separately. Fixes [issue 4859](#4859). Motivation and Context --- As noted in [issue 4859](#4859), `CipherSeed` MAC keys are derived incorrectly; in fact, the same main key is derived twice to obtain the MAC key and encryption key used for `CipherSeed` encryption. This work refactors for several goals: - The password is only hashed a single time, so MAC and encryption keys are produced at the same time. This makes it easier to identify that they are being derived correctly, which they now are. - Internal secrets are zeroized. Notably, passphrases from callers are _not_ yet zeroized, which should be done in subsequent work. - MACs are verified in constant time using `subtle` functionality. - The code is much clearer, making it easier to identify secret and non-secret data for review. How Has This Been Tested? --- Existing tests, as the external API does not change. Manual inspection that keys are now derived correctly. BREAKING CHANGE: Existing (version 0) seeds no longer work. Only version 1 seeds are valid.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
To generate an encrypted
CipherSeed, a user-supplied passphrase is used to produce a 64-byte main key. This key is intended to be split into two keys: a 32-byte MAC key, and a 32-byte encryption key. This is implemented incorrectly, such that the last 32 bytes of the main key are used as the MAC key instead of the first 32 bytes. This means the MAC and encryption keys are identical, which is dangerous in general.Since the password key derivation function is already run twice, one fix is to run it (using 32-byte output) with a different salt for both keys, by appending a different byte flag to the original salt for each derived key. Another option is to run the key derivation function only once, and parse both derived keys at the same time; however, this might make proper zeroization more challenging (see below).
While we're at it, MAC verification should be done in constant time.
This is also a good opportunity to address the overarching topic in issue 4846 and ensure that secret data produced or handled during
CipherSeedprocessing is properly zeroized. While theCipherSeedstruct itself zeroizes on drop, there are several other locations where passphrase, entropy, or other key data are not.The text was updated successfully, but these errors were encountered: