Fix spurious panic on bogus funding txn that confirm and are spent

In c02b6a3 we moved the
`payment_preimage` copy from inside the macro which only runs if we
are spending an output we know is an HTLC output to doing it for
any script that matches our expected length. This can panic if an
inbound channel is created with a bogus funding transaction that
has a witness program of the HTLC-Success/-Offered length but which
does not have a second-to-last witness element which is 32 bytes.

Luckily this panic is relatively simple for downstream users to
work around - if an invalid-length-copy panic occurs, simply remove
the ChannelMonitor from the bogus channel on startup and run
without it. Because the channel must be funded by a bogus script in
order to reach this panic, the channel will already have closed by
the time the funding transaction is spent, and there can be no
local funds in such a channel, so removing the `ChannelMonitor`
wholesale is completely safe.

In order to test this we have to disable an in-line assertion that
checks that our transactions match expected scripts which we do by
checking for the specific bogus script that we now use in
`test_invalid_funding_tx`.

Thanks to Eugene Siegel for reporting this issue.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -1729,6 +1729,26 @@ macro_rules! fail_unbroadcast_htlcs {
 	} }
 }
 
+// In the `test_invalid_funding_tx` test, we need a bogus script which matches the HTLC-Accepted
+// witness length match (ie is 136 bytes long). We generate one here which we also use in some
+// in-line tests later.
+
+#[cfg(test)]
+pub fn deliberately_bogus_accepted_htlc_witness_program() -> Vec<u8> {
+	let mut ret = [opcodes::all::OP_NOP.into_u8(); 136];
+	ret[131] = opcodes::all::OP_DROP.into_u8();
+	ret[132] = opcodes::all::OP_DROP.into_u8();
+	ret[133] = opcodes::all::OP_DROP.into_u8();
+	ret[134] = opcodes::all::OP_DROP.into_u8();
+	ret[135] = opcodes::OP_TRUE.into_u8();
+	Vec::from(&ret[..])
+}
+
+#[cfg(test)]
+pub fn deliberately_bogus_accepted_htlc_witness() -> Vec<Vec<u8>> {
+	vec![Vec::new(), Vec::new(), Vec::new(), Vec::new(), deliberately_bogus_accepted_htlc_witness_program().into()].into()
+}
+
 impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 	/// Inserts a revocation secret into this channel monitor. Prunes old preimages if neither
 	/// needed by holder commitment transactions HTCLs nor by counterparty ones. Unless we haven't already seen
@@ -2701,14 +2721,21 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 					if *idx == input.previous_output.vout {
 						#[cfg(test)]
 						{
-						        // If the expected script is a known type, check that the witness
-						        // appears to be spending the correct type (ie that the match would
-						        // actually succeed in BIP 158/159-style filters).
-						        if _script_pubkey.is_v0_p2wsh() {
-						                assert_eq!(&bitcoin::Address::p2wsh(&Script::from(input.witness.last().unwrap().to_vec()), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
-						        } else if _script_pubkey.is_v0_p2wpkh() {
-						                assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap(), bitcoin::Network::Bitcoin).unwrap().script_pubkey(), _script_pubkey);
-						        } else { panic!(); }
+							// If the expected script is a known type, check that the witness
+							// appears to be spending the correct type (ie that the match would
+							// actually succeed in BIP 158/159-style filters).
+							if _script_pubkey.is_v0_p2wsh() {
+								if input.witness.last().unwrap().to_vec() == deliberately_bogus_accepted_htlc_witness_program() {
+									// In at least one test we use a deliberately bogus witness
+									// script which hit an old panic. Thus, we check for that here
+									// and avoid the assert if its the expected bogus script.
+									return true;
+								}
+
+								assert_eq!(&bitcoin::Address::p2wsh(&Script::from(input.witness.last().unwrap().to_vec()), bitcoin::Network::Bitcoin).script_pubkey(), _script_pubkey);
+							} else if _script_pubkey.is_v0_p2wpkh() {
+								assert_eq!(&bitcoin::Address::p2wpkh(&bitcoin::PublicKey::from_slice(&input.witness.last().unwrap()).unwrap(), bitcoin::Network::Bitcoin).unwrap().script_pubkey(), _script_pubkey);
+							} else { panic!(); }
 						}
 						return true;
 					}
@@ -2793,10 +2820,13 @@ impl<Signer: Sign> ChannelMonitorImpl<Signer> {
 			let prev_last_witness_len = input.witness.second_to_last().map(|w| w.len()).unwrap_or(0);
 			let revocation_sig_claim = (witness_items == 3 && htlctype == Some(HTLCType::OfferedHTLC) && prev_last_witness_len == 33)
 				|| (witness_items == 3 && htlctype == Some(HTLCType::AcceptedHTLC) && prev_last_witness_len == 33);
-			let accepted_preimage_claim = witness_items == 5 && htlctype == Some(HTLCType::AcceptedHTLC);
+			let accepted_preimage_claim = witness_items == 5 && htlctype == Some(HTLCType::AcceptedHTLC)
+				&& input.witness.second_to_last().unwrap().len() == 32;
 			#[cfg(not(fuzzing))]
 			let accepted_timeout_claim = witness_items == 3 && htlctype == Some(HTLCType::AcceptedHTLC) && !revocation_sig_claim;
-			let offered_preimage_claim = witness_items == 3 && htlctype == Some(HTLCType::OfferedHTLC) && !revocation_sig_claim;
+			let offered_preimage_claim = witness_items == 3 && htlctype == Some(HTLCType::OfferedHTLC) &&
+				!revocation_sig_claim && input.witness.second_to_last().unwrap().len() == 32;
+
 			#[cfg(not(fuzzing))]
 			let offered_timeout_claim = witness_items == 5 && htlctype == Some(HTLCType::OfferedHTLC);
 

lightning/src/ln/functional_tests.rs

@@ -37,7 +37,7 @@ use util::config::UserConfig;
 
 use bitcoin::hash_types::BlockHash;
 use bitcoin::blockdata::block::{Block, BlockHeader};
-use bitcoin::blockdata::script::Builder;
+use bitcoin::blockdata::script::{Builder, Script};
 use bitcoin::blockdata::opcodes;
 use bitcoin::blockdata::constants::genesis_block;
 use bitcoin::network::constants::Network;
@@ -9424,6 +9424,10 @@ fn test_invalid_funding_tx() {
 	// funding transactions from their counterparties, leading to a multi-implementation critical
 	// security vulnerability (though we always sanitized properly, we've previously had
 	// un-released crashes in the sanitization process).
+	//
+	// Further, if the funding transaction is consensus-valid, confirms, and is later spent, we'd
+	// previously have crashed in `ChannelMonitor` even though we closed the channel as bogus and
+	// gave up on it. We test this here by generating such a transaction.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
@@ -9434,9 +9438,19 @@ fn test_invalid_funding_tx() {
 	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id()));
 
 	let (temporary_channel_id, mut tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100_000, 42);
+
+	// Create a witness program which can be spent by a 4-empty-stack-elements witness and which is
+	// 136 bytes long. This matches our "accepted HTLC preimage spend" matching, previously causing
+	// a panic as we'd try to extract a 32 byte preimage from a witness element without checking
+	// its length.
+	let mut wit_program: Vec<u8> = channelmonitor::deliberately_bogus_accepted_htlc_witness_program();
+	assert!(chan_utils::HTLCType::scriptlen_to_htlctype(wit_program.len()).unwrap() ==
+		chan_utils::HTLCType::AcceptedHTLC);
+
+	let wit_program_script: Script = wit_program.clone().into();
 	for output in tx.output.iter_mut() {
 		// Make the confirmed funding transaction have a bogus script_pubkey
-		output.script_pubkey = bitcoin::Script::new();
+		output.script_pubkey = Script::new_v0_p2wsh(&wit_program_script.wscript_hash());
 	}
 
 	nodes[0].node.funding_transaction_generated_unchecked(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx.clone(), 0).unwrap();
@@ -9466,6 +9480,28 @@ fn test_invalid_funding_tx() {
 		} else { panic!(); }
 	} else { panic!(); }
 	assert_eq!(nodes[1].node.list_channels().len(), 0);
+
+	// Now confirm a spend of the (bogus) funding transaction. As long as the witness is 5 elements
+	// long the ChannelMonitor will try to read 32 bytes from the second-to-last element, panicing
+	// as its not 32 bytes long.
+	let mut spend_tx = Transaction {
+		version: 2i32, lock_time: 0,
+		input: tx.output.iter().enumerate().map(|(idx, _)| TxIn {
+			previous_output: BitcoinOutPoint {
+				txid: tx.txid(),
+				vout: idx as u32,
+			},
+			script_sig: Script::new(),
+			sequence: 0xfffffffd,
+			witness: Witness::from_vec(channelmonitor::deliberately_bogus_accepted_htlc_witness())
+		}).collect(),
+		output: vec![TxOut {
+			value: 1000,
+			script_pubkey: Script::new(),
+		}]
+	};
+	check_spends!(spend_tx, tx);
+	mine_transaction(&nodes[1], &spend_tx);
 }
 
 fn do_test_tx_confirmed_skipping_blocks_immediate_broadcast(test_height_before_timelock: bool) {