From fddacf729964cf4e06ece7d0d85edc8a8366075f Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Fri, 25 Mar 2022 00:25:38 -0400 Subject: [PATCH 1/8] Disallow Orchard ivk = 0 on IncomingViewingKey::from and SpendingKey generation --- zebra-chain/src/orchard/address.rs | 4 ++- zebra-chain/src/orchard/arbitrary.rs | 3 +- zebra-chain/src/orchard/keys.rs | 35 +++++++++++++------ zebra-chain/src/orchard/keys/tests.rs | 5 +-- .../peer_set/set/tests/prop.txt | 7 ++++ 5 files changed, 40 insertions(+), 14 deletions(-) create mode 100644 zebra-network/proptest-regressions/peer_set/set/tests/prop.txt 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 e0601b6f01f..551d6b0e014 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}; @@ -203,7 +203,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; } @@ -595,7 +600,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, } @@ -642,7 +647,9 @@ impl From for [u8; 64] { } } -impl From for IncomingViewingKey { +impl TryFrom for IncomingViewingKey { + type Error = &'static str; + /// Commit^ivk_rivk(ak, nk) := /// SinsemillaShortCommit_rcm(︁ /// "z.cash:Orchard-CommitIvk", @@ -652,8 +659,8 @@ impl From for IncomingViewingKey { /// /// #[allow(non_snake_case)] - fn from(fvk: FullViewingKey) -> Self { - let mut M: BitVec = BitVec::new(); + fn try_from(fvk: FullViewingKey) -> Result { + let mut M: BitVec = BitVec::new(); // I2LEBSP_l^Orchard_base(ak)︁ let ak_bytes = @@ -674,10 +681,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_bytes(&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") } } } 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()); diff --git a/zebra-network/proptest-regressions/peer_set/set/tests/prop.txt b/zebra-network/proptest-regressions/peer_set/set/tests/prop.txt new file mode 100644 index 00000000000..69f79049fa3 --- /dev/null +++ b/zebra-network/proptest-regressions/peer_set/set/tests/prop.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc a1098eb7e8d9970d8624949e224c398cc3f522a3da8669906cda4dbe27977da8 # shrinks to total_number_of_peers = 2 From c4e46b7600f5626b17944be0584218b04e2d28d3 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Fri, 25 Mar 2022 01:09:22 -0400 Subject: [PATCH 2/8] Check that ivk scalar values parsed from bytes are never 0 --- zebra-chain/src/orchard/keys.rs | 46 ++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index 551d6b0e014..ca13986c363 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/zebra-chain/src/orchard/keys.rs @@ -647,6 +647,43 @@ impl From for [u8; 64] { } } +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_bytes(&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; @@ -819,6 +856,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 @@ -1017,10 +1060,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); From fddb696ede8b87d92ffb015cf4f0a9d034c56460 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Mon, 28 Mar 2022 12:32:20 -0400 Subject: [PATCH 3/8] Update zebra-chain/src/orchard/keys.rs Co-authored-by: Daira Hopwood --- zebra-chain/src/orchard/keys.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index ca13986c363..db8811d34a0 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/zebra-chain/src/orchard/keys.rs @@ -207,7 +207,7 @@ impl SpendingKey { continue; } - // "if ivk = {0, ⊥ }, discard this key and repeat with a new sk" + // "if ivk ∈ {0, ⊥}, discard this key and repeat with a new sk" if IncomingViewingKey::try_from(FullViewingKey::from(sk)).is_err() { continue; } From ff972056259e9a668fb726242c73a2a7b63fd192 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Thu, 26 May 2022 20:49:54 -0400 Subject: [PATCH 4/8] Switch away from removed pallas::Scalar::from_bytes to PrimeField::from_repr --- zebra-chain/src/orchard/keys.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index db8811d34a0..8310138d877 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/zebra-chain/src/orchard/keys.rs @@ -666,7 +666,7 @@ impl TryFrom<[u8; 64]> for IncomingViewingKey { let mut ivk_bytes = [0u8; 32]; ivk_bytes[..].copy_from_slice(&bytes[32..]); - let possible_scalar = pallas::Scalar::from_bytes(&ivk_bytes); + let possible_scalar = pallas::Scalar::from_repr(ivk_bytes); if possible_scalar.is_some().into() { let scalar = possible_scalar.unwrap(); @@ -718,7 +718,7 @@ impl TryFrom for IncomingViewingKey { ) .expect("deriving orchard commit^ivk should not output ⊥ "); - let ivk_ctoption = pallas::Scalar::from_bytes(&commit_x.into()); + let ivk_ctoption = pallas::Scalar::from_repr(commit_x.into()); // if ivk ∈ {0, ⊥}, discard this key From 333b9c12c703c14a437a4ad4fa687549b647cedc Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Fri, 3 Jun 2022 17:23:32 +0200 Subject: [PATCH 5/8] Fix updated Orchard IVK derivation around updated BitVec API --- zebra-chain/src/orchard/keys.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index 8310138d877..8815f2aee8a 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/zebra-chain/src/orchard/keys.rs @@ -697,7 +697,7 @@ impl TryFrom for IncomingViewingKey { /// #[allow(non_snake_case)] fn try_from(fvk: FullViewingKey) -> Result { - let mut M: BitVec = BitVec::new(); + let mut M: BitVec = BitVec::new(); // I2LEBSP_l^Orchard_base(ak)︁ let ak_bytes = From de38688b60ce57c25713321c52ed307a2ef30555 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Sat, 4 Jun 2022 10:34:43 -0500 Subject: [PATCH 6/8] Remove spurious proptest regressions --- .../proptest-regressions/peer_set/set/tests/prop.txt | 7 ------- 1 file changed, 7 deletions(-) delete mode 100644 zebra-network/proptest-regressions/peer_set/set/tests/prop.txt diff --git a/zebra-network/proptest-regressions/peer_set/set/tests/prop.txt b/zebra-network/proptest-regressions/peer_set/set/tests/prop.txt deleted file mode 100644 index 69f79049fa3..00000000000 --- a/zebra-network/proptest-regressions/peer_set/set/tests/prop.txt +++ /dev/null @@ -1,7 +0,0 @@ -# Seeds for failure cases proptest has generated in the past. It is -# automatically read and these particular cases re-run before any -# novel cases are generated. -# -# It is recommended to check this file in to source control so that -# everyone who runs the test benefits from these saved cases. -cc a1098eb7e8d9970d8624949e224c398cc3f522a3da8669906cda4dbe27977da8 # shrinks to total_number_of_peers = 2 From 81299ee61b18151d34eab97b49788defc28cf970 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Wed, 15 Jun 2022 21:08:35 -0400 Subject: [PATCH 7/8] Update zebra-chain/src/orchard/keys.rs Co-authored-by: Janito Vaqueiro Ferreira Filho --- zebra-chain/src/orchard/keys.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index 8815f2aee8a..64cc8718294 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/zebra-chain/src/orchard/keys.rs @@ -664,7 +664,7 @@ impl TryFrom<[u8; 64]> for IncomingViewingKey { let dk = DiversifierKey::from(dk_bytes); let mut ivk_bytes = [0u8; 32]; - ivk_bytes[..].copy_from_slice(&bytes[32..]); + ivk_bytes.copy_from_slice(&bytes[32..]); let possible_scalar = pallas::Scalar::from_repr(ivk_bytes); From 5a12adb5d44844d3619fab1cee9472edb7b0ac21 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Sat, 2 Jul 2022 20:45:45 -0300 Subject: [PATCH 8/8] allow `unwrap_in_result` lint in added code --- zebra-chain/src/orchard/keys.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index 6f0ba7f7b5b..8f0b3235a05 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/zebra-chain/src/orchard/keys.rs @@ -700,6 +700,7 @@ impl TryFrom for IncomingViewingKey { /// /// #[allow(non_snake_case)] + #[allow(clippy::unwrap_in_result)] fn try_from(fvk: FullViewingKey) -> Result { let mut M: BitVec = BitVec::new();