From 285b3faf77a4923e180a4b123df8449ba22ef6eb Mon Sep 17 00:00:00 2001 From: Devrandom Date: Mon, 9 Aug 2021 16:48:06 +0200 Subject: [PATCH] Enforce signing counterparty commitment only after revocation --- lightning/src/chain/keysinterface.rs | 8 ++++++++ lightning/src/ln/channel.rs | 8 +++++++- lightning/src/util/enforcing_trait_impls.rs | 12 ++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index ac81e543..d912740a 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -230,6 +230,11 @@ pub trait BaseSign { // // TODO: Document the things someone using this interface should enforce before signing. fn sign_counterparty_commitment(&self, commitment_tx: &CommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; + /// Validate the counterparty's revocation. + /// + /// This is required in order for the signer to make sure that the state has moved + /// forward and it is safe to sign the next counterparty commitment. + fn validate_counterparty_revocation(&self, idx: u64, secret: &SecretKey); /// Create a signatures for a holder's commitment transaction and its claiming HTLC transactions. /// This will only ever be called with a non-revoked commitment_tx. This will be called with the @@ -592,6 +597,9 @@ impl BaseSign for InMemorySigner { Ok((commitment_sig, htlc_sigs)) } + fn validate_counterparty_revocation(&self, _idx: u64, _secret: &SecretKey) { + } + fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 2cd7e3c2..9725650f 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -2742,8 +2742,10 @@ impl Channel { return Err(ChannelError::Close("Peer sent revoke_and_ack after we'd started exchanging closing_signeds".to_owned())); } + let secret = secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned()); + if let Some(counterparty_prev_commitment_point) = self.counterparty_prev_commitment_point { - if PublicKey::from_secret_key(&self.secp_ctx, &secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned())) != counterparty_prev_commitment_point { + if PublicKey::from_secret_key(&self.secp_ctx, &secret) != counterparty_prev_commitment_point { return Err(ChannelError::Close("Got a revoke commitment secret which didn't correspond to their current pubkey".to_owned())); } } @@ -2765,6 +2767,10 @@ impl Channel { *self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None; } + self.holder_signer.validate_counterparty_revocation( + self.cur_counterparty_commitment_transaction_number + 1, + &secret + ); self.commitment_secrets.provide_secret(self.cur_counterparty_commitment_transaction_number + 1, msg.per_commitment_secret) .map_err(|_| ChannelError::Close("Previous secrets did not match new one".to_owned()))?; self.latest_monitor_update_id += 1; diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 18ed0ea8..19408e98 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -122,12 +122,21 @@ impl BaseSign for EnforcingSigner { // These commitment numbers are backwards counting. We expect either the same as the previously encountered, // or the next one. assert!(last_commitment_number == actual_commitment_number || last_commitment_number - 1 == actual_commitment_number, "{} doesn't come after {}", actual_commitment_number, last_commitment_number); + // Ensure that the counterparty doesn't get more than two broadcastable commitments - + // the last and the one we are trying to sign + assert!(actual_commitment_number >= state.last_counterparty_revoked_commitment - 2, "cannot sign a commitment if second to last wasn't revoked - signing {} revoked {}", actual_commitment_number, state.last_counterparty_revoked_commitment); state.last_counterparty_commitment = cmp::min(last_commitment_number, actual_commitment_number) } Ok(self.inner.sign_counterparty_commitment(commitment_tx, secp_ctx).unwrap()) } + fn validate_counterparty_revocation(&self, idx: u64, _secret: &SecretKey) { + let mut state = self.state.lock().unwrap(); + assert!(idx == state.last_counterparty_revoked_commitment || idx == state.last_counterparty_revoked_commitment - 1, "expecting to validate the current or next counterparty revocation - trying {}, current {}", idx, state.last_counterparty_revoked_commitment); + state.last_counterparty_revoked_commitment = idx; + } + fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { let trusted_tx = self.verify_holder_commitment_tx(commitment_tx, secp_ctx); let commitment_txid = trusted_tx.txid(); @@ -230,6 +239,8 @@ impl EnforcingSigner { pub struct EnforcementState { /// The last counterparty commitment number we signed, backwards counting pub last_counterparty_commitment: u64, + /// The last counterparty commitment they revoked, backwards counting + pub last_counterparty_revoked_commitment: u64, /// The last holder commitment number we revoked, backwards counting pub last_holder_revoked_commitment: u64, /// The last validated holder commitment number, backwards counting @@ -241,6 +252,7 @@ impl EnforcementState { pub fn new() -> Self { EnforcementState { last_counterparty_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER, + last_counterparty_revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER, last_holder_revoked_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER, last_holder_commitment: INITIAL_REVOKED_COMMITMENT_NUMBER, } -- 2.30.2