wip: Check that holder witnesses sighash types are SIGHASH_(ALL/DEFAULT)

Author: dunxen

lightning/src/ln/channel.rs

@@ -14,7 +14,7 @@ use bitcoin::constants::ChainHash;
 use bitcoin::script::{Builder, Script, ScriptBuf, WScriptHash};
 use bitcoin::sighash::EcdsaSighashType;
 use bitcoin::transaction::{Transaction, TxIn, TxOut};
-use bitcoin::{Weight, Witness};
+use bitcoin::{TapSighashType, Weight, Witness};
 
 use bitcoin::hash_types::{BlockHash, Txid};
 use bitcoin::hashes::sha256::Hash as Sha256;
@@ -7586,11 +7586,54 @@ where
 		}
 	}
 
-	fn verify_interactive_tx_signatures(&mut self, _witnesses: &Vec<Witness>) {
-		if let Some(ref mut _signing_session) = self.interactive_tx_signing_session {
-			// Check that sighash_all was used:
-			// TODO(dual_funding): Check sig for sighash
+	// A very basic check that anything resembling a signature in provided witnesses uses the
+	// SIGHASH_ALL sighash type.
+	//
+	// May have false positives for elements that have the same length as Schnorr signatures.
+	// If the length is 64 bytes, this doesn't matter as it's implicitly treated as SIGHASH_DEFAULT
+	// (= SIGHASH_ALL). If it is instead 65 bytes and the last byte could be interpreted as a real
+	// sighash type, then this function would return an error if it is not equal to SIGHASH_ALL.
+	fn verify_interactive_tx_signatures(
+		&mut self, witnesses: &Vec<Witness>,
+	) -> Result<(), APIError> {
+		for witness in witnesses {
+			for element in witness {
+				match element.len() {
+					// Possibly a DER-encoded ECDSA signature with a sighash type byte assuming low-S
+					70..=72 => {
+						if bitcoin::ecdsa::Signature::from_slice(element)
+							.map(|sig| matches!(sig.sighash_type, EcdsaSighashType::All))
+							.unwrap_or(true)
+						{
+							continue;
+						}
+
+						return Err(APIError::APIMisuseError {
+							err: format!(
+								"An ECDSA signature in a provided witness does not use SIGHASH_ALL"
+							),
+						});
+					},
+					// Schnorr sig + sighash type byte.
+					// If this were just 64 bytes, it would implicitly be SIGHASH_DEFAULT (= SIGHASH_ALL)
+					65 => {
+						if bitcoin::taproot::Signature::from_slice(element)
+							.map(|sig| matches!(sig.sighash_type, TapSighashType::All))
+							.unwrap_or(true)
+						{
+							continue;
+						}
+
+						return Err(APIError::APIMisuseError {
+							err:
+								format!("A Schnorr signature in a provided witness does not use SIGHASH_DEFAULT or SIGHASH_ALL")
+						});
+					},
+					_ => continue,
+				}
+			}
 		}
+		Ok(())
 	}
 
 	pub fn funding_transaction_signed<L: Deref>(
@@ -7599,7 +7642,7 @@ where
 	where
 		L::Target: Logger,
 	{
-		self.verify_interactive_tx_signatures(&witnesses);
+		self.verify_interactive_tx_signatures(&witnesses)?;
 		if let Some(ref mut signing_session) = self.interactive_tx_signing_session {
 			let logger = WithChannelContext::from(logger, &self.context, None);
 			if let Some(holder_tx_signatures) = signing_session