Make scalars always reduced

CHANGELOG.md

@@ -22,9 +22,11 @@ major series.
   whenever using `EdwardsBasepointTable` or `RistrettoBasepointTable`
 * `Scalar::from_canonical_bytes` now returns `CtOption`
 * `Scalar::is_canonical` now returns `Choice`
+* Remove `Scalar::from_bits` and `Scalar::from_bits_clamped`
 
 #### Other changes
 
+* Add `EdwardsPoint::{mul_base, mul_base_clamped}`, `MontgomeryPoint::{mul_base, mul_base_clamped}`, and `BasepointTable::mul_base_clamped`
 * Add `precomputed-tables` feature
 * Update Maintenance Policies for SemVer
 * Migrate documentation to docs.rs hosted

Cargo.toml

@@ -66,6 +66,7 @@ packed_simd = { version = "0.3.4", package = "packed_simd_2", features = ["into_
 default = ["alloc", "precomputed-tables", "zeroize"]
 alloc = ["zeroize?/alloc"]
 precomputed-tables = []
+legacy_compatibility = []
 
 [profile.dev]
 opt-level = 2

README.md

@@ -56,6 +56,7 @@ curve25519-dalek = "4.0.0-rc.1"
 | `rand_core`        |          | Enables `Scalar::random` and `RistrettoPoint::random`. This is an optional dependency whose version is not subject to SemVer. See [below](#public-api-semver-exemptions) for more details. |
 | `digest`           |          | Enables `RistrettoPoint::{from_hash, hash_from_bytes}` and `Scalar::{from_hash, hash_from_bytes}`. This is an optional dependency whose version is not subject to SemVer. See [below](#public-api-semver-exemptions) for more details. |
 | `serde`            |          | Enables `serde` serialization/deserialization for all the point and scalar types. |
+| `legacy_compatibility`|       | Enables `Scalar::from_bits`, which allows the user to build unreduced scalars whose arithmetic is broken. Do not use this unless you know what you're doing. |
 
 To disable the default features when using `curve25519-dalek` as a dependency,
 add `default-features = false` to the dependency in your `Cargo.toml`. To
@@ -78,6 +79,7 @@ latest breaking changes are below:
 * Replace methods `Scalar::{zero, one}` with constants `Scalar::{ZERO, ONE}`
 * `Scalar::from_canonical_bytes` now returns `CtOption`
 * `Scalar::is_canonical` now returns `Choice`
+* Remove `Scalar::from_bits` and `Scalar::from_bits_clamped`
 * Deprecate `EdwardsPoint::hash_from_bytes` and rename it
   `EdwardsPoint::nonspec_map_to_curve`
 * Require including a new trait, `use curve25519_dalek::traits::BasepointTable`

benches/dalek_benchmarks.rs

@@ -300,11 +300,34 @@ mod montgomery_benches {
 mod scalar_benches {
     use super::*;
 
-    fn scalar_inversion<M: Measurement>(c: &mut BenchmarkGroup<M>) {
+    fn scalar_arith<M: Measurement>(c: &mut BenchmarkGroup<M>) {
+        let mut rng = thread_rng();
+
         c.bench_function("Scalar inversion", |b| {
             let s = Scalar::from(897987897u64).invert();
             b.iter(|| s.invert());
         });
+        c.bench_function("Scalar addition", |b| {
+            b.iter_batched(
+                || (Scalar::random(&mut rng), Scalar::random(&mut rng)),
+                |(a, b)| a + b,
+                BatchSize::SmallInput,
+            );
+        });
+        c.bench_function("Scalar subtraction", |b| {
+            b.iter_batched(
+                || (Scalar::random(&mut rng), Scalar::random(&mut rng)),
+                |(a, b)| a - b,
+                BatchSize::SmallInput,
+            );
+        });
+        c.bench_function("Scalar multiplication", |b| {
+            b.iter_batched(
+                || (Scalar::random(&mut rng), Scalar::random(&mut rng)),
+                |(a, b)| a * b,
+                BatchSize::SmallInput,
+            );
+        });
     }
 
     fn batch_scalar_inversion<M: Measurement>(c: &mut BenchmarkGroup<M>) {
@@ -329,7 +352,7 @@ mod scalar_benches {
         let mut c = Criterion::default();
         let mut g = c.benchmark_group("scalar benches");
 
-        scalar_inversion(&mut g);
+        scalar_arith(&mut g);
         batch_scalar_inversion(&mut g);
     }
 }

src/backend/serial/scalar_mul/variable_base.rs

@@ -16,6 +16,7 @@ pub(crate) fn mul(point: &EdwardsPoint, scalar: &Scalar) -> EdwardsPoint {
     //    s = s_0 + s_1*16^1 + ... + s_63*16^63,
     //
     // with `-8 ≤ s_i < 8` for `0 ≤ i < 63` and `-8 ≤ s_63 ≤ 8`.
+    // This decomposition requires s < 2^255, which is guaranteed by Scalar invariant #1.
     let scalar_digits = scalar.as_radix_16();
     // Compute s*P as
     //

src/edwards.rs

@@ -44,8 +44,8 @@
 //!
 //! ## Scalars
 //!
-//! Scalars are represented by the [`Scalar`] struct.  To construct a scalar with a specific bit
-//! pattern, see [`Scalar::from_bits`].
+//! Scalars are represented by the [`Scalar`] struct. To construct a scalar, see
+//! [`Scalar::from_canonical_bytes`] or [`Scalar::from_bytes_mod_order_wide`].
 //!
 //! ## Scalar Multiplication
 //!
@@ -118,7 +118,7 @@ use zeroize::Zeroize;
 use crate::constants;
 
 use crate::field::FieldElement;
-use crate::scalar::Scalar;
+use crate::scalar::{clamp_integer, Scalar};
 
 use crate::montgomery::MontgomeryPoint;
 
@@ -728,6 +728,34 @@ impl EdwardsPoint {
             scalar * constants::ED25519_BASEPOINT_TABLE
         }
     }
+
+    /// Multiply this point by `clamp_integer(bytes)`. For a description of clamping, see
+    /// [`clamp_integer`].
+    pub fn mul_clamped(self, bytes: [u8; 32]) -> Self {
+        // We have to construct a Scalar that is not reduced mod l, which breaks scalar invariant
+        // #2. But #2 is not necessary for correctness of variable-base multiplication. All that
+        // needs to hold is invariant #1, i.e., the scalar is less than 2^255. This is guaranteed
+        // by clamping.
+        // Further, we don't do any reduction or arithmetic with this clamped value, so there's no
+        // issues arising from the fact that the curve point is not necessarily in the prime-order
+        // subgroup.
+        let s = Scalar {
+            bytes: clamp_integer(bytes),
+        };
+        s * self
+    }
+
+    /// Multiply the basepoint by `clamp_integer(bytes)`. For a description of clamping, see
+    /// [`clamp_integer`].
+    pub fn mul_base_clamped(bytes: [u8; 32]) -> Self {
+        // See reasoning in Self::mul_clamped why it is OK to make an unreduced Scalar here. We
+        // note that fixed-base multiplication is also defined for all values of `bytes` less than
+        // 2^255.
+        let s = Scalar {
+            bytes: clamp_integer(bytes),
+        };
+        Self::mul_base(&s)
+    }
 }
 
 // ------------------------------------------------------------------------
@@ -875,7 +903,7 @@ macro_rules! impl_basepoint_table {
         ///
         /// Normally, the radix-256 tables would allow for only 32 additions per scalar
         /// multiplication.  However, due to the fact that standardised definitions of
-        /// legacy protocols—such as x25519—require allowing unreduced 255-bit scalar
+        /// legacy protocols—such as x25519—require allowing unreduced 255-bit scalars
         /// invariants, when converting such an unreduced scalar's representation to
         /// radix-\\(2^{8}\\), we cannot guarantee the carry bit will fit in the last
         /// coefficient (the coefficients are `i8`s).  When, \\(w\\), the power-of-2 of
@@ -1224,8 +1252,7 @@ impl Debug for EdwardsPoint {
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::field::FieldElement;
-    use crate::scalar::Scalar;
+    use crate::{field::FieldElement, scalar::Scalar};
     use subtle::ConditionallySelectable;
 
     #[cfg(feature = "alloc")]
@@ -1465,16 +1492,13 @@ mod test {
         assert_eq!(aP128, aP256);
     }
 
-    /// Check a unreduced scalar multiplication by the basepoint tables.
+    /// Check unreduced scalar multiplication by the basepoint tables is the same no matter what
+    /// radix the table is.
     #[cfg(feature = "precomputed-tables")]
     #[test]
     fn basepoint_tables_unreduced_scalar() {
         let P = &constants::ED25519_BASEPOINT_POINT;
-        let a = Scalar::from_bits([
-            0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
-            0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
-            0xFF, 0xFF, 0xFF, 0xFF,
-        ]);
+        let a = crate::scalar::test::LARGEST_UNREDUCED_SCALAR;
 
         let table_radix16 = EdwardsBasepointTableRadix16::create(P);
         let table_radix32 = EdwardsBasepointTableRadix32::create(P);
@@ -1515,6 +1539,33 @@ mod test {
         assert_eq!(bp16.compress(), BASE16_CMPRSSD);
     }
 
+    /// Check that mul_base_clamped and mul_clamped agree
+    #[test]
+    fn mul_base_clamped() {
+        let mut csprng = rand_core::OsRng;
+
+        // Test agreement on a large integer. Even after clamping, this is not reduced mod l.
+        let a_bytes = [0xff; 32];
+        assert_eq!(
+            EdwardsPoint::mul_base_clamped(a_bytes),
+            constants::ED25519_BASEPOINT_POINT.mul_clamped(a_bytes)
+        );
+
+        // Test agreement on random integers
+        for _ in 0..100 {
+            use rand_core::RngCore;
+
+            // This will be reduced mod l with probability l / 2^256 ≈ 6.25%
+            let mut a_bytes = [0u8; 32];
+            csprng.fill_bytes(&mut a_bytes);
+
+            assert_eq!(
+                EdwardsPoint::mul_base_clamped(a_bytes),
+                constants::ED25519_BASEPOINT_POINT.mul_clamped(a_bytes)
+            );
+        }
+    }
+
     #[test]
     #[cfg(feature = "alloc")]
     fn impl_sum() {
@@ -1617,16 +1668,11 @@ mod test {
     // A single iteration of a consistency check for MSM.
     #[cfg(feature = "alloc")]
     fn multiscalar_consistency_iter(n: usize) {
-        use core::iter;
         let mut rng = rand::thread_rng();
 
         // Construct random coefficients x0, ..., x_{n-1},
         // followed by some extra hardcoded ones.
-        let xs = (0..n)
-            .map(|_| Scalar::random(&mut rng))
-            // The largest scalar allowed by the type system, 2^255-1
-            .chain(iter::once(Scalar::from_bits([0xff; 32])))
-            .collect::<Vec<_>>();
+        let xs = (0..n).map(|_| Scalar::random(&mut rng)).collect::<Vec<_>>();
         let check = xs.iter().map(|xi| xi * xi).sum::<Scalar>();
 
         // Construct points G_i = x_i * B

src/montgomery.rs

@@ -57,7 +57,7 @@ use core::{
 use crate::constants::{APLUS2_OVER_FOUR, MONTGOMERY_A, MONTGOMERY_A_NEG};
 use crate::edwards::{CompressedEdwardsY, EdwardsPoint};
 use crate::field::FieldElement;
-use crate::scalar::Scalar;
+use crate::scalar::{clamp_integer, Scalar};
 
 use crate::traits::Identity;
 
@@ -123,6 +123,34 @@ impl MontgomeryPoint {
         EdwardsPoint::mul_base(scalar).to_montgomery()
     }
 
+    /// Multiply this point by `clamp_integer(bytes)`. For a description of clamping, see
+    /// [`clamp_integer`].
+    pub fn mul_clamped(self, bytes: [u8; 32]) -> Self {
+        // We have to construct a Scalar that is not reduced mod l, which breaks scalar invariant
+        // #2. But #2 is not necessary for correctness of variable-base multiplication. All that
+        // needs to hold is invariant #1, i.e., the scalar is less than 2^255. This is guaranteed
+        // by clamping.
+        // Further, we don't do any reduction or arithmetic with this clamped value, so there's no
+        // issues arising from the fact that the curve point is not necessarily in the prime-order
+        // subgroup.
+        let s = Scalar {
+            bytes: clamp_integer(bytes),
+        };
+        s * self
+    }
+
+    /// Multiply the basepoint by `clamp_integer(bytes)`. For a description of clamping, see
+    /// [`clamp_integer`].
+    pub fn mul_base_clamped(bytes: [u8; 32]) -> Self {
+        // See reasoning in Self::mul_clamped why it is OK to make an unreduced Scalar here. We
+        // note that fixed-base multiplication is also defined for all values of `bytes` less than
+        // 2^255.
+        let s = Scalar {
+            bytes: clamp_integer(bytes),
+        };
+        Self::mul_base(&s)
+    }
+
     /// View this `MontgomeryPoint` as an array of bytes.
     pub const fn as_bytes(&self) -> &[u8; 32] {
         &self.0
@@ -342,6 +370,9 @@ impl<'a, 'b> Mul<&'b Scalar> for &'a MontgomeryPoint {
             W: FieldElement::ONE,
         };
 
+        // NOTE: The below swap-double-add routine skips the first iteration, i.e., it assumes the
+        // MSB of `scalar` is 0. This is allowed, since it follows from Scalar invariant #1.
+
         // Go through the bits from most to least significant, using a sliding window of 2
         let mut bits = scalar.bits_le().rev();
         let mut prev_bit = bits.next().unwrap();
@@ -391,8 +422,7 @@ mod test {
     #[cfg(feature = "alloc")]
     use alloc::vec::Vec;
 
-    #[cfg(feature = "rand_core")]
-    use rand_core::OsRng;
+    use rand_core::RngCore;
 
     #[test]
     fn identity_in_different_coordinates() {
@@ -476,18 +506,44 @@ mod test {
     }
 
     #[test]
-    #[cfg(feature = "rand_core")]
     fn montgomery_ladder_matches_edwards_scalarmult() {
-        let mut csprng: OsRng = OsRng;
+        let mut csprng = rand_core::OsRng;
 
-        let s: Scalar = Scalar::random(&mut csprng);
-        let p_edwards = EdwardsPoint::mul_base(&s);
-        let p_montgomery: MontgomeryPoint = p_edwards.to_montgomery();
+        for _ in 0..100 {
+            let s: Scalar = Scalar::random(&mut csprng);
+            let p_edwards = EdwardsPoint::mul_base(&s);
+            let p_montgomery: MontgomeryPoint = p_edwards.to_montgomery();
 
-        let expected = s * p_edwards;
-        let result = s * p_montgomery;
+            let expected = s * p_edwards;
+            let result = s * p_montgomery;
 
-        assert_eq!(result, expected.to_montgomery())
+            assert_eq!(result, expected.to_montgomery())
+        }
+    }
+
+    /// Check that mul_base_clamped and mul_clamped agree
+    #[test]
+    fn mul_base_clamped() {
+        let mut csprng = rand_core::OsRng;
+
+        // Test agreement on a large integer. Even after clamping, this is not reduced mod l.
+        let a_bytes = [0xff; 32];
+        assert_eq!(
+            MontgomeryPoint::mul_base_clamped(a_bytes),
+            constants::X25519_BASEPOINT.mul_clamped(a_bytes)
+        );
+
+        // Test agreement on random integers
+        for _ in 0..100 {
+            // This will be reduced mod l with probability l / 2^256 ≈ 6.25%
+            let mut a_bytes = [0u8; 32];
+            csprng.fill_bytes(&mut a_bytes);
+
+            assert_eq!(
+                MontgomeryPoint::mul_base_clamped(a_bytes),
+                constants::X25519_BASEPOINT.mul_clamped(a_bytes)
+            );
+        }
     }
 
     #[cfg(feature = "alloc")]