dalek-cryptography / curve25519-dalek

A pure-Rust implementation of group operations on Ristretto and Curve25519
Other
867 stars 439 forks source link

ed25519-dalek: SigningKey.to_scalar() docs are incorrect #565

Closed DavidBuchanan314 closed 9 months ago

DavidBuchanan314 commented 1 year ago

https://github.com/dalek-cryptography/curve25519-dalek/blob/c66973c823b45ef99cd9ed3c6cc58cca9805d6d9/ed25519-dalek/src/signing.rs#L477-L494

The docs say that it's a) a byte representation b) unreduced - but iiuc, the Scalar type is neither. I assume these docs used to be correct, but became stale during refactoring.

Further, I don't see an obvious way to use the resulting value to instantiate a x25519_dalek::StaticSecret - from my PoV it would be more useful if to_scalar really did return the unreduced raw bytes, but the name would be a bit misleading. (maybe making that use-case difficult is intentional, if so, fair enough I guess)

rozbb commented 9 months ago

Indeed, this test fails

#[test]
fn privkey_scalar_yields_x25519_pubkey() {
    let signing_privkey = SigningKey::generate(&mut rand::thread_rng());
    let signing_pubkey = signing_privkey.verifying_key().to_montgomery();
    let signing_scalar = signing_privkey.to_scalar();

    let x_privkey = x25519_dalek::StaticSecret::from(signing_scalar.to_bytes());
    let x_pubkey = x25519_dalek::PublicKey::from(&x_privkey);

    assert_eq!(signing_pubkey.to_bytes(), x_pubkey.to_bytes());
}
rozbb commented 9 months ago

The reason these tests pass is because they're computing the key shares directly as signing_key.verifying_key().to_montgomery(), and using signing_key.to_scalar() as the scalars.

I think an appropriate solution to this is to rename the method in question to SigningKey::to_scalar_bytes() and return the real unreduced scalar bytes. This partially reverts the change in https://github.com/dalek-cryptography/curve25519-dalek/pull/545 .

tarcieri commented 9 months ago

@rozbb we've been over that before. @dignifiedquire hit something very similar to it, which is why we added the Scalar based APIs to avoid transcoding Scalar -> bytes -> Scalar.

https://github.com/RustCrypto/nacl-compat/pull/137

Exposing the unclamped bytes is an alternative, but IMO one we should provide in an additive manner, and we should perhaps just better document the caveats of clamped/unclamped bytes, especially in regard to Scalar::to_bytes.

rozbb commented 9 months ago

Closed by #599