diff --git a/smite-scenarios/src/executor.rs b/smite-scenarios/src/executor.rs index 1c01b145..92444d1a 100644 --- a/smite-scenarios/src/executor.rs +++ b/smite-scenarios/src/executor.rs @@ -17,7 +17,9 @@ use smite::channel_tx::{ build_funding_transaction, }; use smite::noise::{ConnectionError, NoiseConnection}; -use smite::oracles::{AcceptChannelContext, AcceptChannelOracle, Oracle}; +use smite::oracles::{ + AcceptChannelContext, AcceptChannelOracle, FundingSignedContext, FundingSignedOracle, Oracle, +}; use smite::pending_channel::PendingChannel; use smite::violation::Violation; use smite_ir::operation::AcceptChannelField; @@ -491,7 +493,10 @@ impl Executor { log::debug!("[{:?}] RecvFundingSigned: waiting", start.elapsed()); let fs = recv_funding_signed(&mut self.conn)?; log::debug!("[{:?}] RecvFundingSigned: received", start.elapsed()); - verify_funding_signed(&fs, &self.channel_states)?; + FundingSignedOracle.evaluate(&FundingSignedContext { + funding_signed: &fs, + channel: self.channel_states.get(&fs.channel_id), + })?; Some(Variable::ChannelId(fs.channel_id)) } @@ -960,6 +965,10 @@ fn build_funding_created( &accept_channel.funding_pubkey, open_channel.funding_satoshis, ); + // TODO: Once we support sending malformed signatures, update this state + // when constructing one so that the peer's acceptance can be detected as a + // violation. + let sent_invalid_signature = opener_funding_pubkey != open_channel.funding_pubkey; // Building the same message again must not clobber a channel whose state // has already been established (and possibly advanced). @@ -970,6 +979,7 @@ fn build_funding_created( state, is_funding_outpoint_valid, mined_txids.contains(&funding_outpoint.txid), + sent_invalid_signature, ) }); @@ -1316,29 +1326,6 @@ fn is_channel_ready_expected( }) } -/// Verifies the counterparty's signature from a `funding_signed` message using -/// the channel state associated with the message's `channel_id`. -/// -/// # Errors -/// -/// Returns [`Violation::UnknownChannel`] if no channel state exists for the -/// given `channel_id`, or [`Violation::InvalidCounterpartySignature`] if the -/// signature is invalid for the holder's initial commitment transaction. -fn verify_funding_signed( - fs: &FundingSigned, - channel_states: &HashMap, -) -> Result<(), Violation> { - let state = channel_states - .get(&fs.channel_id) - .ok_or(Violation::UnknownChannel(fs.channel_id))?; - - state - .config - .verify_counterparty_signature(&state.commitment, &state.holder, &fs.signature) - .then_some(()) - .ok_or(Violation::InvalidCounterpartySignature(fs.channel_id)) -} - /// Records a sent `open_channel`, keyed by `temporary_channel_id`, so the /// funding flow can build commitments from the values actually put on the wire. /// @@ -3597,10 +3584,13 @@ mod tests { std::time::Instant::now(), ) .unwrap_err(); - assert!(matches!( - err, - ExecuteError::Violation(Violation::UnknownChannel(id)) if id == channel_id - )); + let ExecuteError::Violation(Violation::InvalidFundingSigned(id, reason)) = &err else { + panic!("unexpected error: {err:?}"); + }; + assert_eq!(*id, channel_id); + assert!( + reason.contains("unknown channel_id: no funding_created was sent for this channel") + ); } #[test] @@ -3637,10 +3627,11 @@ mod tests { std::time::Instant::now(), ) .unwrap_err(); - assert!(matches!( - err, - ExecuteError::Violation(Violation::InvalidCounterpartySignature(id)) if id == channel_id - )); + let ExecuteError::Violation(Violation::InvalidFundingSigned(id, reason)) = &err else { + panic!("unexpected error: {err:?}"); + }; + assert_eq!(*id, channel_id); + assert!(reason.contains("invalid funding_signed: signature is not valid")); } #[test] @@ -3866,17 +3857,41 @@ mod tests { #[test] fn execute_recv_channel_ready_invalid_funding_outpoint_is_noop() { - let (mut executor, channel_id, _) = recv_channel_ready_executor(); + let channel_id = ChannelId::v1_from_funding_outpoint(OutPoint { + txid: "b86cbf63856be414fb9636160d729a29935fb998524410475878295272021727" + .parse() + .unwrap(), + vout: 0, + }); + + // The expected signature here was computed using LDK as the source of + // truth. + let fs_bytes = Message::FundingSigned(FundingSigned { + channel_id, + signature: "30440220096b230642dcafeb2271e80b66eb7e7d7b5750af43a253d103bda600d27c10d302206b535c31d1c2a810f87950d17ed368cca017cdce6a26b23608da2fb62e9d8015".parse().unwrap(), + }) + .encode(); + let cr_bytes = Message::ChannelReady(ChannelReady { + channel_id, + second_per_commitment_point: sample_pubkey(1), + tlvs: ChannelReadyTlvs::default(), + }) + .encode(); - // Corrupt the negotiated opener funding pubkey so the broadcast funding - // transaction's output no longer pays the negotiated 2-of-2 script, - // marking the funding outpoint invalid. + let mut executor = Executor::new( + MockConnection::new(), + MockBitcoinCli { + utxos: vec![sample_utxo()], + change_spk: sample_change_spk(), + ..Default::default() + }, + sample_context(), + ); + executor.conn.queue_recv(fs_bytes); + executor.conn.queue_recv(cr_bytes); executor .negotiations - .get_mut(&ChannelId::new([0xbb; 32])) - .unwrap() - .open_channel - .funding_pubkey = sample_pubkey(1); + .insert(ChannelId::new([0xbb; 32]), sample_funding_negotiation()); let mut instrs = send_funding_created_and_recv_funding_signed_instructions(); instrs.extend([ @@ -3890,6 +3905,11 @@ mod tests { }, ]); + // We will create a transaction with the wrong acceptor funding pubkey, + // so the broadcast funding transaction's output no longer pays the + // negotiated 2-of-2 script, making the funding outpoint invalid. + instrs[3].inputs = vec![0]; + // With invalid funding outpoint the target does not owe us a // `channel_ready`, so `RecvChannelReady` must be a no-op. executor diff --git a/smite/src/channel_tx/commitment.rs b/smite/src/channel_tx/commitment.rs index 0b451b60..6aa15fef 100644 --- a/smite/src/channel_tx/commitment.rs +++ b/smite/src/channel_tx/commitment.rs @@ -150,6 +150,12 @@ pub struct ChannelState { /// after the block height at which they receive `funding_created`, so they /// may never observe it and never send `channel_ready`. pub was_funding_mined_prematurely: bool, + /// Whether we have ever sent a signature the peer was required to reject, + /// such as a `funding_created`, `commitment_signed`, or HTLC signature etc. + /// it cannot verify. Set on the first occurrence and never cleared, since + /// BOLT 2 requires the peer to fail the channel or disconnect in response, + /// any subsequent positive response is therefore a violation. + pub sent_invalid_signature: bool, } impl Side { @@ -179,6 +185,7 @@ impl ChannelState { commitment: CommitmentState, is_funding_outpoint_valid: bool, was_funding_mined_prematurely: bool, + sent_invalid_signature: bool, ) -> Self { Self { config, @@ -188,6 +195,7 @@ impl ChannelState { acceptor_next_per_commitment_point: None, is_funding_outpoint_valid, was_funding_mined_prematurely, + sent_invalid_signature, } } diff --git a/smite/src/oracles.rs b/smite/src/oracles.rs index ee2d7042..937b2fc2 100644 --- a/smite/src/oracles.rs +++ b/smite/src/oracles.rs @@ -3,9 +3,11 @@ //! Oracles evaluate conditions beyond simple crashes. mod accept_channel; +mod funding_signed; use super::violation::Violation; pub use accept_channel::{AcceptChannelContext, AcceptChannelOracle}; +pub use funding_signed::{FundingSignedContext, FundingSignedOracle}; /// `Oracle` evaluates a condition against some context pub trait Oracle { diff --git a/smite/src/oracles/funding_signed.rs b/smite/src/oracles/funding_signed.rs new file mode 100644 index 00000000..583f5666 --- /dev/null +++ b/smite/src/oracles/funding_signed.rs @@ -0,0 +1,215 @@ +//! BOLT 2 `funding_signed` oracle, for the v1 outbound channel funding flow. + +use super::Oracle; +use crate::bolt::FundingSigned; +use crate::channel_tx::ChannelState; +use crate::violation::Violation; + +/// Context for `FundingSignedOracle` +pub struct FundingSignedContext<'a> { + /// The `funding_signed` received from the peer. + pub funding_signed: &'a FundingSigned, + /// The channel the `funding_signed` belongs to, identified by its + /// `channel_id`, or `None` if no matching `funding_created` was sent. + pub channel: Option<&'a ChannelState>, +} + +/// Checks whether the `funding_created` answered by a `funding_signed` satisfied +/// the BOLT 2 v1 channel establishment requirements for acceptance, and whether +/// the `funding_signed` itself satisfies them. +pub struct FundingSignedOracle; + +impl Oracle> for FundingSignedOracle { + fn evaluate(&self, context: &FundingSignedContext<'_>) -> Result<(), Violation> { + // Check that the `funding_signed` answers a known `funding_created`. + let Some(channel) = context.channel else { + return Err(Violation::InvalidFundingSigned( + context.funding_signed.channel_id, + "unknown channel_id: no funding_created was sent for this channel".to_string(), + )); + }; + + // Check whether we sent a valid signature during `funding_created`. + if channel.sent_invalid_signature { + return Err(Violation::InvalidFundingSigned( + context.funding_signed.channel_id, + "accepted invalid funding_created: signature is not valid".to_string(), + )); + } + + // Check that the `funding_signed` itself is valid. + // + // NOTE: There are two possible violations here: the target may have + // sent an invalid signature, or a valid signature may have been mapped + // to an existing channel_id due to the XOR relationship between + // channel_id and the funding outpoint. BOLT 2 does not specify how to + // handle the latter case, but LND, LDK, and Eclair all reject such + // channels. Since channel_id is reused across messages, accepting a + // collision could cause cross-channel state contamination and lead to + // more serious bugs. Such collisions are also extremely unlikely to + // occur naturally, so the target should reject them, and we catch them + // here as well. see: https://github.com/ElementsProject/lightning/issues/9274#issuecomment-5316110622 + if !channel.config.verify_counterparty_signature( + &channel.commitment, + &channel.holder, + &context.funding_signed.signature, + ) { + return Err(Violation::InvalidFundingSigned( + context.funding_signed.channel_id, + "invalid funding_signed: signature is not valid".to_string(), + )); + } + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::bolt::{COMPACT_SIGNATURE_SIZE, ChannelId}; + use crate::channel_tx::{ChannelConfig, ChannelPartyConfig, HolderIdentity, Side}; + use bitcoin::OutPoint; + use bitcoin::secp256k1::ecdsa::Signature; + use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; + + fn secret_key(seed: u8) -> SecretKey { + SecretKey::from_slice(&[seed; 32]).expect("valid secret key") + } + + fn pubkey(seed: u8) -> PublicKey { + PublicKey::from_secret_key(&Secp256k1::new(), &secret_key(seed)) + } + + /// Valid channel state for testing. + fn channel_state() -> ChannelState { + let pkey1 = pubkey(1); + let pkey2 = pubkey(2); + + let config = ChannelConfig { + funding_outpoint: OutPoint { + txid: "09b0549b35f14ee862f63bd75811c6c27963c4dea6766ec6836952ec78df1e7e" + .parse() + .expect("valid txid hex"), + vout: 0, + }, + funding_satoshis: 10_000_000, + channel_type: vec![0x10, 0x00], + opener: ChannelPartyConfig { + funding_pubkey: pkey1, + payment_basepoint: pkey1, + revocation_basepoint: pkey1, + delayed_payment_basepoint: pkey1, + dust_limit_satoshis: 546, + to_self_delay: 144, + }, + acceptor: ChannelPartyConfig { + funding_pubkey: pkey2, + payment_basepoint: pkey2, + revocation_basepoint: pkey2, + delayed_payment_basepoint: pkey2, + dust_limit_satoshis: 546, + to_self_delay: 144, + }, + minimum_depth: 8, + }; + let commitment = config + .new_initial_commitment(3_000_000_000, 15_000, pkey1, pkey2) + .expect("valid initial commitment"); + let holder = HolderIdentity { + side: Side::Opener, + funding_privkey: secret_key(1), + }; + + ChannelState::new(config, holder, commitment, true, false, false) + } + + /// Valid `funding_signed` message for testing. + fn funding_signed(channel: &ChannelState) -> FundingSigned { + let acceptor = HolderIdentity { + side: Side::Acceptor, + funding_privkey: secret_key(2), + }; + let outpoint = OutPoint { + txid: "09b0549b35f14ee862f63bd75811c6c27963c4dea6766ec6836952ec78df1e7e" + .parse() + .expect("valid txid"), + vout: 0, + }; + FundingSigned { + channel_id: ChannelId::v1_from_funding_outpoint(outpoint), + signature: channel + .config + .sign_counterparty_commitment(&channel.commitment, &acceptor), + } + } + + #[track_caller] + fn assert_pass(funding_signed: &FundingSigned, channel: Option<&ChannelState>) { + if let Err(err) = FundingSignedOracle.evaluate(&FundingSignedContext { + funding_signed, + channel, + }) { + panic!("expected pass, got: {err}"); + } + } + + #[track_caller] + fn assert_fail(funding_signed: &FundingSigned, channel: Option<&ChannelState>, expected: &str) { + match FundingSignedOracle.evaluate(&FundingSignedContext { + funding_signed, + channel, + }) { + Err(Violation::InvalidFundingSigned(chan_id, reason)) => { + assert_eq!(funding_signed.channel_id, chan_id); + assert!( + reason.contains(expected), + "unexpected failure reason: {reason}" + ); + } + _ => panic!("expected failure: {expected}"), + } + } + + #[test] + fn conforming_funding_signed_passes() { + let channel = channel_state(); + + assert_pass(&funding_signed(&channel), Some(&channel)); + } + + #[test] + fn funding_signed_for_unknown_channel_id() { + assert_fail( + &funding_signed(&channel_state()), + None, + "unknown channel_id: no funding_created was sent for this channel", + ); + } + + #[test] + fn funding_created_with_invalid_signature() { + let mut channel = channel_state(); + channel.sent_invalid_signature = true; + + assert_fail( + &funding_signed(&channel), + Some(&channel), + "accepted invalid funding_created: signature is not valid", + ); + } + + #[test] + fn funding_signed_with_invalid_signature() { + let channel = channel_state(); + let mut fs = funding_signed(&channel); + fs.signature = Signature::from_compact(&[0u8; COMPACT_SIGNATURE_SIZE]) + .expect("zero bytes parse as a signature"); + + assert_fail( + &fs, + Some(&channel), + "invalid funding_signed: signature is not valid", + ); + } +} diff --git a/smite/src/violation.rs b/smite/src/violation.rs index 83547ee2..26cc6b89 100644 --- a/smite/src/violation.rs +++ b/smite/src/violation.rs @@ -36,13 +36,17 @@ pub enum Violation { #[error("invalid accept_channel for temporary_channel_id {0}: {1}")] InvalidAcceptChannel(ChannelId, String), - /// The target sent a `funding_signed` or `channel_ready` for a `channel_id` - /// we never opened, i.e. one for which no state was ever established. + /// The target's `funding_signed` broke a BOLT 2 requirement, as judged by + /// [`crate::oracles::FundingSignedOracle`]. The reason names the breached + /// requirement, one of: + /// - it names a `channel_id` we sent no `funding_created` for, + /// - it answers a `funding_created` with an invalid signature, or + /// - its signature is not valid for the holder's commitment transaction. + #[error("invalid funding_signed for channel_id {0}: {1}")] + InvalidFundingSigned(ChannelId, String), + + /// The target sent a `channel_ready` for a `channel_id` we never opened, + /// i.e. one for which no state was ever established. #[error("unknown channel: no tracked state for channel_id {0}")] UnknownChannel(ChannelId), - - /// The target's `funding_signed` signature failed to verify against the - /// holder's initial commitment transaction. - #[error("invalid counterparty signature for channel_id {0}")] - InvalidCounterpartySignature(ChannelId), }