From 6c480ae887b751e868423d606d706e352d9615f0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 14 Jun 2022 10:54:39 +0000 Subject: [PATCH] Fix spurious panic on bogus funding txn that confirm and are spent In c02b6a3807488e1943d79792c5ac0ee52530b971 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. --- lightning/src/chain/channelmonitor.rs | 50 +++++++++++++++++++++------ lightning/src/ln/functional_tests.rs | 40 +++++++++++++++++++-- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 3f7a1055b6d..48f821d5f0c 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/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 { + 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![Vec::new(), Vec::new(), Vec::new(), Vec::new(), deliberately_bogus_accepted_htlc_witness_program().into()].into() +} + impl ChannelMonitorImpl { /// 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 ChannelMonitorImpl { 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 ChannelMonitorImpl { 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); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index c304161947a..9ecbee739f1 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/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 = 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) {