From bdd2808dbb7622657e08ea6ee57565ac1e38fede Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Mon, 1 Aug 2022 05:06:37 -0400 Subject: [PATCH] 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 * 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 * allow `unwrap_in_result` lint in added code Co-authored-by: Daira Hopwood Co-authored-by: Janito Vaqueiro Ferreira Filho Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Alfredo Garcia --- zebra-chain/src/orchard/address.rs | 4 +- zebra-chain/src/orchard/arbitrary.rs | 3 +- zebra-chain/src/orchard/keys.rs | 80 +++++++++++++++++++++++---- zebra-chain/src/orchard/keys/tests.rs | 5 +- 4 files changed, 78 insertions(+), 14 deletions(-) diff --git a/zebra-chain/src/orchard/address.rs b/zebra-chain/src/orchard/address.rs index 24cef75fe5b..d1a59f2e459 100644 --- a/zebra-chain/src/orchard/address.rs +++ b/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)); diff --git a/zebra-chain/src/orchard/arbitrary.rs b/zebra-chain/src/orchard/arbitrary.rs index 82acd76014a..187465d1707 100644 --- a/zebra-chain/src/orchard/arbitrary.rs +++ b/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)) }) diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index 759b1156075..8f0b3235a05 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/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 for [u8; 64] { } } -impl From 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 { + 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 for IncomingViewingKey { + type Error = &'static str; + /// Commit^ivk_rivk(ak, nk) := /// SinsemillaShortCommit_rcm(︁ /// "z.cash:Orchard-CommitIvk", @@ -656,7 +700,8 @@ impl From for IncomingViewingKey { /// /// #[allow(non_snake_case)] - fn from(fvk: FullViewingKey) -> Self { + #[allow(clippy::unwrap_in_result)] + fn try_from(fvk: FullViewingKey) -> Result { let mut M: BitVec = BitVec::new(); // I2LEBSP_l^Orchard_base(ak)︁ @@ -678,10 +723,18 @@ impl From for IncomingViewingKey { ) .expect("deriving orchard commit^ivk should not output ⊥ "); - Self { - dk: fvk.into(), - // mod r_P - ivk: pallas::Scalar::from_repr(commit_x.into()).unwrap(), + let ivk_ctoption = pallas::Scalar::from_repr(commit_x.into()); + + // if ivk ∈ {0, ⊥}, discard this key + + // [`Scalar::is_zero()`] is constant-time under the hood, and ivk is mod r_P + if ivk_ctoption.is_some().into() && !::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 for DiversifierKey { } } +impl From<[u8; 32]> for DiversifierKey { + fn from(bytes: [u8; 32]) -> DiversifierKey { + DiversifierKey(bytes) + } +} + impl From 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. /// /// /// +// TODO: refine so that the inner `Scalar` != 0 #[derive(Copy, Clone, Debug)] pub struct EphemeralPrivateKey(pub(crate) pallas::Scalar); diff --git a/zebra-chain/src/orchard/keys/tests.rs b/zebra-chain/src/orchard/keys/tests.rs index 6c2ca812ee8..57ee746149c 100644 --- a/zebra-chain/src/orchard/keys/tests.rs +++ b/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());