Verify the holder provided valid witnesses and uses SIGHASH_ALL

LDK checks the following:
 * Each input spends an output that is one of P2WPKH, P2WSH, or P2TR.
   These were already checked by LDK when the inputs to be contributed
   were provided.
 * All signatures use the `SIGHASH_ALL` sighash type.
 * P2WPKH and P2TR key path spends are valid (verifies signatures)

NOTE:
 * When checking P2WSH spends, LDK tries to decode 70-72 byte witness
   elements as ECDSA signatures with a sighash flag. If the internal
   DER-decoding fails, then LDK just assumes it wasn't a signature and
   carries with checks. If the element can be decoded as an ECDSA
   signature, the the sighash flag must be `SIGHASH_ALL`.
 * When checking P2TR script-path spends, LDK assumes all elements of
   exactly 65 bytes with the last byte matching any valid sighash flag
   byte are schnorr signatures and checks that the sighash type is
   `SIGHASH_ALL`. If the last byte is not any valid sighash flag, the
   element is assumed not to be a signature and is ignored. Elements of
   64 bytes are not checked because if they were schnorr signatures then
   they would implicitly be `SIGHASH_DEFAULT` which is an alias of
   `SIGHASH_ALL`.

Author: dunxen

lightning/src/ln/channel.rs

@@ -12,9 +12,9 @@ use bitcoin::amount::Amount;
 use bitcoin::consensus::encode;
 use bitcoin::constants::ChainHash;
 use bitcoin::script::{Builder, Script, ScriptBuf, WScriptHash};
-use bitcoin::sighash::EcdsaSighashType;
+use bitcoin::sighash::{EcdsaSighashType, SighashCache};
 use bitcoin::transaction::{Transaction, TxIn, TxOut};
-use bitcoin::{Weight, Witness};
+use bitcoin::{TapSighashType, Weight, Witness, XOnlyPublicKey};
 
 use bitcoin::hash_types::{BlockHash, Txid};
 use bitcoin::hashes::sha256::Hash as Sha256;
@@ -23,7 +23,7 @@ use bitcoin::hashes::Hash;
 
 use bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE;
 use bitcoin::secp256k1::{ecdsa::Signature, Secp256k1};
-use bitcoin::secp256k1::{PublicKey, SecretKey};
+use bitcoin::secp256k1::{Message, PublicKey, SecretKey};
 use bitcoin::{secp256k1, sighash};
 
 use crate::chain::chaininterface::{
@@ -7586,11 +7586,141 @@ 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
+	fn verify_interactive_tx_signatures(
+		&mut self, witnesses: &Vec<Witness>,
+	) -> Result<(), APIError> {
+		if let Some(session) = &self.interactive_tx_signing_session {
+			let unsigned_tx = session.unsigned_tx();
+			let built_tx = unsigned_tx.build_unsigned_tx();
+			let prev_outputs: Vec<&TxOut> =
+				unsigned_tx.inputs().map(|input| input.prev_output()).collect::<Vec<_>>();
+			let all_prevouts = sighash::Prevouts::All(&prev_outputs[..]);
+
+			let mut cache = SighashCache::new(&built_tx);
+			let secp = Secp256k1::verification_only();
+
+			let script_pubkeys = unsigned_tx
+				.inputs()
+				.enumerate()
+				.filter(|(_, input)| input.is_local(unsigned_tx.holder_is_initiator()));
+
+			'inputs: for ((input_idx, input), witness) in script_pubkeys.zip(witnesses) {
+				let prev_output = input.prev_output();
+				let script_pubkey = &prev_output.script_pubkey;
+
+				// P2WPKH
+				if script_pubkey.is_p2wpkh() {
+					if witness.len() != 2 {
+						return Err(APIError::APIMisuseError {
+							err: format!(
+								"The witness for input at index {input_idx} does not have the correct number of elements for a P2WPKH spend. Expected 2 got {}",
+								witness.len()
+							),
+						});
+					}
+					let sighash = cache
+						.p2wpkh_signature_hash(
+							input_idx,
+							script_pubkey,
+							prev_output.value,
+							EcdsaSighashType::All,
+						)
+						.expect("Transaction is ready for signing");
+					let msg = Message::from_digest_slice(&sighash[..]).unwrap();
+					let pubkey = PublicKey::from_slice(&witness[1]).map_err(|_| APIError::APIMisuseError { err: format!("The witness for input at index {input_idx} contains an invalid ECDSA public key") })?;
+					let sig = bitcoin::ecdsa::Signature::from_slice(&witness[0]).map_err(|_| APIError::APIMisuseError { err: format!("The witness for input at index {input_idx} contains an invalid signature") })?;
+					secp.verify_ecdsa(&msg, &sig.signature, &pubkey).map_err(|_| {
+						APIError::APIMisuseError { err:
+						format!("Failed signature verification for input at index {input_idx} for P2WPKH spend.") }
+					})?;
+					continue 'inputs;
+				}
+
+				// P2TR key path spend witness includes signature and optional annex
+				if script_pubkey.is_p2tr() && witness.len() <= 2 {
+					if witness.len() == 0 {
+						return Err(APIError::APIMisuseError {
+							err: format!(
+								"The witness for input at index {input_idx} for a P2TR spend has 0 elements, which is invalid",
+							),
+						});
+					}
+					let sighash = cache
+						.taproot_key_spend_signature_hash(
+							input_idx,
+							&all_prevouts,
+							TapSighashType::All,
+						)
+						.expect("Transaction is ready for signing");
+					let msg = Message::from_digest_slice(&sighash[..]).unwrap();
+					let pubkey = match script_pubkey.instructions().collect::<Vec<Result<bitcoin::script::Instruction, _>>>()[1] {
+						Ok(bitcoin::script::Instruction::PushBytes(push_bytes)) => {
+							XOnlyPublicKey::from_slice(push_bytes.as_bytes())
+						},
+						_ => return Err(APIError::APIMisuseError { err: format!("The scriptPubKey of the previous output for input at index {input_idx} for a P2TR key path spend has an invalid public key") }),
+					}.map_err(|_| APIError::APIMisuseError { err: format!("") })?;
+					let sig = bitcoin::taproot::Signature::from_slice(&witness[0]).map_err(|_| APIError::APIMisuseError { err: format!("The witness for input at index {input_idx} for a P2TR key path spend has an invalid signature") })?;
+					secp.verify_schnorr(&sig.signature, &msg, &pubkey).map_err(|_| {
+						APIError::APIMisuseError { err: format!("Failed signature verification for input at index {input_idx} for P2WPKH spend.") }
+					})?;
+					continue 'inputs;
+				}
+
+				// P2WSH - No validation just sighash checks
+				if script_pubkey.is_p2wsh() {
+					'elements: 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 'elements;
+								}
+
+								return Err(APIError::APIMisuseError {
+									err: format!(
+										"An ECDSA signature in the witness for input {input_idx} does not use SIGHASH_ALL"
+									),
+								});
+							},
+							_ => continue 'elements,
+						}
+					}
+					continue 'inputs;
+				}
+
+				// P2TR script path - No validation, just sighash checks
+				if script_pubkey.is_p2tr() {
+					'elements: for element in witness {
+						match element.len() {
+							// 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 'elements;
+								}
+
+								return Err(APIError::APIMisuseError {
+									err:
+										format!("A (likely) Schnorr signature in the witness for input {input_idx} does not use SIGHASH_DEFAULT or SIGHASH_ALL")
+								});
+							},
+							_ => continue 'elements,
+						}
+					}
+					continue 'inputs;
+				}
+
+				debug_assert!(false, "We don't allow contributing inputs that are not spending P2WPKH, P2WSH, or P2TR");
+				return Err(APIError::APIMisuseError { err: format!("Input at index {input_idx} does not spend from one of P2WPKH, P2WSH, or P2TR") });
+			}
 		}
+		Ok(())
 	}
 
 	pub fn funding_transaction_signed<L: Deref>(
@@ -7599,7 +7729,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

lightning/src/ln/channelmanager.rs

@@ -5897,19 +5897,32 @@ where
 	/// counterparty's signature(s) the funding transaction will automatically be broadcast via the
 	/// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed.
 	///
-	/// `SIGHASH_ALL` MUST be used for all signatures when providing signatures.
-	///
-	/// <div class="warning">
-	/// WARNING: LDK makes no attempt to prevent the counterparty from using non-standard inputs which
-	///  will prevent the funding transaction from being relayed on the bitcoin network and hence being
-	///  confirmed.
-	/// </div>
+	/// `SIGHASH_ALL` MUST be used for all signatures when providing signatures, otherwise your
+	/// funds can be held hostage!
+	///
+	/// LDK checks the following:
+	///  * Each input spends an output that is one of P2WPKH, P2WSH, or P2TR.
+	///    These were already checked by LDK when the inputs to be contributed were provided.
+	///  * All signatures use the `SIGHASH_ALL` sighash type.
+	///  * P2WPKH and P2TR key path spends are valid (verifies signatures)
+	///
+	/// NOTE:
+	///  * When checking P2WSH spends, LDK tries to decode 70-72 byte witness elements as ECDSA
+	///    signatures with a sighash flag. If the internal DER-decoding fails, then LDK just
+	///    assumes it wasn't a signature and carries with checks. If the element can be decoded
+	///    as an ECDSA signature, the the sighash flag must be `SIGHASH_ALL`.
+	///  * When checking P2TR script-path spends, LDK assumes all elements of exactly 65 bytes
+	///    with the last byte matching any valid sighash flag byte are schnorr signatures and checks
+	///    that the sighash type is `SIGHASH_ALL`. If the last byte is not any valid sighash flag, the
+	///    element is assumed not to be a signature and is ignored. Elements of 64 bytes are not
+	///    checked because if they were schnorr signatures then they would implicitly be `SIGHASH_DEFAULT`
+	///    which is an alias of `SIGHASH_ALL`.
 	///
 	/// Returns [`ChannelUnavailable`] when a channel is not found or an incorrect
 	/// `counterparty_node_id` is provided.
 	///
 	/// Returns [`APIMisuseError`] when a channel is not in a state where it is expecting funding
-	/// signatures.
+	/// signatures or if any of the checks described above fail.
 	///
 	/// [`ChannelUnavailable`]: APIError::ChannelUnavailable
 	/// [`APIMisuseError`]: APIError::APIMisuseError

lightning/src/ln/interactivetxs.rs

@@ -213,6 +213,16 @@ pub(crate) struct NegotiatedTxInput {
 	prev_output: TxOut,
 }
 
+impl NegotiatedTxInput {
+	pub(super) fn is_local(&self, holder_is_initiator: bool) -> bool {
+		!is_serial_id_valid_for_counterparty(holder_is_initiator, self.serial_id)
+	}
+
+	pub(super) fn prev_output(&self) -> &TxOut {
+		&self.prev_output
+	}
+}
+
 impl_writeable_tlv_based!(NegotiatedTxInput, {
 	(1, serial_id, required),
 	(3, txin, required),
@@ -363,6 +373,10 @@ impl ConstructedTransaction {
 			.zip(witnesses)
 			.for_each(|(input, witness)| input.witness = witness);
 	}
+
+	pub fn holder_is_initiator(&self) -> bool {
+		self.holder_is_initiator
+	}
 }
 
 /// The InteractiveTxSigningSession coordinates the signing flow of interactively constructed