Make scalars always reduced (#519)

* Removed Scalar::{from_bits, from_bytes_clamped}; all constructible scalars are now reduced mod l

* Made Scalar::reduce() not pub; fixed test warning

* Added benches for scalar add/sub/mul

* Docs

* Added EdwardsPoint::mul_base_clamped and gated Scalar::from_bits behind legacy_compatibility

* Added unit test for Mul impl on unreduced Scalars

* Added Montgomery::mul_base_clamped

* Added BasepointTable::mul_base_clamped

* Removed invalid scalar arithmetic test; this functionality is no longer supported

* Made clamp_integer() const

* Updated readme and changelog

* Added BasepointTable::mul_base_clamped to tests

* Added proper deprecation notice to Scalar::from_bits; added legacy_compatibility to Makefile and docsrs flags

CHANGELOG.md

@@ -24,9 +24,12 @@ major series.
   whenever using `EdwardsBasepointTable` or `RistrettoBasepointTable`
 * `Scalar::from_canonical_bytes` now returns `CtOption`
 * `Scalar::is_canonical` now returns `Choice`
+* Remove `Scalar::from_bytes_clamped` and `Scalar::reduce`
+* Deprecate and feature-gate `Scalar::from_bits` behind `legacy_compatibility`
 
 #### 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

@@ -28,7 +28,7 @@ rustdoc-args = [
     "--cfg", "docsrs",
 ]
 rustc-args = ["--cfg", "curve25519_dalek_backend=\"simd\""]
-features = ["serde", "rand_core", "digest"]
+features = ["serde", "rand_core", "digest", "legacy_compatibility"]
 
 [dev-dependencies]
 sha2 = { version = "0.10", default-features = false }
@@ -66,6 +66,7 @@ packed_simd = { version = "0.3.8", package = "packed_simd_2", features = ["into_
 default = ["alloc", "precomputed-tables", "zeroize"]
 alloc = ["zeroize?/alloc"]
 precomputed-tables = []
+legacy_compatibility = []
 
 [profile.dev]
 opt-level = 2

Makefile

@@ -1,4 +1,4 @@
-FEATURES := serde rand_core digest
+FEATURES := serde rand_core digest legacy_compatibility
 
 export RUSTFLAGS := --cfg=curve25519_dalek_backend="simd"
 export RUSTDOCFLAGS := \

README.md

@@ -56,6 +56,7 @@ curve25519-dalek = "4.0.0-rc.2"
 | `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
@@ -77,6 +78,8 @@ latest breaking changes in high level 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_bytes_clamped` and `Scalar::reduce`
+* Deprecate and feature-gate `Scalar::from_bits` behind `legacy_compatibility`
 * 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/backend/serial/u32/scalar.rs

@@ -18,7 +18,8 @@ use zeroize::Zeroize;
 
 use crate::constants;
 
-/// The `Scalar29` struct represents an element in ℤ/lℤ as 9 29-bit limbs
+/// The `Scalar29` struct represents an element in \\(\mathbb{Z} / \ell\mathbb{Z}\\) as 9 29-bit
+/// limbs
 #[derive(Copy, Clone)]
 pub struct Scalar29(pub [u32; 9]);
 

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")]
@@ -1234,6 +1261,8 @@ mod test {
     #[cfg(feature = "precomputed-tables")]
     use crate::constants::ED25519_BASEPOINT_TABLE;
 
+    use rand_core::RngCore;
+
     /// X coordinate of the basepoint.
     /// = 15112221349535400772501151409588531511454012693041857206046113283949847762202
     static BASE_X_COORD_BYTES: [u8; 32] = [
@@ -1465,16 +1494,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 +1541,55 @@ 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;
+
+        // Make a random curve point in the curve. Give it torsion to make things interesting.
+        #[cfg(feature = "precomputed-tables")]
+        let random_point = {
+            let mut b = [0u8; 32];
+            csprng.fill_bytes(&mut b);
+            EdwardsPoint::mul_base_clamped(b) + constants::EIGHT_TORSION[1]
+        };
+        // Make a basepoint table from the random point. We'll use this with mul_base_clamped
+        #[cfg(feature = "precomputed-tables")]
+        let random_table = EdwardsBasepointTableRadix256::create(&random_point);
+
+        // Now test scalar mult. agreement on the default basepoint as well as random_point
+
+        // Test that mul_base_clamped and mul_clamped agree on a large integer. Even after
+        // clamping, this integer 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)
+        );
+        #[cfg(feature = "precomputed-tables")]
+        assert_eq!(
+            random_table.mul_base_clamped(a_bytes),
+            random_point.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!(
+                EdwardsPoint::mul_base_clamped(a_bytes),
+                constants::ED25519_BASEPOINT_POINT.mul_clamped(a_bytes)
+            );
+            #[cfg(feature = "precomputed-tables")]
+            assert_eq!(
+                random_table.mul_base_clamped(a_bytes),
+                random_point.mul_clamped(a_bytes)
+            );
+        }
+    }
+
     #[test]
     #[cfg(feature = "alloc")]
     fn impl_sum() {
@@ -1617,16 +1692,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