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) {