Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
asssaf opened this issue Nov 22, 2018 · 3 comments
Closed

Comments

@asssaf
Copy link

asssaf commented Nov 22, 2018

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.

@asssaf asssaf changed the title network seed derivation using sha256 is vulnerable length extension attack network seed derivation using sha256 is vulnerable to length extension attack Nov 22, 2018
Fang- added a commit to Fang-/fora-posts that referenced this issue Nov 23, 2018
As described in urbit/urbit-key-generation#55, the use of SHA2-256 in networking seed derivation is vulnerable to length-extension attacks. This change patches that vulnerability.

Since network seeds have already been generated with revision 0, that case needs to remain backwards-compatible, and thus will not change. All other cases will change, doing SHA2-256d (hashing twice with SHA2-256) to protect against length extension attacks.

Note that network seeds generated with revision 0 are not vulnerable to length extension attacks themselves, because the revision number as part of the salt should never have leading zeroes.
@Fang-
Copy link
Member

Fang- commented Nov 23, 2018

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
Copy link
Author

asssaf commented Nov 25, 2018

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
Copy link
Contributor

jtobin commented Nov 26, 2018

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants