Skip to content

Commit cb56c8f

Browse files
committed
Merge #281: Backport slicing improvements 0.26.x
6fee387 Bump version to 0.26.2 (Philip Robinson) 4428361 pset: fix slicing in KeySource::deserialize (Philip Robinson) 6e082a9 address: do a better job slicing bech32 data (Philip Robinson) acf5bc8 transaction: use better error typing for pegin destructuring (Philip Robinson) 6659691 blind: use array splitting in TxOut::unblind (fix potential DoS?) (Philip Robinson) 6005720 rewrite Address::from_base58 to eliminate all the unwraps (Andrew Poelstra) d216a7c Add arrays.rs and slices.rs from bitcoin-internals 0.5 (Philip Robinson) Pull request description: There are a number of useful improvements from #279 we would like to include in 0.26.x. ACKs for top commit: apoelstra: ACK 6fee387; successfully ran local tests Tree-SHA512: dd5d142dd76cbe472c67823989f79ed1bb1024282b788cd840bd4cf71db95ef277aa4eeb416ebe829a01f4dd22ed672dbc293e181bddaa9ccb0814e47dd1cc19
2 parents f6342bc + 6fee387 commit cb56c8f

12 files changed

Lines changed: 391 additions & 47 deletions

File tree

Cargo-latest.lock

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ checksum = "48c757948c5ede0e46177b7add2e67155f70e33c07fea8284df6576da70b3719"
190190

191191
[[package]]
192192
name = "elements"
193-
version = "0.26.1"
193+
version = "0.26.2"
194194
dependencies = [
195195
"bech32",
196196
"bincode",

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "elements"
3-
version = "0.26.1"
3+
version = "0.26.2"
44
authors = ["Andrew Poelstra <apoelstra@blockstream.com>"]
55
description = "Library with support for de/serialization, parsing and executing on data structures and network messages related to Elements"
66
license = "CC0-1.0"

src/address.rs

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ use crate::blech32::{Blech32, Blech32m};
2626
use crate::hashes::Hash;
2727
use bitcoin::base58;
2828
use bitcoin::PublicKey;
29+
use crate::internals::array::ArrayExt as _;
30+
use crate::internals::slice::SliceExt;
2931
use secp256k1_zkp;
3032
use secp256k1_zkp::Secp256k1;
3133
use secp256k1_zkp::Verification;
@@ -470,13 +472,17 @@ impl Address {
470472
};
471473

472474
let (blinding_pubkey, program) = match blinded {
473-
true => (
475+
true => {
476+
let (pk, rest) = SliceExt::split_first_chunk::<33>(data.as_slice())
477+
.ok_or(AddressError::InvalidSegwitV0Encoding)?;
478+
(
474479
Some(
475-
secp256k1_zkp::PublicKey::from_slice(&data[..33])
480+
secp256k1_zkp::PublicKey::from_slice(pk)
476481
.map_err(AddressError::InvalidBlindingPubKey)?,
477-
),
478-
data[33..].to_vec(),
479-
),
482+
),
483+
rest.to_vec(),
484+
)
485+
},
480486
false => (None, data),
481487
};
482488

@@ -489,35 +495,41 @@ impl Address {
489495

490496
// data.len() should be >= 1 when this method is called
491497
fn from_base58(data: &[u8], params: &'static AddressParams) -> Result<Address, AddressError> {
498+
let len_error = AddressError::InvalidLength(data.len());
492499
// When unblinded, the structure is:
493500
// <1: regular prefix> <20: hash160>
494501
// When blinded, the structure is:
495502
// <1: blinding prefix> <1: regular prefix> <33: blinding pubkey> <20: hash160>
496503

497-
let blinded = data[0] == params.blinded_prefix;
498-
let prefix = match (blinded, data.len()) {
499-
(true, 55) => data[1],
500-
(false, 21) => data[0],
501-
(_, len) => return Err(AddressError::InvalidLength(len)),
504+
let (blinding_prefix, blinded_data) = match data.split_first() {
505+
Some(v) => v,
506+
None => return Err(len_error),
502507
};
503508

504-
let (blinding_pubkey, payload_data) = match blinded {
505-
true => (
506-
Some(
507-
secp256k1_zkp::PublicKey::from_slice(&data[2..35])
508-
.map_err(AddressError::InvalidBlindingPubKey)?,
509-
),
510-
&data[35..],
511-
),
512-
false => (None, &data[1..]),
509+
let (prefix, blinding_pubkey, hash) = if *blinding_prefix == params.blinded_prefix {
510+
let (prefix, pubkey_and_hash) = match blinded_data.split_first() {
511+
Some(v) => v,
512+
None => return Err(len_error),
513+
};
514+
515+
let pubkey_and_hash = <&[u8; 53]>::try_from(pubkey_and_hash).map_err(|_| len_error)?;
516+
let (pubkey, hash) = pubkey_and_hash.split_array::<33, 20>();
517+
518+
let blinding_pubkey = secp256k1_zkp::PublicKey::from_slice(pubkey)
519+
.map_err(AddressError::InvalidBlindingPubKey)?;
520+
521+
(prefix, Some(blinding_pubkey), hash)
522+
} else {
523+
let hash = <&[u8; 20]>::try_from(blinded_data).map_err(|_| len_error)?;
524+
(blinding_prefix, None, hash)
513525
};
514526

515-
let payload = if prefix == params.p2pkh_prefix {
516-
Payload::PubkeyHash(PubkeyHash::from_slice(payload_data).unwrap())
517-
} else if prefix == params.p2sh_prefix {
518-
Payload::ScriptHash(ScriptHash::from_slice(payload_data).unwrap())
527+
let payload = if *prefix == params.p2pkh_prefix {
528+
Payload::PubkeyHash(PubkeyHash::from_byte_array(*hash))
529+
} else if *prefix == params.p2sh_prefix {
530+
Payload::ScriptHash(ScriptHash::from_byte_array(*hash))
519531
} else {
520-
return Err(AddressError::InvalidAddressVersion(prefix));
532+
return Err(AddressError::InvalidAddressVersion(*prefix));
521533
};
522534

523535
Ok(Address {

src/blind.rs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
//! # Transactions Blinding
1616
//!
1717
18+
use crate::internals::array::ArrayExt as _;
19+
use crate::internals::slice::SliceExt;
1820
use std::{self, collections::BTreeMap, fmt};
1921

2022
use secp256k1_zkp::{
@@ -775,15 +777,31 @@ impl TxOut {
775777
additional_generator,
776778
)?;
777779

778-
let (asset, asset_bf) = opening.message.as_ref().split_at(32);
779-
let asset = AssetId::from_slice(asset)?;
780-
let asset_bf = AssetBlindingFactor::from_slice(&asset_bf[..32])?;
780+
// Use `MissingRangeproof` error because it's available so does not require
781+
// API breaks. In a later PR we should extend that enum and add #[non_exhaustive]
782+
// to it. The maybe-better `MalformedAssetId` error requires we start with a
783+
// std `FromSliceError` which we don't have.
784+
let asset_and_bf = SliceExt::split_first_chunk::<64>(opening.message.as_ref())
785+
.ok_or(UnblindError::MissingRangeproof)?
786+
.0;
787+
let (asset_id, asset_bf) = asset_and_bf.split_array();
788+
789+
let asset_id = AssetId::from_byte_array(*asset_id);
790+
let asset_bf = AssetBlindingFactor::from_byte_array(*asset_bf)?;
791+
if let Asset::Confidential(own_asset) = self.asset {
792+
let secp = Secp256k1::signing_only(); // needed to avoid API break
793+
let asset = Generator::new_blinded(&secp, asset_id.into_tag(), asset_bf.into_inner());
794+
if asset != own_asset {
795+
// See above about use of MissingRangeproof.
796+
return Err(UnblindError::MissingRangeproof);
797+
}
798+
}
781799

782800
let value = opening.value;
783801
let value_bf = ValueBlindingFactor(opening.blinding_factor);
784802

785803
Ok(TxOutSecrets {
786-
asset,
804+
asset: asset_id,
787805
asset_bf,
788806
value,
789807
value_bf,

src/confidential.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -730,6 +730,11 @@ impl AssetBlindingFactor {
730730
AssetBlindingFactor(Tweak::new(rng))
731731
}
732732

733+
/// Create from bytes.
734+
pub fn from_byte_array(bytes: [u8; 32]) -> Result<Self, secp256k1_zkp::Error> {
735+
Ok(AssetBlindingFactor(Tweak::from_inner(bytes)?))
736+
}
737+
733738
/// Create from bytes.
734739
pub fn from_slice(bytes: &[u8]) -> Result<Self, secp256k1_zkp::Error> {
735740
Ok(AssetBlindingFactor(Tweak::from_slice(bytes)?))

src/internals/array.rs

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
//! Contains extensions related to arrays.
2+
3+
use std::convert::TryInto;
4+
5+
/// Extension trait for arrays.
6+
pub trait ArrayExt {
7+
/// The item type the array is storing.
8+
type Item;
9+
10+
/// Just like the slicing operation, this returns an array `LEN` items long at position
11+
/// `OFFSET`.
12+
///
13+
/// The correctness of this operation is compile-time checked.
14+
///
15+
/// Note that unlike slicing where the second number is the end index, here the second number
16+
/// is array length!
17+
fn sub_array<const OFFSET: usize, const LEN: usize>(&self) -> &[Self::Item; LEN];
18+
19+
/// Returns an item at given statically-known index.
20+
///
21+
/// This is just like normal indexing except the check happens at compile time.
22+
fn get_static<const INDEX: usize>(&self) -> &Self::Item { &self.sub_array::<INDEX, 1>()[0] }
23+
24+
/// Returns the first item in an array.
25+
///
26+
/// Fails to compile if the array is empty.
27+
///
28+
/// Note that this method's name intentionally shadows the `std`'s `first` method which
29+
/// returns `Option`. The rationale is that given the known length of the array, we always know
30+
/// that this will not return `None` so trying to keep the `std` method around is pointless.
31+
/// Importing the trait will also cause compile failures - that's also intentional to expose
32+
/// the places where useless checks are made.
33+
fn first(&self) -> &Self::Item { self.get_static::<0>() }
34+
35+
/// Splits the array into two, non-overlapping smaller arrays covering the entire range.
36+
///
37+
/// This is almost equivalent to just calling [`sub_array`](Self::sub_array) twice, except it also
38+
/// checks that the arrays don't overlap and that they cover the full range. This is very useful
39+
/// for demonstrating correctness, especially when chained. Using this technique even revealed
40+
/// a bug in the past. ([#4195](https://github.com/rust-bitcoin/rust-bitcoin/issues/4195))
41+
fn split_array<const LEFT: usize, const RIGHT: usize>(
42+
&self,
43+
) -> (&[Self::Item; LEFT], &[Self::Item; RIGHT]);
44+
45+
/// Splits the array into the first element and the remaining, one element shorter, array.
46+
///
47+
/// Fails to compile if the array is empty.
48+
///
49+
/// Note that this method's name intentionally shadows the `std`'s `split_first` method which
50+
/// returns `Option`. The rationale is that given the known length of the array, we always know
51+
/// that this will not return `None` so trying to keep the `std` method around is pointless.
52+
/// Importing the trait will also cause compile failures - that's also intentional to expose
53+
/// the places where useless checks are made.
54+
fn split_first<const RIGHT: usize>(&self) -> (&Self::Item, &[Self::Item; RIGHT]) {
55+
let (first, remaining) = self.split_array::<1, RIGHT>();
56+
(&first[0], remaining)
57+
}
58+
59+
/// Splits the array into the last element and the remaining, one element shorter, array.
60+
///
61+
/// Fails to compile if the array is empty.
62+
///
63+
/// Note that this method's name intentionally shadows the `std`'s `split_last` method which
64+
/// returns `Option`. The rationale is that given the known length of the array, we always know
65+
/// that this will not return `None` so trying to keep the `std` method around is pointless.
66+
/// Importing the trait will also cause compile failures - that's also intentional to expose
67+
/// the places where useless checks are made.
68+
///
69+
/// The returned tuple is also reversed just as `std` for consistency and simpler diffs when
70+
/// migrating.
71+
fn split_last<const LEFT: usize>(&self) -> (&Self::Item, &[Self::Item; LEFT]) {
72+
let (remaining, last) = self.split_array::<LEFT, 1>();
73+
(&last[0], remaining)
74+
}
75+
}
76+
77+
impl<const N: usize, T> ArrayExt for [T; N] {
78+
type Item = T;
79+
80+
fn sub_array<const OFFSET: usize, const LEN: usize>(&self) -> &[Self::Item; LEN] {
81+
#[allow(clippy::let_unit_value)]
82+
let () = Hack::<N, OFFSET, LEN>::IS_VALID_RANGE;
83+
84+
self[OFFSET..(OFFSET + LEN)].try_into().expect("this is also compiler-checked above")
85+
}
86+
87+
fn split_array<const LEFT: usize, const RIGHT: usize>(
88+
&self,
89+
) -> (&[Self::Item; LEFT], &[Self::Item; RIGHT]) {
90+
#[allow(clippy::let_unit_value)]
91+
let () = Hack2::<N, LEFT, RIGHT>::IS_FULL_RANGE;
92+
93+
(self.sub_array::<0, LEFT>(), self.sub_array::<LEFT, RIGHT>())
94+
}
95+
}
96+
97+
struct Hack<const N: usize, const OFFSET: usize, const LEN: usize>;
98+
99+
impl<const N: usize, const OFFSET: usize, const LEN: usize> Hack<N, OFFSET, LEN> {
100+
const IS_VALID_RANGE: () = assert!(OFFSET + LEN <= N);
101+
}
102+
103+
struct Hack2<const N: usize, const LEFT: usize, const RIGHT: usize>;
104+
105+
impl<const N: usize, const LEFT: usize, const RIGHT: usize> Hack2<N, LEFT, RIGHT> {
106+
const IS_FULL_RANGE: () = assert!(LEFT + RIGHT == N);
107+
}

src/internals/mod.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// SPDX-License-Identifier: CC0-1.0
2+
3+
//! These modules are copied from the bitcoin-internals 0.5 crate from rust-bitcoin
4+
pub mod array;
5+
pub mod slice;

0 commit comments

Comments
 (0)