dsa: Eq is missing for Components, SigningKey and VerifyingKey

[dignifiedquire]

I don't see any internal details that would make an impl/derive invalid

[tarcieri]

Derived Eq on SigningKey would introduce a timing sidechannel

[dignifiedquire]

how so, if there is already PartialEq, to which it could delegate?

[tarcieri]

It doesn't currently impl PartialEq, but regardless, the derived implementation will use a data-dependent short-circuiting check, rather than one that always performs the comparison in constant-time

[json420]

I'm probably missing something obvious, but doesn't SigningKey currently do short-circuit comparison because it derives PartialEq? As far as I can tell BigUint doesn't do constant time comparison.

/// DSA private key.
///
/// The [`(try_)sign_digest_with_rng`](::signature::RandomizedDigestSigner) API uses regular non-deterministic signatures,
/// while the [`(try_)sign_digest`](::signature::DigestSigner) API uses deterministic signatures as described in RFC 6979
#[derive(Clone, PartialEq)]
#[must_use]
pub struct SigningKey {
    /// Public key
    verifying_key: VerifyingKey,

    /// Private component x
    x: Zeroizing<BigUint>,
}

[tarcieri]

Oh, I guess it does derive PartialEq (not sure how I missed that before). I guess that will be less of an issue when #906 lands and it switches to crypto-bigint

[json420]

Yeah, crypto-bigint will nearly solve it, but correct me if I'm wrong, comparing Component would still short-circuit after the first non-matching struct member, right?

/// The common components of an DSA keypair
///
/// (the prime p, quotient q and generator g)
#[derive(Clone, Debug, PartialEq, PartialOrd)]
#[must_use]
pub struct Components {
    /// Prime p
    p: BigUint,

    /// Quotient q
    q: BigUint,

    /// Generator g
    g: BigUint,
}

I don't know enough about DSA to know how much of a problem that is security wise... but it does sound like a fun thing for me to try to fix after #906 lands.

[tarcieri]

Only x is secret, the other components are part of the public key

[aumetra]

Short circuiting doesn't matter as long as it isn't on the limbs of the private factor in the signing key, since the components are public knowledge anyway. So after the crypto-bigint migration it should be fine IMO?

[tarcieri]

Yep, crypto_bigint::Uint compares in constant-time internally, even when using the PartialEq API