fix(consensus): ban peers sending invalid shielded proofs (#11054)

Failed Orchard/Ironwood Halo2 proofs, Orchard binding signatures, and Sprout
JoinSplit signatures returned untyped string errors that collapsed to
`InternalDowncastError` (mempool misbehaviour score 0) in `From<BoxError>`. A
peer could therefore force expensive shielded verification indefinitely without
ever being banned; the `RedPallas`/`Ed25519` entries already in the ban list
were dead because the conversion never produced those variants.

Type these verification failures as `Halo2VerificationFailed` /
`SaplingVerificationFailed`, downcast `ed25519` and `reddsa` errors in
`From<BoxError>`, and give them and the non-canonical proof-size errors
(`OrchardProofSize`, `IronwoodProofSize`) a ban-worthy misbehaviour score.

Ported from Zakura (#283, #285).

Refs: GHSA-2p4c-3q4q-p463

Author: upbqdn

zebra-consensus/src/error.rs

@@ -24,6 +24,9 @@ use crate::{block::MAX_BLOCK_SIGOPS, transaction::check::MAX_STANDARD_SCRIPTSIG_
 #[cfg(any(test, feature = "proptest-impl"))]
 use proptest_derive::Arbitrary;
 
+#[cfg(test)]
+mod tests;
+
 /// Workaround for format string identifier rules.
 const MAX_EXPIRY_HEIGHT: block::Height = block::Height::MAX_EXPIRY_HEIGHT;
 
@@ -161,8 +164,13 @@ pub enum TransactionError {
     #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))]
     RedPallas(zebra_chain::primitives::reddsa::Error),
 
-    // temporary error type until #1186 is fixed
-    #[error("Downcast from BoxError to redjubjub::Error failed: {0}")]
+    #[error("Sapling proof or signature verification failed")]
+    SaplingVerificationFailed,
+
+    #[error("Orchard or Ironwood Halo2 proof verification failed")]
+    Halo2VerificationFailed,
+
+    #[error("could not convert an asynchronous verification error: {0}")]
     InternalDowncastError(String),
 
     #[error("either vpub_old or vpub_new must be zero")]
@@ -335,12 +343,26 @@ impl From<amount::Error> for TransactionError {
 // TODO: use a dedicated variant and From impl for each concrete type, and update callers (#5732)
 impl From<BoxError> for TransactionError {
     fn from(mut err: BoxError) -> Self {
-        // TODO: handle redpallas::Error, ScriptInvalid, InvalidSignature
+        // Preserve the concrete shielded proof/signature verification error types so they keep
+        // their mempool misbehaviour score. Without these downcasts a failed Orchard/Ironwood
+        // Halo2 proof, Orchard binding signature, or Sprout JoinSplit signature would collapse to
+        // `InternalDowncastError` (score 0), letting a peer force verification without being banned.
+        // See <https://github.com/ZcashFoundation/zebra/security/advisories/GHSA-2p4c-3q4q-p463>.
+        match err.downcast::<zebra_chain::primitives::ed25519::Error>() {
+            Ok(e) => return TransactionError::Ed25519(*e),
+            Err(e) => err = e,
+        }
+
         match err.downcast::<zebra_chain::primitives::redjubjub::Error>() {
             Ok(e) => return TransactionError::RedJubjub(*e),
             Err(e) => err = e,
         }
 
+        match err.downcast::<zebra_chain::primitives::reddsa::Error>() {
+            Ok(e) => return TransactionError::RedPallas(*e),
+            Err(e) => err = e,
+        }
+
         match err.downcast::<ValidateContextError>() {
             Ok(e) => return (*e).into(),
             Err(e) => err = e,
@@ -394,6 +416,10 @@ impl TransactionError {
             | Ed25519(_)
             | RedJubjub(_)
             | RedPallas(_)
+            | SaplingVerificationFailed
+            | Halo2VerificationFailed
+            | OrchardProofSize
+            | IronwoodProofSize
             | BothVPubsNonZero
             | DisabledAddToSproutPool
             | NegativeOrchardValueBalance

zebra-consensus/src/error/tests.rs

@@ -0,0 +1,50 @@
+//! Tests for [`TransactionError`] conversion and mempool misbehaviour scoring.
+
+use super::*;
+
+/// Boxed shielded proof and signature verification errors must keep their concrete type when
+/// converted back from a [`BoxError`], so the mempool can assign them a misbehaviour score instead
+/// of collapsing them to [`TransactionError::InternalDowncastError`] (score 0). See
+/// <https://github.com/ZcashFoundation/zebra/security/advisories/GHSA-2p4c-3q4q-p463>.
+#[test]
+fn boxed_signature_errors_are_preserved() {
+    let ed25519_error: BoxError =
+        Box::new(zebra_chain::primitives::ed25519::Error::InvalidSignature);
+    assert_eq!(
+        TransactionError::from(ed25519_error),
+        TransactionError::Ed25519(zebra_chain::primitives::ed25519::Error::InvalidSignature)
+    );
+
+    let redjubjub_error: BoxError =
+        Box::new(zebra_chain::primitives::redjubjub::Error::InvalidSignature);
+    assert_eq!(
+        TransactionError::from(redjubjub_error),
+        TransactionError::RedJubjub(zebra_chain::primitives::redjubjub::Error::InvalidSignature)
+    );
+
+    let redpallas_error: BoxError =
+        Box::new(zebra_chain::primitives::reddsa::Error::InvalidSignature);
+    assert_eq!(
+        TransactionError::from(redpallas_error),
+        TransactionError::RedPallas(zebra_chain::primitives::reddsa::Error::InvalidSignature)
+    );
+}
+
+/// Every shielded proof/signature verification failure, and non-canonical Orchard/Ironwood proof
+/// sizes, must earn a ban-worthy mempool misbehaviour score, so a peer forcing expensive
+/// verification with invalid proofs is disconnected rather than allowed to keep sending. See
+/// <https://github.com/ZcashFoundation/zebra/security/advisories/GHSA-2p4c-3q4q-p463>.
+#[test]
+fn verification_errors_have_high_misbehavior_score() {
+    for error in [
+        TransactionError::SaplingVerificationFailed,
+        TransactionError::Halo2VerificationFailed,
+        TransactionError::OrchardProofSize,
+        TransactionError::IronwoodProofSize,
+        TransactionError::Ed25519(zebra_chain::primitives::ed25519::Error::InvalidSignature),
+        TransactionError::RedJubjub(zebra_chain::primitives::redjubjub::Error::InvalidSignature),
+        TransactionError::RedPallas(zebra_chain::primitives::reddsa::Error::InvalidSignature),
+    ] {
+        assert_eq!(error.mempool_misbehavior_score(), 100, "{error:?}");
+    }
+}

zebra-consensus/src/primitives/halo2.rs

@@ -19,7 +19,7 @@ use rand::thread_rng;
 use zcash_protocol::value::ZatBalance;
 use zebra_chain::{parameters::NetworkUpgrade, transaction::SigHash};
 
-use crate::BoxError;
+use crate::{error::TransactionError, BoxError};
 use thiserror::Error;
 use tokio::sync::watch;
 use tower::Service;
@@ -441,7 +441,7 @@ impl Verifier {
         if spawn_fifo(move || item.verify_single(vk)).await? {
             Ok(())
         } else {
-            Err("could not validate orchard proof".into())
+            Err(TransactionError::Halo2VerificationFailed.into())
         }
     }
 }
@@ -501,7 +501,7 @@ impl Service<BatchControl<Item>> for Verifier {
                             } else {
                                 tracing::trace!(?is_valid, "invalid halo2 proof");
                                 metrics::counter!("proofs.halo2.invalid").increment(1);
-                                Err("could not validate halo2 proofs".into())
+                                Err(TransactionError::Halo2VerificationFailed.into())
                             }
                         }
                         Err(_recv_error) => panic!("verifier was dropped without flushing"),

zebra-consensus/src/primitives/sapling.rs

@@ -8,7 +8,7 @@ use std::{
     task::{Context, Poll},
 };
 
-use futures::{future::BoxFuture, FutureExt, TryFutureExt};
+use futures::{future::BoxFuture, FutureExt};
 use once_cell::sync::Lazy;
 use rand::thread_rng;
 use tokio::sync::watch;
@@ -21,6 +21,8 @@ use zcash_proofs::prover::LocalTxProver;
 use zcash_protocol::value::ZatBalance;
 use zebra_chain::transaction::SigHash;
 
+use crate::{error::TransactionError, BoxError};
+
 /// Sapling prover containing spend and output params for the Sapling circuit.
 ///
 /// Used to:
@@ -108,28 +110,28 @@ impl Service<BatchControl<Item>> for Verifier {
                     .batch
                     .check_bundle(item.bundle, item.sighash.into())
                     .then_some(())
-                    .ok_or("invalid Sapling bundle");
+                    .ok_or(TransactionError::SaplingVerificationFailed);
 
                 async move {
-                    bundle_check?;
+                    bundle_check.map_err(BoxError::from)?;
 
                     rx.changed()
                         .await
-                        .map_err(|_| "verifier was dropped without flushing")
-                        .and_then(|_| {
-                            // We use a new channel for each batch, so we always get the correct
-                            // batch result here.
-                            rx.borrow()
-                                .ok_or("threadpool unexpectedly dropped channel sender")?
-                                .then(|| {
-                                    metrics::counter!("proofs.sapling.verified").increment(1);
-                                })
-                                .ok_or_else(|| {
-                                    metrics::counter!("proofs.sapling.invalid").increment(1);
-                                    "batch verification of Sapling shielded data failed"
-                                })
-                        })
-                        .map_err(Self::Error::from)
+                        .map_err(|_| BoxError::from("verifier was dropped without flushing"))?;
+
+                    // We use a new channel for each batch, so we always get the correct
+                    // batch result here.
+                    let is_valid = rx.borrow().ok_or_else(|| {
+                        BoxError::from("threadpool unexpectedly dropped channel sender")
+                    })?;
+
+                    if is_valid {
+                        metrics::counter!("proofs.sapling.verified").increment(1);
+                        Ok(())
+                    } else {
+                        metrics::counter!("proofs.sapling.invalid").increment(1);
+                        Err(BoxError::from(TransactionError::SaplingVerificationFailed))
+                    }
                 }
                 .boxed()
             }
@@ -180,20 +182,23 @@ pub fn verify_single(
             .batch
             .check_bundle(item.bundle, item.sighash.into())
             .then_some(())
-            .ok_or("invalid Sapling bundle");
-        check?;
+            .ok_or(TransactionError::SaplingVerificationFailed);
+        check.map_err(BoxError::from)?;
 
-        tokio::task::spawn_blocking(move || {
+        let is_valid = tokio::task::spawn_blocking(move || {
             let (spend_vk, output_vk) = SAPLING.verifying_keys();
 
             mem::take(&mut verifier.batch).validate(&spend_vk, &output_vk, thread_rng())
         })
         .await
-        .map_err(|_| "Sapling bundle validation thread panicked")?
-        .then_some(())
-        .ok_or("invalid proof or sig in Sapling bundle")
+        .map_err(|_| BoxError::from("Sapling bundle validation thread panicked"))?;
+
+        if is_valid {
+            Ok(())
+        } else {
+            Err(BoxError::from(TransactionError::SaplingVerificationFailed))
+        }
     }
-    .map_err(Box::from)
     .boxed()
 }
 

zebra-consensus/src/transaction/tests.rs

@@ -2747,12 +2747,7 @@ fn v4_with_modified_joinsplit_is_rejected() {
     zebra_test::MULTI_THREADED_RUNTIME.block_on(async {
         v4_with_joinsplit_is_rejected_for_modification(
             JoinSplitModification::CorruptSignature,
-            // TODO: Fix error downcast
-            // Err(TransactionError::Ed25519(ed25519::Error::InvalidSignature))
-            TransactionError::InternalDowncastError(
-                "downcast to known transaction error type failed, original error: InvalidSignature"
-                    .to_string(),
-            ),
+            TransactionError::Ed25519(ed25519::Error::InvalidSignature),
         )
         .await;