NCC audit fixes #907
NCC audit fixes #907
Conversation
| @@ -213,7 +213,7 @@ impl SignPrimitive<Secp256k1> for Scalar { | |||
| // Compute 𝒔 as a signature over 𝒓 and 𝒛. | |||
| let s = k_inv * (z + (r * self)); | |||
|
|
|||
| if s.is_zero().into() { | |||
| if s.is_zero().into() || r.is_zero().into() { | |||
There was a problem hiding this comment.
FWIW, these checks duplicate the checks here:
https://github.com/RustCrypto/signatures/blob/800eda2/ecdsa/src/lib.rs#L206-L216
There was a problem hiding this comment.
I opened RustCrypto/signatures#730 which hopefully clarifies both the logic and documentation around checks that r and s are non-zero.
Note that these checks are part of the invariant of the ecdsa::Signature type and the constructors return an error if either component is zero.
There was a problem hiding this comment.
There's actually a lot of duplicated logic there which would be nice to merge somehow.
But I would disagree that these checks are redundant just because Signature::from_scalars implements them. It is a part of the public API, and it makes sense for it to preserve that invariant too.
There was a problem hiding this comment.
There's actually a lot of duplicated logic there which would be nice to merge somehow.
I think I'd like to extract some functions like ecdsa::hazmat::{sign_raw, verify_raw}... the duplication largely exists to implement low-S normalization, but if it can call a low-level function we don't need to duplicate the rest of the implementation. See also #908 for a verification-side analogue of this problem (where the generic implementation is being used, but low-S normalization is no longer honored)
But I would disagree that these checks are redundant just because Signature::from_scalars implements them. It is a part of the public API, and it makes sense for it to preserve that invariant too.
If it makes the code easier to audit I'm fine with it.
| @@ -40,7 +40,7 @@ pub use elliptic_curve::ecdh::diffie_hellman; | |||
|
|
|||
| use crate::{AffinePoint, Secp256k1}; | |||
|
|
|||
| /// NIST P-256 Ephemeral Diffie-Hellman Secret. | |||
| /// secp256k1 Ephemeral Diffie-Hellman Secret. | |||
There was a problem hiding this comment.
I opened #929 with this fix individually
|
I think this check is invalidated by the latest changes anyway, and happens in |
(Audit is not finished, so this is a draft)
try_sign_prehashed()- even though it's very unlikely, but so is s=0, and FIPS186-5 requires this check.