From 61ce4dd3d63872172a7412c45b91df59011296c8 Mon Sep 17 00:00:00 2001
From: Jack Grigg <jack@z.cash>
Date: Fri, 30 Nov 2018 00:30:37 +0000
Subject: [PATCH] Enforce range checks when reading Amounts

---
 .../src/transaction/components.rs             | 86 ++++++++++++++++++-
 zcash_primitives/src/transaction/mod.rs       |  2 +-
 2 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/zcash_primitives/src/transaction/components.rs b/zcash_primitives/src/transaction/components.rs
index 735d6bb..ebfc1ea 100644
--- a/zcash_primitives/src/transaction/components.rs
+++ b/zcash_primitives/src/transaction/components.rs
@@ -20,9 +20,42 @@ const PHGR_PROOF_SIZE: usize = (33 + 33 + 65 + 33 + 33 + 33 + 33 + 33);
 const ZC_NUM_JS_INPUTS: usize = 2;
 const ZC_NUM_JS_OUTPUTS: usize = 2;
 
+const COIN: i64 = 1_0000_0000;
+const MAX_MONEY: i64 = 21_000_000 * COIN;
+
 #[derive(Clone, Copy, Debug, PartialEq)]
 pub struct Amount(pub i64);
 
+impl Amount {
+    // Read an Amount from a signed 64-bit little-endian integer.
+    pub fn read_i64<R: Read>(mut reader: R, allow_negative: bool) -> io::Result<Self> {
+        let amount = reader.read_i64::<LittleEndian>()?;
+        if 0 <= amount && amount <= MAX_MONEY {
+            Ok(Amount(amount))
+        } else if allow_negative && -MAX_MONEY <= amount && amount < 0 {
+            Ok(Amount(amount))
+        } else {
+            Err(io::Error::new(
+                io::ErrorKind::InvalidData,
+                "Amount not in {0..MAX_MONEY}",
+            ))
+        }
+    }
+
+    // Read an Amount from an unsigned 64-bit little-endian integer.
+    pub fn read_u64<R: Read>(mut reader: R) -> io::Result<Self> {
+        let amount = reader.read_u64::<LittleEndian>()?;
+        if amount <= MAX_MONEY as u64 {
+            Ok(Amount(amount as i64))
+        } else {
+            Err(io::Error::new(
+                io::ErrorKind::InvalidData,
+                "Amount not in {0..MAX_MONEY}",
+            ))
+        }
+    }
+}
+
 pub struct Script(pub Vec<u8>);
 
 impl Script {
@@ -88,7 +121,7 @@ pub struct TxOut {
 
 impl TxOut {
     pub fn read<R: Read>(mut reader: &mut R) -> io::Result<Self> {
-        let value = Amount(reader.read_i64::<LittleEndian>()?);
+        let value = Amount::read_i64(&mut reader, false)?;
         let script_pubkey = Script::read(&mut reader)?;
 
         Ok(TxOut {
@@ -217,8 +250,8 @@ pub struct JSDescription {
 
 impl JSDescription {
     pub fn read<R: Read>(mut reader: R, use_groth: bool) -> io::Result<Self> {
-        let vpub_old = Amount(reader.read_i64::<LittleEndian>()?);
-        let vpub_new = Amount(reader.read_i64::<LittleEndian>()?);
+        let vpub_old = Amount::read_u64(&mut reader)?;
+        let vpub_new = Amount::read_u64(&mut reader)?;
 
         let mut anchor = [0; 32];
         reader.read_exact(&mut anchor)?;
@@ -300,3 +333,50 @@ impl JSDescription {
         writer.write_all(&self.ciphertexts[1])
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{Amount, MAX_MONEY};
+
+    #[test]
+    fn amount_in_range() {
+        let zero = b"\x00\x00\x00\x00\x00\x00\x00\x00";
+        assert_eq!(Amount::read_u64(&zero[..]).unwrap(), Amount(0));
+        assert_eq!(Amount::read_i64(&zero[..], false).unwrap(), Amount(0));
+        assert_eq!(Amount::read_i64(&zero[..], true).unwrap(), Amount(0));
+
+        let neg_one = b"\xff\xff\xff\xff\xff\xff\xff\xff";
+        assert!(Amount::read_u64(&neg_one[..]).is_err());
+        assert!(Amount::read_i64(&neg_one[..], false).is_err());
+        assert_eq!(Amount::read_i64(&neg_one[..], true).unwrap(), Amount(-1));
+
+        let max_money = b"\x00\x40\x07\x5a\xf0\x75\x07\x00";
+        assert_eq!(Amount::read_u64(&max_money[..]).unwrap(), Amount(MAX_MONEY));
+        assert_eq!(
+            Amount::read_i64(&max_money[..], false).unwrap(),
+            Amount(MAX_MONEY)
+        );
+        assert_eq!(
+            Amount::read_i64(&max_money[..], true).unwrap(),
+            Amount(MAX_MONEY)
+        );
+
+        let max_money_p1 = b"\x01\x40\x07\x5a\xf0\x75\x07\x00";
+        assert!(Amount::read_u64(&max_money_p1[..]).is_err());
+        assert!(Amount::read_i64(&max_money_p1[..], false).is_err());
+        assert!(Amount::read_i64(&max_money_p1[..], true).is_err());
+
+        let neg_max_money = b"\x00\xc0\xf8\xa5\x0f\x8a\xf8\xff";
+        assert!(Amount::read_u64(&neg_max_money[..]).is_err());
+        assert!(Amount::read_i64(&neg_max_money[..], false).is_err());
+        assert_eq!(
+            Amount::read_i64(&neg_max_money[..], true).unwrap(),
+            Amount(-MAX_MONEY)
+        );
+
+        let neg_max_money_m1 = b"\xff\xbf\xf8\xa5\x0f\x8a\xf8\xff";
+        assert!(Amount::read_u64(&neg_max_money_m1[..]).is_err());
+        assert!(Amount::read_i64(&neg_max_money_m1[..], false).is_err());
+        assert!(Amount::read_i64(&neg_max_money_m1[..], true).is_err());
+    }
+}
diff --git a/zcash_primitives/src/transaction/mod.rs b/zcash_primitives/src/transaction/mod.rs
index 1c8f2c4..74d34fb 100644
--- a/zcash_primitives/src/transaction/mod.rs
+++ b/zcash_primitives/src/transaction/mod.rs
@@ -114,7 +114,7 @@ impl Transaction {
         };
 
         let (value_balance, shielded_spends, shielded_outputs) = if is_sapling_v4 {
-            let vb = Amount(reader.read_i64::<LittleEndian>()?);
+            let vb = Amount::read_i64(&mut reader, true)?;
             let ss = Vector::read(&mut reader, SpendDescription::read)?;
             let so = Vector::read(&mut reader, OutputDescription::read)?;
             (vb, ss, so)