fix(errors): Handle randomness generation and invalid random values as errors in cryptographic code (#6385)

* add error handling

* change error name

* Error types oriented around the primary types we expose in the zebra-chain API

* Fix Ok spelling

* orchard::note::new(): return NoteError if randomness produces invalid Pallas point

---------

Co-authored-by: Deirdre Connolly <durumcrustulum@gmail.com>
Co-authored-by: teor <teor@riseup.net>

zebra-chain/src/error.rs

@@ -0,0 +1,53 @@
+//! Errors that can occur inside any `zebra-chain` submodule.
+use thiserror::Error;
+
+/// Errors related to random bytes generation.
+#[derive(Error, Copy, Clone, Debug, PartialEq, Eq)]
+pub enum RandError {
+    /// Error of the `try_fill_bytes` function.
+    #[error("failed to generate a secure stream of random bytes")]
+    FillBytes,
+}
+
+/// An error type pertaining to shielded notes.
+#[derive(Error, Copy, Clone, Debug, PartialEq, Eq)]
+pub enum NoteError {
+    /// Errors of type `RandError`.
+    #[error("Randomness generation failure")]
+    InsufficientRandomness(#[from] RandError),
+    /// Error of `pallas::Point::from_bytes()` for new rho randomness.
+    #[error("failed to generate an Orchard note's rho.")]
+    InvalidRho,
+}
+
+/// An error type pertaining to note commitments.
+#[derive(Error, Copy, Clone, Debug, PartialEq, Eq)]
+pub enum NoteCommitmentError {
+    /// Errors of type `RandError`.
+    #[error("Randomness generation failure")]
+    InsufficientRandomness(#[from] RandError),
+    /// Error of `jubjub::AffinePoint::try_from`.
+    #[error("failed to generate a sapling::NoteCommitment from a diversifier")]
+    InvalidDiversifier,
+}
+
+/// An error type pertaining to key generation, parsing, modification,
+/// randomization.
+#[derive(Error, Copy, Clone, Debug, PartialEq, Eq)]
+pub enum KeyError {
+    /// Errors of type `RandError`.
+    #[error("Randomness generation failure")]
+    InsufficientRandomness(#[from] RandError),
+}
+
+/// An error type pertaining to payment address generation, parsing,
+/// modification, diversification.
+#[derive(Error, Copy, Clone, Debug, PartialEq, Eq)]
+pub enum AddressError {
+    /// Errors of type `RandError`.
+    #[error("Randomness generation failure")]
+    InsufficientRandomness(#[from] RandError),
+    /// Errors pertaining to diversifier generation.
+    #[error("Randomness did not hash into the Jubjub group for producing a new diversifier")]
+    DiversifierGenerationFailure,
+}

zebra-chain/src/lib.rs

@@ -23,6 +23,7 @@ pub mod block;
 pub mod chain_sync_status;
 pub mod chain_tip;
 pub mod diagnostic;
+pub mod error;
 pub mod fmt;
 pub mod history_tree;
 pub mod orchard;

zebra-chain/src/orchard/commitment.rs

@@ -16,6 +16,7 @@ use rand_core::{CryptoRng, RngCore};
 
 use crate::{
     amount::Amount,
+    error::RandError,
     serialization::{
         serde_helpers, ReadZcashExt, SerializationError, ZcashDeserialize, ZcashSerialize,
     },
@@ -26,14 +27,16 @@ use super::sinsemilla::*;
 /// Generates a random scalar from the scalar field 𝔽_{q_P}.
 ///
 /// <https://zips.z.cash/protocol/nu5.pdf#pallasandvesta>
-pub fn generate_trapdoor<T>(csprng: &mut T) -> pallas::Scalar
+pub fn generate_trapdoor<T>(csprng: &mut T) -> Result<pallas::Scalar, RandError>
 where
     T: RngCore + CryptoRng,
 {
     let mut bytes = [0u8; 64];
-    csprng.fill_bytes(&mut bytes);
+    csprng
-    // pallas::Scalar::from_bytes_wide() reduces the input modulo q_P under the hood.
+        .try_fill_bytes(&mut bytes)
-    pallas::Scalar::from_uniform_bytes(&bytes)
+        .map_err(|_| RandError::FillBytes)?;
+    // pallas::Scalar::from_uniform_bytes() reduces the input modulo q_P under the hood.
+    Ok(pallas::Scalar::from_uniform_bytes(&bytes))
 }
 
 /// The randomness used in the Simsemilla hash for note commitment.
@@ -227,13 +230,13 @@ impl ValueCommitment {
     /// Generate a new _ValueCommitment_.
     ///
     /// <https://zips.z.cash/protocol/nu5.pdf#concretehomomorphiccommit>
-    pub fn randomized<T>(csprng: &mut T, value: Amount) -> Self
+    pub fn randomized<T>(csprng: &mut T, value: Amount) -> Result<Self, RandError>
     where
         T: RngCore + CryptoRng,
     {
-        let rcv = generate_trapdoor(csprng);
+        let rcv = generate_trapdoor(csprng)?;
 
-        Self::new(rcv, value)
+        Ok(Self::new(rcv, value))
     }
 
     /// Generate a new `ValueCommitment` from an existing `rcv on a `value`.

zebra-chain/src/orchard/keys.rs

@@ -13,8 +13,11 @@ use halo2::{
 };
 use rand_core::{CryptoRng, RngCore};
 
-use crate::serialization::{
+use crate::{
-    serde_helpers, ReadZcashExt, SerializationError, ZcashDeserialize, ZcashSerialize,
+    error::RandError,
+    serialization::{
+        serde_helpers, ReadZcashExt, SerializationError, ZcashDeserialize, ZcashSerialize,
+    },
 };
 
 use super::sinsemilla::*;
@@ -102,14 +105,16 @@ impl Diversifier {
     /// Generate a new `Diversifier`.
     ///
     /// <https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents>
-    pub fn new<T>(csprng: &mut T) -> Self
+    pub fn new<T>(csprng: &mut T) -> Result<Self, RandError>
     where
         T: RngCore + CryptoRng,
     {
         let mut bytes = [0u8; 11];
-        csprng.fill_bytes(&mut bytes);
+        csprng
+            .try_fill_bytes(&mut bytes)
+            .map_err(|_| RandError::FillBytes)?;
 
-        Self::from(bytes)
+        Ok(Self::from(bytes))
     }
 }
 

zebra-chain/src/orchard/note.rs

@@ -4,7 +4,10 @@ use group::{ff::PrimeField, GroupEncoding};
 use halo2::pasta::pallas;
 use rand_core::{CryptoRng, RngCore};
 
-use crate::amount::{Amount, NonNegative};
+use crate::{
+    amount::{Amount, NonNegative},
+    error::{NoteError, RandError},
+};
 
 use super::{address::Address, sinsemilla::extract_p};
 
@@ -22,13 +25,15 @@ mod arbitrary;
 pub struct SeedRandomness(pub(crate) [u8; 32]);
 
 impl SeedRandomness {
-    pub fn new<T>(csprng: &mut T) -> Self
+    pub fn new<T>(csprng: &mut T) -> Result<Self, RandError>
     where
         T: RngCore + CryptoRng,
     {
         let mut bytes = [0u8; 32];
-        csprng.fill_bytes(&mut bytes);
+        csprng
-        Self(bytes)
+            .try_fill_bytes(&mut bytes)
+            .map_err(|_| RandError::FillBytes)?;
+        Ok(Self(bytes))
     }
 }
 
@@ -57,14 +62,22 @@ impl From<Nullifier> for Rho {
 }
 
 impl Rho {
-    pub fn new<T>(csprng: &mut T) -> Self
+    pub fn new<T>(csprng: &mut T) -> Result<Self, NoteError>
     where
         T: RngCore + CryptoRng,
     {
         let mut bytes = [0u8; 32];
-        csprng.fill_bytes(&mut bytes);
+        csprng
+            .try_fill_bytes(&mut bytes)
+            .map_err(|_| NoteError::from(RandError::FillBytes))?;
 
-        Self(extract_p(pallas::Point::from_bytes(&bytes).unwrap()))
+        let possible_point = pallas::Point::from_bytes(&bytes);
+
+        if possible_point.is_some().into() {
+            Ok(Self(extract_p(possible_point.unwrap())))
+        } else {
+            Err(NoteError::InvalidRho)
+        }
     }
 }
 
@@ -108,15 +121,15 @@ impl Note {
         address: Address,
         value: Amount<NonNegative>,
         nf_old: Nullifier,
-    ) -> Self
+    ) -> Result<Self, RandError>
     where
         T: RngCore + CryptoRng,
     {
-        Self {
+        Ok(Self {
             address,
             value,
             rho: nf_old.into(),
-            rseed: SeedRandomness::new(csprng),
+            rseed: SeedRandomness::new(csprng)?,
-        }
+        })
     }
 }

zebra-chain/src/sapling/commitment.rs

@@ -12,6 +12,7 @@ use rand_core::{CryptoRng, RngCore};
 
 use crate::{
     amount::{Amount, NonNegative},
+    error::{NoteCommitmentError, RandError},
     serialization::{
         serde_helpers, ReadZcashExt, SerializationError, ZcashDeserialize, ZcashSerialize,
     },
@@ -34,14 +35,16 @@ use pedersen_hashes::*;
 /// trapdoor generators.
 ///
 /// <https://zips.z.cash/protocol/protocol.pdf#jubjub>
-pub fn generate_trapdoor<T>(csprng: &mut T) -> jubjub::Fr
+pub fn generate_trapdoor<T>(csprng: &mut T) -> Result<jubjub::Fr, RandError>
 where
     T: RngCore + CryptoRng,
 {
     let mut bytes = [0u8; 64];
-    csprng.fill_bytes(&mut bytes);
+    csprng
+        .try_fill_bytes(&mut bytes)
+        .map_err(|_| RandError::FillBytes)?;
     // Fr::from_bytes_wide() reduces the input modulo r via Fr::from_u512()
-    jubjub::Fr::from_bytes_wide(&bytes)
+    Ok(jubjub::Fr::from_bytes_wide(&bytes))
 }
 
 /// The randomness used in the Pedersen Hash for note commitment.
@@ -104,7 +107,7 @@ impl NoteCommitment {
         diversifier: Diversifier,
         transmission_key: TransmissionKey,
         value: Amount<NonNegative>,
-    ) -> Option<(CommitmentRandomness, Self)>
+    ) -> Result<(CommitmentRandomness, Self), NoteCommitmentError>
     where
         T: RngCore + CryptoRng,
     {
@@ -120,11 +123,9 @@ impl NoteCommitment {
         // The `TryFrom<Diversifier>` impls for the `jubjub::*Point`s handles
         // calling `DiversifyHash` implicitly.
 
-        let g_d_bytes: [u8; 32] = if let Ok(g_d) = jubjub::AffinePoint::try_from(diversifier) {
+        let g_d_bytes = jubjub::AffinePoint::try_from(diversifier)
-            g_d.to_bytes()
+            .map_err(|_| NoteCommitmentError::InvalidDiversifier)?
-        } else {
+            .to_bytes();
-            return None;
-        };
 
         let pk_d_bytes = <[u8; 32]>::from(transmission_key);
         let v_bytes = value.to_bytes();
@@ -133,9 +134,9 @@ impl NoteCommitment {
         s.extend(pk_d_bytes);
         s.extend(v_bytes);
 
-        let rcm = CommitmentRandomness(generate_trapdoor(csprng));
+        let rcm = CommitmentRandomness(generate_trapdoor(csprng)?);
 
-        Some((
+        Ok((
             rcm,
             NoteCommitment::from(windowed_pedersen_commitment(rcm.0, &s)),
         ))
@@ -265,13 +266,13 @@ impl ValueCommitment {
     /// Generate a new _ValueCommitment_.
     ///
     /// <https://zips.z.cash/protocol/protocol.pdf#concretehomomorphiccommit>
-    pub fn randomized<T>(csprng: &mut T, value: Amount) -> Self
+    pub fn randomized<T>(csprng: &mut T, value: Amount) -> Result<Self, RandError>
     where
         T: RngCore + CryptoRng,
     {
-        let rcv = generate_trapdoor(csprng);
+        let rcv = generate_trapdoor(csprng)?;
 
-        Self::new(rcv, value)
+        Ok(Self::new(rcv, value))
     }
 
     /// Generate a new _ValueCommitment_ from an existing _rcv_ on a _value_.

zebra-chain/src/sapling/keys.rs

@@ -15,6 +15,7 @@ use std::{fmt, io};
 use rand_core::{CryptoRng, RngCore};
 
 use crate::{
+    error::{AddressError, RandError},
     primitives::redjubjub::{self, SpendAuth},
     serialization::{
         serde_helpers, ReadZcashExt, SerializationError, ZcashDeserialize, ZcashSerialize,
@@ -180,18 +181,24 @@ impl Diversifier {
     ///
     /// <https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents>
     /// <https://zips.z.cash/protocol/protocol.pdf#concretediversifyhash>
-    pub fn new<T>(csprng: &mut T) -> Self
+    pub fn new<T>(csprng: &mut T) -> Result<Self, AddressError>
     where
         T: RngCore + CryptoRng,
     {
-        loop {
+        /// Number of times a `diversify_hash` will try to obtain a diversified base point.
+        const DIVERSIFY_HASH_TRIES: u8 = 2;
+
+        for _ in 0..DIVERSIFY_HASH_TRIES {
             let mut bytes = [0u8; 11];
-            csprng.fill_bytes(&mut bytes);
+            csprng
+                .try_fill_bytes(&mut bytes)
+                .map_err(|_| AddressError::from(RandError::FillBytes))?;
 
             if diversify_hash(bytes).is_some() {
-                break Self(bytes);
+                return Ok(Self(bytes));
             }
         }
+        Err(AddressError::DiversifierGenerationFailure)
     }
 }