search

urbit / urbit-key-generation

Key derivation and HD wallet generation functions for Urbit
MIT License
15 stars 8 forks source link

network seed derivation using sha256 is vulnerable to length extension attack #55

Closed asssaf closed 5 years ago

asssaf commented 5 years ago

I'm not a crypto expert (and haven't reviewed the code too deeply) but it seems that the network seed derivation function uses a raw sha256 hash on concatenated strings so it's vulnerable to a length extension attack (https://en.wikipedia.org/wiki/Length_extension_attack) allowing an attacker which gains access to the network seed to create valid network seeds with higher revisions.

For example let's say the seed for revision 1 was compromised (attacker knows hash(master||constant||1)), they can now produce hash(master||constant||1234), so even if the legitimate owner of the key wants to revoke it by incrementing the version the attacker can create a new seed with the new revision. I think the legitimate owner will require a new master seed in this case.

For key derivation it's more appropriate to use KDF (or HKDF, HMAC-SHA256) or alternatively a hashing algorithm that is not vulnerable (SHA3) or (less ideally) concatenate in a more thoughtful order.

Fang- commented 5 years ago

Hi @asssaf, thank you for your report! We apologize for not catching this.

We discussed the issue internally, and have come up with a solution that will prevent length extension attacks on the networking seed. For more details, please see the update to the Urbit HD Wallet spec here. We'll reflect this change in keygen-js soon.

tl;dr: networking keys generated during the registration flow (using wallet generator) have revision number 0, making them immune against this attack. For the non-zero revision numbers, we're updating the spec to use SHA2-256d instead.

Thank you again for pointing this out! Finding this further down the line would have limited the life-span of these HD wallets, Not Fun.

asssaf commented 5 years ago

Thanks for responding and fixing so quickly! Out of curiosity (not really a crypto expert) why go with sha256d (which has its own, if minor, issues) rather than hmac-sha256 like bip32 (well, that uses hmac-sha512, but similar)

jtobin commented 5 years ago

@asssaf Agreed that hmac-sha256 from the get-go would have possibly been a better choice. Not sure why it wasn't used, myself -- that part of the wallet design predates me.

In the meantime, sha256d did the trick re: the LE attack issue here, while also being a very simple fix. Chur!