From 5671d2930d996e864614703696ed2cba5e47e4f0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 30 Nov 2022 22:34:11 +0000 Subject: [PATCH] Remove unreachable `Err` cases on `derive_*_revocation_key` The `derive_{public,private}_revocation_key` methods hash the two input keys and then multiply the two input keys by hashed values before adding them together. Because addition can fail if the tweak is the inverse of the secret key this method currently returns a `Result`. However, it is not cryptographically possible to reach the error case - in order to create an issue, the point-multiplied-by-hash values must be the inverse of each other, however each point commits the SHA-256 hash of both keys together. Thus, because changing either key changes the hashes (and the ultimate points added together) in an unpredictable way, there should be no way to construct such points. --- lightning/src/chain/channelmonitor.rs | 34 ++++++++++----------------- lightning/src/chain/keysinterface.rs | 17 +++++++------- lightning/src/ln/chan_utils.rs | 27 +++++++++++++-------- lightning/src/ln/channel.rs | 4 ++-- 4 files changed, 40 insertions(+), 42 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index a93985e09..cc88a0119 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2443,7 +2443,7 @@ impl ChannelMonitorImpl { let secret = self.get_secret(commitment_number).unwrap(); let per_commitment_key = ignore_error!(SecretKey::from_slice(&secret)); let per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key); - let revocation_pubkey = ignore_error!(chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint)); + let revocation_pubkey = chan_utils::derive_public_revocation_key(&self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint); let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); let revokeable_redeemscript = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.counterparty_commitment_params.on_counterparty_tx_csv, &delayed_key); @@ -2556,26 +2556,18 @@ impl ChannelMonitorImpl { } else { return (claimable_outpoints, to_counterparty_output_info); }; if let Some(transaction) = tx { - let revokeable_p2wsh_opt = - if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key( - &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint) - { - let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, - &per_commitment_point, - &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); - Some(chan_utils::get_revokeable_redeemscript(&revocation_pubkey, - self.counterparty_commitment_params.on_counterparty_tx_csv, - &delayed_key).to_v0_p2wsh()) - } else { - debug_assert!(false, "Failed to derive a revocation pubkey key for a commitment state we accepted"); - None - }; - if let Some(revokeable_p2wsh) = revokeable_p2wsh_opt { - for (idx, outp) in transaction.output.iter().enumerate() { - if outp.script_pubkey == revokeable_p2wsh { - to_counterparty_output_info = - Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); - } + let revocation_pubkey = chan_utils::derive_public_revocation_key( + &self.secp_ctx, &per_commitment_point, &self.holder_revocation_basepoint); + let delayed_key = chan_utils::derive_public_key(&self.secp_ctx, + &per_commitment_point, + &self.counterparty_commitment_params.counterparty_delayed_payment_base_key); + let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, + self.counterparty_commitment_params.on_counterparty_tx_csv, + &delayed_key).to_v0_p2wsh(); + for (idx, outp) in transaction.output.iter().enumerate() { + if outp.script_pubkey == revokeable_p2wsh { + to_counterparty_output_info = + Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); } } } diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 266544d78..db1c6e943 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -742,9 +742,9 @@ impl BaseSign for InMemorySigner { } fn sign_justice_revoked_output(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, secp_ctx: &Secp256k1) -> Result { - let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key).map_err(|_| ())?; + let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key); let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key); - let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?; + let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint); let witness_script = { let counterparty_delayedpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().delayed_payment_basepoint); chan_utils::get_revokeable_redeemscript(&revocation_pubkey, self.holder_selected_contest_delay(), &counterparty_delayedpubkey) @@ -755,9 +755,9 @@ impl BaseSign for InMemorySigner { } fn sign_justice_revoked_htlc(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result { - let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key).map_err(|_| ())?; + let revocation_key = chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_key, &self.revocation_base_key); let per_commitment_point = PublicKey::from_secret_key(secp_ctx, &per_commitment_key); - let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint).map_err(|_| ())?; + let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint); let witness_script = { let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); let holder_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); @@ -770,11 +770,10 @@ impl BaseSign for InMemorySigner { fn sign_counterparty_htlc_transaction(&self, htlc_tx: &Transaction, input: usize, amount: u64, per_commitment_point: &PublicKey, htlc: &HTLCOutputInCommitment, secp_ctx: &Secp256k1) -> Result { let htlc_key = chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &self.htlc_base_key); - let witness_script = if let Ok(revocation_pubkey) = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint) { - let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); - let htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); - chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey) - } else { return Err(()) }; + let revocation_pubkey = chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &self.pubkeys().revocation_basepoint); + let counterparty_htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.counterparty_pubkeys().htlc_basepoint); + let htlcpubkey = chan_utils::derive_public_key(&secp_ctx, &per_commitment_point, &self.pubkeys().htlc_basepoint); + let witness_script = chan_utils::get_htlc_redeemscript_with_explicit_keys(&htlc, self.opt_anchors(), &counterparty_htlcpubkey, &htlcpubkey, &revocation_pubkey); let mut sighash_parts = sighash::SighashCache::new(htlc_tx); let sighash = hash_to_message!(&sighash_parts.segwit_signature_hash(input, &witness_script, amount, EcdsaSighashType::All).unwrap()[..]); Ok(sign(secp_ctx, &sighash, &htlc_key)) diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs index 0dbf18684..0b9381d2d 100644 --- a/lightning/src/ln/chan_utils.rs +++ b/lightning/src/ln/chan_utils.rs @@ -347,10 +347,9 @@ pub fn derive_public_key(secp_ctx: &Secp256k1, per_com /// commitment transaction, thus per_commitment_secret always come from cheater /// and revocation_base_secret always come from punisher, which is the broadcaster /// of the transaction spending with this key knowledge. -/// -/// Note that this is infallible iff we trust that at least one of the two input keys are randomly -/// generated (ie our own). -pub fn derive_private_revocation_key(secp_ctx: &Secp256k1, per_commitment_secret: &SecretKey, countersignatory_revocation_base_secret: &SecretKey) -> Result { +pub fn derive_private_revocation_key(secp_ctx: &Secp256k1, + per_commitment_secret: &SecretKey, countersignatory_revocation_base_secret: &SecretKey) +-> SecretKey { let countersignatory_revocation_base_point = PublicKey::from_secret_key(&secp_ctx, &countersignatory_revocation_base_secret); let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &per_commitment_secret); @@ -369,9 +368,12 @@ pub fn derive_private_revocation_key(secp_ctx: &Secp256k1 Sha256::from_engine(sha).into_inner() }; - let countersignatory_contrib = countersignatory_revocation_base_secret.clone().mul_tweak(&Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())?; - let broadcaster_contrib = per_commitment_secret.clone().mul_tweak(&Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())?; + let countersignatory_contrib = countersignatory_revocation_base_secret.clone().mul_tweak(&Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap()) + .expect("Multiplying a secret key by a hash is expected to never fail per secp256k1 docs"); + let broadcaster_contrib = per_commitment_secret.clone().mul_tweak(&Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap()) + .expect("Multiplying a secret key by a hash is expected to never fail per secp256k1 docs"); countersignatory_contrib.add_tweak(&Scalar::from_be_bytes(broadcaster_contrib.secret_bytes()).unwrap()) + .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak commits to the key.") } /// Derives a per-commitment-transaction revocation public key from its constituent parts. This is @@ -385,7 +387,9 @@ pub fn derive_private_revocation_key(secp_ctx: &Secp256k1 /// /// Note that this is infallible iff we trust that at least one of the two input keys are randomly /// generated (ie our own). -pub fn derive_public_revocation_key(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, countersignatory_revocation_base_point: &PublicKey) -> Result { +pub fn derive_public_revocation_key(secp_ctx: &Secp256k1, + per_commitment_point: &PublicKey, countersignatory_revocation_base_point: &PublicKey) +-> PublicKey { let rev_append_commit_hash_key = { let mut sha = Sha256::engine(); sha.input(&countersignatory_revocation_base_point.serialize()); @@ -401,9 +405,12 @@ pub fn derive_public_revocation_key(secp_ctx: &Secp2 Sha256::from_engine(sha).into_inner() }; - let countersignatory_contrib = countersignatory_revocation_base_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap())?; - let broadcaster_contrib = per_commitment_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap())?; + let countersignatory_contrib = countersignatory_revocation_base_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(rev_append_commit_hash_key).unwrap()) + .expect("Multiplying a valid public key by a hash is expected to never fail per secp256k1 docs"); + let broadcaster_contrib = per_commitment_point.clone().mul_tweak(&secp_ctx, &Scalar::from_be_bytes(commit_append_rev_hash_key).unwrap()) + .expect("Multiplying a valid public key by a hash is expected to never fail per secp256k1 docs"); countersignatory_contrib.combine(&broadcaster_contrib) + .expect("Addition only fails if the tweak is the inverse of the key. This is not possible when the tweak commits to the key.") } /// The set of public keys which are used in the creation of one commitment transaction. @@ -479,7 +486,7 @@ impl TxCreationKeys { pub fn derive_new(secp_ctx: &Secp256k1, per_commitment_point: &PublicKey, broadcaster_delayed_payment_base: &PublicKey, broadcaster_htlc_base: &PublicKey, countersignatory_revocation_base: &PublicKey, countersignatory_htlc_base: &PublicKey) -> Result { Ok(TxCreationKeys { per_commitment_point: per_commitment_point.clone(), - revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base)?, + revocation_key: derive_public_revocation_key(&secp_ctx, &per_commitment_point, &countersignatory_revocation_base), broadcaster_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_htlc_base), countersignatory_htlc_key: derive_public_key(&secp_ctx, &per_commitment_point, &countersignatory_htlc_base), broadcaster_delayed_payment_key: derive_public_key(&secp_ctx, &per_commitment_point, &broadcaster_delayed_payment_base), diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 0c0cbe83f..ef7c77f0b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -7968,10 +7968,10 @@ mod tests { assert_eq!(chan_utils::derive_private_key(&secp_ctx, &per_commitment_point, &base_secret), SecretKey::from_slice(&hex::decode("cbced912d3b21bf196a766651e436aff192362621ce317704ea2f75d87e7be0f").unwrap()[..]).unwrap()); - assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).unwrap().serialize()[..], + assert_eq!(chan_utils::derive_public_revocation_key(&secp_ctx, &per_commitment_point, &base_point).serialize()[..], hex::decode("02916e326636d19c33f13e8c0c3a03dd157f332f3e99c317c141dd865eb01f8ff0").unwrap()[..]); - assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret).unwrap(), + assert_eq!(chan_utils::derive_private_revocation_key(&secp_ctx, &per_commitment_secret, &base_secret), SecretKey::from_slice(&hex::decode("d09ffff62ddb2297ab000cc85bcb4283fdeb6aa052affbc9dddcf33b61078110").unwrap()[..]).unwrap()); } -- 2.39.5