mirrors/rust-lightning
1
0
Fork 0
mirror of https://github.com/lightningdevkit/rust-lightning.git synced 2025-02-25 07:17:40 +01:00
Code Issues Projects Releases Packages Wiki Activity

ChannelKeys - separate commitment revocation from getting the per-commitment point

Browse source 
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.
This commit is contained in:
Devrandom 2020-07-11 03:19:43 -07:00
parent 50df4cf464
commit b19d4475cb
5 changed files with 55 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

25
lightning/src/chain/keysinterface.rs
View file

@ -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<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> 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<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> 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)
}

4
lightning/src/ln/chan_utils.rs
View file

@ -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,

42
lightning/src/ln/channel.rs
View file

@ -785,13 +785,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
/// 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<TxCreationKeys, ChannelError> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}
}
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}
// 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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
//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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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() })
}
}

8
lightning/src/ln/functional_tests.rs
View file

@ -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 });

12
lightning/src/util/enforcing_trait_impls.rs
View file

@ -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<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> 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<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
// 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())
}