Revert "Write random_mod in terms of new random_bits" (#1060)

This reverts commit ba4f0e0 (#1026)

For whatever reason this breaks the `dsa` test suite.

I'm in the middle of a major refactoring in `crypto-bigint` and this
took quite a bit of bisecting to figure out on top of all of that, so I
don't have time to investigate why.

cc @mrdomino

Author: tarcieri

src/traits.rs

@@ -639,6 +639,7 @@ pub trait Encoding: Sized {
     /// Byte array representation.
     type Repr: AsRef<[u8]>
         + AsMut<[u8]>
+        + Copy
         + Clone
         + Sized
         + for<'a> TryFrom<&'a [u8], Error: core::error::Error>;

src/uint/boxed/encoding.rs

@@ -1,7 +1,7 @@
 //! Const-friendly decoding operations for [`BoxedUint`].
 
 use super::BoxedUint;
-use crate::{CtEq, CtOption, DecodeError, Encoding, Limb, Word, uint::encoding};
+use crate::{CtEq, CtOption, DecodeError, Limb, Word, uint::encoding};
 use alloc::{boxed::Box, string::String, vec::Vec};
 
 #[cfg(feature = "serde")]
@@ -245,28 +245,6 @@ impl BoxedUint {
     }
 }
 
-impl Encoding for BoxedUint {
-    type Repr = Box<[u8]>;
-
-    fn to_be_bytes(&self) -> Self::Repr {
-        BoxedUint::to_be_bytes(self)
-    }
-
-    fn to_le_bytes(&self) -> Self::Repr {
-        BoxedUint::to_le_bytes(self)
-    }
-
-    fn from_be_bytes(bytes: Self::Repr) -> Self {
-        BoxedUint::from_be_slice(&bytes, (bytes.len() * 8).try_into().expect("overflow"))
-            .expect("decode error")
-    }
-
-    fn from_le_bytes(bytes: Self::Repr) -> Self {
-        BoxedUint::from_le_slice(&bytes, (bytes.len() * 8).try_into().expect("overflow"))
-            .expect("decode error")
-    }
-}
-
 /// Decoder target producing a Vec<Limb>
 #[derive(Default)]
 struct VecDecodeByLimb {

src/uint/boxed/rand.rs

@@ -3,7 +3,7 @@
 use super::BoxedUint;
 use crate::{
     NonZero, RandomBits, RandomBitsError, RandomMod,
-    uint::rand::{random_bits_core, random_mod_vartime_core},
+    uint::rand::{random_bits_core, random_mod_core},
 };
 use rand_core::{RngCore, TryRngCore};
 
@@ -28,15 +28,15 @@ impl RandomBits for BoxedUint {
         }
 
         let mut ret = BoxedUint::zero_with_precision(bits_precision);
-        random_bits_core(rng, &mut ret, bit_length).map_err(RandomBitsError::RandCore)?;
+        random_bits_core(rng, &mut ret.limbs, bit_length)?;
         Ok(ret)
     }
 }
 
 impl RandomMod for BoxedUint {
     fn random_mod_vartime<R: RngCore + ?Sized>(rng: &mut R, modulus: &NonZero<Self>) -> Self {
         let mut n = BoxedUint::zero_with_precision(modulus.bits_precision());
-        let Ok(()) = random_mod_vartime_core(rng, &mut n, modulus, modulus.bits());
+        let Ok(()) = random_mod_core(rng, &mut n, modulus, modulus.bits());
         n
     }
 
@@ -45,7 +45,7 @@ impl RandomMod for BoxedUint {
         modulus: &NonZero<Self>,
     ) -> Result<Self, R::Error> {
         let mut n = BoxedUint::zero_with_precision(modulus.bits_precision());
-        random_mod_vartime_core(rng, &mut n, modulus, modulus.bits())?;
+        random_mod_core(rng, &mut n, modulus, modulus.bits())?;
         Ok(n)
     }
 }

src/uint/rand.rs

@@ -1,7 +1,7 @@
 //! Random number generator support
 
-use super::Uint;
-use crate::{CtLt, Encoding, Limb, NonZero, Random, RandomBits, RandomBitsError, RandomMod};
+use super::{Uint, Word};
+use crate::{CtLt, Encoding, Limb, NonZero, Random, RandomBits, RandomBitsError, RandomMod, Zero};
 use rand_core::{RngCore, TryRngCore};
 
 impl<const LIMBS: usize> Random for Uint<LIMBS> {
@@ -25,26 +25,45 @@ impl<const LIMBS: usize> Random for Uint<LIMBS> {
 /// `rng.fill_bytes(&mut bytes[..i]); rng.fill_bytes(&mut bytes[i..])` constructs the same `bytes`,
 /// as long as `i` is a multiple of `X`.
 /// Note that the `TryRngCore` trait does _not_ require this behaviour from `rng`.
-pub(crate) fn random_bits_core<T, R: TryRngCore + ?Sized>(
+pub(crate) fn random_bits_core<R: TryRngCore + ?Sized>(
     rng: &mut R,
-    x: &mut T,
-    n_bits: u32,
-) -> Result<(), R::Error>
-where
-    T: Encoding,
-{
-    if n_bits == 0 {
+    zeroed_limbs: &mut [Limb],
+    bit_length: u32,
+) -> Result<(), RandomBitsError<R::Error>> {
+    if bit_length == 0 {
         return Ok(());
     }
 
-    let n_bytes = n_bits.div_ceil(u8::BITS) as usize;
-    let hi_mask = u8::MAX >> ((u8::BITS - (n_bits % u8::BITS)) % u8::BITS);
+    let buffer: Word = 0;
+    let mut buffer = buffer.to_be_bytes();
 
-    let mut buffer = x.to_le_bytes();
-    let slice = buffer.as_mut();
-    rng.try_fill_bytes(&mut slice[..n_bytes])?;
-    slice[n_bytes - 1] &= hi_mask;
-    *x = T::from_le_bytes(buffer);
+    let nonzero_limbs = bit_length.div_ceil(Limb::BITS) as usize;
+    let partial_limb = bit_length % Limb::BITS;
+    let mask = Word::MAX >> ((Word::BITS - partial_limb) % Word::BITS);
+
+    for i in 0..nonzero_limbs - 1 {
+        rng.try_fill_bytes(&mut buffer)
+            .map_err(RandomBitsError::RandCore)?;
+        zeroed_limbs[i] = Limb(Word::from_le_bytes(buffer));
+    }
+
+    // This algorithm should sample the same number of random bytes, regardless of the pointer width
+    // of the target platform. To this end, special attention has to be paid to the case where
+    // bit_length - 1 < 32 mod 64. Bit strings of that size can be represented using `2X+1` 32-bit
+    // words or `X+1` 64-bit words. Note that 64*(X+1) - 32*(2X+1) = 32. Hence, if we sample full
+    // words only, a 64-bit platform will sample 32 bits more than a 32-bit platform. We prevent
+    // this by forcing both platforms to only sample 4 bytes for the last word in this case.
+    let slice = if partial_limb > 0 && partial_limb <= 32 {
+        // Note: we do not have to zeroize the second half of the buffer, as the mask will take
+        // care of this in the end.
+        &mut buffer[0..4]
+    } else {
+        buffer.as_mut_slice()
+    };
+
+    rng.try_fill_bytes(slice)
+        .map_err(RandomBitsError::RandCore)?;
+    zeroed_limbs[nonzero_limbs - 1] = Limb(Word::from_le_bytes(buffer) & mask);
 
     Ok(())
 }
@@ -74,46 +93,72 @@ impl<const LIMBS: usize> RandomBits for Uint<LIMBS> {
                 bits_precision,
             });
         }
-        let mut x = Self::ZERO;
-        random_bits_core(rng, &mut x, bit_length).map_err(RandomBitsError::RandCore)?;
-        Ok(x)
+        let mut limbs = [Limb::ZERO; LIMBS];
+        random_bits_core(rng, &mut limbs, bit_length)?;
+        Ok(Self::from(limbs))
     }
 }
 
 impl<const LIMBS: usize> RandomMod for Uint<LIMBS> {
-    fn random_mod_vartime<R: RngCore + ?Sized>(rng: &mut R, modulus: &NonZero<Self>) -> Self {
-        let mut x = Self::ZERO;
-        let Ok(()) = random_mod_vartime_core(rng, &mut x, modulus, modulus.bits_vartime());
-        x
+    fn random_mod<R: RngCore + ?Sized>(rng: &mut R, modulus: &NonZero<Self>) -> Self {
+        let mut n = Self::ZERO;
+        let Ok(()) = random_mod_core(rng, &mut n, modulus, modulus.bits_vartime());
+        n
     }
 
     fn try_random_mod_vartime<R: TryRngCore + ?Sized>(
         rng: &mut R,
         modulus: &NonZero<Self>,
     ) -> Result<Self, R::Error> {
-        let mut x = Self::ZERO;
-        random_mod_vartime_core(rng, &mut x, modulus, modulus.bits_vartime())?;
-        Ok(x)
+        let mut n = Self::ZERO;
+        random_mod_core(rng, &mut n, modulus, modulus.bits_vartime())?;
+        Ok(n)
     }
 }
 
-/// Generic implementation of `random_mod_vartime` which can be shared with `BoxedUint`.
+/// Generic implementation of `random_mod` which can be shared with `BoxedUint`.
 // TODO(tarcieri): obtain `n_bits` via a trait like `Integer`
-pub(super) fn random_mod_vartime_core<T, R: TryRngCore + ?Sized>(
+pub(super) fn random_mod_core<T, R: TryRngCore + ?Sized>(
     rng: &mut R,
-    x: &mut T,
+    n: &mut T,
     modulus: &NonZero<T>,
     n_bits: u32,
 ) -> Result<(), R::Error>
 where
-    T: Encoding + CtLt,
+    T: AsMut<[Limb]> + AsRef<[Limb]> + CtLt + Zero,
 {
+    #[cfg(target_pointer_width = "64")]
+    let mut next_word = || rng.try_next_u64();
+    #[cfg(target_pointer_width = "32")]
+    let mut next_word = || rng.try_next_u32();
+
+    let n_limbs = n_bits.div_ceil(Limb::BITS) as usize;
+
+    let hi_word_modulus = modulus.as_ref().as_ref()[n_limbs - 1].0;
+    let mask = !0 >> hi_word_modulus.leading_zeros();
+    let mut hi_word = next_word()? & mask;
+
     loop {
-        random_bits_core(rng, x, n_bits)?;
-        if x.ct_lt(modulus).into() {
-            return Ok(());
+        while hi_word > hi_word_modulus {
+            hi_word = next_word()? & mask;
+        }
+        // Set high limb
+        n.as_mut()[n_limbs - 1] = Limb::from_le_bytes(hi_word.to_le_bytes());
+        // Set low limbs
+        for i in 0..n_limbs - 1 {
+            // Need to deserialize from little-endian to make sure that two 32-bit limbs
+            // deserialized sequentially are equal to one 64-bit limb produced from the same
+            // byte stream.
+            n.as_mut()[i] = Limb::from_le_bytes(next_word()?.to_le_bytes());
+        }
+        // If the high limb is equal to the modulus' high limb, it's still possible
+        // that the full uint is too big so we check and repeat if it is.
+        if n.ct_lt(modulus).into() {
+            break;
         }
+        hi_word = next_word()? & mask;
     }
+    Ok(())
 }
 
 #[cfg(test)]
@@ -223,7 +268,7 @@ mod tests {
 
         let bit_length = 989;
         let mut val = U1024::ZERO;
-        random_bits_core(&mut rng, &mut val, bit_length).expect("safe");
+        random_bits_core(&mut rng, val.as_mut_limbs(), bit_length).expect("safe");
 
         assert_eq!(
             val,
@@ -242,41 +287,6 @@ mod tests {
         );
     }
 
-    /// Make sure random_mod_vartime output is consistent across platforms
-    #[test]
-    fn random_mod_vartime_platform_independence() {
-        let mut rng = get_four_sequential_rng();
-
-        let modulus = NonZero::new(U256::from_u32(8192)).unwrap();
-        let mut vals = [U256::ZERO; 5];
-        for val in &mut vals {
-            *val = U256::random_mod_vartime(&mut rng, &modulus);
-        }
-        let expected = [55, 3378, 2172, 1657, 5323];
-        for (want, got) in expected.into_iter().zip(vals.into_iter()) {
-            // assert_eq!(got.as_words()[0], want);
-            assert_eq!(got, U256::from_u32(want));
-        }
-
-        let modulus =
-            NonZero::new(U256::ZERO.wrapping_sub(&U256::from_u64(rng.next_u64()))).unwrap();
-        let val = U256::random_mod_vartime(&mut rng, &modulus);
-        assert_eq!(
-            val,
-            U256::from_be_hex("E17653A37F1BCC44277FA208E6B31E08CDC4A23A7E88E660EF781C7DD2D368BA")
-        );
-
-        let mut state = [0u8; 16];
-        rng.fill_bytes(&mut state);
-
-        assert_eq!(
-            state,
-            [
-                105, 47, 30, 235, 242, 2, 67, 197, 163, 64, 75, 125, 34, 120, 40, 134,
-            ],
-        );
-    }
-
     /// Test that random bytes are sampled consecutively.
     #[test]
     fn random_bits_4_bytes_sequential() {
@@ -287,8 +297,9 @@ mod tests {
             let mut rng = get_four_sequential_rng();
             let mut first = U1024::ZERO;
             let mut second = U1024::ZERO;
-            random_bits_core(&mut rng, &mut first, bit_length).expect("safe");
-            random_bits_core(&mut rng, &mut second, U1024::BITS - bit_length).expect("safe");
+            random_bits_core(&mut rng, first.as_mut_limbs(), bit_length).expect("safe");
+            random_bits_core(&mut rng, second.as_mut_limbs(), U1024::BITS - bit_length)
+                .expect("safe");
             assert_eq!(second.shl(bit_length).bitor(&first), RANDOM_OUTPUT);
         }
     }

tests/monty_form.rs

@@ -52,7 +52,7 @@ where
     T: Unsigned + Bounded + Encoding,
     <T as Unsigned>::Monty: Invert<Output = CtOption<T::Monty>>,
 {
-    let r = T::from_be_bytes(bytes.clone());
+    let r = T::from_be_bytes(bytes);
     let rm = <T as Unsigned>::Monty::new(r.clone(), monty_params);
     let rm_inv = rm.invert();
     prop_assume!(bool::from(rm_inv.is_some()), "r={:?} is not invertible", r);