From 142b0d624e8ef71aea4aeb1d3591c6a5b59a771d Mon Sep 17 00:00:00 2001 From: Devrandom Date: Wed, 13 Jan 2021 17:36:07 -0800 Subject: [PATCH] Let some tests disable revocation policy check When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic. --- fuzz/src/chanmon_consistency.rs | 3 +- lightning/src/ln/functional_test_utils.rs | 1 - lightning/src/ln/functional_tests.rs | 27 ++++++++++---- lightning/src/util/enforcing_trait_impls.rs | 41 ++++++++++++++------- lightning/src/util/test_utils.rs | 7 +++- 5 files changed, 54 insertions(+), 25 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index cfb8f41b5..8e79ac5f9 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -182,7 +182,7 @@ impl KeysInterface for KeyProvider { (0, 0), ); let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment, false) } fn get_secure_random_bytes(&self) -> [u8; 32] { @@ -202,6 +202,7 @@ impl KeysInterface for KeyProvider { inner, last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), revoked_commitment, + disable_revocation_policy_check: false, }) } } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index e487b1528..a7394a84b 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -97,7 +97,6 @@ pub struct TestChanMonCfg { pub persister: test_utils::TestPersister, pub logger: test_utils::TestLogger, pub keys_manager: test_utils::TestKeysInterface, - } pub struct NodeCfg<'a> { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 6bac24911..e4d7a33c8 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2470,7 +2470,9 @@ fn test_justice_tx() { bob_config.peer_channel_config_limits.force_announced_channel_preference = false; bob_config.own_channel_config.our_to_self_delay = 6 * 24 * 3; let user_cfgs = [Some(alice_config), Some(bob_config)]; - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; + chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &user_cfgs); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -2600,7 +2602,8 @@ fn revoked_output_claim() { #[test] fn claim_htlc_outputs_shared_tx() { // Node revoked old state, htlcs haven't time out yet, claim them in shared justice tx - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -2670,7 +2673,8 @@ fn claim_htlc_outputs_shared_tx() { #[test] fn claim_htlc_outputs_single_tx() { // Node revoked old state, htlcs have timed out, claim each of them in separated justice tx - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -4993,7 +4997,8 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() { #[test] fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -5059,7 +5064,8 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { #[test] fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() { - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -7040,7 +7046,8 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { // We can have at most two valid local commitment tx, so both cases must be covered, and both txs must be checked to get them all as // HTLC could have been removed from lastest local commitment tx but still valid until we get remote RAA - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -7379,7 +7386,10 @@ fn test_data_loss_protect() { let fee_estimator; let tx_broadcaster; let chain_source; - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + // We broadcast during Drop because chanmon is out of sync with chanmgr, which would cause a panic + // during signing due to revoked tx + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let keys_manager = &chanmon_cfgs[0].keys_manager; let monitor; let node_state_0; @@ -7699,7 +7709,8 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { // In case of penalty txn with too low feerates for getting into mempools, RBF-bump them to sure // we're able to claim outputs on revoked HTLC transactions before timelocks expiration - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index af497cbbe..88c1754f1 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -27,31 +27,49 @@ use ln::msgs::DecodeError; /// Initial value for revoked commitment downward counter pub const INITIAL_REVOKED_COMMITMENT_NUMBER: u64 = 1 << 48; -/// An implementation of ChannelKeys that enforces some policy checks. +/// An implementation of ChannelKeys that enforces some policy checks. The current checks +/// are an incomplete set. They include: +/// +/// - When signing, the holder transaction has not been revoked +/// - When revoking, the holder transaction has not been signed +/// - The holder commitment number is monotonic and without gaps +/// - The counterparty commitment number is monotonic and without gaps +/// - The pre-derived keys and pre-built transaction in CommitmentTransaction were correctly built /// /// Eventually we will probably want to expose a variant of this which would essentially /// be what you'd want to run on a hardware wallet. #[derive(Clone)] pub struct EnforcingChannelKeys { pub inner: InMemoryChannelKeys, + /// The last counterparty commitment number we signed, backwards counting pub last_commitment_number: Arc>>, + /// The last holder commitment number we revoked, backwards counting pub revoked_commitment: Arc>, + pub disable_revocation_policy_check: bool, } impl EnforcingChannelKeys { + /// Construct an EnforcingChannelKeys pub fn new(inner: InMemoryChannelKeys) -> Self { Self { inner, last_commitment_number: Arc::new(Mutex::new(None)), - revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)) + revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)), + disable_revocation_policy_check: false } } - pub fn new_with_revoked(inner: InMemoryChannelKeys, revoked_commitment: Arc>) -> Self { + /// Construct an EnforcingChannelKeys with externally managed storage + /// + /// Since there are multiple copies of this struct for each channel, some coordination is needed + /// so that all copies are aware of revocations. A pointer to this state is provided here, usually + /// by an implementation of KeysInterface. + pub fn new_with_revoked(inner: InMemoryChannelKeys, revoked_commitment: Arc>, disable_revocation_policy_check: bool) -> Self { Self { inner, last_commitment_number: Arc::new(Mutex::new(None)), - revoked_commitment + revoked_commitment, + disable_revocation_policy_check } } } @@ -62,8 +80,6 @@ impl ChannelKeys for EnforcingChannelKeys { } fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { - println!("XXX revoke {} for {}", idx, self.inner.commitment_seed[0]); - { let mut revoked = self.revoked_commitment.lock().unwrap(); assert!(idx == *revoked || idx == *revoked - 1, "can only revoke the current or next unrevoked commitment - trying {}, revoked {}", idx, *revoked); @@ -98,11 +114,11 @@ impl ChannelKeys for EnforcingChannelKeys { let revoked = self.revoked_commitment.lock().unwrap(); let commitment_number = trusted_tx.commitment_number(); - println!("XXX sign {} for {}", commitment_number, self.inner.commitment_seed[0]); if *revoked - 1 != commitment_number && *revoked - 2 != commitment_number { - println!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", - *revoked, commitment_number, self.inner.commitment_seed[0]); - return Err(()); + if !self.disable_revocation_policy_check { + panic!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", + *revoked, commitment_number, self.inner.commitment_seed[0]) + } } for (this_htlc, sig) in trusted_tx.htlcs().iter().zip(&commitment_tx.counterparty_htlc_sigs) { @@ -116,8 +132,6 @@ impl ChannelKeys for EnforcingChannelKeys { secp_ctx.verify(&sighash, sig, &keys.countersignatory_htlc_key).unwrap(); } - // TODO: enforce the ChannelKeys contract - error if this commitment was already revoked - // TODO: need the commitment number Ok(self.inner.sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap()) } @@ -159,12 +173,13 @@ impl Writeable for EnforcingChannelKeys { impl Readable for EnforcingChannelKeys { fn read(reader: &mut R) -> Result { - let inner: InMemoryChannelKeys = Readable::read(reader)?; + let inner = Readable::read(reader)?; let last_commitment_number = Readable::read(reader)?; Ok(EnforcingChannelKeys { inner, last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)), + disable_revocation_policy_check: false, }) } } diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index a93ce3cfb..4bf8aa39d 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -422,6 +422,7 @@ pub struct TestKeysInterface { backing: keysinterface::KeysManager, pub override_session_priv: Mutex>, pub override_channel_id_priv: Mutex>, + pub disable_revocation_policy_check: bool, revoked_commitments: Mutex>>>, } @@ -434,7 +435,7 @@ impl keysinterface::KeysInterface for TestKeysInterface { fn get_channel_keys(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingChannelKeys { let keys = self.backing.get_channel_keys(inbound, channel_value_satoshis); let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment, self.disable_revocation_policy_check) } fn get_secure_random_bytes(&self) -> [u8; 32] { @@ -464,6 +465,7 @@ impl keysinterface::KeysInterface for TestKeysInterface { inner, last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), revoked_commitment, + disable_revocation_policy_check: self.disable_revocation_policy_check, }) } } @@ -476,13 +478,14 @@ impl TestKeysInterface { backing: keysinterface::KeysManager::new(seed, network, now.as_secs(), now.subsec_nanos()), override_session_priv: Mutex::new(None), override_channel_id_priv: Mutex::new(None), + disable_revocation_policy_check: false, revoked_commitments: Mutex::new(HashMap::new()), } } pub fn derive_channel_keys(&self, channel_value_satoshis: u64, user_id_1: u64, user_id_2: u64) -> EnforcingChannelKeys { let keys = self.backing.derive_channel_keys(channel_value_satoshis, user_id_1, user_id_2); let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment, self.disable_revocation_policy_check) } fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc> { -- 2.39.5