Merge #279: Improve slicing and error handling around slices

5e0d577 pset: fix slicing in KeySource::deserialize (Andrew Poelstra)
4384012 address: do a better job slicing bech32 data (Andrew Poelstra)
cae359d transaction: use better error typing for pegin destructuring (Andrew Poelstra)
6809a7f blind: use array splitting in TxOut::unblind (fix potential DoS?) (Andrew Poelstra)
ee50a5f rewrite Address::from_base58 to eliminate all the unwraps (Andrew Poelstra)
ed024b2 add bitcoin-internals dependency (Andrew Poelstra)

Pull request description:

  Uses the `bitcoin-internals` library which provides us a bunch of array/slice methods that aren't available in std in our MSRV. Eventually we will be able to drop the dep and get these features directly from std.
  
  This replaces a bunch of slice accesses with arrays, cleaning up error paths and eliminating panics (though most are inaccessible assuming you use known-valid data from the blockchain).
  
  Builds on #278 (which I will merge in the next hour).
  
  Will do a followup that cleans up the error types; I deliberately avoided breaking the API to make this one easy to backport.


ACKs for top commit:
  apoelstra:
    ACK 5e0d577; successfully ran local tests
  philipr-za:
    ACK 5e0d577


Tree-SHA512: b9238a0a2c642061c47671b955eb19771485448f8b455be268f2a5ee223f29f0ef09f67f792fcf9400481826c5a9c5ec2d0ee6eceb2a3da458819178f5f54d37

Author: apoelstra

Cargo-recent.lock

@@ -26,7 +26,7 @@ version = "0.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "2c8d66485a3a2ea485c1913c4572ce0256067a5377ac8c75c4960e1cda98605f"
 dependencies = [
- "bitcoin-internals",
+ "bitcoin-internals 0.3.0",
  "bitcoin_hashes",
 ]
 
@@ -66,7 +66,7 @@ dependencies = [
  "base58ck",
  "base64 0.21.7",
  "bech32",
- "bitcoin-internals",
+ "bitcoin-internals 0.3.0",
  "bitcoin-io",
  "bitcoin-units",
  "bitcoin_hashes",
@@ -85,6 +85,12 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "bitcoin-internals"
+version = "0.5.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a30a22d1f112dde8e16be7b45c63645dc165cef254f835b3e1e9553e485cfa64"
+
 [[package]]
 name = "bitcoin-io"
 version = "0.1.3"
@@ -103,7 +109,7 @@ version = "0.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "5285c8bcaa25876d07f37e3d30c303f2609179716e11d688f51e8f1fe70063e2"
 dependencies = [
- "bitcoin-internals",
+ "bitcoin-internals 0.3.0",
  "serde",
 ]
 
@@ -203,6 +209,7 @@ dependencies = [
  "bech32",
  "bincode",
  "bitcoin",
+ "bitcoin-internals 0.5.0",
  "getrandom 0.2.16",
  "hex-conservative 1.1.0",
  "rand",

Cargo.toml

@@ -29,6 +29,7 @@ base64 = ["bitcoin/base64"]
 [dependencies]
 bech32 = "0.11.0"
 bitcoin = "0.32.2"
+internals = { package = "bitcoin-internals", version = "0.5" }
 secp256k1-zkp = { version = "0.11.0", features = ["global-context", "hashes"] }
 
 # Used for ContractHash::from_json_contract.
@@ -188,6 +189,7 @@ zero_sized_map_values = "warn"
 
 [package.metadata.rbmt.lint]
 allowed_duplicates = [
+    "bitcoin-internals",
     "hex-conservative",
 ]
 

elementsd-tests/Cargo.toml

@@ -16,6 +16,7 @@ rand = "0.8"
 [package.metadata.rbmt.lint]
 # FIXME the bulk of these are because elementsd/bitcoind is much older than rust-bitcoin.
 allowed_duplicates = [
+    "bitcoin-internals",
     "hex-conservative",
     "base64",
     "getrandom",

fuzz/Cargo.toml

@@ -22,6 +22,7 @@ use_self = "warn"
 
 [package.metadata.rbmt.lint]
 allowed_duplicates = [
+    "bitcoin-internals",
     "hex-conservative",
     "getrandom",
     "wasi",

src/address.rs

@@ -26,6 +26,8 @@ use crate::blech32::{Blech32, Blech32m};
 use crate::hashes::Hash;
 use bitcoin::base58;
 use bitcoin::PublicKey;
+use internals::array::ArrayExt as _;
+use internals::slice::SliceExt;
 use secp256k1_zkp;
 use secp256k1_zkp::Secp256k1;
 use secp256k1_zkp::Verification;
@@ -467,13 +469,17 @@ impl Address {
         };
 
         let (blinding_pubkey, program) = match blinded {
-            true => (
+            true => {
+                let (pk, rest) = SliceExt::split_first_chunk::<33>(data.as_slice())
+                    .ok_or(AddressError::InvalidSegwitV0Encoding)?;
+                (
                 Some(
-                    secp256k1_zkp::PublicKey::from_slice(&data[..33])
+                    secp256k1_zkp::PublicKey::from_slice(pk)
                         .map_err(AddressError::InvalidBlindingPubKey)?,
-                ),
-                data[33..].to_vec(),
-            ),
+                    ),
+                    rest.to_vec(),
+                )
+            },
             false => (None, data),
         };
 
@@ -486,35 +492,39 @@ impl Address {
 
     // data.len() should be >= 1 when this method is called
     fn from_base58(data: &[u8], params: &'static AddressParams) -> Result<Address, AddressError> {
+        let len_error = AddressError::InvalidLength(data.len());
         // When unblinded, the structure is:
         // <1: regular prefix> <20: hash160>
         // When blinded, the structure is:
         // <1: blinding prefix> <1: regular prefix> <33: blinding pubkey> <20: hash160>
 
-        let blinded = data[0] == params.blinded_prefix;
-        let prefix = match (blinded, data.len()) {
-            (true, 55) => data[1],
-            (false, 21) => data[0],
-            (_, len) => return Err(AddressError::InvalidLength(len)),
+        let Some((blinding_prefix, blinded_data)) = data.split_first() else {
+            return Err(len_error);
         };
 
-        let (blinding_pubkey, payload_data) = match blinded {
-            true => (
-                Some(
-                    secp256k1_zkp::PublicKey::from_slice(&data[2..35])
-                        .map_err(AddressError::InvalidBlindingPubKey)?,
-                ),
-                &data[35..],
-            ),
-            false => (None, &data[1..]),
+        let (prefix, blinding_pubkey, hash) = if *blinding_prefix == params.blinded_prefix {
+            let Some((prefix, pubkey_and_hash)) = blinded_data.split_first() else {
+                return Err(len_error);
+            };
+
+            let pubkey_and_hash = <&[u8; 53]>::try_from(pubkey_and_hash).map_err(|_| len_error)?;
+            let (pubkey, hash) = pubkey_and_hash.split_array::<33, 20>();
+
+            let blinding_pubkey = secp256k1_zkp::PublicKey::from_slice(pubkey)
+                .map_err(AddressError::InvalidBlindingPubKey)?;
+
+            (prefix, Some(blinding_pubkey), hash)
+        } else {
+            let hash = <&[u8; 20]>::try_from(blinded_data).map_err(|_| len_error)?;
+            (blinding_prefix, None, hash)
         };
 
-        let payload = if prefix == params.p2pkh_prefix {
-            Payload::PubkeyHash(PubkeyHash::from_slice(payload_data).unwrap())
-        } else if prefix == params.p2sh_prefix {
-            Payload::ScriptHash(ScriptHash::from_slice(payload_data).unwrap())
+        let payload = if *prefix == params.p2pkh_prefix {
+            Payload::PubkeyHash(PubkeyHash::from_byte_array(*hash))
+        } else if *prefix == params.p2sh_prefix {
+            Payload::ScriptHash(ScriptHash::from_byte_array(*hash))
         } else {
-            return Err(AddressError::InvalidAddressVersion(prefix));
+            return Err(AddressError::InvalidAddressVersion(*prefix));
         };
 
         Ok(Address {

src/blind.rs

@@ -15,7 +15,8 @@
 //! # Transactions Blinding
 //!
 
-use core::convert::TryFrom;
+use internals::array::ArrayExt as _;
+use internals::slice::SliceExt;
 use std::{self, collections::BTreeMap, fmt};
 
 use secp256k1_zkp::{
@@ -761,16 +762,31 @@ impl TxOut {
             additional_generator,
         )?;
 
-        let (asset, asset_bf) = opening.message.as_ref().split_at(32);
-        let asset = <[u8; 32]>::try_from(asset).map_err(UnblindError::MalformedAssetId)?;
-        let asset = AssetId::from_byte_array(asset);
-        let asset_bf = AssetBlindingFactor::from_slice(&asset_bf[..32])?;
+        // Use `MissingRangeproof` error because it's available so does not require
+        // API breaks. In a later PR we should extend that enum and add #[non_exhaustive]
+        // to it. The maybe-better `MalformedAssetId` error requires we start with a
+        // std `FromSliceError` which we don't have.
+        let asset_and_bf = SliceExt::split_first_chunk::<64>(opening.message.as_ref())
+            .ok_or(UnblindError::MissingRangeproof)?
+            .0;
+        let (asset_id, asset_bf) = asset_and_bf.split_array();
+
+        let asset_id = AssetId::from_byte_array(*asset_id);
+        let asset_bf = AssetBlindingFactor::from_byte_array(*asset_bf)?;
+        if let Asset::Confidential(own_asset) = self.asset {
+            let secp = Secp256k1::signing_only(); // needed to avoid API break
+            let asset = Generator::new_blinded(&secp, asset_id.into_tag(), asset_bf.into_inner());
+            if asset != own_asset {
+                // See above about use of MissingRangeproof.
+                return Err(UnblindError::MissingRangeproof);
+            }
+        }
 
         let value = opening.value;
         let value_bf = ValueBlindingFactor(opening.blinding_factor);
 
         Ok(TxOutSecrets {
-            asset,
+            asset: asset_id,
             asset_bf,
             value,
             value_bf,

src/confidential.rs

@@ -789,6 +789,11 @@ impl AssetBlindingFactor {
         s.parse()
     }
 
+    /// Create from bytes.
+    pub fn from_byte_array(bytes: [u8; 32]) -> Result<Self, secp256k1_zkp::Error> {
+        Ok(AssetBlindingFactor(Tweak::from_inner(bytes)?))
+    }
+
     /// Create from bytes.
     pub fn from_slice(bytes: &[u8]) -> Result<Self, secp256k1_zkp::Error> {
         Ok(AssetBlindingFactor(Tweak::from_slice(bytes)?))

src/pset/serialize.rs

@@ -17,7 +17,6 @@
 //! Defines traits used for (de)serializing PSET values into/from raw
 //! bytes in PSET key-value pairs.
 
-use std::convert::TryFrom;
 use std::io;
 
 use crate::confidential::{self, AssetBlindingFactor};
@@ -29,6 +28,7 @@ use crate::{AssetId, BlockHash, Script, Transaction, TxOut, Txid};
 use bitcoin;
 use bitcoin::bip32::{ChildNumber, Fingerprint, KeySource};
 use bitcoin::{key::XOnlyPublicKey, PublicKey};
+use internals::slice::SliceExt;
 use secp256k1_zkp::{self, RangeProof, SurjectionProof, Tweak};
 
 use super::map::{PsbtSighashType, TapTree};
@@ -176,16 +176,15 @@ impl Serialize for KeySource {
 
 impl Deserialize for KeySource {
     fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error> {
-        let Ok(prefix) = <[u8; 4]>::try_from(&bytes[0..4]) else {
+        let Some((prefix, mut rest)) = SliceExt::split_first_chunk::<4>(bytes) else {
             return Err(io::Error::from(io::ErrorKind::UnexpectedEof).into());
         };
 
         let fprint: Fingerprint = Fingerprint::from(prefix);
         let mut dpath: Vec<ChildNumber> = Vec::default();
 
-        let mut d = &bytes[4..];
-        while !d.is_empty() {
-            let index = u32::consensus_decode(&mut d)?;
+        while !rest.is_empty() {
+            let index = u32::consensus_decode(&mut rest)?;
             dpath.push(index.into());
         }
 

src/transaction.rs

@@ -20,6 +20,7 @@ use std::collections::HashMap;
 use std::convert::TryFrom;
 
 use bitcoin::{self, VarInt};
+use internals::slice::SliceExt;
 use crate::hashes::Hash;
 
 use crate::{confidential, ContractHash};
@@ -432,12 +433,12 @@ impl<'tx> PeginData<'tx> {
         pegin_witness: &'tx [Vec<u8>],
         prevout: bitcoin::OutPoint,
     ) -> Result<PeginData<'tx>, &'static str> {
-        if pegin_witness.len() != 6 {
+        let Ok(pegin_witness) = <&[Vec<u8>; 6]>::try_from(pegin_witness)  else {
             return Err("size not 6");
-        }
-        if pegin_witness[5].len() < 80 {
+        };
+        let Some((block_header, _)) = SliceExt::split_first_chunk::<80>(pegin_witness[5].as_slice()) else {
             return Err("merkle proof too short");
-        }
+        };
 
         Ok(PeginData {
             outpoint: prevout,
@@ -448,7 +449,7 @@ impl<'tx> PeginData<'tx> {
             claim_script: &pegin_witness[3],
             tx: &pegin_witness[4],
             merkle_proof: &pegin_witness[5],
-            referenced_block: bitcoin::BlockHash::hash(&pegin_witness[5][0..80]),
+            referenced_block: bitcoin::BlockHash::hash(block_header),
         })
     }