fix!: improve wallet key derivation by use of proper domain separation (see issue #4170) #4316
Conversation
|
Just out of interest this is a breaking change |
|
Length extension attack resistance isn't strictly a requirement here, as key derivation security doesn't depend on it. However, it is the case that any underlying hash function susceptible to length extension is probably a poor choice for other design or performance reasons anyway. |
| @@ -84,13 +87,17 @@ where | |||
|
|
|||
| /// Derive a new private key from master key: derived_key=SHA256(master_key||branch_seed||index) | |||
There was a problem hiding this comment.
This comment is no longer accurate, as SHA-256 is not resistant to length extension attacks, and the input structure implied by the comment is out of date and no longer correct.
There was a problem hiding this comment.
Absolutely ! Thank you for taking notice of it !
| // piece of data for concatenation, reducing the risk of collisions due to redundance of variable length | ||
| // input | ||
| let derive_key = DomainSeparatedHasher::<D, GenericHashDomain>::new(DOMAIN_SEPARATION_LABEL) | ||
| .chain(self.seed.entropy().to_vec().to_hex()) |
There was a problem hiding this comment.
Any reason to use a hex encoding here? It seems unnecessary, and the use of the hashing API is already a breaking change.
There was a problem hiding this comment.
You are totally right Aaron, one can simply chain the seed entropy as it is.
stringhandler
changed the title
…e current hashing API, which concatenates the length of each data piece in-between cconcatenation'
jorgeantonio21
force-pushed
the
ja-wallet-key-derivation
branch
from
c63468b to
16e638d
Compare
base_layer/key_manager/src/lib.rs
Outdated
| pub fn base_layer_key_manager_mac_generation() -> HashingDomain { | ||
| HashingDomain::new("base_layer.key_manager.cipher_seed.mac_generation") | ||
| } | ||
|
|
||
| pub fn base_layer_key_manager_argon2_encoding() -> HashingDomain { | ||
| HashingDomain::new("base_layer.key_manager.cipher_seed.argon2_encoding") | ||
| } | ||
|
|
||
| pub fn base_layer_key_manager_chacha20_encoding() -> HashingDomain { | ||
| HashingDomain::new("base_layer.key_manager.cipher_seed.chacha20_encoding") | ||
| } |
There was a problem hiding this comment.
I see these functions are not being used in this PR; I suppose they were created for use elsewhere?
There was a problem hiding this comment.
Ah I see, used in #4296.
base_layer/key_manager/src/lib.rs
Outdated
| } | ||
|
|
||
| pub fn base_layer_key_manager() -> HashingDomain { | ||
| HashingDomain::new("base_layer.key_manager.key_manager") |
There was a problem hiding this comment.
| HashingDomain::new("base_layer.key_manager.key_manager") | |
| HashingDomain::new("base_layer.key_manager") |
Description
--- Add domain separation for wallet key derivation for type
KeyManager.Motivation and Context
--- The current wallet key derivation uses plain concatenation of variable length inputs. This procedure is known to not be collision resistant. A better methodology, as pointed here, is to prepend the length of each datum to it before passing it through the hash function. We attain this by using the current hashing API, which gives a suitable interface for domain separation, (which always prepends input length).
How Has This Been Tested?
--- Unit tests.