Skip to content

Commit b661164

Browse files
devrandomdevrandom
committed
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.
1 parent bb369b5 commit b661164

File tree

5 files changed

+56
-36
lines changed

5 files changed

+56
-36
lines changed

‎lightning/src/chain/keysinterface.rs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -195,9 +195,21 @@ impl Readable for SpendableOutputDescriptor {
195195
// TODO: We should remove Clone by instead requesting a new ChannelKeys copy when we create
196196
// ChannelMonitors instead of expecting to clone the one out of the Channel into the monitors.
197197
pub trait ChannelKeys : Send+Clone {
198-
/// Gets the commitment seed for a specific commitment number
199-
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards
200-
fn commitment_secret(&self, idx: u64) -> [u8; 32];
198+
/// Gets the per-commitment point for a specific commitment number
199+
///
200+
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards.
201+
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey;
202+
/// Gets the commitment secret for a specific commitment number as part of the revocation process
203+
///
204+
/// An implementation must error here if the commitment was already signed.
205+
/// After this method is called, an implementation must ensure that it will refuse sign the relevant
206+
/// local commitment transaction.
207+
///
208+
/// May be called more than once for the same index.
209+
///
210+
/// Note that the commitment number starts at (1 << 48) - 1 and counts backwards.
211+
/// TODO: return a Result so we can signal a validation error
212+
fn release_commitment_secret(&self, idx: u64) -> [u8; 32];
201213
/// Gets the local channel public keys and basepoints
202214
fn pubkeys(&self) -> &ChannelPublicKeys;
203215
/// Gets arbitrary identifiers describing the set of keys which are provided back to you in
@@ -217,6 +229,7 @@ pub trait ChannelKeys : Send+Clone {
217229
/// Create a signature for a local commitment transaction. This will only ever be called with
218230
/// the same local_commitment_tx (or a copy thereof), though there are currently no guarantees
219231
/// that it will not be called multiple times.
232+
/// The commitment must not have been revoked.
220233
//
221234
// TODO: Document the things someone using this interface should enforce before signing.
222235
// TODO: Add more input vars to enable better checking (preferably removing commitment_tx and
@@ -405,7 +418,12 @@ impl InMemoryChannelKeys {
405418
}
406419

407420
impl ChannelKeys for InMemoryChannelKeys {
408-
fn commitment_secret(&self, idx: u64) -> [u8; 32] {
421+
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey {
422+
let commitment_secret = SecretKey::from_slice(&chan_utils::build_commitment_secret(&self.commitment_seed, idx)).unwrap();
423+
PublicKey::from_secret_key(secp_ctx, &commitment_secret)
424+
}
425+
426+
fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
409427
chan_utils::build_commitment_secret(&self.commitment_seed, idx)
410428
}
411429

‎lightning/src/ln/chan_utils.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -497,8 +497,8 @@ pub fn build_htlc_transaction(prev_hash: &Txid, feerate_per_kw: u32, to_self_del
497497

498498
#[derive(Clone)]
499499
/// We use this to track local commitment transactions and put off signing them until we are ready
500-
/// to broadcast. Eventually this will require a signer which is possibly external, but for now we
501-
/// just pass in the SecretKeys required.
500+
/// to broadcast. This class can be used inside a signer implementation to generate a signature
501+
/// given the relevant secret key.
502502
pub struct LocalCommitmentTransaction {
503503
// TODO: We should migrate away from providing the transaction, instead providing enough to
504504
// allow the ChannelKeys to construct it from scratch. Luckily we already have HTLC data here,

‎lightning/src/ln/channel.rs

Lines changed: 17 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -780,13 +780,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
780780
Ok(chan)
781781
}
782782

783-
// Utilities to derive keys:
784-
785-
fn build_local_commitment_secret(&self, idx: u64) -> SecretKey {
786-
let res = self.local_keys.commitment_secret(idx);
787-
SecretKey::from_slice(&res).unwrap()
788-
}
789-
790783
// Utilities to build transactions:
791784

792785
fn get_commitment_transaction_number_obscure_factor(&self) -> u64 {
@@ -1118,7 +1111,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
11181111
/// The result is a transaction which we can revoke ownership of (ie a "local" transaction)
11191112
/// TODO Some magic rust shit to compile-time check this?
11201113
fn build_local_transaction_keys(&self, commitment_number: u64) -> Result<TxCreationKeys, ChannelError> {
1121-
let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(commitment_number));
1114+
let per_commitment_point = self.local_keys.get_per_commitment_point(commitment_number, &self.secp_ctx);
11221115
let delayed_payment_base = &self.local_keys.pubkeys().delayed_payment_basepoint;
11231116
let htlc_basepoint = &self.local_keys.pubkeys().htlc_basepoint;
11241117
let their_pubkeys = self.their_pubkeys.as_ref().unwrap();
@@ -2020,8 +2013,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
20202013
}
20212014
}
20222015

2023-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number - 1));
2024-
let per_commitment_secret = self.local_keys.commitment_secret(self.cur_local_commitment_transaction_number + 1);
2016+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number - 1, &self.secp_ctx);
2017+
let per_commitment_secret = self.local_keys.release_commitment_secret(self.cur_local_commitment_transaction_number + 1);
20252018

20262019
// Update state now that we've passed all the can-fail calls...
20272020
let mut need_our_commitment = false;
@@ -2606,8 +2599,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
26062599
let funding_locked = if self.monitor_pending_funding_locked {
26072600
assert!(!self.channel_outbound, "Funding transaction broadcast without FundingBroadcastSafe!");
26082601
self.monitor_pending_funding_locked = false;
2609-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2610-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2602+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
26112603
Some(msgs::FundingLocked {
26122604
channel_id: self.channel_id(),
26132605
next_per_commitment_point: next_per_commitment_point,
@@ -2659,8 +2651,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
26592651
}
26602652

26612653
fn get_last_revoke_and_ack(&self) -> msgs::RevokeAndACK {
2662-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number));
2663-
let per_commitment_secret = self.local_keys.commitment_secret(self.cur_local_commitment_transaction_number + 2);
2654+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
2655+
let per_commitment_secret = self.local_keys.release_commitment_secret(self.cur_local_commitment_transaction_number + 2);
26642656
msgs::RevokeAndACK {
26652657
channel_id: self.channel_id,
26662658
per_commitment_secret,
@@ -2743,7 +2735,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
27432735
if msg.next_remote_commitment_number > 0 {
27442736
match msg.data_loss_protect {
27452737
OptionalField::Present(ref data_loss) => {
2746-
if self.local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1) != data_loss.your_last_per_commitment_secret {
2738+
let expected_point = self.local_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - msg.next_remote_commitment_number + 1, &self.secp_ctx);
2739+
let given_secret = SecretKey::from_slice(&data_loss.your_last_per_commitment_secret)
2740+
.map_err(|_| ChannelError::Close("Peer sent a garbage channel_reestablish with unparseable secret key"))?;
2741+
if expected_point != PublicKey::from_secret_key(&self.secp_ctx, &given_secret) {
27472742
return Err(ChannelError::Close("Peer sent a garbage channel_reestablish with secret key not matching the commitment height provided"));
27482743
}
27492744
if msg.next_remote_commitment_number > INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
@@ -2779,8 +2774,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
27792774
}
27802775

27812776
// We have OurFundingLocked set!
2782-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2783-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2777+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
27842778
return Ok((Some(msgs::FundingLocked {
27852779
channel_id: self.channel_id(),
27862780
next_per_commitment_point: next_per_commitment_point,
@@ -2810,8 +2804,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
28102804

28112805
let resend_funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number == 1 {
28122806
// We should never have to worry about MonitorUpdateFailed resending FundingLocked
2813-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
2814-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
2807+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
28152808
Some(msgs::FundingLocked {
28162809
channel_id: self.channel_id(),
28172810
next_per_commitment_point: next_per_commitment_point,
@@ -3397,8 +3390,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
33973390
//a protocol oversight, but I assume I'm just missing something.
33983391
if need_commitment_update {
33993392
if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) == 0 {
3400-
let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
3401-
let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
3393+
let next_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
34023394
return Ok((Some(msgs::FundingLocked {
34033395
channel_id: self.channel_id,
34043396
next_per_commitment_point: next_per_commitment_point,
@@ -3449,7 +3441,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
34493441
panic!("Tried to send an open_channel for a channel that has already advanced");
34503442
}
34513443

3452-
let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
3444+
let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
34533445
let local_keys = self.local_keys.pubkeys();
34543446

34553447
msgs::OpenChannel {
@@ -3469,7 +3461,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
34693461
payment_point: local_keys.payment_point,
34703462
delayed_payment_basepoint: local_keys.delayed_payment_basepoint,
34713463
htlc_basepoint: local_keys.htlc_basepoint,
3472-
first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
3464+
first_per_commitment_point,
34733465
channel_flags: if self.config.announced_channel {1} else {0},
34743466
shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() })
34753467
}
@@ -3486,7 +3478,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
34863478
panic!("Tried to send an accept_channel for a channel that has already advanced");
34873479
}
34883480

3489-
let local_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
3481+
let first_per_commitment_point = self.local_keys.get_per_commitment_point(self.cur_local_commitment_transaction_number, &self.secp_ctx);
34903482
let local_keys = self.local_keys.pubkeys();
34913483

34923484
msgs::AcceptChannel {
@@ -3503,7 +3495,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
35033495
payment_point: local_keys.payment_point,
35043496
delayed_payment_basepoint: local_keys.delayed_payment_basepoint,
35053497
htlc_basepoint: local_keys.htlc_basepoint,
3506-
first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
3498+
first_per_commitment_point,
35073499
shutdown_scriptpubkey: OptionalField::Present(if self.config.commit_upfront_shutdown_pubkey { self.get_closing_scriptpubkey() } else { Builder::new().into_script() })
35083500
}
35093501
}

‎lightning/src/ln/functional_tests.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1609,15 +1609,15 @@ fn test_fee_spike_violation_fails_htlc() {
16091609
let chan_keys = local_chan.get_local_keys();
16101610
let pubkeys = chan_keys.pubkeys();
16111611
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
1612-
chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 2))
1612+
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2))
16131613
};
16141614
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_payment_point, remote_secret1) = {
16151615
let chan_lock = nodes[1].node.channel_state.lock().unwrap();
16161616
let remote_chan = chan_lock.by_id.get(&chan.2).unwrap();
16171617
let chan_keys = remote_chan.get_local_keys();
16181618
let pubkeys = chan_keys.pubkeys();
16191619
(pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, pubkeys.payment_point,
1620-
chan_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 1))
1620+
chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1))
16211621
};
16221622

16231623
// Assemble the set of keys we can use for signatures for our commitment_signed message.
@@ -8129,8 +8129,8 @@ fn test_counterparty_raa_skip_no_crash() {
81298129
let local_keys = &guard.by_id.get_mut(&channel_id).unwrap().local_keys;
81308130
const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
81318131
let next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
8132-
&SecretKey::from_slice(&local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
8133-
let per_commitment_secret = local_keys.commitment_secret(INITIAL_COMMITMENT_NUMBER);
8132+
&SecretKey::from_slice(&local_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
8133+
let per_commitment_secret = local_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
81348134

81358135
nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(),
81368136
&msgs::RevokeAndACK { channel_id, per_commitment_secret, next_per_commitment_point });

‎lightning/src/util/enforcing_trait_impls.rs

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,15 @@ impl EnforcingChannelKeys {
4848
}
4949

5050
impl ChannelKeys for EnforcingChannelKeys {
51-
fn commitment_secret(&self, idx: u64) -> [u8; 32] { self.inner.commitment_secret(idx) }
51+
fn get_per_commitment_point<T: secp256k1::Signing + secp256k1::Verification>(&self, idx: u64, secp_ctx: &Secp256k1<T>) -> PublicKey {
52+
self.inner.get_per_commitment_point(idx, secp_ctx)
53+
}
54+
55+
fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
56+
// TODO: enforce the ChannelKeys contract - error here if we already signed this commitment
57+
self.inner.release_commitment_secret(idx)
58+
}
59+
5260
fn pubkeys(&self) -> &ChannelPublicKeys { self.inner.pubkeys() }
5361
fn key_derivation_params(&self) -> (u64, u64) { self.inner.key_derivation_params() }
5462

@@ -71,6 +79,8 @@ impl ChannelKeys for EnforcingChannelKeys {
7179
}
7280

7381
fn sign_local_commitment<T: secp256k1::Signing + secp256k1::Verification>(&self, local_commitment_tx: &LocalCommitmentTransaction, secp_ctx: &Secp256k1<T>) -> Result<Signature, ()> {
82+
// TODO: enforce the ChannelKeys contract - error if this commitment was already revoked
83+
// TODO: need the commitment number
7484
Ok(self.inner.sign_local_commitment(local_commitment_tx, secp_ctx).unwrap())
7585
}
7686

0 commit comments

Comments
 (0)