Disallow Orchard ivk = 0 on IncomingViewingKey::from & SpendingKey generation (#3962)

* Disallow Orchard ivk = 0 on IncomingViewingKey::from and SpendingKey generation

* Check that ivk scalar values parsed from bytes are never 0

* Update zebra-chain/src/orchard/keys.rs

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

* Switch away from removed pallas::Scalar::from_bytes to PrimeField::from_repr

* Fix updated Orchard IVK derivation around updated BitVec API

* Remove spurious proptest regressions

* Update zebra-chain/src/orchard/keys.rs

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>

* allow `unwrap_in_result` lint in added code

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>

zebra-chain/src/orchard/address.rs

@@ -47,7 +47,9 @@ mod tests {
         // Default diversifier, where index = 0.
         let diversifier_key = keys::DiversifierKey::from(full_viewing_key);
 
-        let incoming_viewing_key = keys::IncomingViewingKey::from(full_viewing_key);
+        // This should fail with negligible probability.
+        let incoming_viewing_key = keys::IncomingViewingKey::try_from(full_viewing_key)
+            .expect("a valid incoming viewing key");
 
         let diversifier = keys::Diversifier::from(diversifier_key);
         let transmission_key = keys::TransmissionKey::from((incoming_viewing_key, diversifier));

zebra-chain/src/orchard/arbitrary.rs

@@ -140,7 +140,8 @@ impl Arbitrary for keys::TransmissionKey {
                 let diversifier_key = keys::DiversifierKey::from(full_viewing_key);
 
                 let diversifier = Diversifier::from(diversifier_key);
-                let incoming_viewing_key = keys::IncomingViewingKey::from(full_viewing_key);
+                let incoming_viewing_key = keys::IncomingViewingKey::try_from(full_viewing_key)
+                    .expect("a valid incoming viewing key");
 
                 Self::from((incoming_viewing_key, diversifier))
             })

zebra-chain/src/orchard/keys.rs

@@ -18,7 +18,7 @@ use bitvec::prelude::*;
 use fpe::ff1::{BinaryNumeralString, FF1};
 use group::{ff::PrimeField, prime::PrimeCurveAffine, Group, GroupEncoding};
 use halo2::{
-    arithmetic::{Coordinates, CurveAffine, FieldExt},
+    arithmetic::{Coordinates, CurveAffine, Field, FieldExt},
     pasta::pallas,
 };
 use rand_core::{CryptoRng, RngCore};
@@ -205,7 +205,12 @@ impl SpendingKey {
             let sk = Self::from_bytes(bytes, network);
 
             // "if ask = 0, discard this key and repeat with a new sk"
-            if SpendAuthorizingKey::from(sk).0 == pallas::Scalar::zero() {
+            if SpendAuthorizingKey::from(sk).0.is_zero().into() {
+                continue;
+            }
+
+            // "if ivk ∈ {0, ⊥}, discard this key and repeat with a new sk"
+            if IncomingViewingKey::try_from(FullViewingKey::from(sk)).is_err() {
                 continue;
             }
 
@@ -599,7 +604,7 @@ impl PartialEq for FullViewingKey {
 #[derive(Copy, Clone)]
 pub struct IncomingViewingKey {
     dk: DiversifierKey,
-    // TODO: refine type
+    // TODO: refine type, so that IncomingViewingkey.ivk cannot be 0
     ivk: pallas::Scalar,
 }
 
@@ -646,7 +651,46 @@ impl From<IncomingViewingKey> for [u8; 64] {
     }
 }
 
-impl From<FullViewingKey> for IncomingViewingKey {
+impl TryFrom<[u8; 64]> for IncomingViewingKey {
+    type Error = &'static str;
+
+    /// Convert an array of bytes into a [`IncomingViewingKey`].
+    ///
+    /// Returns an error if the encoding is malformed or if it [encodes the scalar additive
+    /// identity, 0][1].
+    ///
+    /// > ivk MUST be in the range {1 .. 𝑞P - 1}
+    ///
+    /// [1]: https://zips.z.cash/protocol/protocol.pdf#orchardinviewingkeyencoding
+    fn try_from(bytes: [u8; 64]) -> Result<Self, Self::Error> {
+        let mut dk_bytes = [0u8; 32];
+        dk_bytes.copy_from_slice(&bytes[..32]);
+        let dk = DiversifierKey::from(dk_bytes);
+
+        let mut ivk_bytes = [0u8; 32];
+        ivk_bytes.copy_from_slice(&bytes[32..]);
+
+        let possible_scalar = pallas::Scalar::from_repr(ivk_bytes);
+
+        if possible_scalar.is_some().into() {
+            let scalar = possible_scalar.unwrap();
+            if scalar.is_zero().into() {
+                Err("pallas::Scalar value for Orchard IncomingViewingKey is 0")
+            } else {
+                Ok(Self {
+                    dk,
+                    ivk: possible_scalar.unwrap(),
+                })
+            }
+        } else {
+            Err("Invalid pallas::Scalar value for Orchard IncomingViewingKey")
+        }
+    }
+}
+
+impl TryFrom<FullViewingKey> for IncomingViewingKey {
+    type Error = &'static str;
+
     /// Commit^ivk_rivk(ak, nk) :=
     ///     SinsemillaShortCommit_rcm(︁
     ///        "z.cash:Orchard-CommitIvk",
@@ -656,7 +700,8 @@ impl From<FullViewingKey> for IncomingViewingKey {
     /// <https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents>
     /// <https://zips.z.cash/protocol/nu5.pdf#concreteprfs>
     #[allow(non_snake_case)]
-    fn from(fvk: FullViewingKey) -> Self {
+    #[allow(clippy::unwrap_in_result)]
+    fn try_from(fvk: FullViewingKey) -> Result<Self, Self::Error> {
         let mut M: BitVec<u8, Lsb0> = BitVec::new();
 
         // I2LEBSP_l^Orchard_base(ak)︁
@@ -678,10 +723,18 @@ impl From<FullViewingKey> for IncomingViewingKey {
         )
         .expect("deriving orchard commit^ivk should not output ⊥ ");
 
-        Self {
+        let ivk_ctoption = pallas::Scalar::from_repr(commit_x.into());
-            dk: fvk.into(),
+
-            // mod r_P
+        // if ivk ∈ {0, ⊥}, discard this key
-            ivk: pallas::Scalar::from_repr(commit_x.into()).unwrap(),
+
+        // [`Scalar::is_zero()`] is constant-time under the hood, and ivk is mod r_P
+        if ivk_ctoption.is_some().into() && !<bool>::from(ivk_ctoption.unwrap().is_zero()) {
+            Ok(Self {
+                dk: fvk.into(),
+                ivk: ivk_ctoption.unwrap(),
+            })
+        } else {
+            Err("generated ivk is the additive identity 0, invalid")
         }
     }
 }
@@ -808,6 +861,12 @@ impl From<FullViewingKey> for DiversifierKey {
     }
 }
 
+impl From<[u8; 32]> for DiversifierKey {
+    fn from(bytes: [u8; 32]) -> DiversifierKey {
+        DiversifierKey(bytes)
+    }
+}
+
 impl From<DiversifierKey> for [u8; 32] {
     fn from(dk: DiversifierKey) -> [u8; 32] {
         dk.0
@@ -1006,10 +1065,11 @@ impl From<&OutgoingCipherKey> for [u8; 32] {
 
 // TODO: implement PrivateKey: #2192
 
-/// An ephemeral private key for Orchard key agreement.
+/// An _ephemeral private key_ for Orchard key agreement.
 ///
 /// <https://zips.z.cash/protocol/nu5.pdf#concreteorchardkeyagreement>
 /// <https://zips.z.cash/protocol/nu5.pdf#saplingandorchardencrypt>
+// TODO: refine so that the inner `Scalar` != 0
 #[derive(Copy, Clone, Debug)]
 pub struct EphemeralPrivateKey(pub(crate) pallas::Scalar);
 

zebra-chain/src/orchard/keys/tests.rs

@@ -33,7 +33,8 @@ fn generate_keys_from_test_vectors() {
         let diversifier_key = DiversifierKey::from(full_viewing_key);
         assert_eq!(diversifier_key, test_vector.dk);
 
-        let incoming_viewing_key = IncomingViewingKey::from(full_viewing_key);
+        let incoming_viewing_key =
+            IncomingViewingKey::try_from(full_viewing_key).expect("a valid incoming viewing key");
         assert_eq!(<[u8; 32]>::from(incoming_viewing_key.ivk), test_vector.ivk);
 
         let outgoing_viewing_key = OutgoingViewingKey::from(full_viewing_key);
@@ -84,7 +85,7 @@ proptest! {
         // Test ConstantTimeEq, Eq, PartialEq
         assert_eq!(diversifier_key, diversifier_key.clone());
 
-        let incoming_viewing_key = IncomingViewingKey::from(full_viewing_key);
+        let incoming_viewing_key = IncomingViewingKey::try_from(full_viewing_key).expect("a valid incoming viewing key");
         // Test ConstantTimeEq, Eq, PartialEq
         assert_eq!(incoming_viewing_key, incoming_viewing_key.clone());