Stop panicking on invalid orchard nullifiers (#2267)

* stop panicking on invalid orchard nullifiers

* add context to error

* use `from_bytes_wide` for nullifiers in arbitrary

* orchard::Nullifier vec to array conversion is a bit clearer and simpler

Co-authored-by: teor <teor@riseup.net>

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<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
         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)?,

zebra-chain/src/orchard/arbitrary.rs

@@ -1,12 +1,15 @@
 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};
 
 use super::{keys, note, Action, AuthorizedAction, Flags, NoteCommitment, ValueCommitment};
 
-use std::marker::PhantomData;
+use std::{
+    convert::{TryFrom, TryInto},
+    marker::PhantomData,
+};
 
 impl Arbitrary for Action {
     type Parameters = ();
@@ -41,8 +44,12 @@ impl Arbitrary for note::Nullifier {
     fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
         use halo2::arithmetic::FieldExt;
 
-        (any::<u64>())
+        (vec(any::<u8>(), 64))
-            .prop_map(|number| Self::from(pallas::Scalar::from_u64(number).to_bytes()))
+            .prop_map(|bytes| {
+                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")
+            })
             .boxed()
     }
 

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,19 @@ impl Hash for Nullifier {
     }
 }
 
-impl From<[u8; 32]> for Nullifier {
+impl TryFrom<[u8; 32]> for Nullifier {
-    fn from(bytes: [u8; 32]) -> Self {
+    type Error = SerializationError;
-        Self(pallas::Base::from_bytes(&bytes).unwrap())
+
+    fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
+        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 for orchard Nullifier",
+            ))
+        }
     }
 }
 

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::<u8>()).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 for orchard Nullifier").to_string()
+    );
+}