Bug fix using same seed for channel keys generation #229
Conversation
|
Great, was what I would like to do at first, but didn't in keys interface rush writing for demo :) |
|
Oops, thanks for this, totally forgot to fix pre-merge. I think we should think about folding in RNG input as well here - the channel keys need backup anyway as they're mostly used in combination with remote input and we don't want an old backup to end up replaying keys.
…On October 28, 2018 5:43:44 AM UTC, yuntai ***@***.***> wrote:
You can view, comment on, or merge this pull request online at:
#229
-- Commit Summary --
* Bug fix using same seed for channel keys generation
-- File Changes --
M src/chain/keysinterface.rs (15)
-- Patch Links --
https://github.com/rust-bitcoin/rust-lightning/pull/229.patch
https://github.com/rust-bitcoin/rust-lightning/pull/229.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#229
|
|
ah.. then, for channel keys, HD stuff is any helpful? hash(rng) is enough? |
|
If we have more stuff available, shoving it in the hash won't hurt and RNGs have been more than problematic on some platforms many times - hashing rng output plus something derived from the master key seems like a good idea. Maybe be extra doubly overkill careful and shove the current time in there as well to make sure keys can't be duplicated, even though it doesn't help with security. |
|
I'm still not so sure why we need to keep around a fixed channel master key (for a fixed user seed) if we randomize the derived keys anyway. To me, hash(rng||timestamp) as a seed for generating channel_keys alone seems enough and simple(no id tracking). |
|
One of the long-term goals of the key interface is to move towards a world where we can function without using our own RNG dependency and just push that complexity onto the user (for, eg, a hardware wallet or some other device with shitty entropy). |
|
@ariard Sorry, didn't realize you are in the middle of work for this. As this patch is a quick point fix, hope it won't conflict too much with your local work. |
|
Ah sorry no I didn't work further on the channel key generation, because it's only a default implementation for KeysInterface. Long term it should be up to the user to write it's own scheme. |
src/chain/keysinterface.rs
Outdated
| let mut seed = [0u8; 32 + 4 + 8 + 33]; | ||
| rng::fill_bytes(&mut seed[..32]); | ||
| let now = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time went backwards"); | ||
| seed[32..32+4].copy_from_slice(&byte_utils::be32_to_array(now.subsec_micros())); |
There was a problem hiding this comment.
I think you want nanos.
src/chain/keysinterface.rs
Outdated
| seed[32..32+4].copy_from_slice(&byte_utils::be32_to_array(now.subsec_micros())); | ||
| seed[32+4..32+4+8].copy_from_slice(&byte_utils::be64_to_array(now.as_secs())); | ||
|
|
||
| let child_ix = self.channel_child_index.fetch_add(1, Ordering::SeqCst); |
There was a problem hiding this comment.
RelAcq should be fine here.
src/chain/keysinterface.rs
Outdated
|
|
||
| let child_ix = self.channel_child_index.fetch_add(1, Ordering::SeqCst); | ||
| let child_privkey = self.channel_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32)).expect("Your RNG is busted"); | ||
| let child_pubkey = ExtendedPubKey::from_private(&self.secp_ctx, &child_privkey); |
There was a problem hiding this comment.
Why convert to pub? Just hash the secret key?
| ChannelKeys::new_from_seed(&seed) | ||
| let mut seed = [0u8; 32 + 4 + 8 + 33]; | ||
| rng::fill_bytes(&mut seed[..32]); | ||
| let now = SystemTime::now().duration_since(UNIX_EPOCH).expect("Time went backwards"); |
There was a problem hiding this comment.
Can you add a comment here something like:
"We only seriously intend to rely on the channel_master_key for true secure entropy, everything else just ensures uniqueness. We generally don't expect all clients to have non-broken RNGs here, so we also include the current time as a fallback to get uniqueness."
yuntai
force-pushed
the
201810-sameseedfix
branch
from
e01d721 to
55c9f76
Compare
| let child_ix = self.channel_child_index.fetch_add(1, Ordering::AcqRel); | ||
| let child_privkey = self.channel_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32)).expect("Your RNG is busted"); | ||
| seed[32+4+8..32+4+8+32].copy_from_slice(&child_privkey.secret_key[..32]); | ||
| ChannelKeys::new_from_seed(&Sha256dHash::from_data(&seed).as_bytes()) |
There was a problem hiding this comment.
No need to double-sha here.
|
|
||
| let child_ix = self.channel_child_index.fetch_add(1, Ordering::AcqRel); | ||
| let child_privkey = self.channel_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32)).expect("Your RNG is busted"); | ||
| seed[32+4+8..32+4+8+32].copy_from_slice(&child_privkey.secret_key[..32]); |
There was a problem hiding this comment.
nit: secret_key is 32 bytes.
| ChannelKeys::new_from_seed(&seed) | ||
| // We only seriously intend to rely on the channel_master_key for true secure entropy, | ||
| // everything else just ensures uniqueness. We generally don't expect all clients | ||
| // to have non-broken RNGs here, so we also include the current time as a fallback to get uniqueness." |
There was a problem hiding this comment.
nit: Oops, you left the " at the end here.
No description provided.