From 11f0de03bf6a9223ea0b1acc5b8fbec4112825f7 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Wed, 9 Jun 2021 15:08:09 -0300 Subject: [PATCH 1/6] stop panicking on invalid orchard nullifiers --- zebra-chain/src/orchard/action.rs | 4 +-- zebra-chain/src/orchard/arbitrary.rs | 7 ++++-- zebra-chain/src/orchard/note/nullifiers.rs | 21 ++++++++++++---- zebra-chain/src/transaction/tests/vectors.rs | 26 ++++++++++++++++++++ 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/zebra-chain/src/orchard/action.rs b/zebra-chain/src/orchard/action.rs index 33dfcb56c9c..a4136ba1a67 100644 --- a/zebra-chain/src/orchard/action.rs +++ b/zebra-chain/src/orchard/action.rs @@ -1,4 +1,4 @@ -use std::io; +use std::{convert::TryFrom, io}; use halo2::pasta::pallas; @@ -61,7 +61,7 @@ impl ZcashDeserialize for Action { fn zcash_deserialize(mut reader: R) -> Result { Ok(Action { cv: ValueCommitment::zcash_deserialize(&mut reader)?, - nullifier: Nullifier::from(reader.read_32_bytes()?), + nullifier: Nullifier::try_from(reader.read_32_bytes()?)?, rk: reader.read_32_bytes()?.into(), cm_x: pallas::Base::zcash_deserialize(&mut reader)?, ephemeral_key: keys::EphemeralPublicKey::zcash_deserialize(&mut reader)?, diff --git a/zebra-chain/src/orchard/arbitrary.rs b/zebra-chain/src/orchard/arbitrary.rs index ded9d3d0d0a..f061fbf5f65 100644 --- a/zebra-chain/src/orchard/arbitrary.rs +++ b/zebra-chain/src/orchard/arbitrary.rs @@ -6,7 +6,7 @@ use crate::primitives::redpallas::{Signature, SpendAuth, VerificationKeyBytes}; use super::{keys, note, Action, AuthorizedAction, Flags, NoteCommitment, ValueCommitment}; -use std::marker::PhantomData; +use std::{convert::TryFrom, marker::PhantomData}; impl Arbitrary for Action { type Parameters = (); @@ -42,7 +42,10 @@ impl Arbitrary for note::Nullifier { use halo2::arithmetic::FieldExt; (any::()) - .prop_map(|number| Self::from(pallas::Scalar::from_u64(number).to_bytes())) + .prop_map(|number| { + Self::try_from(pallas::Scalar::from_u64(number).to_bytes()) + .expect("a valid generated nullifier") + }) .boxed() } diff --git a/zebra-chain/src/orchard/note/nullifiers.rs b/zebra-chain/src/orchard/note/nullifiers.rs index 897ce6482d9..d1579886518 100644 --- a/zebra-chain/src/orchard/note/nullifiers.rs +++ b/zebra-chain/src/orchard/note/nullifiers.rs @@ -3,13 +3,16 @@ use halo2::{arithmetic::FieldExt, pasta::pallas}; -use crate::serialization::serde_helpers; +use crate::serialization::{serde_helpers, SerializationError}; use super::super::{ commitment::NoteCommitment, keys::NullifierDerivingKey, note::Note, sinsemilla::*, }; -use std::hash::{Hash, Hasher}; +use std::{ + convert::TryFrom, + hash::{Hash, Hasher}, +}; /// A cryptographic permutation, defined in [poseidonhash]. /// @@ -46,9 +49,17 @@ impl Hash for Nullifier { } } -impl From<[u8; 32]> for Nullifier { - fn from(bytes: [u8; 32]) -> Self { - Self(pallas::Base::from_bytes(&bytes).unwrap()) +impl TryFrom<[u8; 32]> for Nullifier { + type Error = SerializationError; + + fn try_from(bytes: [u8; 32]) -> Result { + let possible_point = pallas::Base::from_bytes(&bytes); + + if possible_point.is_some().into() { + Ok(Self(possible_point.unwrap())) + } else { + Err(SerializationError::Parse("Invalid pallas::Base value")) + } } } diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index c9e0d088e42..c9685447986 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -315,3 +315,29 @@ fn fake_v5_round_trip_for_network(network: Network) { ); } } + +#[test] +fn invalid_orchard_nullifier() { + zebra_test::init(); + + use std::convert::TryFrom; + + // generated by proptest using something as: + // ```rust + // ... + // array::uniform32(any::()).prop_map(|x| Self::try_from(x).unwrap()).boxed() + // ... + // ``` + let invalid_nullifier_bytes = [ + 62, 157, 27, 63, 100, 228, 1, 82, 140, 16, 238, 78, 68, 19, 221, 184, 189, 207, 230, 95, + 194, 216, 165, 24, 110, 221, 139, 195, 106, 98, 192, 71, + ]; + + assert_eq!( + orchard::Nullifier::try_from(invalid_nullifier_bytes) + .err() + .unwrap() + .to_string(), + SerializationError::Parse("Invalid pallas::Base value").to_string() + ); +} From 46edb646ca069c18d71659999014c3b3df58aaef Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Thu, 10 Jun 2021 11:19:01 -0300 Subject: [PATCH 2/6] add context to error --- zebra-chain/src/orchard/note/nullifiers.rs | 4 +++- zebra-chain/src/transaction/tests/vectors.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/zebra-chain/src/orchard/note/nullifiers.rs b/zebra-chain/src/orchard/note/nullifiers.rs index d1579886518..831ea161b3a 100644 --- a/zebra-chain/src/orchard/note/nullifiers.rs +++ b/zebra-chain/src/orchard/note/nullifiers.rs @@ -58,7 +58,9 @@ impl TryFrom<[u8; 32]> for Nullifier { if possible_point.is_some().into() { Ok(Self(possible_point.unwrap())) } else { - Err(SerializationError::Parse("Invalid pallas::Base value")) + Err(SerializationError::Parse( + "Invalid pallas::Base value for orchard Nullifier", + )) } } } diff --git a/zebra-chain/src/transaction/tests/vectors.rs b/zebra-chain/src/transaction/tests/vectors.rs index c9685447986..b7bbe4d2449 100644 --- a/zebra-chain/src/transaction/tests/vectors.rs +++ b/zebra-chain/src/transaction/tests/vectors.rs @@ -338,6 +338,6 @@ fn invalid_orchard_nullifier() { .err() .unwrap() .to_string(), - SerializationError::Parse("Invalid pallas::Base value").to_string() + SerializationError::Parse("Invalid pallas::Base value for orchard Nullifier").to_string() ); } From 854f32d1f9e133f928a8670675c304882b92b344 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Mon, 14 Jun 2021 11:38:28 -0300 Subject: [PATCH 3/6] use `from_bytes_wide` for nullifiers in arbitrary --- zebra-chain/src/orchard/arbitrary.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/zebra-chain/src/orchard/arbitrary.rs b/zebra-chain/src/orchard/arbitrary.rs index f061fbf5f65..7ee59f353a6 100644 --- a/zebra-chain/src/orchard/arbitrary.rs +++ b/zebra-chain/src/orchard/arbitrary.rs @@ -1,6 +1,6 @@ use group::prime::PrimeCurveAffine; use halo2::pasta::pallas; -use proptest::{arbitrary::any, array, prelude::*}; +use proptest::{arbitrary::any, array, collection::vec, prelude::*}; use crate::primitives::redpallas::{Signature, SpendAuth, VerificationKeyBytes}; @@ -41,9 +41,11 @@ impl Arbitrary for note::Nullifier { fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { use halo2::arithmetic::FieldExt; - (any::()) - .prop_map(|number| { - Self::try_from(pallas::Scalar::from_u64(number).to_bytes()) + (vec(any::(), 64)) + .prop_map(|bytes| { + let mut b = [0u8; 64]; + b.copy_from_slice(bytes.as_slice()); + Self::try_from(pallas::Scalar::from_bytes_wide(&b).to_bytes()) .expect("a valid generated nullifier") }) .boxed() From 976118234b170efcb20172391530a31f7c101966 Mon Sep 17 00:00:00 2001 From: Deirdre Connolly Date: Mon, 14 Jun 2021 20:17:51 -0400 Subject: [PATCH 4/6] orchard::Nullifier vec to array conversion is a bit clearer and simpler Co-authored-by: teor --- zebra-chain/src/orchard/arbitrary.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/zebra-chain/src/orchard/arbitrary.rs b/zebra-chain/src/orchard/arbitrary.rs index 7ee59f353a6..ddcafa89653 100644 --- a/zebra-chain/src/orchard/arbitrary.rs +++ b/zebra-chain/src/orchard/arbitrary.rs @@ -43,9 +43,8 @@ impl Arbitrary for note::Nullifier { (vec(any::(), 64)) .prop_map(|bytes| { - let mut b = [0u8; 64]; - b.copy_from_slice(bytes.as_slice()); - Self::try_from(pallas::Scalar::from_bytes_wide(&b).to_bytes()) + let bytes = bytes.try_into().expect("array is the correct length"); + Self::try_from(pallas::Scalar::from_bytes_wide(&bytes).to_bytes()) .expect("a valid generated nullifier") }) .boxed() From 9bab7bf2ca0795fadaf29e2fdd825f20e7eef66e Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 15 Jun 2021 11:41:18 +1000 Subject: [PATCH 5/6] Try refactor again --- zebra-chain/src/orchard/arbitrary.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zebra-chain/src/orchard/arbitrary.rs b/zebra-chain/src/orchard/arbitrary.rs index ddcafa89653..be08dc96a44 100644 --- a/zebra-chain/src/orchard/arbitrary.rs +++ b/zebra-chain/src/orchard/arbitrary.rs @@ -43,7 +43,7 @@ impl Arbitrary for note::Nullifier { (vec(any::(), 64)) .prop_map(|bytes| { - let bytes = bytes.try_into().expect("array is the correct length"); + let bytes = bytes.as_slice().try_into().expect("vec is the correct length"); Self::try_from(pallas::Scalar::from_bytes_wide(&bytes).to_bytes()) .expect("a valid generated nullifier") }) From b56ff6ade7117632816d5e7468e06c10a56c61f4 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 15 Jun 2021 12:11:52 +1000 Subject: [PATCH 6/6] use TryInto --- zebra-chain/src/orchard/arbitrary.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/zebra-chain/src/orchard/arbitrary.rs b/zebra-chain/src/orchard/arbitrary.rs index be08dc96a44..0147b01a196 100644 --- a/zebra-chain/src/orchard/arbitrary.rs +++ b/zebra-chain/src/orchard/arbitrary.rs @@ -6,7 +6,10 @@ use crate::primitives::redpallas::{Signature, SpendAuth, VerificationKeyBytes}; use super::{keys, note, Action, AuthorizedAction, Flags, NoteCommitment, ValueCommitment}; -use std::{convert::TryFrom, marker::PhantomData}; +use std::{ + convert::{TryFrom, TryInto}, + marker::PhantomData, +}; impl Arbitrary for Action { type Parameters = (); @@ -43,7 +46,7 @@ impl Arbitrary for note::Nullifier { (vec(any::(), 64)) .prop_map(|bytes| { - let bytes = bytes.as_slice().try_into().expect("vec is the correct length"); + let bytes = bytes.try_into().expect("vec is the correct length"); Self::try_from(pallas::Scalar::from_bytes_wide(&bytes).to_bytes()) .expect("a valid generated nullifier") })