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

Ed25519 BIP32 #6301

Closed
t-nelson opened this issue Oct 9, 2019 · 21 comments
Closed

Ed25519 BIP32 #6301

t-nelson opened this issue Oct 9, 2019 · 21 comments
Labels
locked issue security Pull requests that address a security vulnerability
Milestone

Comments

@t-nelson
Copy link
Contributor

t-nelson commented Oct 9, 2019

We need a BIP32esque deterministic key derivation mechanism. This is tricky due to Ed25519's non-prime cofactor.

Notes on Ed25519 BIP32 quirks:

Potential starting point for a Rust implementation. Seems woefully light on tests

@garious
Copy link
Contributor

garious commented Oct 10, 2019

cc: #5246

@mvines mvines added this to the The Future! milestone Oct 14, 2019
@danpaul000
Copy link
Contributor

@t-nelson Is this task really a QA issue, or is it simply a new feature? Perhaps we should have a separate QA issue tracking its testing? How time sensitive is this issue?

@garious
Copy link
Contributor

garious commented Oct 16, 2019

Just acknowledging the "Ed25519's non-prime cofactor" concern is time sensitive. The implementation can fall under another ticket.

@danpaul000
Copy link
Contributor

So is there an action item that needs to be completed for this task, if no implementation? Do we need to more publicly acknowledge the non-prime cofactor issue?

@garious
Copy link
Contributor

garious commented Oct 16, 2019

Yeah, an HD wallet decision that's known to be not vulnerable to that attack.

Maybe QA isn't the right place for this, but if we do this right, there's QA work we don't have to do.

@t-nelson
Copy link
Contributor Author

I'd say this is a feature request with a potential QA action item depending on implementation.

Agreed with Greg. If we can find a BIP32 crate that supports Ed25519 and has sufficient test coverage for the non-prime cofactor quirks, that'd be ideal.

@t-nelson
Copy link
Contributor Author

cc/ @rob-solana

@rob-solana
Copy link
Contributor

Is this another starting point?
https://ristretto.group/
https://crates.io/crates/bip32
This is the 0.0.0 crate to which I referred.

@t-nelson
Copy link
Contributor Author

Potentially. The code is MIA though 🤕

@jstarry jstarry self-assigned this Oct 31, 2019
@tarcieri
Copy link

My personal recommendation would be to use Ristretto to key new systems if possible (or failing that, just stick with secp256k1 or another prime order curve). I say this as someone playing around with Ed25519 hierarchical derivation schemes around the same time BIP32 was being formulated (please don't use the linked scheme, it's broken due to repeated "clamping").

If you really want to use Ed25519, there are a number of different incompatible schemes for hierarchical key derivation, none of which I can confidently recommend and all of which I find quite messy. The main ones I'm aware of are Tor's Next Generation Hidden Service Keys, SLIP-0010, and Dmitry Khovratovich's "BIP32-Ed25519". I'd suggest reading Jeff Burdges' writeup on them as they all have issues.

I'd still like to write the bip32 crate, with the goal of implementing the original BIP32 for secp256k1 in terms of a PrimeOrderGroup trait, and then swap out secp256k1 for another prime order group: Ristretto255. I'd recommend using Ristretto for new designs if it meets your needs: since it's a prime order group, it eliminates all of the subgroup complexities and related attacks, and allows for a derivation protocol which is identical to BIP32 save for the group it operates over, as opposed to a FrankenBIP32 with a lot of added complexity around ensuring derived keys are in the same subgroup.

@t-nelson
Copy link
Contributor Author

@tarcieri Thanks so much for taking the time out to reply here! It looks like I have some evening reading to do

@jstarry
Copy link
Member

jstarry commented Oct 31, 2019

Thanks for the recommendations @tarcieri, they are very much appreciated!

@jstarry
Copy link
Member

jstarry commented Nov 7, 2019

After quite a bit of reading, I'm still unable to give a solid proposition on how to move forward here but would like to share what I've learned.

As @t-nelson pointed out, Ed25519 is not compatible with BIP32 due to its small cofactor and also due to its "bit clamping" (used to prevent attacks: 1, 2) as discussed in these threads:
https://moderncrypto.org/mail-archive/curves/2017/000858.html
https://moderncrypto.org/mail-archive/curves/2017/000866.html

A number of work arounds have been implemented to enable BIP 32 for Ed25519. But they are not compatible with each other as mentioned by @tarcieri. This has certainly caused a fair bit of confusion in the monero project for example. If Solana chooses a work-around, we should be clear on one implementation to prevent UX issues.

The 2 most common work arounds are:

  1. BIP32-Ed25519
    • Used by Cardano / Tezos
    • Existing Ledger / Trezor hardware wallet support
    • Supports soft/hardened key derivation
    • Prone to key recovery attack as already mentioned by @t-nelson. The attack assumes a mis-implementation detail where a wallet developer fails to limit the max depth of the hierarchy.
  2. SLIP-0010
    • Used by Stellar
    • Existing Trezor hardware wallet support
    • Limited to hardened key derivation

Rust implementations

I have not been able to find Rust implementations of either of the above most common implementations. Here's what I have been able to find:

Other approaches we could take for BIP32

  1. Use secp256k1
    • Obviously well supported and popular choice for BIP32
  2. Use ristretto
    • Very limited wallet support, all I found was polkawallet
    • Very new, still a draft
  3. Tor's approach
    • Soft / hard key derivation
    • Ignores Ed25519 clamping
  4. Torsion safe representation
    • Never implemented AFAIK

Concerns

Changing a HKD scheme after choosing one is likely to be a really big pain. So we should be thoughtful about our decision here.

@jstarry jstarry removed their assignment Nov 7, 2019
@t-nelson
Copy link
Contributor Author

t-nelson commented Nov 7, 2019

Concerns

Changing a HKD scheme after choosing one is likely to be a really big pain. So we should be thoughtful about our decision here.

Nailed it

@garious
Copy link
Contributor

garious commented Dec 17, 2019

Remove this from SLP2 Security?

@t-nelson
Copy link
Contributor Author

@garious garious added the security Pull requests that address a security vulnerability label Dec 18, 2019
@garious
Copy link
Contributor

garious commented Feb 15, 2020

@CriesofCarrots, fyi

@AndyChenW
Copy link

it's not supported yet? unbelievable.

@t-nelson
Copy link
Contributor Author

@CriesofCarrots
Copy link
Contributor

We still have some ideas about ways to take this further for file-system wallets, but since the current implementation covers most use-cases, closing this.

@github-actions
Copy link
Contributor

This issue has been automatically locked since there has not been any activity in past 7 days after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked issue security Pull requests that address a security vulnerability
Projects
None yet
Development

No branches or pull requests

9 participants