Skip to content

Commit 572903c

Browse files
dr-orlovskydr-orlovsky
committed
Merge rust-bitcoin/rust-bitcoin#776: Change EcdsaSig hash type deser in psbt
abe52f6 Cleanup/Dedup psbt (De)Serialization code (sanket1729) fbd86dc Update documentation of EcdsaSig::from_slice (sanket1729) 85009a7 Update documentation of from_u32_consensus (sanket1729) 0fed04e Change EcdsaSig hash type deser (sanket1729) Pull request description: Changes the parsing behavior in PSBT on non-standard sighash types to give an explicit error, rather than silently mangling the parsed value ACKs for top commit: dr-orlovsky: ACK abe52f6 apoelstra: ACK abe52f6 Kixunil: ACK abe52f6 Tree-SHA512: 1d5dbe3aa5885ca16649cf8ea05a7476e8dd977dd870b79358d97a3ce383bee93754d2b88163e7db3792cdc4b9cb867356409c8eea4e110877577ad196ba0786
2 parents 2494af9 + 5eb93c0 commit 572903c

File tree

4 files changed

+49
-38
lines changed

4 files changed

+49
-38
lines changed

‎src/blockdata/transaction.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -758,6 +758,10 @@ impl EcdsaSigHashType {
758758
///
759759
/// **Note**: this replicates consensus behaviour, for current standardness rules correctness
760760
/// you probably want [Self::from_u32_standard].
761+
/// This might cause unexpected behavior because it does not roundtrip. That is,
762+
/// `EcdsaSigHashType::from_u32_consensus(n) as u32 != n` for non-standard values of
763+
/// `n`. While verifying signatures, the user should retain the `n` and use it compute the
764+
/// signature hash message.
761765
pub fn from_u32_consensus(n: u32) -> EcdsaSigHashType {
762766
// In Bitcoin Core, the SignatureHash function will mask the (int32) value with
763767
// 0x1f to (apparently) deactivate ACP when checking for SINGLE and NONE bits.

‎src/util/ecdsa.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
1919
use prelude::*;
2020
use core::str::FromStr;
21-
use core::fmt;
21+
use core::{fmt, iter};
2222
use hashes::hex::{self, FromHex};
2323
use blockdata::transaction::NonStandardSigHashType;
2424
use secp256k1;
@@ -43,7 +43,7 @@ impl EcdsaSig {
4343
}
4444
}
4545

46-
/// Deserialize from slice
46+
/// Deserialize from slice following the standardness rules for [`EcdsaSigHashType`]
4747
pub fn from_slice(sl: &[u8]) -> Result<Self, EcdsaSigError> {
4848
let (hash_ty, sig) = sl.split_last()
4949
.ok_or(EcdsaSigError::EmptySignature)?;
@@ -57,9 +57,10 @@ impl EcdsaSig {
5757
/// Serialize EcdsaSig
5858
pub fn to_vec(&self) -> Vec<u8> {
5959
// TODO: add support to serialize to a writer to SerializedSig
60-
let mut ser_sig = self.sig.serialize_der().to_vec();
61-
ser_sig.push(self.hash_ty.as_u32() as u8);
62-
ser_sig
60+
self.sig.serialize_der()
61+
.iter().map(|x| *x)
62+
.chain(iter::once(self.hash_ty as u8))
63+
.collect()
6364
}
6465
}
6566

‎src/util/psbt/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -823,7 +823,7 @@ mod tests {
823823
let err = hex_psbt!("70736274ff010071020000000127744ababf3027fe0d6cf23a96eee2efb188ef52301954585883e69b6624b2420000000000ffffffff02787c01000000000016001483a7e34bd99ff03a4962ef8a1a101bb295461ece606b042a010000001600147ac369df1b20e033d6116623957b0ac49f3c52e8000000000001012b00f2052a010000002251205a2c2cf5b52cf31f83ad2e8da63ff03183ecd8f609c7510ae8a48e03910a075701172102fe349064c98d6e2a853fa3c9b12bd8b304a19c195c60efa7ee2393046d3fa232000000").unwrap_err();
824824
assert_eq!(err.to_string(), "parse failed: Invalid xonly public key");
825825
let err = hex_psbt!("70736274ff010071020000000127744ababf3027fe0d6cf23a96eee2efb188ef52301954585883e69b6624b2420000000000ffffffff02787c01000000000016001483a7e34bd99ff03a4962ef8a1a101bb295461ece606b042a010000001600147ac369df1b20e033d6116623957b0ac49f3c52e8000000000001012b00f2052a010000002251205a2c2cf5b52cf31f83ad2e8da63ff03183ecd8f609c7510ae8a48e03910a0757011342173bb3d36c074afb716fec6307a069a2e450b995f3c82785945ab8df0e24260dcd703b0cbf34de399184a9481ac2b3586db6601f026a77f7e4938481bc34751701aa000000").unwrap_err();
826-
assert_eq!(err.to_string(), "parse failed: Invalid Schnorr signature len");
826+
assert_eq!(err.to_string(), "parse failed: Invalid Schnorr signature length");
827827
let err = hex_psbt!("70736274ff010071020000000127744ababf3027fe0d6cf23a96eee2efb188ef52301954585883e69b6624b2420000000000ffffffff02787c01000000000016001483a7e34bd99ff03a4962ef8a1a101bb295461ece606b042a010000001600147ac369df1b20e033d6116623957b0ac49f3c52e8000000000001012b00f2052a010000002251205a2c2cf5b52cf31f83ad2e8da63ff03183ecd8f609c7510ae8a48e03910a0757221602fe349064c98d6e2a853fa3c9b12bd8b304a19c195c60efa7ee2393046d3fa2321900772b2da75600008001000080000000800100000000000000000000").unwrap_err();
828828
assert_eq!(err.to_string(), "parse failed: Invalid xonly public key");
829829
let err = hex_psbt!("70736274ff01007d020000000127744ababf3027fe0d6cf23a96eee2efb188ef52301954585883e69b6624b2420000000000ffffffff02887b0100000000001600142382871c7e8421a00093f754d91281e675874b9f606b042a010000002251205a2c2cf5b52cf31f83ad2e8da63ff03183ecd8f609c7510ae8a48e03910a0757000000000001012b00f2052a010000002251205a2c2cf5b52cf31f83ad2e8da63ff03183ecd8f609c7510ae8a48e03910a0757000001052102fe349064c98d6e2a853fa3c9b12bd8b304a19c195c60efa7ee2393046d3fa23200").unwrap_err();
@@ -833,9 +833,9 @@ mod tests {
833833
let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b6924214022cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2cd970e15f53fc0c82f950fd560ffa919b76172be017368a89913af074f400b094089756aa3739ccc689ec0fcf3a360be32cc0b59b16e93a1e8bb4605726b2ca7a3ff706c4176649632b2cc68e1f912b8a578e3719ce7710885c7a966f49bcd43cb0000").unwrap_err();
834834
assert_eq!(err.to_string(), "PSBT error: Hash Parse Error: bad slice length 33 (expected 32)");
835835
let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b69241142cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2cd970e15f53fc0c82f950fd560ffa919b76172be017368a89913af074f400b094289756aa3739ccc689ec0fcf3a360be32cc0b59b16e93a1e8bb4605726b2ca7a3ff706c4176649632b2cc68e1f912b8a578e3719ce7710885c7a966f49bcd43cb01010000").unwrap_err();
836-
assert_eq!(err.to_string(), "parse failed: Invalid Schnorr signature len");
836+
assert_eq!(err.to_string(), "parse failed: Invalid Schnorr signature length");
837837
let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b69241142cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2cd970e15f53fc0c82f950fd560ffa919b76172be017368a89913af074f400b093989756aa3739ccc689ec0fcf3a360be32cc0b59b16e93a1e8bb4605726b2ca7a3ff706c4176649632b2cc68e1f912b8a578e3719ce7710885c7a966f49bcd43cb0000").unwrap_err();
838-
assert_eq!(err.to_string(), "parse failed: Invalid Schnorr signature len");
838+
assert_eq!(err.to_string(), "parse failed: Invalid Schnorr signature length");
839839
let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b6926315c150929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac06f7d62059e9497a1a4a267569d9876da60101aff38e3529b9b939ce7f91ae970115f2e490af7cc45c4f78511f36057ce5c5a5c56325a29fb44dfc203f356e1f80023202cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2acc00000").unwrap_err();
840840
assert_eq!(err.to_string(), "parse failed: Invalid control block");
841841
let err = hex_psbt!("70736274ff01005e02000000019bd48765230bf9a72e662001f972556e54f0c6f97feb56bcb5600d817f6995260100000000ffffffff0148e6052a01000000225120030da4fce4f7db28c2cb2951631e003713856597fe963882cb500e68112cca63000000000001012b00f2052a01000000225120c2247efbfd92ac47f6f40b8d42d169175a19fa9fa10e4a25d7f35eb4dd85b6926115c150929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac06f7d62059e9497a1a4a267569d9876da60101aff38e3529b9b939ce7f91ae970115f2e490af7cc45c4f78511f36057ce5c5a5c56325a29fb44dfc203f356e123202cb13ac68248de806aa6a3659cf3c03eb6821d09c8114a4e868febde865bb6d2acc00000").unwrap_err();

‎src/util/psbt/serialize.rs

Lines changed: 36 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,19 @@ use io;
2424

2525
use blockdata::script::Script;
2626
use blockdata::witness::Witness;
27-
use blockdata::transaction::{EcdsaSigHashType, Transaction, TxOut};
27+
use blockdata::transaction::{Transaction, TxOut};
2828
use consensus::encode::{self, serialize, Decodable, Encodable, deserialize_partial};
2929
use secp256k1::{self, XOnlyPublicKey};
3030
use util::bip32::{ChildNumber, Fingerprint, KeySource};
3131
use hashes::{hash160, ripemd160, sha256, sha256d, Hash};
32-
use util::ecdsa::EcdsaSig;
32+
use util::ecdsa::{EcdsaSig, EcdsaSigError};
33+
use util::psbt;
3334
use util::taproot::{TapBranchHash, TapLeafHash, ControlBlock, LeafVersion};
3435
use schnorr;
36+
3537
use super::map::{TapTree, PsbtSigHashType};
3638

3739
use util::taproot::TaprootBuilder;
38-
use util::sighash::SchnorrSigHashType;
3940
/// A trait for serializing a value as raw data for insertion into PSBT
4041
/// key-value pairs.
4142
pub trait Serialize {
@@ -89,25 +90,35 @@ impl Deserialize for secp256k1::PublicKey {
8990

9091
impl Serialize for EcdsaSig {
9192
fn serialize(&self) -> Vec<u8> {
92-
let mut buf = Vec::with_capacity(72);
93-
buf.extend(self.sig.serialize_der().iter());
94-
buf.push(self.hash_ty as u8);
95-
buf
93+
self.to_vec()
9694
}
9795
}
9896

9997
impl Deserialize for EcdsaSig {
10098
fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error> {
101-
let (sighash_byte, signature) = bytes.split_last()
102-
.ok_or(encode::Error::ParseFailed("empty partial signature data"))?;
103-
Ok(EcdsaSig {
104-
sig: secp256k1::ecdsa::Signature::from_der(signature)
105-
.map_err(|_| encode::Error::ParseFailed("non-DER encoded signature"))?,
106-
// NB: Since BIP-174 says "the signature as would be pushed to the stack from
107-
// a scriptSig or witness" we should use a consensus deserialization and do
108-
// not error on a non-standard values.
109-
hash_ty: EcdsaSigHashType::from_u32_consensus(*sighash_byte as u32)
110-
})
99+
// NB: Since BIP-174 says "the signature as would be pushed to the stack from
100+
// a scriptSig or witness" we should ideally use a consensus deserialization and do
101+
// not error on a non-standard values. However,
102+
//
103+
// 1) the current implementation of from_u32_consensus(`flag`) does not preserve
104+
// the sighash byte `flag` mapping all unknown values to EcdsaSighashType::All or
105+
// EcdsaSigHashType::AllPlusAnyOneCanPay. Therefore, break the invariant
106+
// EcdsaSig::from_slice(&sl[..]).to_vec = sl.
107+
//
108+
// 2) This would cause to have invalid signatures because the sighash message
109+
// also has a field sighash_u32 (See BIP141). For example, when signing with non-standard
110+
// 0x05, the sighash message would have the last field as 0x05u32 while, the verification
111+
// would use check the signature assuming sighash_u32 as `0x01`.
112+
match EcdsaSig::from_slice(&bytes) {
113+
Ok(sig) => Ok(sig),
114+
Err(EcdsaSigError::EmptySignature) =>
115+
Err(encode::Error::ParseFailed("Empty partial signature data")),
116+
Err(EcdsaSigError::NonStandardSigHashType(flag)) =>
117+
Err(encode::Error::from(psbt::Error::NonStandardSigHashType(flag))),
118+
Err(EcdsaSigError::Secp256k1(..)) =>
119+
Err(encode::Error::ParseFailed("Invalid Ecdsa signature")),
120+
Err(EcdsaSigError::HexEncoding(..)) => unreachable!("Decoding from slice, not hex")
121+
}
111122
}
112123
}
113124

@@ -194,20 +205,15 @@ impl Serialize for schnorr::SchnorrSig {
194205

195206
impl Deserialize for schnorr::SchnorrSig {
196207
fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error> {
197-
match bytes.len() {
198-
65 => {
199-
let hash_ty = SchnorrSigHashType::from_u8(bytes[64])
200-
.map_err(|_| encode::Error::ParseFailed("Invalid Sighash type"))?;
201-
let sig = secp256k1::schnorr::Signature::from_slice(&bytes[..64])
202-
.map_err(|_| encode::Error::ParseFailed("Invalid Schnorr signature"))?;
203-
Ok(schnorr::SchnorrSig{ sig, hash_ty })
204-
}
205-
64 => {
206-
let sig = secp256k1::schnorr::Signature::from_slice(&bytes[..64])
207-
.map_err(|_| encode::Error::ParseFailed("Invalid Schnorr signature"))?;
208-
Ok(schnorr::SchnorrSig{ sig, hash_ty: SchnorrSigHashType::Default })
208+
match schnorr::SchnorrSig::from_slice(&bytes) {
209+
Ok(sig) => Ok(sig),
210+
Err(schnorr::SchnorrSigError::InvalidSighashType(flag)) => {
211+
Err(encode::Error::from(psbt::Error::NonStandardSigHashType(flag as u32)))
209212
}
210-
_ => Err(encode::Error::ParseFailed("Invalid Schnorr signature len"))
213+
Err(schnorr::SchnorrSigError::InvalidSchnorrSigSize(_)) =>
214+
Err(encode::Error::ParseFailed("Invalid Schnorr signature length")),
215+
Err(schnorr::SchnorrSigError::Secp256k1(..)) =>
216+
Err(encode::Error::ParseFailed("Invalid Schnorr signature")),
211217
}
212218
}
213219
}

0 commit comments

Comments
 (0)