Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure derive_channel_keys doesn't panic if per-run seed is high #1935

Merged
merged 2 commits into from
Jan 3, 2023
Merged

Ensure derive_channel_keys doesn't panic if per-run seed is high #1935

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34de734
Ensure `derive_channel_keys` doesn't panic if per-run seed is high
TheBlueMatt Dec 28, 2022
5dde803
Ensure the per-channel key derivation counter doesn't role over
TheBlueMatt Dec 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how more variables makes things more readable, the indentation as-is makes clear what's being expected where, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, most people find that more comfortable to read, but the PR's perfectly fine as is.

).expect("Your RNG is busted");
unique_start.input(&child_privkey.private_key[..]);

let seed = Sha256::from_engine(unique_start).into_inner();
Expand Down Expand Up @@ -1261,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());
Expand Down