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.

Sign up for GitHub

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

Jump to bottom

Explicitly error on non-standard sighash types #233

Merged
merged 1 commit into from
Jul 8, 2021
Merged

Explicitly error on non-standard sighash types #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/interpreter/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// If not, see <http://creativecommons.org/publicdomain/zero/1.0/>.
//

use bitcoin::hashes::hash160;
use bitcoin::hashes::{hash160, hex::ToHex};
use bitcoin::{self, secp256k1};
use std::{error, fmt};

Expand All @@ -39,6 +39,8 @@ pub enum Error {
InsufficientSignaturesMultiSig,
/// Signature failed to verify
InvalidSignature(bitcoin::PublicKey),
/// Last byte of this signature isn't a standard sighash type
NonStandardSigHash(Vec<u8>),
/// Miniscript error
Miniscript(::Error),
/// MultiSig requires 1 extra zero element apart from the `k` signatures
Expand Down Expand Up @@ -128,6 +130,13 @@ impl fmt::Display for Error {
Error::IncorrectWScriptHash => f.write_str("witness script did not match scriptpubkey"),
Error::InsufficientSignaturesMultiSig => f.write_str("Insufficient signatures for CMS"),
Error::InvalidSignature(pk) => write!(f, "bad signature with pk {}", pk),
Error::NonStandardSigHash(ref sig) => {
write!(
f,
"Non standard sighash type for signature '{}'",
sig.to_hex()
)
}
Error::NonEmptyWitness => f.write_str("legacy spend had nonempty witness"),
Error::NonEmptyScriptSig => f.write_str("segwit spend had nonempty scriptsig"),
Error::Miniscript(ref e) => write!(f, "parse error: {}", e),
Expand Down
3 changes: 2 additions & 1 deletion src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,8 @@ where
F: FnOnce(&bitcoin::PublicKey, BitcoinSig) -> bool,
{
if let Some((sighash_byte, sig)) = sigser.split_last() {
let sighashtype = bitcoin::SigHashType::from_u32(*sighash_byte as u32);
let sighashtype = bitcoin::SigHashType::from_u32_standard(*sighash_byte as u32)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the interpreter should use the from_u32_consensus. It is used to interpret the transactions that are already present in the blockchain. The main use is for block explorers which should accept non-standard transactions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but i thought we shouldn't be parsing invalid-by-standardness Script? Or is it called by parse_insane_*?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of parse_insane was to parse non-safe miniscripts or miniscripts containing duplicate keys or miniscripts possibly exceeding resource limits while satisfaction.

parse_insane does not currently support parsing non-standard scripts, so what you are doing in this PR seems correct. Having a non-standard miniscript is out of scope for now. This is can be addressed later(while we address #165)

.map_err(|_| Error::NonStandardSigHash([sig, &[*sighash_byte]].concat().to_vec()))?;
let sig = secp256k1::Signature::from_der(sig)?;
if verify_sig(pk, (sig, sighashtype)) {
Ok(sig)
Expand Down
6 changes: 3 additions & 3 deletions src/miniscript/satisfy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ use miniscript::limits::{
HEIGHT_TIME_THRESHOLD, SEQUENCE_LOCKTIME_DISABLE_FLAG, SEQUENCE_LOCKTIME_TYPE_FLAG,
};
use util::witness_size;
use Error;
use Miniscript;
use ScriptContext;
use Terminal;
Expand All @@ -43,9 +42,10 @@ pub type Preimage32 = [u8; 32];
/// Helper function to create BitcoinSig from Rawsig
/// Useful for downstream when implementing Satisfier.
/// Returns underlying secp if the Signature is not of correct format
pub fn bitcoinsig_from_rawsig(rawsig: &[u8]) -> Result<BitcoinSig, Error> {
pub fn bitcoinsig_from_rawsig(rawsig: &[u8]) -> Result<BitcoinSig, ::interpreter::Error> {
let (flag, sig) = rawsig.split_last().unwrap();
let flag = bitcoin::SigHashType::from_u32(*flag as u32);
let flag = bitcoin::SigHashType::from_u32_standard(*flag as u32)
.map_err(|_| ::interpreter::Error::NonStandardSigHash([sig, &[*flag]].concat().to_vec()))?;
let sig = secp256k1::Signature::from_der(sig)?;
Ok((sig, flag))
}
Expand Down
9 changes: 8 additions & 1 deletion src/psbt/finalizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,14 @@ pub fn finalize<C: secp256k1::Verification>(
));
}
let (flag, sig) = rawsig.split_last().unwrap();
let flag = bitcoin::SigHashType::from_u32(*flag as u32);
let flag = bitcoin::SigHashType::from_u32_standard(*flag as u32).map_err(|_| {
super::Error::InputError(
InputError::Interpreter(interpreter::Error::NonStandardSigHash(
[sig, &[*flag]].concat().to_vec(),
)),
n,
)
})?;
if target != flag {
return Err(Error::InputError(
InputError::WrongSigHashFlag {
Expand Down