restrict ExpandedSK::sign visibility to avoid pk oracle

[alxiong]

The least disruptive API changes might be turn ExpandedSecretKey::sign_*() to pub(crate), leaving only the correct KeyPair.sign_*() API for signing to public invocation, which is always correct and not vulnerable to "Signing Function Oracle" attack.

[tarcieri]

While this partly addresses the issue, the public: PublicKey field of ed25519_dalek::Keypair is still marked pub, which is effectively the same problem as this function.

To really address the issue, these fields also need to be marked private, and the PublicKey always deterministically computed from SecretKey internally within Keypair.

[alxiong]

Yes, you are right. Thanks for catching this @tarcieri, fixed in 835fa55.

Now given a struct Keypair, user should only have read access, not internal mutability to its fields.

[Akagi201]

When will this PR be merged? @alxiong @tarcieri I think the issue is critical, and should be fixed asap.

[alxiong]

it really isn't our call, maybe maintainers like @isislovecruft and @hdevalence can review then approve?

[blckngm]

KeyPair::from_bytes should probably be removed too.

[alxiong]

KeyPair::from_bytes should probably be removed too.

instead of removing it, I added simple validation logic to ensure matching key pair in fd8c5fb @sopium wdyt?

[blckngm]

Well it's certainly better backward compatibility.

But if backward compatibility is less a concern, I think a better API would be to have a ExpandedKeyPair struct (or just change KeyPair to be like this): it will store expanded secret key and public key, and ensure by construction that the public key matches the secret key. This will have mostly the same performance benefit as using ExpandedSecretKey as well as be safe against the attack.

[blckngm]

If the ExpandedSecretKey::sign_*() methods are changed to pub(crate), there's not much point in exposing ExpandedSecretKey in public API at all.

[blckngm]

instead of removing it, I added simple validation logic to ensure matching key pair in fd8c5fb @sopium wdyt?

I guess what I have in mind is similar to https://github.com/dalek-cryptography/ed25519-dalek/pull/210: allow constructing a KeyPair from just the secret key or secret key bytes.

[tarcieri]

@sopium "expanded secret key" means something different: an Ed25519 private key is a 256-bit "seed" which is expanded into 512-bits using SHA-512. The left half of that value is the canonical private scalar, and the other half is a random unrelated input used when deterministically deriving r.

An Ed25519 keypair includes both the expanded secret key and the public key.

[GrishaVar]

I suggested having a Keypair::from_secret_bytes in #210, but Keypair::from_bytes seems important to me for serialising. I suppose most use cases would only need PublicKey::from_bytes though...

[rozbb]

Added a few docs and did formatting. We should be able to merge/publish this soon. A few notes/questions

1. It seems like ExpandedSecretKey is entirely useless from the user's perspective, now that it can't sign(). Do you think we should make it private?
2. The lack of ExpandedSecretKey functionality impacts latency-sensitive usecases. To alleviate this, should we put an Option<ExpandedSecretKey> field inside of Keypair? This way, the first sign() will expand the secret key and memoize the result so later sign() functions are quicker. Downsides: it's a little messy, and the latency of Keypair::sign() technically leaks whether or not this is the first time being run.
3. Should the Keypair::public_key() and Keypair::secret_key() getters should return references to their respective fields, rather than copies? I don't know if it makes an actual difference here, but I figured I'd ask.

[tarcieri]

@rozbb returning a reference would make it possible to impl the signature::Keypair trait.

[rozbb]

Ah good point. Probably should be a ref then. And the secret key could also be a ref (probably better to not have clones of it lying around; if a user really needs a copy, they can roundtrip it through serialization).

Also, regarding points 1 and 2 above, is the extra expansion really a big deal for performance? Signing already requires two SHA512 ops, plus two scalar mults. I'm not sure an extra SHA512 over 32B worth optimizing? The official Go implementation does the hash every time.

[tarcieri]

Yeah, the C implementation also does the expansion / clamping every time too

[rozbb]

Ok, then are we good to merge once ExpandedSecretKey is made private? I'll do some doc fixes after that.

If so, @alxiong could you make the following changes:

1. Doc tests are currently failing due to old example code in lib.rs. Could you update those?
2. Could you please make ExpandedSecretKey a private type?

[alxiong]

Done @rozbb

(btw, didn't remove the unused methods ExpandedSecretKey::from/to_bytes())

[alxiong]

Ah.. what would you suggest we should do with serde test code for ExpandedSecretKey currently lives in integration test folder? Should we move all of them to unit tests in src/secret.rs ? @rozbb

[rozbb]

I believe ExpandedSecretKey never appears inside a Serialize type, right? In that case, we can remove its serde impls entirely, and remove those tests.

[alxiong]

no, with feature = "serde" on, De/Serialize are implemented for ExpandedSecretKey

given now this struct is only internal pub(crate), not sure how useful these serde function is. I'm just worried that there might be cases where we want to/from_bytes of the expanded key at some point in the future.

would you still recommend removing it?

[rozbb]

I think it should be removed. If we need it in the future we can just revert. @tarcieri does this all look good to you?

[tarcieri]

@rozbb yeah removing the serde impls for ExpandedSecretKey makes sense if it's being removed from the public API

[tarcieri]

The current MSRV test is failing because the ed25519 crate is Rust 2021 edition.

The easiest way to work around that is probably to check in Cargo.lock and ensure it's computed for ed25519 v1.3.0, possibly by adding a temporary =1.3.0 version requirement when computing Cargo.lock.

Edit: I'd probably suggest someone do that in a separate PR to keep those changes out-of-band from these.

[rozbb]

Thanks again!

[Shnatsel]

The fix has been merged but still hasn't shipped to crates.io. Is there anything in particular holding up a new release with the fix?

[tarcieri]

The PR included breaking changes to the API to eliminate potential misuses, so a fix can only go out as part of ed25519-dalek v2.0.

That release will include a bump to curve25519-dalek v4.0, whenever that is released: https://github.com/dalek-cryptography/curve25519-dalek/issues/405

[benma]

I was relying on ed25519_dalek::ExpandedSecretKey::from_bytes in my project. What is the alternative to this now that this is gone?

See https://github.com/digitalbitbox/bitbox02-firmware/blob/ec285643a8128f0a45710807979c48c5c083e49c/src/rust/bitbox02-rust/src/keystore/ed25519.rs#L43

[tarcieri]

See ample previous discussion on #298

[benma]

Thanks, looks like the PR https://github.com/dalek-cryptography/ed25519-dalek/pull/299 linked therein would solve my issue.