cleanup unwraps

Author: baloo

Cargo.lock

@@ -3,7 +3,7 @@
 //!
 
 use crate::{size::KeySize, two};
-use crypto_bigint::{BoxedUint, NonZero};
+use crypto_bigint::{BoxedUint, NonZero, Odd};
 use pkcs8::der::{
     self, DecodeValue, Encode, EncodeValue, Header, Length, Reader, Sequence, Tag, Writer,
     asn1::UintRef,
@@ -17,7 +17,7 @@ use signature::rand_core::CryptoRng;
 #[must_use]
 pub struct Components {
     /// Prime p
-    p: NonZero<BoxedUint>,
+    p: Odd<BoxedUint>,
 
     /// Quotient q
     q: NonZero<BoxedUint>,
@@ -31,11 +31,11 @@ pub struct Components {
 impl Components {
     /// Construct the common components container from its inner values (p, q and g)
     pub fn from_components(
-        p: NonZero<BoxedUint>,
+        p: Odd<BoxedUint>,
         q: NonZero<BoxedUint>,
         g: NonZero<BoxedUint>,
     ) -> signature::Result<Self> {
-        if *p < two() || *q < two() || g > p {
+        if *p < two() || *q < two() || *g > *p {
             return Err(signature::Error::new());
         }
 
@@ -59,7 +59,7 @@ impl Components {
 
     /// DSA prime p
     #[must_use]
-    pub const fn p(&self) -> &NonZero<BoxedUint> {
+    pub const fn p(&self) -> &Odd<BoxedUint> {
         &self.p
     }
 
@@ -88,7 +88,7 @@ impl<'a> DecodeValue<'a> for Components {
         let q = BoxedUint::from_be_slice(q.as_bytes(), (q.as_bytes().len() * 8) as u32).unwrap();
         let g = BoxedUint::from_be_slice(g.as_bytes(), (g.as_bytes().len() * 8) as u32).unwrap();
 
-        let p = NonZero::new(p).unwrap();
+        let p = Odd::new(p).unwrap();
         let q = NonZero::new(q).unwrap();
         let g = NonZero::new(g).unwrap();
 

dsa/src/components.rs

@@ -1,4 +1,4 @@
-use crypto_bigint::BoxedUint;
+use crypto_bigint::{BoxedUint, NonZero};
 use signature::rand_core::CryptoRng;
 
 mod components;
@@ -13,10 +13,13 @@ pub use self::keypair::keypair;
 
 /// Calculate the upper and lower bounds for generating values like p or q
 #[inline]
-fn calculate_bounds(size: u32) -> (BoxedUint, BoxedUint) {
+fn calculate_bounds(size: u32) -> (NonZero<BoxedUint>, NonZero<BoxedUint>) {
     let lower = BoxedUint::one().widen(size + 1).shl(size - 1);
     let upper = BoxedUint::one().widen(size + 1).shl(size);
 
+    let lower = NonZero::new(lower).expect("[bug] shl can't go backward");
+    let upper = NonZero::new(upper).expect("[bug] shl can't go backward");
+
     (lower, upper)
 }
 

dsa/src/generate.rs

@@ -11,6 +11,7 @@ use crate::{
 use crypto_bigint::{
     BoxedUint, NonZero, Odd, RandomBits,
     modular::{BoxedMontyForm, BoxedMontyParams},
+    subtle::CtOption,
 };
 use signature::rand_core::CryptoRng;
 
@@ -22,73 +23,74 @@ use signature::rand_core::CryptoRng;
 pub fn common<R: CryptoRng + ?Sized>(
     rng: &mut R,
     KeySize { l, n }: KeySize,
-) -> (NonZero<BoxedUint>, NonZero<BoxedUint>, NonZero<BoxedUint>) {
+) -> (Odd<BoxedUint>, NonZero<BoxedUint>, NonZero<BoxedUint>) {
     // Calculate the lower and upper bounds of p and q
     let (p_min, p_max) = calculate_bounds(l);
-    let (q_min, q_max) = calculate_bounds(n);
+    let (q_min, q_max): (NonZero<_>, _) = calculate_bounds(n);
 
-    let (p, q) = 'gen_pq: loop {
+    let (p, q): (Odd<_>, _) = 'gen_pq: loop {
         let q = generate_prime(n, rng);
-        if q < q_min || q > q_max {
+        if q < *q_min || q > *q_max {
             continue;
         }
-        let q = NonZero::new(q).unwrap();
+        let q = NonZero::new(q)
+            .expect("[bug] invariant violation, q is above q_min which itself is NonZero");
 
         // Attempt to find a prime p which has a subgroup of the order q
         for _ in 0..4096 {
             let m = 'gen_m: loop {
                 let m = BoxedUint::random_bits(rng, l);
 
-                if m > p_min && m < p_max {
+                if m > *p_min && m < *p_max {
                     break 'gen_m m;
                 }
             };
-            let rem = NonZero::new((two() * &*q).widen(m.bits_precision())).unwrap();
+            let rem = NonZero::new((two() * &*q).widen(m.bits_precision()))
+                .expect("[bug] 2 * NonZero can't be zero");
 
             let mr = &m % &rem;
             let p = m - mr + BoxedUint::one();
-            let p = NonZero::new(p).unwrap();
 
-            if crypto_primes::is_prime_with_rng(rng, &*p) {
+            if crypto_primes::is_prime_with_rng(rng, &p) {
+                let p = Odd::new(p).expect("[bug] Any even number would be prime. P is at least 2^L and L is at least 1024.");
                 break 'gen_pq (p, q);
             }
         }
     };
 
+    // Q needs to be the same precision as P for the operations below.
     let q = q.widen(l);
 
     // Generate g using the unverifiable method as defined by Appendix A.2.1
     let e = (&*p - &BoxedUint::one()) / &q;
-    let mut h = BoxedUint::one().widen(q.bits_precision());
+    let mut h = BoxedUint::one().widen(l);
     let g = loop {
-        let params = BoxedMontyParams::new_vartime(Odd::new((*p).clone()).unwrap());
+        let params = BoxedMontyParams::new_vartime(p.clone());
         let form = BoxedMontyForm::new(h.clone(), params);
         let g = form.pow(&e).retrieve();
 
         if !bool::from(g.is_one()) {
+            // TODO(baloo): shouldn't we check e can't be 1 here?
+            //              and g could still be zero right? In which case just loop around?
             break NonZero::new(g).unwrap();
         }
 
-        // https://github.com/RustCrypto/crypto-bigint/issues/784
-        #[allow(clippy::assign_op_pattern)]
-        {
-            h = h + BoxedUint::one();
-        }
+        h += BoxedUint::one();
     };
 
-    let q = NonZero::new(q.shorten(n)).unwrap();
+    let q = NonZero::new(q.shorten(n)).expect("[bug] q_min(2^N-1) < q < q_max(2^N), Q is non zero");
 
     (p, q, g)
 }
 
 /// Calculate the public component from the common components and the private component
 #[inline]
-pub fn public(components: &Components, x: &NonZero<BoxedUint>) -> NonZero<BoxedUint> {
+pub fn public(components: &Components, x: &NonZero<BoxedUint>) -> CtOption<NonZero<BoxedUint>> {
     let p = components.p();
     let g = components.g();
 
-    let params = BoxedMontyParams::new_vartime(Odd::new((**p).clone()).unwrap());
+    let params = BoxedMontyParams::new_vartime(p.clone());
     let form = BoxedMontyForm::new((**g).clone(), params);
 
-    NonZero::new(form.pow(x).retrieve()).unwrap()
+    NonZero::new(form.pow(x).retrieve())
 }

dsa/src/generate/components.rs

@@ -10,15 +10,19 @@ use signature::rand_core::CryptoRng;
 /// Generate a new keypair
 #[inline]
 pub fn keypair<R: CryptoRng + ?Sized>(rng: &mut R, components: Components) -> SigningKey {
-    let x = loop {
-        let x = BoxedUint::random_mod(rng, components.q());
-        if let Some(x) = NonZero::new(x).into() {
-            break x;
+    let (x, y) = loop {
+        let x = 'gen_x: loop {
+            let x = BoxedUint::random_mod(rng, components.q());
+            if let Some(x) = NonZero::new(x).into() {
+                break 'gen_x x;
+            }
+        };
+
+        if let Some(y) = components::public(&components, &x).into_option() {
+            break (x, y);
         }
     };
 
-    let y = components::public(&components, &x);
-
     VerifyingKey::from_components(components, y)
         .and_then(|verifying_key| SigningKey::from_components(verifying_key, x))
         .expect("[Bug] Newly generated keypair considered invalid")

dsa/src/generate/keypair.rs

@@ -20,16 +20,19 @@ fn strip_leading_zeros(buffer: &[u8], desired_size: usize) -> &[u8] {
 ///
 /// Secret number k and its modular multiplicative inverse with q
 #[inline]
-pub fn secret_number_rfc6979<D>(signing_key: &SigningKey, hash: &[u8]) -> (BoxedUint, BoxedUint)
+pub fn secret_number_rfc6979<D>(
+    signing_key: &SigningKey,
+    hash: &[u8],
+) -> Result<(BoxedUint, BoxedUint), signature::Error>
 where
     D: Digest + BlockSizeUser + FixedOutputReset,
 {
     let q = signing_key.verifying_key().components().q();
     let size = (q.bits() / 8) as usize;
+    let hash = BoxedUint::from_be_slice(&hash[..min(size, hash.len())], q.bits_precision())
+        .map_err(|_| signature::Error::new())?;
 
     // Reduce hash mod q
-    let hash =
-        BoxedUint::from_be_slice(&hash[..min(size, hash.len())], q.bits_precision()).unwrap();
     let hash = (hash % q).to_be_bytes();
     let hash = strip_leading_zeros(&hash, size);
 
@@ -43,11 +46,11 @@ where
     loop {
         rfc6979::generate_k_mut::<D>(x_bytes, q_bytes, hash, &[], &mut buffer);
 
-        let k = BoxedUint::from_be_slice(&buffer, q.bits_precision()).unwrap();
+        let k = BoxedUint::from_be_slice(&buffer, q.bits_precision())
+            .map_err(|_| signature::Error::new())?;
         if let Some(inv_k) = k.inv_mod(q).into() {
             if (bool::from(k.is_nonzero())) & (k < **q) {
-                //debug_assert!(bool::from(k.is_odd()));
-                return (k, inv_k);
+                return Ok((k, inv_k));
             }
         }
     }
@@ -62,7 +65,7 @@ where
 pub fn secret_number<R: TryCryptoRng + ?Sized>(
     rng: &mut R,
     components: &Components,
-) -> Option<(BoxedUint, BoxedUint)> {
+) -> Result<Option<(BoxedUint, BoxedUint)>, signature::Error> {
     let q = components.q();
     let n = q.bits();
     let q = q.widen(n + 64);
@@ -71,17 +74,18 @@ pub fn secret_number<R: TryCryptoRng + ?Sized>(
     // Attempt to try a fitting secret number
     // Give up after 4096 tries
     for _ in 0..4096 {
-        let c = BoxedUint::try_random_bits(rng, n + 64).unwrap();
-        let rem = NonZero::new((&**q - &BoxedUint::one()).widen(c.bits_precision())).unwrap();
+        let c = BoxedUint::try_random_bits(rng, n + 64).map_err(|_| signature::Error::new())?;
+        let rem = NonZero::new((&**q - &BoxedUint::one()).widen(c.bits_precision()))
+            .expect("[bug] minimum size for q is to 2^(160 - 1)");
         let k = (c % rem) + BoxedUint::one();
 
         if let Some(inv_k) = k.inv_mod(q).into() {
             // `k` and `k^-1` both have to be in the range `[1, q-1]`
             if (inv_k > BoxedUint::zero() && inv_k < **q) && (k > BoxedUint::zero() && k < **q) {
-                return Some((k, inv_k));
+                return Ok(Some((k, inv_k)));
             }
         }
     }
 
-    None
+    Ok(None)
 }

dsa/src/generate/secret_number.rs

@@ -26,10 +26,10 @@
 #![cfg_attr(feature = "hazmat", doc = "```")]
 #![cfg_attr(not(feature = "hazmat"), doc = "```ignore")]
 //! # use dsa::{Components, SigningKey, VerifyingKey};
-//! # use crypto_bigint::{BoxedUint, NonZero};
+//! # use crypto_bigint::{BoxedUint, NonZero, Odd};
 //! # let read_common_parameters = ||
 //! #     (
-//! #          NonZero::new(BoxedUint::one()).unwrap(),
+//! #          Odd::new(BoxedUint::one()).unwrap(),
 //! #          NonZero::new(BoxedUint::one()).unwrap(),
 //! #          NonZero::new(BoxedUint::one()).unwrap(),
 //! #     );
@@ -77,8 +77,8 @@ pub const OID: ObjectIdentifier = ObjectIdentifier::new_unwrap("1.2.840.10040.4.
 
 use alloc::{boxed::Box, vec::Vec};
 use pkcs8::der::{
-    self, Decode, DecodeValue, Encode, EncodeValue, Header, Length, Reader, Sequence, Writer,
-    asn1::UintRef,
+    self, Decode, DecodeValue, Encode, EncodeValue, FixedTag, Header, Length, Reader, Sequence,
+    Writer, asn1::UintRef,
 };
 use signature::SignatureEncoding;
 
@@ -120,11 +120,17 @@ impl<'a> DecodeValue<'a> for Signature {
             let r = UintRef::decode(reader)?;
             let s = UintRef::decode(reader)?;
 
-            let r = BoxedUint::from_be_slice(r.as_bytes(), r.as_bytes().len() as u32 * 8).unwrap();
-            let s = BoxedUint::from_be_slice(s.as_bytes(), s.as_bytes().len() as u32 * 8).unwrap();
-
-            let r = NonZero::new(r).unwrap();
-            let s = NonZero::new(s).unwrap();
+            let r = BoxedUint::from_be_slice(r.as_bytes(), r.as_bytes().len() as u32 * 8)
+                .map_err(|_| UintRef::TAG.value_error())?;
+            let s = BoxedUint::from_be_slice(s.as_bytes(), s.as_bytes().len() as u32 * 8)
+                .map_err(|_| UintRef::TAG.value_error())?;
+
+            let r = NonZero::new(r)
+                .into_option()
+                .ok_or(UintRef::TAG.value_error())?;
+            let s = NonZero::new(s)
+                .into_option()
+                .ok_or(UintRef::TAG.value_error())?;
 
             Ok(Self::from_components(r, s))
         })