Ensure derive_channel_keys doesn't panic if per-run seed is high #1935
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b04d1b8 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.
Codecov ReportBase: 90.77% // Head: 91.72% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1935 +/- ##
==========================================
+ Coverage 90.77% 91.72% +0.94%
==========================================
Files 94 96 +2
Lines 49603 57157 +7554
Branches 49603 57157 +7554
==========================================
+ Hits 45028 52427 +7399
- Misses 4575 4730 +155
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
arik-so
reviewed
Jan 3, 2023
| @@ -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") | |||
There was a problem hiding this comment.
great catch, but this is getting too complicated. Let's extract chan_id % (1 << 31) into a variable, and then let's extract ChildNumber::from_hardened_idx into a separate variable, and pass that to the method in one line.
There was a problem hiding this comment.
I don't see how more variables makes things more readable, the indentation as-is makes clear what's being expected where, no?
There was a problem hiding this comment.
Well, most people find that more comfortable to read, but the PR's perfectly fine as is.
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.
TheBlueMatt
force-pushed
the
2022-12-no-non-time-panic
branch
from
f544b3b
to
5dde803
Compare
|
Pushed the fixup of the comments in the last commit - diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs
index 2a3fb2cc8..d260d0294 100644
--- a/lightning/src/chain/keysinterface.rs
+++ b/lightning/src/chain/keysinterface.rs
@@ -1264,8 +1264,9 @@ 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);
- // child_idx is the only thing guaranteed to make each channel unique without a restart
+ // `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
- // role over, we're screwed. Because we only support 32-bit+ systems, assert that our
- // AtomicUsize doesn't reach u32::MAX.
+ // 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]; |
tnull
approved these changes
Jan 3, 2023
arik-so
approved these changes
Jan 3, 2023
MaxFangX
added a commit
to lexe-app/rust-lightning
that referenced
this pull request
Jan 12, 2023
phlip9
pushed a commit
to lexe-app/lexe-public
that referenced
this pull request
Jan 23, 2023
Like 0.0.112, 0.0.113 is broken; LDK panics with 50% probability if we use random nanos: lightningdevkit/rust-lightning#1935 This PR updates LDK past 0.0.113 to the commit that merges the fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
b04d1b8 changed the way we
calculate the
channel_keys_idto include the 128-bituser_channel_idas well, shifting the counter up four bytes andthe
starting_time_nanosfield up into the second four bytes.In
derive_channel_keyswe hash the fullchannel_keys_idwith anHD-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. Becausethe
channel_keys_idfields were shifted up four bytes, that isnow a reference to the
starting_time_nanosvalue. This should befine, 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_nanoswhich has the high bit set. For those usingthe 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.
I'm a little less concerned about this for rust users, but I do think we should backport this to 113 bindings, as bindings users more often use non-time for the
starting_time_nanosfield.