From 3dcd407d66d2b447cade98d3fcfba507c74d48ac Mon Sep 17 00:00:00 2001
From: Alfredo Garcia <oxarbitrage@gmail.com>
Date: Mon, 14 Jun 2021 21:04:18 -0300
Subject: [PATCH] Security: Stop panicking on invalid reserved orchard::Flags
 bits (#2284)

* stop panicking in deserialize orchard flags

* make the test simpler

Co-authored-by: Janito Vaqueiro Ferreira Filho <janito.vff@gmail.com>
---
 book/src/dev/rfcs/0010-v5-transaction.md |  3 ++
 zebra-chain/src/orchard/shielded_data.rs | 17 +++++++++---
 zebra-chain/src/orchard/tests.rs         |  1 +
 zebra-chain/src/orchard/tests/prop.rs    | 35 ++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 4 deletions(-)
 create mode 100644 zebra-chain/src/orchard/tests/prop.rs

diff --git a/book/src/dev/rfcs/0010-v5-transaction.md b/book/src/dev/rfcs/0010-v5-transaction.md
index 70ddf4a33..a707dfa0a 100644
--- a/book/src/dev/rfcs/0010-v5-transaction.md
+++ b/book/src/dev/rfcs/0010-v5-transaction.md
@@ -416,6 +416,9 @@ bitflags! {
 
 This type is also defined in `orchard/shielded_data.rs`.
 
+Note: A [consensus rule](https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus) was added to the protocol specification stating that:
+> In a version 5 transaction, the reserved bits 2..7 of the flagsOrchard field MUST be zero.
+
 ## Test Plan
 [test-plan]: #test-plan
 
diff --git a/zebra-chain/src/orchard/shielded_data.rs b/zebra-chain/src/orchard/shielded_data.rs
index ad0ae2bb7..f22091028 100644
--- a/zebra-chain/src/orchard/shielded_data.rs
+++ b/zebra-chain/src/orchard/shielded_data.rs
@@ -133,6 +133,14 @@ bitflags! {
     ///
     /// The spend and output flags are passed to the `Halo2Proof` verifier, which verifies
     /// the relevant note spending and creation consensus rules.
+    ///
+    /// Consensus rules:
+    ///
+    /// - "In a version 5 transaction, the reserved bits 2..7 of the flagsOrchard field MUST be zero."
+    /// ([`bitflags`](https://docs.rs/bitflags/1.2.1/bitflags/index.html) restricts its values to the
+    /// set of valid flags)
+    /// - "In a version 5 coinbase transaction, the enableSpendsOrchard flag MUST be 0."
+    /// (Checked in zebra-consensus)
     #[derive(Deserialize, Serialize)]
     pub struct Flags: u8 {
         /// Enable spending non-zero valued Orchard notes.
@@ -141,7 +149,6 @@ bitflags! {
         const ENABLE_SPENDS = 0b00000001;
         /// Enable creating new non-zero valued Orchard notes.
         const ENABLE_OUTPUTS = 0b00000010;
-        // Reserved, zeros (bits 2 .. 7)
     }
 }
 
@@ -155,8 +162,10 @@ impl ZcashSerialize for Flags {
 
 impl ZcashDeserialize for Flags {
     fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
-        let flags = Flags::from_bits(reader.read_u8()?).unwrap();
-
-        Ok(flags)
+        // Consensus rule: "In a version 5 transaction,
+        // the reserved bits 2..7 of the flagsOrchard field MUST be zero."
+        // https://zips.z.cash/protocol/protocol.pdf#txnencodingandconsensus
+        Flags::from_bits(reader.read_u8()?)
+            .ok_or(SerializationError::Parse("invalid reserved orchard flags"))
     }
 }
diff --git a/zebra-chain/src/orchard/tests.rs b/zebra-chain/src/orchard/tests.rs
index 67af9b072..a7ac2ac35 100644
--- a/zebra-chain/src/orchard/tests.rs
+++ b/zebra-chain/src/orchard/tests.rs
@@ -1 +1,2 @@
 mod preallocate;
+mod prop;
diff --git a/zebra-chain/src/orchard/tests/prop.rs b/zebra-chain/src/orchard/tests/prop.rs
new file mode 100644
index 000000000..29b6add5d
--- /dev/null
+++ b/zebra-chain/src/orchard/tests/prop.rs
@@ -0,0 +1,35 @@
+use proptest::prelude::*;
+use std::io::Cursor;
+
+use crate::{
+    orchard,
+    serialization::{ZcashDeserializeInto, ZcashSerialize},
+};
+
+proptest! {
+    /// Make sure only valid flags deserialize
+    #[test]
+    fn flag_roundtrip_bytes(flags in any::<u8>()) {
+
+        let mut serialized = Cursor::new(Vec::new());
+        flags.zcash_serialize(&mut serialized)?;
+
+        serialized.set_position(0);
+        let maybe_deserialized = (&mut serialized).zcash_deserialize_into();
+
+        let invalid_bits_mask = !orchard::Flags::all().bits();
+        match orchard::Flags::from_bits(flags) {
+            Some(valid_flags) => {
+                prop_assert_eq!(maybe_deserialized.ok(), Some(valid_flags));
+                prop_assert_eq!(flags & invalid_bits_mask, 0);
+            }
+            None => {
+                prop_assert_eq!(
+                    maybe_deserialized.err().unwrap().to_string(),
+                    "parse error: invalid reserved orchard flags"
+                );
+                prop_assert_ne!(flags & invalid_bits_mask, 0);
+            }
+        }
+    }
+}