From 34de734194f88a63a9507aeeafb14ef6e8775eb2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 28 Dec 2022 17:44:33 +0000 Subject: [PATCH 1/2] Ensure `derive_channel_keys` doesn't panic if per-run seed is high b04d1b868fe28bea2e4c711e6e6d2470d2b98d77 changed the way we calculate the `channel_keys_id` to include the 128-bit `user_channel_id` as well, shifting the counter up four bytes and the `starting_time_nanos` field up into the second four bytes. In `derive_channel_keys` we hash the full `channel_keys_id` with an HD-derived key from our master seed. Previously, that key was derived with an index of the per-restart counter, re-calculated by pulling the second four bytes out of the `user_channel_id`. Because the `channel_keys_id` fields were shifted up four bytes, that is now a reference to the `starting_time_nanos` value. This should be fine, the derivation doesn't really add any value here, its all being hashed anyway, except that derivation IDs must be below 2^31. This implies that we panic if the user passes a `starting_time_nanos` which has the high bit set. For those using the nanosecond part of the current time this isn't an issue - the value cannot exceed 1_000_000, which does not have the high bit set, however, some users may use some other per-run seed. Thus, here we simply drop the high bit from the seed, ensuring we don't panic. Note that this is backwards compatible as it only changes the key derivation in cases where we previously panicked. Ideally we'd drop the derivation entirely, but that would break backwards compatibility of key derivation. --- lightning/src/chain/keysinterface.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 1426ff5ce88..0a464193d0d 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -1051,7 +1051,9 @@ impl KeysManager { // We only seriously intend to rely on the channel_master_key for true secure // entropy, everything else just ensures uniqueness. We rely on the unique_start (ie // starting_time provided in the constructor) to be unique. - let child_privkey = self.channel_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(chan_id as u32).expect("key space exhausted")).expect("Your RNG is busted"); + let child_privkey = self.channel_master_key.ckd_priv(&self.secp_ctx, + ChildNumber::from_hardened_idx((chan_id as u32) % (1 << 31)).expect("key space exhausted") + ).expect("Your RNG is busted"); unique_start.input(&child_privkey.private_key[..]); let seed = Sha256::from_engine(unique_start).into_inner(); From 5dde803d087dfbaab4e84241eaddd8407cc5baef Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 28 Dec 2022 18:12:29 +0000 Subject: [PATCH 2/2] Ensure the per-channel key derivation counter doesn't role over Previously, the `derive_channel_keys` derivation ID asserted that the high bit of the per-channel key derivation counter doesn't role over as it checked the 31st bit was zero. As we no longer do that, we should ensure the assertion in `generate_channel_keys_id` asserts that we don't role over. --- lightning/src/chain/keysinterface.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 0a464193d0d..d260d029493 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -1263,7 +1263,12 @@ impl KeysInterface for KeysManager { fn generate_channel_keys_id(&self, _inbound: bool, _channel_value_satoshis: u64, user_channel_id: u128) -> [u8; 32] { let child_idx = self.channel_child_index.fetch_add(1, Ordering::AcqRel); - assert!(child_idx <= core::u32::MAX as usize); + // `child_idx` is the only thing guaranteed to make each channel unique without a restart + // (though `user_channel_id` should help, depending on user behavior). If it manages to + // roll over, we may generate duplicate keys for two different channels, which could result + // in loss of funds. Because we only support 32-bit+ systems, assert that our `AtomicUsize` + // doesn't reach `u32::MAX`. + assert!(child_idx < core::u32::MAX as usize, "2^32 channels opened without restart"); let mut id = [0; 32]; id[0..4].copy_from_slice(&(child_idx as u32).to_be_bytes()); id[4..8].copy_from_slice(&self.starting_time_nanos.to_be_bytes());