Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security: Stop panicking on invalid reserved orchard::Flags bits #2284

Merged
merged 7 commits into from
Jun 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions book/src/dev/rfcs/0010-v5-transaction.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 13 additions & 4 deletions zebra-chain/src/orchard/shielded_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
}
}

Expand All @@ -155,8 +162,10 @@ impl ZcashSerialize for Flags {

oxarbitrage marked this conversation as resolved.
Show resolved Hide resolved
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"))
}
}
1 change: 1 addition & 0 deletions zebra-chain/src/orchard/tests.rs
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
mod preallocate;
mod prop;
35 changes: 35 additions & 0 deletions zebra-chain/src/orchard/tests/prop.rs
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
}