From 28eed1cba0acdd0e9804324118cae95c08eaa9d8 Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Fri, 27 Sep 2019 00:37:30 +0000 Subject: [PATCH 1/3] Add PublicKey::verify_strict() and Keypair::verify_strict() methods. --- benches/ed25519_benchmarks.rs | 12 +++++ src/ed25519.rs | 72 +++++++++++++++++++++++++ src/public.rs | 99 +++++++++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+) diff --git a/benches/ed25519_benchmarks.rs b/benches/ed25519_benchmarks.rs index e07eb61..0af2812 100644 --- a/benches/ed25519_benchmarks.rs +++ b/benches/ed25519_benchmarks.rs @@ -56,6 +56,17 @@ mod ed25519_benches { }); } + fn verify_strict(c: &mut Criterion) { + let mut csprng: ThreadRng = thread_rng(); + let keypair: Keypair = Keypair::generate(&mut csprng); + let msg: &[u8] = b""; + let sig: Signature = keypair.sign(msg); + + c.bench_function("Ed25519 strict signature verification", move |b| { + b.iter(| | keypair.verify_strict(msg, &sig)) + }); + } + fn verify_batch_signatures(c: &mut Criterion) { static BATCH_SIZES: [usize; 8] = [4, 8, 16, 32, 64, 96, 128, 256]; @@ -90,6 +101,7 @@ mod ed25519_benches { sign, sign_expanded_key, verify, + verify_strict, verify_batch_signatures, key_generation, } diff --git a/src/ed25519.rs b/src/ed25519.rs index a76b091..43da922 100644 --- a/src/ed25519.rs +++ b/src/ed25519.rs @@ -447,6 +447,78 @@ impl Keypair { { self.public.verify_prehashed(prehashed_message, context, signature) } + + /// Strictly verify a signature on a message with this keypair's public key. + /// + /// # On The (Multiple) Sources of Malleability in Ed25519 Signatures + /// + /// This version of verification is technically non-RFC8032 compliant. The + /// following explains why. + /// + /// 1. Scalar Malleability + /// + /// The authors of the RFC explicitly stated that verification of an ed25519 + /// signature must fail if the scalar `s` is not properly reduced mod \ell: + /// + /// > To verify a signature on a message M using public key A, with F + /// > being 0 for Ed25519ctx, 1 for Ed25519ph, and if Ed25519ctx or + /// > Ed25519ph is being used, C being the context, first split the + /// > signature into two 32-octet halves. Decode the first half as a + /// > point R, and the second half as an integer S, in the range + /// > 0 <= s < L. Decode the public key A as point A'. If any of the + /// > decodings fail (including S being out of range), the signature is + /// > invalid.) + /// + /// All `verify_*()` functions within ed25519-dalek perform this check. + /// + /// 2. Point malleability + /// + /// The authors of the RFC added in a malleability check to step #3 in + /// §5.1.7, for small torsion components in the `R` value of the signature, + /// *which is not strictly required*, as they state: + /// + /// > Check the group equation [8][S]B = [8]R + [8][k]A'. It's + /// > sufficient, but not required, to instead check [S]B = R + [k]A'. + /// + /// # History of Malleability Checks + /// + /// As originally defined (cf. the "Malleability" section in the README of + /// this repo), ed25519 signatures didn't consider *any* form of + /// malleability to be an issue. Later the scalar malleability was + /// considered important. Still later, particularly with interests in + /// cryptocurrency design and in unique identities (e.g. for Signal users, + /// Tor onion services, etc.), the group element malleability became a + /// concern. + /// + /// However, libraries had already been created to conform to the original + /// definition. One well-used library in particular even implemented the + /// group element malleability check, *but only for batch verification*! + /// Which meant that even using the same library, a single signature could + /// verify fine individually, but suddenly, when verifying it with a bunch + /// of other signatures, the whole batch would fail! + /// + /// # "Strict" Verification + /// + /// This method performs *both* of the above signature malleability checks. + /// + /// It must be done as a separate method because one doesn't simply get to + /// change the definition of a cryptographic primitive ten years + /// after-the-fact with zero consideration for backwards compatibility in + /// hardware and protocols which have it already have the older definition + /// baked in. + /// + /// # Return + /// + /// Returns `Ok(())` if the signature is valid, and `Err` otherwise. + #[allow(non_snake_case)] + pub fn verify_strict( + &self, + message: &[u8], + signature: &Signature, + ) -> Result<(), SignatureError> + { + self.public.verify_strict(message, signature) + } } #[cfg(feature = "serde")] diff --git a/src/public.rs b/src/public.rs index ae3bfa3..f25c88f 100644 --- a/src/public.rs +++ b/src/public.rs @@ -244,6 +244,105 @@ impl PublicKey { Err(SignatureError(InternalError::VerifyError)) } } + + /// Strictly verify a signature on a message with this keypair's public key. + /// + /// # On The (Multiple) Sources of Malleability in Ed25519 Signatures + /// + /// This version of verification is technically non-RFC8032 compliant. The + /// following explains why. + /// + /// 1. Scalar Malleability + /// + /// The authors of the RFC explicitly stated that verification of an ed25519 + /// signature must fail if the scalar `s` is not properly reduced mod \ell: + /// + /// > To verify a signature on a message M using public key A, with F + /// > being 0 for Ed25519ctx, 1 for Ed25519ph, and if Ed25519ctx or + /// > Ed25519ph is being used, C being the context, first split the + /// > signature into two 32-octet halves. Decode the first half as a + /// > point R, and the second half as an integer S, in the range + /// > 0 <= s < L. Decode the public key A as point A'. If any of the + /// > decodings fail (including S being out of range), the signature is + /// > invalid.) + /// + /// All `verify_*()` functions within ed25519-dalek perform this check. + /// + /// 2. Point malleability + /// + /// The authors of the RFC added in a malleability check to step #3 in + /// §5.1.7, for small torsion components in the `R` value of the signature, + /// *which is not strictly required*, as they state: + /// + /// > Check the group equation [8][S]B = [8]R + [8][k]A'. It's + /// > sufficient, but not required, to instead check [S]B = R + [k]A'. + /// + /// # History of Malleability Checks + /// + /// As originally defined (cf. the "Malleability" section in the README of + /// this repo), ed25519 signatures didn't consider *any* form of + /// malleability to be an issue. Later the scalar malleability was + /// considered important. Still later, particularly with interests in + /// cryptocurrency design and in unique identities (e.g. for Signal users, + /// Tor onion services, etc.), the group element malleability became a + /// concern. + /// + /// However, libraries had already been created to conform to the original + /// definition. One well-used library in particular even implemented the + /// group element malleability check, *but only for batch verification*! + /// Which meant that even using the same library, a single signature could + /// verify fine individually, but suddenly, when verifying it with a bunch + /// of other signatures, the whole batch would fail! + /// + /// # "Strict" Verification + /// + /// This method performs *both* of the above signature malleability checks. + /// + /// It must be done as a separate method because one doesn't simply get to + /// change the definition of a cryptographic primitive ten years + /// after-the-fact with zero consideration for backwards compatibility in + /// hardware and protocols which have it already have the older definition + /// baked in. + /// + /// # Return + /// + /// Returns `Ok(())` if the signature is valid, and `Err` otherwise. + #[allow(non_snake_case)] + pub fn verify_strict( + &self, + message: &[u8], + signature: &Signature, + ) -> Result<(), SignatureError> + { + let mut h: Sha512 = Sha512::new(); + let R: EdwardsPoint; + let k: Scalar; + let minus_A: EdwardsPoint = -self.1; + let signature_R: EdwardsPoint; + + match signature.R.decompress() { + None => return Err(SignatureError(InternalError::VerifyError)), + Some(x) => signature_R = x, + } + + // Logical OR is fine here as we're not trying to be constant time. + if signature_R.is_small_order() || self.1.is_small_order() { + return Err(SignatureError(InternalError::VerifyError)); + } + + h.input(signature.R.as_bytes()); + h.input(self.as_bytes()); + h.input(&message); + + k = Scalar::from_hash(h); + R = EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s); + + if R == signature_R { + Ok(()) + } else { + Err(SignatureError(InternalError::VerifyError)) + } + } } #[cfg(feature = "serde")] From ce2260afab60c6ef1cda5c7571aef1f69019c7d9 Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Thu, 3 Oct 2019 23:42:16 +0000 Subject: [PATCH 2/3] Implement stricter scalar malleability checking for signatures. Previously, we were checking that the highest 3 bits were unset, which still leaves 2^253 - 2^252 + 27742317777372353535851937790883648493 potential scalars for the `s` component of a signature which are not strictly mod \ell. This change fixes that. Note: This change makes ed25519-dalek incompatible with ed25519-donna in that some signatures produced by donna will be verifiable by donna but NOT VERIFIABLE by dalek. On the other hand, libsodium exports a -DED25519_COMPAT feature, which when enabled, means it is compatible with dalek with the `legacy_compatibility` feature disabled. Otherwise, libsodium's behaviour is identical to the behaviour enabled by default in this patch. --- Cargo.toml | 2 ++ src/signature.rs | 34 +++++++++++++++++++++++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 453d72e..28411f1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,6 +69,8 @@ alloc = ["curve25519-dalek/alloc", "rand_os"] nightly = ["curve25519-dalek/nightly", "clear_on_drop/nightly"] batch = ["rand"] asm = ["sha2/asm"] +# This features turns off stricter checking for scalar malleability in signatures +legacy_compatibility = [] yolocrypto = ["curve25519-dalek/yolocrypto"] u64_backend = ["curve25519-dalek/u64_backend"] u32_backend = ["curve25519-dalek/u32_backend"] diff --git a/src/signature.rs b/src/signature.rs index d5079fd..653155d 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -71,6 +71,31 @@ impl Debug for Signature { } } +#[cfg(feature = "legacy_compatibility")] +#[inline(always)] +fn check_scalar(bytes: [u8; 32]) -> Result { + // The highest 3 bits must not be set. No other checking for the + // remaining 2^253 - 2^252 + 27742317777372353535851937790883648493 + // potential non-reduced scalars is performed. + // + // This is compatible with ed25519-donna and libsodium when + // -DED25519_COMPAT is NOT specified. + if bytes[31] & 224 != 0 { + return Err(SignatureError(InternalError::ScalarFormatError)); + } + + Ok(Scalar::from_bits(bytes)) +} + +#[cfg(not(feature = "legacy_compatibility"))] +#[inline(always)] +fn check_scalar(bytes: [u8; 32]) -> Result { + match Scalar::from_canonical_bytes(bytes) { + None => return Err(SignatureError(InternalError::ScalarFormatError)), + Some(x) => return Ok(x), + }; +} + impl Signature { /// Convert this `Signature` to a byte array. #[inline] @@ -97,13 +122,16 @@ impl Signature { lower.copy_from_slice(&bytes[..32]); upper.copy_from_slice(&bytes[32..]); - if upper[31] & 224 != 0 { - return Err(SignatureError(InternalError::ScalarFormatError)); + let s: Scalar; + + match check_scalar(upper) { + Ok(x) => s = x, + Err(x) => return Err(x), } Ok(Signature { R: CompressedEdwardsY(lower), - s: Scalar::from_bits(upper), + s: s, }) } } From 2d5fe86f3062ec5edf36a73d5b4799d9756ce3ce Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Mon, 7 Oct 2019 19:03:15 +0000 Subject: [PATCH 3/3] Document anti-malleability features/functionality. --- README.md | 42 ++++++++++++++++++++++++++++++++++++++--- src/signature.rs | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 9cb233c..6941314 100644 --- a/README.md +++ b/README.md @@ -108,9 +108,45 @@ after the fact, breaking compatibility with every other implementation. In short, if malleable signatures are bad for your protocol, don't use them. Consider using a curve25519-based Verifiable Random Function (VRF), such as [Trevor Perrin's VXEdDSA](https://www.whispersystems.org/docs/specifications/xeddsa/), -instead. We -[plan](https://github.com/dalek-cryptography/curve25519-dalek/issues/9) to -eventually support VXEdDSA in curve25519-dalek. +instead. + +#### The `legacy_compatibility` Feature + +By default, this library performs a stricter check for malleability in the +scalar component of a signature, upon signature deserialisation. This stricter +check, that `s < \ell` where `\ell` is the order of the basepoint, is +[mandated by RFC8032](https://tools.ietf.org/html/rfc8032#section-5.1.7). +However, that RFC was standardised a decade after the original paper, which, as +described above, (usually, falsely) stated that malleability was inconsequential. + +Because of this, most ed25519 implementations only perform a limited, hackier +check that the most significant three bits of the scalar are unset. If you need +compatibility with legacy implementations, including: + +* ed25519-donna +* Golang's /x/crypto ed25519 +* libsodium (only when built with `-DED25519_COMPAT`) +* NaCl's "ref" implementation +* probably a bunch of others + +then enable `ed25519-dalek`'s `legacy_compatibility` feature. Please note and +be forewarned that doing so allows for signature malleability, meaning that +there may be two different and "valid" signatures with the same key for the same +message, which is obviously incredibly dangerous in a number of contexts, +including—but not limited to—identification protocols and cryptocurrency +transactions. + +#### The `verify_strict()` Function + +The scalar component of a signature is not the only source of signature +malleability, however. Both the public key used for signature verification and +the group element component of the signature are malleable, as they may contain +a small torsion component as a consquence of the curve25519 group not being of +prime order, but having a small cofactor of 8. + +If you wish to also eliminate this source of signature malleability, please +review the +[documentation for the `verify_strict()` function](https://doc.dalek.rs/ed25519_dalek/struct.PublicKey.html#method.verify_strict). # Installation diff --git a/src/signature.rs b/src/signature.rs index 653155d..8bcbbe3 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -108,6 +108,55 @@ impl Signature { } /// Construct a `Signature` from a slice of bytes. + /// + /// # Scalar Malleability Checking + /// + /// As originally specified in the ed25519 paper (cf. the "Malleability" + /// section of the README in this repo), no checks whatsoever were performed + /// for signature malleability. + /// + /// Later, a semi-functional, hacky check was added to most libraries to + /// "ensure" that the scalar portion, `s`, of the signature was reduced `mod + /// \ell`, the order of the basepoint: + /// + /// ```ignore + /// if signature.s[31] & 224 != 0 { + /// return Err(); + /// } + /// ``` + /// + /// This bit-twiddling ensures that the most significant three bits of the + /// scalar are not set: + /// + /// ```python,ignore + /// >>> 0b00010000 & 224 + /// 0 + /// >>> 0b00100000 & 224 + /// 32 + /// >>> 0b01000000 & 224 + /// 64 + /// >>> 0b10000000 & 224 + /// 128 + /// ``` + /// + /// However, this check is hacky and insufficient to check that the scalar is + /// fully reduced `mod \ell = 2^252 + 27742317777372353535851937790883648493` as + /// it leaves us with a guanteed bound of 253 bits. This means that there are + /// `2^253 - 2^252 + 2774231777737235353585193779088364849311` remaining scalars + /// which could cause malleabilllity. + /// + /// RFC8032 [states](https://tools.ietf.org/html/rfc8032#section-5.1.7): + /// + /// > To verify a signature on a message M using public key A, [...] + /// > first split the signature into two 32-octet halves. Decode the first + /// > half as a point R, and the second half as an integer S, in the range + /// > 0 <= s < L. Decode the public key A as point A'. If any of the + /// > decodings fail (including S being out of range), the signature is + /// > invalid. + /// + /// However, by the time this was standardised, most libraries in use were + /// only checking the most significant three bits. (See also the + /// documentation for `PublicKey.verify_strict`.) #[inline] pub fn from_bytes(bytes: &[u8]) -> Result { if bytes.len() != SIGNATURE_LENGTH {