From 6e8e890512a80653f2069f63b88991b6b48cc20f Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 30 Sep 2024 22:15:27 +0200 Subject: [PATCH 1/4] Fix overflow when initializing a Sieve with T::MAX --- src/hazmat/sieve.rs | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/src/hazmat/sieve.rs b/src/hazmat/sieve.rs index 31baf42..c31f528 100644 --- a/src/hazmat/sieve.rs +++ b/src/hazmat/sieve.rs @@ -141,13 +141,13 @@ impl Sieve { } // Set the new base. - // 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("addition should not overflow by construction"); + match self.base.checked_add(&self.incr.into()).into() { + Some(x) => self.base = x, + None => { + self.last_round = true; + return false; + } + } self.incr = 0; @@ -214,17 +214,15 @@ impl Sieve { let result = if self.current_is_composite() { None } else { - // 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("addition should not overflow by construction"); - if self.safe_primes { - num = num.wrapping_shl_vartime(1) | T::one_like(&self.base); + match self.base.checked_add(&self.incr.into()).into_option() { + Some(mut num) => { + if self.safe_primes { + num = num.wrapping_shl_vartime(1) | T::one_like(&self.base); + } + Some(num) + } + None => None, } - Some(num) }; self.incr += 2; @@ -385,4 +383,10 @@ mod tests { assert!(format!("{s:?}").starts_with("Sieve")); assert_eq!(s.clone(), s); } + #[test] + fn sieve_with_max_start() { + let start = U64::MAX; + let mut sieve = Sieve::new(&start, NonZeroU32::new(U64::BITS).unwrap(), false); + assert!(sieve.next().is_none()); + } } From f00c1f9791a2cc58c59e2dd298b6758eb9a4d076 Mon Sep 17 00:00:00 2001 From: David Palm Date: Mon, 30 Sep 2024 23:18:38 +0200 Subject: [PATCH 2/4] Ensure all code paths are taken --- src/hazmat/sieve.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/hazmat/sieve.rs b/src/hazmat/sieve.rs index c31f528..09efbc1 100644 --- a/src/hazmat/sieve.rs +++ b/src/hazmat/sieve.rs @@ -140,7 +140,7 @@ impl Sieve { return false; } - // Set the new base. + // Set the new base. This increment will not overflow unless the `Sieve` is misused and manipulated. match self.base.checked_add(&self.incr.into()).into() { Some(x) => self.base = x, None => { @@ -217,6 +217,7 @@ impl Sieve { match self.base.checked_add(&self.incr.into()).into_option() { Some(mut num) => { if self.safe_primes { + // Divide by 2 and ensure it's odd with an OR. num = num.wrapping_shl_vartime(1) | T::one_like(&self.base); } Some(num) @@ -231,7 +232,6 @@ impl Sieve { fn next(&mut self) -> Option { // Corner cases handled here - if self.produces_nothing { return None; } @@ -268,7 +268,7 @@ mod tests { use alloc::vec::Vec; use core::num::NonZeroU32; - use crypto_bigint::U64; + use crypto_bigint::{U1024, U64}; use num_prime::nt_funcs::factorize64; use rand_chacha::ChaCha8Rng; use rand_core::{OsRng, SeedableRng}; @@ -383,10 +383,19 @@ mod tests { assert!(format!("{s:?}").starts_with("Sieve")); assert_eq!(s.clone(), s); } + #[test] fn sieve_with_max_start() { let start = U64::MAX; let mut sieve = Sieve::new(&start, NonZeroU32::new(U64::BITS).unwrap(), false); assert!(sieve.next().is_none()); } + + #[test] + fn sieve_with_max_start_and_malicious_manipulation() { + let start = U1024::MAX; + let mut sieve = Sieve::new(&start, NonZeroU32::new(U1024::BITS).unwrap(), false); + sieve.incr = 10; // Cause overflow in update_residues() + assert!(sieve.next().is_none()); + } } From d9798c8c7f6ac3bf2871bdf2b27c87e7e8e742aa Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 8 Oct 2024 10:40:11 +0200 Subject: [PATCH 3/4] Remove contentious test --- src/hazmat/sieve.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/hazmat/sieve.rs b/src/hazmat/sieve.rs index 09efbc1..7c13a25 100644 --- a/src/hazmat/sieve.rs +++ b/src/hazmat/sieve.rs @@ -268,7 +268,7 @@ mod tests { use alloc::vec::Vec; use core::num::NonZeroU32; - use crypto_bigint::{U1024, U64}; + use crypto_bigint::U64; use num_prime::nt_funcs::factorize64; use rand_chacha::ChaCha8Rng; use rand_core::{OsRng, SeedableRng}; @@ -390,12 +390,4 @@ mod tests { let mut sieve = Sieve::new(&start, NonZeroU32::new(U64::BITS).unwrap(), false); assert!(sieve.next().is_none()); } - - #[test] - fn sieve_with_max_start_and_malicious_manipulation() { - let start = U1024::MAX; - let mut sieve = Sieve::new(&start, NonZeroU32::new(U1024::BITS).unwrap(), false); - sieve.incr = 10; // Cause overflow in update_residues() - assert!(sieve.next().is_none()); - } } From 20e06b38a497701e782cf454ac42ec630cce5ac1 Mon Sep 17 00:00:00 2001 From: David Palm Date: Tue, 8 Oct 2024 10:45:26 +0200 Subject: [PATCH 4/4] Revert to previous unwrapping --- src/hazmat/sieve.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/hazmat/sieve.rs b/src/hazmat/sieve.rs index 7c13a25..aa3172d 100644 --- a/src/hazmat/sieve.rs +++ b/src/hazmat/sieve.rs @@ -140,14 +140,14 @@ impl Sieve { return false; } - // Set the new base. This increment will not overflow unless the `Sieve` is misused and manipulated. - match self.base.checked_add(&self.incr.into()).into() { - Some(x) => self.base = x, - None => { - self.last_round = true; - return false; - } - } + // Set the new base. + // 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("Does not overflow by construction"); self.incr = 0;