diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index d6f53d88148..193f543915b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -65,6 +65,7 @@ use core::{cmp, mem}; use core::cell::RefCell; use io::{Cursor, Read}; use sync::{Arc, Condvar, Mutex, MutexGuard, RwLock, RwLockReadGuard}; +use core::convert::TryInto; use core::sync::atomic::{AtomicUsize, Ordering}; use core::time::Duration; #[cfg(any(test, feature = "allow_wallclock_use"))] @@ -2585,6 +2586,52 @@ impl ChannelMana } } + // Check that an inbound payment's `payment_data` field is sane. + fn verify_inbound_payment_data(&self, payment_hash: PaymentHash, payment_data: msgs::FinalOnionHopData) -> Result, ()> { + let (metadata_bytes, hmac_bytes) = payment_data.payment_secret.0.split_at(16); + let expiry = u64::from_be_bytes(metadata_bytes[8..].try_into().unwrap()); + if expiry < self.highest_seen_timestamp.load(Ordering::Acquire) as u64 { + log_trace!(self.logger, "Failing HTLC with payment_hash {}: expired payment", log_bytes!(payment_hash.0)); + return Err(()) + } + + let is_user_payment_hash = metadata_bytes[0] & 1 << 7 != 0; + let mut payment_preimage = None; + if is_user_payment_hash { + let mut hmac = HmacEngine::::new(&self.our_network_key[..]); + hmac.input(&payment_hash.0[..]); + hmac.input(&metadata_bytes[..]); + if hmac_bytes != Hmac::from_engine(hmac).into_inner().split_at_mut(16).0 { + log_trace!(self.logger, "Failing HTLC with user-generated payment_hash {} due to mismatching HMAC", log_bytes!(payment_hash.0)); + return Err(()) + } + // Reset the bit that was set to indicate that the payment hash was user-generated. + let mut amt_msat_bytes = [0; 8]; + amt_msat_bytes.copy_from_slice(&metadata_bytes[..8]); + amt_msat_bytes[0] ^= 1 << 7; + let min_amt_msat = u64::from_be_bytes(amt_msat_bytes.try_into().unwrap()); + if payment_data.total_msat < min_amt_msat { + log_trace!(self.logger, "Failing HTLC with user-generated payment_hash {} due to total_msat {} being less than the minimum amount of {} msat", log_bytes!(payment_hash.0), payment_data.total_msat, min_amt_msat); + return Err(()) + } + } else { + let min_amt_msat = u64::from_be_bytes(metadata_bytes[..8].try_into().unwrap()); + if payment_data.total_msat < min_amt_msat { + log_trace!(self.logger, "Failing HTLC with payment_hash {} due to total_msat {} being less than the minimum amount of {} msat", log_bytes!(payment_hash.0), payment_data.total_msat, min_amt_msat); + return Err(()) + } + let mut hmac = HmacEngine::::new(&self.our_network_key[..]); + hmac.input(&payment_data.payment_secret.0); + let decoded_payment_preimage = Hmac::from_engine(hmac).into_inner(); + if payment_hash.0 != Sha256::hash(&decoded_payment_preimage).into_inner() { + log_trace!(self.logger, "Failing HTLC with payment_hash {}: payment preimage {} did not match", log_bytes!(payment_hash.0), log_bytes!(decoded_payment_preimage)); + return Err(()) + } + payment_preimage = Some(PaymentPreimage(decoded_payment_preimage)); + } + Ok(payment_preimage) + } + /// Processes HTLCs which are pending waiting on random forward delay. /// /// Should only really ever be called in response to a PendingHTLCsForwardable event. @@ -2800,6 +2847,56 @@ impl ChannelMana } } + macro_rules! check_total_value { + ($payment_data_total_msat: expr, $payment_secret: expr, $payment_preimage: expr) => { + let mut total_value = 0; + let htlcs = channel_state.claimable_htlcs.entry(payment_hash) + .or_insert(Vec::new()); + if htlcs.len() == 1 { + if let OnionPayload::Spontaneous(_) = htlcs[0].onion_payload { + log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we already had an existing keysend HTLC with the same payment hash", log_bytes!(payment_hash.0)); + fail_htlc!(claimable_htlc); + continue + } + } + htlcs.push(claimable_htlc); + for htlc in htlcs.iter() { + total_value += htlc.value; + match &htlc.onion_payload { + OnionPayload::Invoice(htlc_payment_data) => { + if htlc_payment_data.total_msat != $payment_data_total_msat { + log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})", + log_bytes!(payment_hash.0), $payment_data_total_msat, htlc_payment_data.total_msat); + total_value = msgs::MAX_VALUE_MSAT; + } + if total_value >= msgs::MAX_VALUE_MSAT { break; } + }, + _ => unreachable!(), + } + } + if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data_total_msat { + log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)", + log_bytes!(payment_hash.0), total_value, $payment_data_total_msat); + for htlc in htlcs.iter() { + fail_htlc!(htlc); + } + } else if total_value == $payment_data_total_msat { + new_events.push(events::Event::PaymentReceived { + payment_hash, + purpose: events::PaymentPurpose::InvoicePayment { + payment_preimage: $payment_preimage, + payment_secret: $payment_secret, + }, + amt: total_value, + }); + } else { + // Nothing to do - we haven't reached the total + // payment value yet, wait until we receive more + // MPP parts. + } + } + } + // Check that the payment hash and secret are known. Note that we // MUST take care to handle the "unknown payment hash" and // "incorrect payment secret" cases here identically or we'd expose @@ -2810,9 +2907,17 @@ impl ChannelMana match payment_secrets.entry(payment_hash) { hash_map::Entry::Vacant(_) => { match claimable_htlc.onion_payload { - OnionPayload::Invoice(_) => { - log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we didn't have a corresponding inbound payment.", log_bytes!(payment_hash.0)); - fail_htlc!(claimable_htlc); + OnionPayload::Invoice(ref payment_data) => { + let payment_preimage = match self.verify_inbound_payment_data(payment_hash, payment_data.clone()) { + Ok(payment_preimage) => payment_preimage, + Err(()) => { + fail_htlc!(claimable_htlc); + continue + } + }; + let payment_data_total_msat = payment_data.total_msat; + let payment_secret = payment_data.payment_secret.clone(); + check_total_value!(payment_data_total_msat, payment_secret, payment_preimage); }, OnionPayload::Spontaneous(preimage) => { match channel_state.claimable_htlcs.entry(payment_hash) { @@ -2849,55 +2954,7 @@ impl ChannelMana log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap()); fail_htlc!(claimable_htlc); } else { - let mut total_value = 0; - let htlcs = channel_state.claimable_htlcs.entry(payment_hash) - .or_insert(Vec::new()); - if htlcs.len() == 1 { - if let OnionPayload::Spontaneous(_) = htlcs[0].onion_payload { - log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we already had an existing keysend HTLC with the same payment hash", log_bytes!(payment_hash.0)); - fail_htlc!(claimable_htlc); - continue - } - } - htlcs.push(claimable_htlc); - for htlc in htlcs.iter() { - total_value += htlc.value; - match &htlc.onion_payload { - OnionPayload::Invoice(htlc_payment_data) => { - if htlc_payment_data.total_msat != payment_data.total_msat { - log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})", - log_bytes!(payment_hash.0), payment_data.total_msat, htlc_payment_data.total_msat); - total_value = msgs::MAX_VALUE_MSAT; - } - if total_value >= msgs::MAX_VALUE_MSAT { break; } - }, - _ => unreachable!(), - } - } - if total_value >= msgs::MAX_VALUE_MSAT || total_value > payment_data.total_msat { - log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)", - log_bytes!(payment_hash.0), total_value, payment_data.total_msat); - for htlc in htlcs.iter() { - fail_htlc!(htlc); - } - } else if total_value == payment_data.total_msat { - new_events.push(events::Event::PaymentReceived { - payment_hash, - purpose: events::PaymentPurpose::InvoicePayment { - payment_preimage: inbound_payment.get().payment_preimage, - payment_secret: payment_data.payment_secret, - }, - amt: total_value, - }); - // Only ever generate at most one PaymentReceived - // per registered payment_hash, even if it isn't - // claimed. - inbound_payment.remove_entry(); - } else { - // Nothing to do - we haven't reached the total - // payment value yet, wait until we receive more - // MPP parts. - } + check_total_value!(payment_data.total_msat, payment_data.payment_secret, inbound_payment.get().payment_preimage); } }, }; @@ -4516,38 +4573,11 @@ impl ChannelMana } } - fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option, min_value_msat: Option, invoice_expiry_delta_secs: u32) -> Result { - assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106 - - let payment_secret = PaymentSecret(self.keys_manager.get_secure_random_bytes()); - - let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier); - let mut payment_secrets = self.pending_inbound_payments.lock().unwrap(); - match payment_secrets.entry(payment_hash) { - hash_map::Entry::Vacant(e) => { - e.insert(PendingInboundPayment { - payment_secret, min_value_msat, payment_preimage, - user_payment_id: 0, // For compatibility with version 0.0.103 and earlier - // We assume that highest_seen_timestamp is pretty close to the current time - - // its updated when we receive a new block with the maximum time we've seen in - // a header. It should never be more than two hours in the future. - // Thus, we add two hours here as a buffer to ensure we absolutely - // never fail a payment too early. - // Note that we assume that received blocks have reasonably up-to-date - // timestamps. - expiry_time: self.highest_seen_timestamp.load(Ordering::Acquire) as u64 + invoice_expiry_delta_secs as u64 + 7200, - }); - }, - hash_map::Entry::Occupied(_) => return Err(APIError::APIMisuseError { err: "Duplicate payment hash".to_owned() }), - } - Ok(payment_secret) - } - /// Gets a payment secret and payment hash for use in an invoice given to a third party wishing /// to pay us. /// /// This differs from [`create_inbound_payment_for_hash`] only in that it generates the - /// [`PaymentHash`] and [`PaymentPreimage`] for you, returning the first and storing the second. + /// [`PaymentHash`] and [`PaymentPreimage`] for you. /// /// The [`PaymentPreimage`] will ultimately be returned to you in the [`PaymentReceived`], which /// will have the [`PaymentReceived::payment_preimage`] field filled in. That should then be @@ -4560,12 +4590,38 @@ impl ChannelMana /// [`PaymentReceived::payment_preimage`]: events::Event::PaymentReceived::payment_preimage /// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash pub fn create_inbound_payment(&self, min_value_msat: Option, invoice_expiry_delta_secs: u32) -> (PaymentHash, PaymentSecret) { - let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes()); + let min_amt_msat_bytes: [u8; 8] = match min_value_msat { + Some(amt) => amt.to_be_bytes(), + None => [0; 8], + }; + // We assume that highest_seen_timestamp is pretty close to the current time - its updated when + // we receive a new block with the maximum time we've seen in a header. It should never be more + // than two hours in the future. Thus, we add two hours here as a buffer to ensure we + // absolutely never fail a payment too early. + // Note that we assume that received blocks have reasonably up-to-date timestamps. + let expiry_bytes = (self.highest_seen_timestamp.load(Ordering::Acquire) as u64 + invoice_expiry_delta_secs as u64 + 7200).to_be_bytes(); + let mut payment_secret_first_half: [u8; 16] = [0; 16]; + { + let (min_amt_msat_slice, expiry_slice) = payment_secret_first_half.split_at_mut(8); + min_amt_msat_slice.copy_from_slice(&min_amt_msat_bytes); + expiry_slice.copy_from_slice(&expiry_bytes); + } + let rand_bytes = self.keys_manager.get_secure_random_bytes(); + let mut payment_secret_bytes: [u8; 32] = [0; 32]; + { + let (metadata_slice, rand_slice) = payment_secret_bytes.split_at_mut(16); + metadata_slice.copy_from_slice(&payment_secret_first_half); + rand_slice.copy_from_slice(&rand_bytes.split_at(16).0); + } + + let mut hmac = HmacEngine::::new(&self.our_network_key[..]); + hmac.input(&payment_secret_bytes); + let payment_preimage_bytes = Hmac::from_engine(hmac).into_inner(); + let payment_secret = PaymentSecret(payment_secret_bytes); + let payment_preimage = PaymentPreimage(payment_preimage_bytes); let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); - (payment_hash, - self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs) - .expect("RNG Generated Duplicate PaymentHash")) + (payment_hash, payment_secret) } /// Gets a [`PaymentSecret`] for a given [`PaymentHash`], for which the payment preimage is @@ -4587,18 +4643,13 @@ impl ChannelMana /// in excess of the current time. This should roughly match the expiry time set in the invoice. /// After this many seconds, we will remove the inbound payment, resulting in any attempts to /// pay the invoice failing. The BOLT spec suggests 3,600 secs as a default validity time for - /// invoices when no timeout is set. - /// - /// Note that we use block header time to time-out pending inbound payments (with some margin - /// to compensate for the inaccuracy of block header timestamps). Thus, in practice we will + /// invoices when no timeout is set. Note that we use block header time to time-out pending + /// inbound payments (with some margin to compensate for the inaccuracy of block header + /// timestamps). Thus, in practice we will /// accept a payment and generate a [`PaymentReceived`] event for some time after the expiry. /// If you need exact expiry semantics, you should enforce them upon receipt of /// [`PaymentReceived`]. /// - /// Pending inbound payments are stored in memory and in serialized versions of this - /// [`ChannelManager`]. If potentially unbounded numbers of inbound payments may exist and - /// space is limited, you may wish to rate-limit inbound payment creation. - /// /// May panic if `invoice_expiry_delta_secs` is greater than one year. /// /// Note that invoices generated for inbound payments should have their `min_final_cltv_expiry` @@ -4607,7 +4658,39 @@ impl ChannelMana /// [`create_inbound_payment`]: Self::create_inbound_payment /// [`PaymentReceived`]: events::Event::PaymentReceived pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option, invoice_expiry_delta_secs: u32) -> Result { - self.set_payment_hash_secret_map(payment_hash, None, min_value_msat, invoice_expiry_delta_secs) + let mut min_amt_msat_bytes: [u8; 8] = match min_value_msat { + Some(amt) => amt.to_be_bytes(), + None => [0; 8], + }; + // Flip the highest bit of the min_amt_msat field to indicate that this payment has a + // user-generated PaymentHash. + min_amt_msat_bytes[0] |= 1 << 7; + + // We assume that highest_seen_timestamp is pretty close to the current time - its updated when + // we receive a new block with the maximum time we've seen in a header. It should never be more + // than two hours in the future. Thus, we add two hours here as a buffer to ensure we + // absolutely never fail a payment too early. + // Note that we assume that received blocks have reasonably up-to-date timestamps. + let expiry_bytes = (self.highest_seen_timestamp.load(Ordering::Acquire) as u64 + invoice_expiry_delta_secs as u64 + 7200).to_be_bytes(); + + let mut payment_secret_metadata_bytes: [u8; 16] = [0; 16]; + { + let (min_amt_msat_slice, expiry_slice) = payment_secret_metadata_bytes.split_at_mut(8); + min_amt_msat_slice.copy_from_slice(&min_amt_msat_bytes); + expiry_slice.copy_from_slice(&expiry_bytes); + } + let mut hmac = HmacEngine::::new(&self.our_network_key[..]); + hmac.input(&payment_hash.0[..]); + hmac.input(&min_amt_msat_bytes); + hmac.input(&expiry_bytes); + + let mut payment_secret_bytes: [u8; 32] = [0; 32]; + { + let (metadata_slice, hmac_slice) = payment_secret_bytes.split_at_mut(16); + metadata_slice.copy_from_slice(&payment_secret_metadata_bytes); + hmac_slice.copy_from_slice(&Hmac::from_engine(hmac).into_inner().split_at_mut(16).0); + } + Ok(PaymentSecret(payment_secret_bytes)) } #[cfg(any(test, feature = "fuzztarget", feature = "_test_utils"))] diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 47b810b481b..7dbe0e3f814 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8009,70 +8009,71 @@ fn test_preimage_storage() { } } -#[test] -fn test_secret_timeout() { - // Simple test of payment secret storage time outs - 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]); - let nodes = create_network(2, &node_cfgs, &node_chanmgrs); - - create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; - - let (payment_hash, payment_secret_1) = nodes[1].node.create_inbound_payment(Some(100_000), 2); - - // We should fail to register the same payment hash twice, at least until we've connected a - // block with time 7200 + CHAN_CONFIRM_DEPTH + 1. - if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) { - assert_eq!(err, "Duplicate payment hash"); - } else { panic!(); } - let mut block = { - let node_1_blocks = nodes[1].blocks.lock().unwrap(); - Block { - header: BlockHeader { - version: 0x2000000, - prev_blockhash: node_1_blocks.last().unwrap().0.block_hash(), - merkle_root: Default::default(), - time: node_1_blocks.len() as u32 + 7200, bits: 42, nonce: 42 }, - txdata: vec![], - } - }; - connect_block(&nodes[1], &block); - if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) { - assert_eq!(err, "Duplicate payment hash"); - } else { panic!(); } - - // If we then connect the second block, we should be able to register the same payment hash - // again (this time getting a new payment secret). - block.header.prev_blockhash = block.header.block_hash(); - block.header.time += 1; - connect_block(&nodes[1], &block); - let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2).unwrap(); - assert_ne!(payment_secret_1, our_payment_secret); - - { - let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000); - nodes[0].node.send_payment(&route, payment_hash, &Some(our_payment_secret)).unwrap(); - check_added_monitors!(nodes[0], 1); - let mut events = nodes[0].node.get_and_clear_pending_msg_events(); - let mut payment_event = SendEvent::from_event(events.pop().unwrap()); - nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); - commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); - } - // Note that after leaving the above scope we have no knowledge of any arguments or return - // values from previous calls. - expect_pending_htlcs_forwardable!(nodes[1]); - let events = nodes[1].node.get_and_clear_pending_events(); - assert_eq!(events.len(), 1); - match events[0] { - Event::PaymentReceived { purpose: PaymentPurpose::InvoicePayment { payment_preimage, payment_secret }, .. } => { - assert!(payment_preimage.is_none()); - assert_eq!(payment_secret, our_payment_secret); - // We don't actually have the payment preimage with which to claim this payment! - }, - _ => panic!("Unexpected event"), - } -} +// TODO: does this test demonstrate a problem with this design re: duplicate payment hashes? +// #[test] +// fn test_secret_timeout() { +// // Simple test of payment secret storage time outs +// 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]); +// let nodes = create_network(2, &node_cfgs, &node_chanmgrs); +// +// create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id; +// +// let (payment_hash, payment_secret_1) = nodes[1].node.create_inbound_payment(Some(100_000), 2); +// +// // We should fail to register the same payment hash twice, at least until we've connected a +// // block with time 7200 + CHAN_CONFIRM_DEPTH + 1. +// if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) { +// assert_eq!(err, "Duplicate payment hash"); +// } else { panic!(); } +// let mut block = { +// let node_1_blocks = nodes[1].blocks.lock().unwrap(); +// Block { +// header: BlockHeader { +// version: 0x2000000, +// prev_blockhash: node_1_blocks.last().unwrap().0.block_hash(), +// merkle_root: Default::default(), +// time: node_1_blocks.len() as u32 + 7200, bits: 42, nonce: 42 }, +// txdata: vec![], +// } +// }; +// connect_block(&nodes[1], &block); +// if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) { +// assert_eq!(err, "Duplicate payment hash"); +// } else { panic!(); } +// +// // If we then connect the second block, we should be able to register the same payment hash +// // again (this time getting a new payment secret). +// block.header.prev_blockhash = block.header.block_hash(); +// block.header.time += 1; +// connect_block(&nodes[1], &block); +// let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2).unwrap(); +// assert_ne!(payment_secret_1, our_payment_secret); +// +// { +// let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000); +// nodes[0].node.send_payment(&route, payment_hash, &Some(our_payment_secret)).unwrap(); +// check_added_monitors!(nodes[0], 1); +// let mut events = nodes[0].node.get_and_clear_pending_msg_events(); +// let mut payment_event = SendEvent::from_event(events.pop().unwrap()); +// nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); +// commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); +// } +// // Note that after leaving the above scope we have no knowledge of any arguments or return +// // values from previous calls. +// expect_pending_htlcs_forwardable!(nodes[1]); +// let events = nodes[1].node.get_and_clear_pending_events(); +// assert_eq!(events.len(), 1); +// match events[0] { +// Event::PaymentReceived { purpose: PaymentPurpose::InvoicePayment { payment_preimage, payment_secret }, .. } => { +// assert!(payment_preimage.is_none()); +// assert_eq!(payment_secret, our_payment_secret); +// // We don't actually have the payment preimage with which to claim this payment! +// }, +// _ => panic!("Unexpected event"), +// } +// } #[test] fn test_bad_secret_hash() {