Merge pull request #26 from fjarri/ncc-audit

NCC audit fixes

Author: fjarri

CHANGELOG.md

@@ -4,6 +4,17 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 
+## [0.3.1] - Unreleased
+
+### Fixed
+
+- `Sieve::new()` now panics when `max_bit_length == 0` (which would lead to incorrect results anyway, so it is not considered a breaking change). ([#26])
+- Default preset now uses A* instead of A base selection method for the Lucas test. This does not change the outcomes, but is implemented as a security recommendation. ([#26])
+
+
+[#26]: https://github.com/nucypher/rust-umbral/pull/26
+
+
 ## [0.3.0] - 2023-05-05
 
 ### Changed

README.md

@@ -10,7 +10,7 @@ This library implements prime number generation and primality checking for [`cry
 In particular:
 
 - Generating random primes and safe primes of given bit size;
-- Sieving iterator and one-time trial division by small integers;
+- Sieving iterator;
 - Miller-Rabin test;
 - Strong and extra strong Lucas tests, and Lucas-V test.
 

src/hazmat/jacobi.rs

@@ -77,10 +77,6 @@ pub(crate) fn jacobi_symbol<const L: usize>(a: i32, p_long: &Uint<L>) -> JacobiS
     if p_long.is_even().into() {
         panic!("`p_long` must be an odd integer, but got {}", p_long);
     }
-    if a == i32::MIN {
-        // Otherwise it will cause an overflow when taking its absolute value.
-        panic!("`a` must be greater than `i32::MIN`");
-    }
 
     let result = JacobiSymbol::One; // Keep track of all the sign flips here.
 
@@ -185,12 +181,6 @@ mod tests {
         let _j = jacobi_symbol(1, &U128::from(4u32));
     }
 
-    #[test]
-    #[should_panic(expected = "`a` must be greater than `i32::MIN`")]
-    fn jacobi_symbol_a_is_too_small() {
-        let _j = jacobi_symbol(i32::MIN, &U128::from(5u32));
-    }
-
     // Reference from `num-modular` - supports long `p`, but only positive `a`.
     fn jacobi_symbol_ref(a: i32, p: &U128) -> JacobiSymbol {
         let a_bi = BigInt::from(a);
@@ -224,11 +214,18 @@ mod tests {
         let a = 2147483647i32; // 2^31 - 1, a prime
         let p = U128::from_be_hex("000000007ffffffeffffffe28000003b"); // (2^31 - 1) * (2^64 - 59)
         assert_eq!(jacobi_symbol(a, &p), JacobiSymbol::Zero);
+        assert_eq!(jacobi_symbol_ref(a, &p), JacobiSymbol::Zero);
 
         // a = x^2 mod p, should give 1.
         let a = 659456i32; // Obtained from x = 2^70
         let p = U128::from_be_hex("ffffffffffffffffffffffffffffff5f"); // 2^128 - 161 - not a prime
         assert_eq!(jacobi_symbol(a, &p), JacobiSymbol::One);
+        assert_eq!(jacobi_symbol_ref(a, &p), JacobiSymbol::One);
+
+        let a = i32::MIN; // -2^31, check that no overflow occurs
+        let p = U128::from_be_hex("000000007ffffffeffffffe28000003b"); // (2^31 - 1) * (2^64 - 59)
+        assert_eq!(jacobi_symbol(a, &p), JacobiSymbol::One);
+        assert_eq!(jacobi_symbol_ref(a, &p), JacobiSymbol::One);
     }
 
     prop_compose! {

src/hazmat/lucas.rs

@@ -1,7 +1,7 @@
 //! Lucas primality test.
 use crypto_bigint::{
     modular::runtime_mod::{DynResidue, DynResidueParams},
-    Integer, Limb, Uint, Word,
+    CheckedAdd, Integer, Limb, Uint, Word,
 };
 
 use super::{
@@ -21,7 +21,8 @@ const MAX_ATTEMPTS: u32 = 10000;
 // (~30x for 1024-bit numbers, ~100x for 2048 bit).
 // On the other hand, if `n` is a non-square we expect to find a `D`
 // in just a few attempts on average (an estimate for the Selfridge method
-// can be found in [^1], section 7; for the brute force method it seems to be about the same).
+// can be found in [^Baillie1980], section 7; for the brute force method
+// it seems to be about the same).
 const ATTEMPTS_BEFORE_SQRT: u32 = 30;
 
 /// A method for selecting the base `(P, Q)` for the Lucas primality test.
@@ -186,7 +187,7 @@ fn decompose<const L: usize>(n: &Uint<L>) -> (u32, Uint<L>) {
     }
 
     // This won't overflow since the original `n` was odd, so we right-shifted at least once.
-    (s, n.wrapping_add(&Uint::<L>::ONE))
+    (s, n.checked_add(&Uint::<L>::ONE).expect("Integer overflow"))
 }
 
 /// The checks to perform in the Lucas test.
@@ -349,7 +350,7 @@ pub fn lucas_test<const L: usize>(
     // Compute d-th element of Lucas sequence (U_d(P, Q), V_d(P, Q)), where:
     //
     // V_0 = 2
-    // U_0 = 1
+    // U_0 = 0
     //
     // U_{2k} = U_k V_k
     // V_{2k} = V_k^2 - 2 Q^k

src/hazmat/miller_rabin.rs

@@ -4,7 +4,7 @@ use rand_core::CryptoRngCore;
 
 use crypto_bigint::{
     modular::runtime_mod::{DynResidue, DynResidueParams},
-    Integer, NonZero, RandomMod, Uint,
+    CheckedAdd, Integer, NonZero, RandomMod, Uint,
 };
 
 use super::Primality;
@@ -104,8 +104,11 @@ impl<const L: usize> MillerRabin<L> {
 
         let range = self.candidate.wrapping_sub(&Uint::<L>::from(4u32));
         let range_nonzero = NonZero::new(range).unwrap();
-        let random =
-            Uint::<L>::random_mod(rng, &range_nonzero).wrapping_add(&Uint::<L>::from(3u32));
+        // This should not overflow as long as `random_mod()` behaves according to the contract
+        // (that is, returns a number within the given range).
+        let random = Uint::<L>::random_mod(rng, &range_nonzero)
+            .checked_add(&Uint::<L>::from(3u32))
+            .expect("Integer overflow");
         self.test(&random)
     }
 }

src/hazmat/sieve.rs

@@ -3,7 +3,7 @@
 
 use alloc::{vec, vec::Vec};
 
-use crypto_bigint::{Random, Uint};
+use crypto_bigint::{CheckedAdd, Random, Uint};
 use rand_core::CryptoRngCore;
 
 use crate::hazmat::precomputed::{SmallPrime, RECIPROCALS, SMALL_PRIMES};
@@ -75,10 +75,14 @@ impl<const L: usize> Sieve<L> {
     /// Note that `start` is adjusted to `2`, or the next `1 mod 2` number (`safe_primes = false`);
     /// and `5`, or `3 mod 4` number (`safe_primes = true`).
     ///
-    /// Panics if `max_bit_length` is greater than the size of the target `Uint`.
+    /// Panics if `max_bit_length` is zero or greater than the size of the target `Uint`.
     ///
     /// If `safe_primes` is `true`, both the returned `n` and `n/2` are sieved.
     pub fn new(start: &Uint<L>, max_bit_length: usize, safe_primes: bool) -> Self {
+        if max_bit_length == 0 {
+            panic!("The requested bit length cannot be zero");
+        }
+
         if max_bit_length > Uint::<L>::BITS {
             panic!(
                 "The requested bit length ({}) is larger than the chosen Uint size",
@@ -148,7 +152,13 @@ impl<const L: usize> Sieve<L> {
         }
 
         // Set the new base.
-        self.base = self.base.wrapping_add(&self.incr.into());
+        // Should not overflow since `incr` is never greater than `incr_limit`,
+        // and the latter is chosen such that it doesn't overflow when added to `base`
+        // (see the rest of this method).
+        self.base = self
+            .base
+            .checked_add(&self.incr.into())
+            .expect("Integer overflow");
 
         self.incr = 0;
 
@@ -206,7 +216,13 @@ impl<const L: usize> Sieve<L> {
         let result = if self.current_is_composite() {
             None
         } else {
-            let mut num = self.base.wrapping_add(&self.incr.into());
+            // The overflow should never happen here since `incr`
+            // is never greater than `incr_limit`, and the latter is chosen such that
+            // it does not overflow when added to `base` (see `update_residues()`).
+            let mut num = self
+                .base
+                .checked_add(&self.incr.into())
+                .expect("Integer overflow");
             if self.safe_primes {
                 num = (num << 1) | Uint::<L>::ONE;
             }
@@ -333,6 +349,12 @@ mod tests {
         check_sieve(13, 4, true, &[]);
     }
 
+    #[test]
+    #[should_panic(expected = "The requested bit length cannot be zero")]
+    fn sieve_zero_bits() {
+        let _sieve = Sieve::new(&U64::ONE, 0, false);
+    }
+
     #[test]
     #[should_panic(expected = "The requested bit length (65) is larger than the chosen Uint size")]
     fn sieve_too_many_bits() {

src/presets.rs

@@ -5,7 +5,7 @@ use rand_core::CryptoRngCore;
 use rand_core::OsRng;
 
 use crate::hazmat::{
-    lucas_test, random_odd_uint, LucasCheck, MillerRabin, Primality, SelfridgeBase, Sieve,
+    lucas_test, random_odd_uint, AStarBase, LucasCheck, MillerRabin, Primality, Sieve,
 };
 
 /// Returns a random prime of size `bit_length` using [`OsRng`] as the RNG.
@@ -100,7 +100,7 @@ pub fn generate_safe_prime_with_rng<const L: usize>(
 ///
 /// Performed checks:
 /// - Miller-Rabin check with base 2;
-/// - Strong Lucas check with Selfridge base (a.k.a. Baillie method A);
+/// - Strong Lucas check with A* base (see [`AStarBase`] for details);
 /// - Miller-Rabin check with a random base.
 ///
 /// See [`MillerRabin`] and [`lucas_test`] for more details about the checks.
@@ -158,7 +158,7 @@ fn _is_prime_with_rng<const L: usize>(rng: &mut impl CryptoRngCore, num: &Uint<L
         return false;
     }
 
-    match lucas_test(num, SelfridgeBase, LucasCheck::Strong) {
+    match lucas_test(num, AStarBase, LucasCheck::Strong) {
         Primality::Composite => return false,
         Primality::Prime => return true,
         _ => {}