Check jubjub key correctness independent of redjubjub / jubjub

zebra-chain/src/sapling/address.rs

@@ -1,6 +1,7 @@
 //! Shielded addresses.
 
 use std::{
+    convert::TryFrom,
     fmt,
     io::{self, Read, Write},
 };
@@ -81,7 +82,8 @@ impl std::str::FromStr for Address {
                         _ => Network::Testnet,
                     },
                     diversifier: keys::Diversifier::from(diversifier_bytes),
-                    transmission_key: keys::TransmissionKey::from(transmission_key_bytes),
+                    transmission_key: keys::TransmissionKey::try_from(transmission_key_bytes)
+                        .unwrap(),
                 })
             }
             _ => Err(SerializationError::Parse("bech32 decoding error")),
@@ -147,7 +149,8 @@ mod tests {
             keys::IncomingViewingKey::from((authorizing_key, nullifier_deriving_key));
 
         let diversifier = keys::Diversifier::new(&mut OsRng);
-        let transmission_key = keys::TransmissionKey::from((incoming_viewing_key, diversifier));
+        let transmission_key = keys::TransmissionKey::try_from((incoming_viewing_key, diversifier))
+            .expect("should be a valid transmission key");
 
         let _sapling_shielded_address = Address {
             network: Network::Mainnet,

zebra-chain/src/sapling/arbitrary.rs

@@ -8,8 +8,9 @@ use rand_chacha::ChaChaRng;
 use crate::primitives::Groth16Proof;
 
 use super::{
-    keys, note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4,
-    PerSpendAnchor, SharedAnchor, Spend, ValueCommitment,
+    keys::{self, find_group_hash},
+    note, tree, FieldNotPresent, NoteCommitment, Output, OutputInTransactionV4, PerSpendAnchor,
+    SharedAnchor, Spend,
 };
 
 impl Arbitrary for Spend<PerSpendAnchor> {
@@ -25,7 +26,10 @@ impl Arbitrary for Spend<PerSpendAnchor> {
         )
             .prop_map(|(per_spend_anchor, nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor,
-                cv: ValueCommitment(AffinePoint::identity()),
+                // Use an arbitrary string to generate a dummy point
+                cv: find_group_hash(*b"arbitrar", b"a")
+                    .try_into()
+                    .expect("find_group_hash returns point in prime-order subgroup"),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -53,7 +57,10 @@ impl Arbitrary for Spend<SharedAnchor> {
         )
             .prop_map(|(nullifier, rk, proof, sig_bytes)| Self {
                 per_spend_anchor: FieldNotPresent,
-                cv: ValueCommitment(AffinePoint::identity()),
+                // Use an arbitrary string to generate a dummy point
+                cv: find_group_hash(*b"arbitrar", b"a")
+                    .try_into()
+                    .expect("find_group_hash returns point in prime-order subgroup"),
                 nullifier,
                 rk,
                 zkproof: proof,
@@ -79,7 +86,10 @@ impl Arbitrary for Output {
             any::<Groth16Proof>(),
         )
             .prop_map(|(enc_ciphertext, out_ciphertext, zkproof)| Self {
-                cv: ValueCommitment(AffinePoint::identity()),
+                // Use an arbitrary string to generate a dummy point
+                cv: find_group_hash(*b"arbitrar", b"a")
+                    .try_into()
+                    .expect("find_group_hash returns point in prime-order subgroup"),
                 cm_u: NoteCommitment(AffinePoint::identity()).extract_u(),
                 ephemeral_key: keys::EphemeralPublicKey(AffinePoint::identity()),
                 enc_ciphertext,

zebra-chain/src/sapling/commitment.rs

@@ -151,7 +151,7 @@ impl NoteCommitment {
 ///
 /// https://zips.z.cash/protocol/protocol.pdf#concretehomomorphiccommit
 #[derive(Clone, Copy, Deserialize, PartialEq, Serialize)]
-pub struct ValueCommitment(#[serde(with = "serde_helpers::AffinePoint")] pub jubjub::AffinePoint);
+pub struct ValueCommitment(#[serde(with = "serde_helpers::AffinePoint")] jubjub::AffinePoint);
 
 impl<'a> std::ops::Add<&'a ValueCommitment> for ValueCommitment {
     type Output = Self;
@@ -185,9 +185,18 @@ impl fmt::Debug for ValueCommitment {
     }
 }
 
-impl From<jubjub::ExtendedPoint> for ValueCommitment {
-    fn from(extended_point: jubjub::ExtendedPoint) -> Self {
-        Self(jubjub::AffinePoint::from(extended_point))
+impl TryFrom<jubjub::ExtendedPoint> for ValueCommitment {
+    type Error = &'static str;
+
+    /// Convert a JubJub point into a ValueCommitment.
+    ///
+    /// Returns an error if the point is of small order.
+    fn try_from(extended_point: jubjub::ExtendedPoint) -> Result<Self, Self::Error> {
+        if extended_point.is_small_order().into() {
+            Err("small order point")
+        } else {
+            Ok(Self(jubjub::AffinePoint::from(extended_point)))
+        }
     }
 }
 
@@ -248,7 +257,12 @@ impl TryFrom<[u8; 32]> for ValueCommitment {
         let possible_point = jubjub::AffinePoint::from_bytes(bytes);
 
         if possible_point.is_some().into() {
-            Ok(Self(possible_point.unwrap()))
+            let point = possible_point.unwrap();
+            if point.is_small_order().into() {
+                Err("small order point")
+            } else {
+                Ok(Self(possible_point.unwrap()))
+            }
         } else {
             Err("Invalid jubjub::AffinePoint value")
         }
@@ -293,7 +307,10 @@ impl ValueCommitment {
         let V = find_group_hash(*b"Zcash_cv", b"v");
         let R = find_group_hash(*b"Zcash_cv", b"r");
 
-        Self::from(V * v + R * rcv)
+        // find_group_hash() returns points in the prime-order subgroup,
+        // so we know this result is also in the same subgroup and we don't
+        // need to check the error.
+        Self::try_from(V * v + R * rcv).expect("is in prime-order subgroup")
     }
 }
 

zebra-chain/src/sapling/keys.rs

@@ -472,9 +472,29 @@ pub struct AuthorizingKey(pub(crate) redjubjub::VerificationKey<SpendAuth>);
 
 impl Eq for AuthorizingKey {}
 
-impl From<[u8; 32]> for AuthorizingKey {
-    fn from(bytes: [u8; 32]) -> Self {
-        Self(redjubjub::VerificationKey::try_from(bytes).unwrap())
+impl TryFrom<[u8; 32]> for AuthorizingKey {
+    type Error = &'static str;
+
+    /// Decode an AuthorizingKey from a byte array.
+    ///
+    /// It must decode to a prime-order point:
+    ///
+    /// > When decoding this representation, the key MUST be considered invalid
+    /// > if abst_J returns ⊥ for either ak or nk, or if ak not in J^{(r)*}
+    ///
+    /// https://zips.z.cash/protocol/protocol.pdf#saplingfullviewingkeyencoding
+    fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
+        let affine_point = jubjub::AffinePoint::from_bytes(bytes);
+        if affine_point.is_none().into() {
+            return Err("derived an invalid Sapling transmission key");
+        }
+        if affine_point.unwrap().is_prime_order().into() {
+            Ok(Self(redjubjub::VerificationKey::try_from(bytes).map_err(
+                |_e| "derived an invalid Sapling transmission key",
+            )?))
+        } else {
+            Err("derived an invalid Sapling transmission key")
+        }
     }
 }
 
@@ -822,7 +842,13 @@ impl Diversifier {
 /// Derived by multiplying a JubJub point [derived][ps] from a
 /// _Diversifier_ by the _IncomingViewingKey_ scalar.
 ///
-/// [ps]: https://zips.z.cash/protocol/protocol.pdf#concretediversifyhash
+/// The diversified TransmissionKey is denoted `pk_d` in the specification.
+/// Note that it can be the identity point, since its type is
+/// [`KA^{Sapling}.PublicPrimeSubgroup`][ka] which in turn is [`J^{(r)}`][jubjub].
+///
+/// [ps]: https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
+/// [ka]: https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
+/// [jubjub]: https://zips.z.cash/protocol/protocol.pdf#jubjub
 #[derive(Copy, Clone, PartialEq)]
 pub struct TransmissionKey(pub(crate) jubjub::AffinePoint);
 
@@ -837,14 +863,21 @@ impl fmt::Debug for TransmissionKey {
 
 impl Eq for TransmissionKey {}
 
-impl From<[u8; 32]> for TransmissionKey {
-    /// Attempts to interpret a byte representation of an
-    /// affine point, failing if the element is not on
-    /// the curve or non-canonical.
+impl TryFrom<[u8; 32]> for TransmissionKey {
+    type Error = &'static str;
+
+    /// Attempts to interpret a byte representation of an affine Jubjub point, failing if the
+    /// element is not on the curve, non-canonical, or not in the prime-order subgroup.
     ///
     /// https://github.com/zkcrypto/jubjub/blob/master/src/lib.rs#L411
-    fn from(bytes: [u8; 32]) -> Self {
-        Self(jubjub::AffinePoint::from_bytes(bytes).unwrap())
+    /// https://zips.z.cash/zip-0216
+    fn try_from(bytes: [u8; 32]) -> Result<Self, Self::Error> {
+        let affine_point = jubjub::AffinePoint::from_bytes(bytes).unwrap();
+        if (affine_point.is_identity() | affine_point.is_prime_order()).into() {
+            Ok(Self(affine_point))
+        } else {
+            Err("derived an invalid Sapling transmission key")
+        }
     }
 }
 
@@ -854,16 +887,23 @@ impl From<TransmissionKey> for [u8; 32] {
     }
 }
 
-impl From<(IncomingViewingKey, Diversifier)> for TransmissionKey {
+impl TryFrom<(IncomingViewingKey, Diversifier)> for TransmissionKey {
+    type Error = &'static str;
+
     /// This includes _KA^Sapling.DerivePublic(ivk, G_d)_, which is just a
     /// scalar mult _\[ivk\]G_d_.
     ///
     /// https://zips.z.cash/protocol/protocol.pdf#saplingkeycomponents
     /// https://zips.z.cash/protocol/protocol.pdf#concretesaplingkeyagreement
-    fn from((ivk, d): (IncomingViewingKey, Diversifier)) -> Self {
-        Self(jubjub::AffinePoint::from(
-            diversify_hash(d.0).unwrap() * ivk.scalar,
-        ))
+    fn try_from((ivk, d): (IncomingViewingKey, Diversifier)) -> Result<Self, Self::Error> {
+        let affine_point = jubjub::AffinePoint::from(
+            diversify_hash(d.0).ok_or("invalid diversifier")? * ivk.scalar,
+        );
+        // We need to ensure that the result point is in the prime-order subgroup.
+        // Since diversify_hash() returns a point in the prime-order subgroup,
+        // then the result point will also be in the prime-order subgroup
+        // and there is no need to check anything.
+        Ok(Self(affine_point))
     }
 }
 
@@ -946,7 +986,8 @@ impl FromStr for FullViewingKey {
                         fvk_hrp::MAINNET => Network::Mainnet,
                         _ => Network::Testnet,
                     },
-                    authorizing_key: AuthorizingKey::from(authorizing_key_bytes),
+                    authorizing_key: AuthorizingKey::try_from(authorizing_key_bytes)
+                        .map_err(SerializationError::Parse)?,
                     nullifier_deriving_key: NullifierDerivingKey::from(
                         nullifier_deriving_key_bytes,
                     ),

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

@@ -22,7 +22,7 @@ impl Arbitrary for TransmissionKey {
 
                 let diversifier = Diversifier::from(spending_key);
 
-                Self::from((incoming_viewing_key, diversifier))
+                Self::try_from((incoming_viewing_key, diversifier)).unwrap()
             })
             .boxed()
     }
@@ -60,7 +60,8 @@ mod tests {
             let diversifier = Diversifier::from(spending_key);
             assert_eq!(diversifier, test_vector.default_d);
 
-            let transmission_key = TransmissionKey::from((incoming_viewing_key, diversifier));
+            let transmission_key = TransmissionKey::try_from((incoming_viewing_key, diversifier))
+                .expect("should be a valid transmission key");
             assert_eq!(transmission_key, test_vector.default_pk_d);
 
             let _full_viewing_key = FullViewingKey {

zebra-consensus/src/transaction/check.rs

@@ -133,13 +133,11 @@ pub fn coinbase_tx_no_prevout_joinsplit_spend(tx: &Transaction) -> Result<(), Tr
 ///
 /// https://zips.z.cash/protocol/protocol.pdf#spenddesc
 pub fn spend_cv_rk_not_small_order(spend: &Spend<PerSpendAnchor>) -> Result<(), TransactionError> {
-    if bool::from(spend.cv.0.is_small_order())
-        || bool::from(
-            jubjub::AffinePoint::from_bytes(spend.rk.into())
-                .unwrap()
-                .is_small_order(),
-        )
-    {
+    if bool::from(
+        jubjub::AffinePoint::from_bytes(spend.rk.into())
+            .unwrap()
+            .is_small_order(),
+    ) {
         Err(TransactionError::SmallOrder)
     } else {
         Ok(())
@@ -151,13 +149,11 @@ pub fn spend_cv_rk_not_small_order(spend: &Spend<PerSpendAnchor>) -> Result<(),
 ///
 /// https://zips.z.cash/protocol/protocol.pdf#outputdesc
 pub fn output_cv_epk_not_small_order(output: &Output) -> Result<(), TransactionError> {
-    if bool::from(output.cv.0.is_small_order())
-        || bool::from(
-            jubjub::AffinePoint::from_bytes(output.ephemeral_key.into())
-                .unwrap()
-                .is_small_order(),
-        )
-    {
+    if bool::from(
+        jubjub::AffinePoint::from_bytes(output.ephemeral_key.into())
+            .unwrap()
+            .is_small_order(),
+    ) {
         Err(TransactionError::SmallOrder)
     } else {
         Ok(())