From: Devrandom Date: Sat, 11 Jul 2020 10:19:43 +0000 (-0700) Subject: ChannelKeys - separate commitment revocation from getting the per-commitment point X-Git-Tag: v0.0.12~47^2 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=b19d4475cbfb4c784d7f2ba3125baf31c81d4df0;p=rust-lightning ChannelKeys - separate commitment revocation from getting the per-commitment point The commitment secret is sensitive - it can be used by an attacker to steal funds if the node also signs the same transaction. Therefore, only release the secret from ChannelKeys when we are revoking a transaction. --- diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 9e5d362bb..55bf7c65d 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -195,9 +195,20 @@ impl Readable for SpendableOutputDescriptor { // TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create // ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors. pub trait ChannelKeys : Send+Clone { - /// Gets the commitment seed for a specific commitment number - /// Note that the commitment number starts at (1 << 48) - 1 and counts backwards - fn commitment_secret(&self, idx: u64) -> [u8; 32]; + /// Gets the per-commitment point for a specific commitment number + /// + /// Note that the commitment number starts at (1 << 48) - 1 and counts backwards. + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> PublicKey; + /// Gets the commitment secret for a specific commitment number as part of the revocation process + /// + /// An external signer implementation should error here if the commitment was already signed + /// and should refuse to sign it in the future. + /// + /// May be called more than once for the same index. + /// + /// Note that the commitment number starts at (1 << 48) - 1 and counts backwards. + /// TODO: return a Result so we can signal a validation error + fn release_commitment_secret(&self, idx: u64) -> [u8; 32]; /// Gets the local channel public keys and basepoints fn pubkeys(&self) -> &ChannelPublicKeys; /// Gets arbitrary identifiers describing the set of keys which are provided back to you in @@ -217,6 +228,7 @@ pub trait ChannelKeys : Send+Clone { /// Create a signature for a local commitment transaction. This will only ever be called with /// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees /// that it will not be called multiple times. + /// An external signer implementation should check that the commitment has not been revoked. // // TODO: Document the things someone using this interface should enforce before signing. // TODO: Add more input vars to enable better checking (preferably removing commitment_tx and @@ -405,7 +417,12 @@ impl InMemoryChannelKeys { } impl ChannelKeys for InMemoryChannelKeys { - fn commitment_secret(&self, idx: u64) -> [u8; 32] { + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> PublicKey { + let commitment_secret = SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx)).unwrap(); + PublicKey::from_secret_key(secp_ctx, &commitment_secret) + } + + fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { chan_utils::build_commitment_secret(&self.commitment_seed, idx) } diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index d780c33c1..ba175733a 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -497,8 +497,8 @@ pub fn build_htlc_transaction(prev_hash: &Txid, feerate_per_kw: u32, to_self_del #[derive(Clone)] /// We use this to track local commitment transactions and put off signing them until we are ready -/// to broadcast. Eventually this will require a signer which is possibly external, but for now we -/// just pass in the SecretKeys required. +/// to broadcast. This class can be used inside a signer implementation to generate a signature +/// given the relevant secret key. pub struct LocalCommitmentTransaction { // TODO: We should migrate away from providing the transaction, instead providing enough to // allow the ChannelKeys to construct it from scratch. Luckily we already have HTLC data here, diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index a0879d4e7..fc3b4fd81 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -785,13 +785,6 @@ impl Channel { Ok(chan) } - // Utilities to derive keys: - - fn build_local_commitment_secret(&self, idx: u64) -> SecretKey { - let res = self.local_keys.commitment_secret(idx); - SecretKey::from_slice(&res).unwrap() - } - // Utilities to build transactions: fn get_commitment_transaction_number_obscure_factor(&self) -> u64 { @@ -1123,7 +1116,7 @@ impl Channel { /// The result is a transaction which we can revoke ownership of (ie a "local" transaction) /// TODO Some magic rust shit to compile-time check this? fn build_local_transaction_keys(&self, commitment_number: u64) -> Result { - let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(commitment_number)); + let per_commitment_point = self.local_keys.get_per_commitment_point(commitment_number, &self.secp_ctx); let delayed_payment_base = &self.local_keys.pubkeys().delayed_payment_basepoint; let htlc_basepoint = &self.local_keys.pubkeys().htlc_basepoint; let their_pubkeys = self.their_pubkeys.as_ref().unwrap(); @@ -2028,8 +2021,8 @@ impl Channel { } } - let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number - 1)); - let per_commitment_secret = self.local_keys.commitment_secret(self.cur_local_commitment_transaction_number + 1); + let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number - 1, &self.secp_ctx); + let per_commitment_secret = self.local_keys.release_commitment_secret(self.cur_local_commitment_transaction_number + 1); // Update state now that we've passed all the can-fail calls... let mut need_our_commitment = false; @@ -2614,8 +2607,7 @@ impl Channel { let funding_locked = if self.monitor_pending_funding_locked { assert!(!self.channel_outbound, "Funding transaction broadcast without FundingBroadcastSafe!"); self.monitor_pending_funding_locked = false; - let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); - let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret); + let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx); Some(msgs::FundingLocked { channel_id: self.channel_id(), next_per_commitment_point: next_per_commitment_point, @@ -2667,8 +2659,8 @@ impl Channel { } fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK { - let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number)); - let per_commitment_secret = self.local_keys.commitment_secret(self.cur_local_commitment_transaction_number + 2); + let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx); + let per_commitment_secret = self.local_keys.release_commitment_secret(self.cur_local_commitment_transaction_number + 2); msgs::RevokeAndACK { channel_id: self.channel_id, per_commitment_secret, @@ -2751,7 +2743,10 @@ impl Channel { if msg.next_remote_commitment_number > 0 { match msg.data_loss_protect { OptionalField::Present(ref data_loss) => { - if self.local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1) != data_loss.your_last_per_commitment_secret { + let expected_point = self.local_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.secp_ctx); + let given_secret = SecretKey::from_slice(&data_loss.your_last_per_commitment_secret) + .map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key".to_owned()))?; + if expected_point != PublicKey::from_secret_key(&self.secp_ctx, &given_secret) { return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided".to_owned())); } if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number { @@ -2787,8 +2782,7 @@ impl Channel { } // We have OurFundingLocked set! - let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); - let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret); + let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx); return Ok((Some(msgs::FundingLocked { channel_id: self.channel_id(), next_per_commitment_point: next_per_commitment_point, @@ -2818,8 +2812,7 @@ impl Channel { let resend_funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number == 1 { // We should never have to worry about MonitorUpdateFailed resending FundingLocked - let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); - let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret); + let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx); Some(msgs::FundingLocked { channel_id: self.channel_id(), next_per_commitment_point: next_per_commitment_point, @@ -3405,8 +3398,7 @@ impl Channel { //a protocol oversight, but I assume I'm just missing something. if need_commitment_update { if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 { - let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); - let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret); + let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx); return Ok((Some(msgs::FundingLocked { channel_id: self.channel_id, next_per_commitment_point: next_per_commitment_point, @@ -3457,7 +3449,7 @@ impl Channel { panic!("Tried to send an open_channel for a channel that has already advanced"); } - let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); + let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx); let local_keys = self.local_keys.pubkeys(); msgs::OpenChannel { @@ -3477,7 +3469,7 @@ impl Channel { payment_point: local_keys.payment_point, delayed_payment_basepoint: local_keys.delayed_payment_basepoint, htlc_basepoint: local_keys.htlc_basepoint, - first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret), + first_per_commitment_point, channel_flags: if self.config.announced_channel {1} else {0}, shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() }) } @@ -3494,7 +3486,7 @@ impl Channel { panic!("Tried to send an accept_channel for a channel that has already advanced"); } - let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number); + let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx); let local_keys = self.local_keys.pubkeys(); msgs::AcceptChannel { @@ -3511,7 +3503,7 @@ impl Channel { payment_point: local_keys.payment_point, delayed_payment_basepoint: local_keys.delayed_payment_basepoint, htlc_basepoint: local_keys.htlc_basepoint, - first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret), + first_per_commitment_point, shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() }) } } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 4d2b155ce..ce9ec1d58 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1612,7 +1612,7 @@ fn test_fee_spike_violation_fails_htlc() { let chan_keys = local_chan.get_local_keys(); let pubkeys = chan_keys.pubkeys(); (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point, - chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)) + chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)) }; let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_payment_point, remote_secret1) = { let chan_lock = nodes[1].node.channel_state.lock().unwrap(); @@ -1620,7 +1620,7 @@ fn test_fee_spike_violation_fails_htlc() { let chan_keys = remote_chan.get_local_keys(); let pubkeys = chan_keys.pubkeys(); (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point, - chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 1)) + chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1)) }; // Assemble the set of keys we can use for signatures for our commitment_signed message. @@ -8132,8 +8132,8 @@ fn test_counterparty_raa_skip_no_crash() { let local_keys = &guard.by_id.get_mut(&channel_id).unwrap().local_keys; const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1; let next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(), - &SecretKey::from_slice(&local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap()); - let per_commitment_secret = local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER); + &SecretKey::from_slice(&local_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap()); + let per_commitment_secret = local_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER); nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &msgs::RevokeAndACK { channel_id, per_commitment_secret, next_per_commitment_point }); diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 88cdd94af..c1d3d4902 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -48,7 +48,15 @@ impl EnforcingChannelKeys { } impl ChannelKeys for EnforcingChannelKeys { - fn commitment_secret(&self, idx: u64) -> [u8; 32] { self.inner.commitment_secret(idx) } + fn get_per_commitment_point(&self, idx: u64, secp_ctx: &Secp256k1) -> PublicKey { + self.inner.get_per_commitment_point(idx, secp_ctx) + } + + fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { + // TODO: enforce the ChannelKeys contract - error here if we already signed this commitment + self.inner.release_commitment_secret(idx) + } + fn pubkeys(&self) -> &ChannelPublicKeys { self.inner.pubkeys() } fn key_derivation_params(&self) -> (u64, u64) { self.inner.key_derivation_params() } @@ -71,6 +79,8 @@ impl ChannelKeys for EnforcingChannelKeys { } fn sign_local_commitment(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { + // TODO: enforce the ChannelKeys contract - error if this commitment was already revoked + // TODO: need the commitment number Ok(self.inner.sign_local_commitment(local_commitment_tx, secp_ctx).unwrap()) }