]> git.bitcoin.ninja Git - rust-lightning/commitdiff
Merge pull request #115 from TheBlueMatt/2018-08-channel-fuzz-fixes
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Fri, 17 Aug 2018 03:04:12 +0000 (23:04 -0400)
committerGitHub <noreply@github.com>
Fri, 17 Aug 2018 03:04:12 +0000 (23:04 -0400)
Fixes from channelmanager fuzzing work

fuzz/fuzz_targets/full_stack_target.rs
src/ln/channel.rs
src/ln/channelmanager.rs
src/util/mod.rs

index 8f7ec2df0a76013918e2ec7b154deceed7057cab..40336124a5575bb9ce56744ebb50fdbcd33521d9 100644 (file)
@@ -10,7 +10,6 @@ use bitcoin::network::constants::Network;
 use bitcoin::network::serialize::{serialize, BitcoinHash};
 use bitcoin::util::hash::Sha256dHash;
 
-use crypto::sha2::Sha256;
 use crypto::digest::Digest;
 
 use lightning::chain::chaininterface::{BroadcasterInterface,ConfirmationTarget,ChainListener,FeeEstimator,ChainWatchInterfaceUtil};
@@ -22,6 +21,7 @@ use lightning::ln::router::Router;
 use lightning::util::events::{EventsProvider,Event};
 use lightning::util::reset_rng_state;
 use lightning::util::logger::Logger;
+use lightning::util::sha2::Sha256;
 
 mod utils;
 
@@ -189,7 +189,7 @@ pub fn do_test(data: &[u8]) {
        }, our_network_key, Arc::clone(&logger));
 
        let mut should_forward = false;
-       let mut payments_received = Vec::new();
+       let mut payments_received: Vec<[u8; 32]> = Vec::new();
        let mut payments_sent = 0;
        let mut pending_funding_generation: Vec<([u8; 32], u64, Script)> = Vec::new();
        let mut pending_funding_signatures = HashMap::new();
@@ -246,7 +246,6 @@ pub fn do_test(data: &[u8]) {
                                let mut sha = Sha256::new();
                                sha.input(&payment_hash);
                                sha.result(&mut payment_hash);
-                               for i in 1..32 { payment_hash[i] = 0; }
                                payments_sent += 1;
                                match channelmanager.send_payment(route, payment_hash) {
                                        Ok(_) => {},
@@ -276,22 +275,14 @@ pub fn do_test(data: &[u8]) {
                        },
                        8 => {
                                for payment in payments_received.drain(..) {
-                                       let mut payment_preimage = None;
-                                       for i in 0..payments_sent {
-                                               let mut payment_hash = [0; 32];
-                                               payment_hash[0..8].copy_from_slice(&be64_to_array(i));
-                                               let mut sha = Sha256::new();
-                                               sha.input(&payment_hash);
-                                               sha.result(&mut payment_hash);
-                                               for i in 1..32 { payment_hash[i] = 0; }
-                                               if payment_hash == payment {
-                                                       payment_hash = [0; 32];
-                                                       payment_hash[0..8].copy_from_slice(&be64_to_array(i));
-                                                       payment_preimage = Some(payment_hash);
-                                                       break;
-                                               }
-                                       }
-                                       channelmanager.claim_funds(payment_preimage.unwrap());
+                                       let mut payment_preimage = [0; 32];
+                                       payment_preimage[0] = payment[0];
+                                       let mut sha = Sha256::new();
+                                       sha.input(&payment_preimage);
+                                       let mut payment_hash_check = [0; 32];
+                                       sha.result(&mut payment_hash_check);
+                                       assert!(payment_hash_check == payment);
+                                       channelmanager.claim_funds(payment_preimage);
                                }
                        },
                        9 => {
@@ -300,13 +291,21 @@ pub fn do_test(data: &[u8]) {
                                }
                        },
                        10 => {
-                               for funding_generation in  pending_funding_generation.drain(..) {
+                               for funding_generation in pending_funding_generation.drain(..) {
                                        let mut tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: vec![TxOut {
                                                        value: funding_generation.1, script_pubkey: funding_generation.2,
                                                }] };
                                        let funding_output = OutPoint::new(Sha256dHash::from_data(&serialize(&tx).unwrap()[..]), 0);
-                                       channelmanager.funding_transaction_generated(&funding_generation.0, funding_output.clone());
-                                       pending_funding_signatures.insert(funding_output, tx);
+                                       let mut found_duplicate_txo = false;
+                                       for chan in channelmanager.list_channels() {
+                                               if chan.channel_id == funding_output.to_channel_id() {
+                                                       found_duplicate_txo = true;
+                                               }
+                                       }
+                                       if !found_duplicate_txo {
+                                               channelmanager.funding_transaction_generated(&funding_generation.0, funding_output.clone());
+                                               pending_funding_signatures.insert(funding_output, tx);
+                                       }
                                }
                        },
                        11 => {
index a1ca7e25e65a5ea8cff9f5d0a27710751ea5ae94..1a195d504a117ea6590c4605974568443e8f3275 100644 (file)
@@ -88,18 +88,21 @@ impl ChannelKeys {
 #[derive(PartialEq)]
 enum HTLCState {
        /// Added by remote, to be included in next local commitment tx.
+       /// Implies HTLCOutput::outbound: false
        RemoteAnnounced,
        /// Included in a received commitment_signed message (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// accept this HTLC. Implies AwaitingRemoteRevoke.
        /// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
        /// a remote revoke_and_ack on a previous state before we can do so.
+       /// Implies HTLCOutput::outbound: false
        AwaitingRemoteRevokeToAnnounce,
        /// Included in a received commitment_signed message (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// accept this HTLC. Implies AwaitingRemoteRevoke.
        /// We have included this HTLC in our latest commitment_signed and are now just waiting on a
        /// revoke_and_ack.
+       /// Implies HTLCOutput::outbound: true
        AwaitingAnnouncedRemoteRevoke,
        /// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we
        /// created it we would have put it in the holding cell instead). When they next revoke_and_ack
@@ -111,22 +114,26 @@ enum HTLCState {
        ///    allowed to remove it, the "can only be removed once committed on both sides" requirement
        ///    doesn't matter to us and its up to them to enforce it, worst-case they jump ahead but
        ///    we'll never get out of sync).
+       /// Implies HTLCOutput::outbound: true
        LocalAnnounced,
        Committed,
        /// Remote removed this (outbound) HTLC. We're waiting on their commitment_signed to finalize
        /// the change (though they'll need to revoke before we fail the payment).
+       /// Implies HTLCOutput::outbound: true
        RemoteRemoved,
        /// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// can do any backwards failing. Implies AwaitingRemoteRevoke.
        /// We also have not yet removed this HTLC in a commitment_signed message, and are waiting on a
        /// remote revoke_and_ack on a previous state before we can do so.
+       /// Implies HTLCOutput::outbound: true
        AwaitingRemoteRevokeToRemove,
        /// Remote removed this and sent a commitment_signed (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// can do any backwards failing. Implies AwaitingRemoteRevoke.
        /// We have removed this HTLC in our latest commitment_signed and are now just waiting on a
        /// revoke_and_ack to drop completely.
+       /// Implies HTLCOutput::outbound: true
        AwaitingRemovedRemoteRevoke,
        /// Removed by us and a new commitment_signed was sent (if we were AwaitingRemoteRevoke when we
        /// created it we would have put it in the holding cell instead). When they next revoke_and_ack
@@ -136,9 +143,11 @@ enum HTLCState {
        /// commitment transaction without it as otherwise we'll have to force-close the channel to
        /// claim it before the timeout (obviously doesn't apply to revoked HTLCs that we can't claim
        /// anyway).
+       /// Implies HTLCOutput::outbound: false
        LocalRemoved,
        /// Removed by us, sent a new commitment_signed and got a revoke_and_ack. Just waiting on an
        /// updated local commitment transaction. Implies local_removed_fulfilled.
+       /// Implies HTLCOutput::outbound: false
        LocalRemovedAwaitingCommitment,
 }
 
@@ -956,7 +965,7 @@ impl Channel {
                for (idx, htlc) in self.pending_htlcs.iter().enumerate() {
                        if !htlc.outbound && htlc.payment_hash == payment_hash_calc {
                                if pending_idx != std::usize::MAX {
-                                       panic!("Duplicate HTLC payment_hash, you probably re-used payment preimages, NEVER DO THIS!");
+                                       panic!("Duplicate HTLC payment_hash, ChannelManager should have prevented this!");
                                }
                                pending_idx = idx;
                        }
@@ -998,8 +1007,14 @@ impl Channel {
                        if htlc.state == HTLCState::Committed {
                                htlc.state = HTLCState::LocalRemoved;
                                htlc.local_removed_fulfilled = true;
-                       } else if htlc.state == HTLCState::RemoteAnnounced {
-                               panic!("Somehow forwarded HTLC prior to remote revocation!");
+                       } else if htlc.state == HTLCState::RemoteAnnounced || htlc.state == HTLCState::AwaitingRemoteRevokeToAnnounce || htlc.state == HTLCState::AwaitingAnnouncedRemoteRevoke {
+                               // Theoretically we can hit this if we get the preimage on an HTLC prior to us
+                               // having forwarded it to anyone. This implies that the sender is busted as someone
+                               // else knows the preimage, but handling this case and implementing the logic to
+                               // take their money would be a lot of (never-tested) code to handle a case that
+                               // hopefully never happens. Instead, we make sure we get the preimage into the
+                               // channel_monitor and pretend we didn't just see the preimage.
+                               return Ok((None, Some(self.channel_monitor.clone())));
                        } else if htlc.state == HTLCState::LocalRemoved || htlc.state == HTLCState::LocalRemovedAwaitingCommitment {
                                return Err(HandleError{err: "Unable to find a pending HTLC which matched the given payment preimage", action: None});
                        } else {
index 0b6b1663ab47b508e527bd718c3b35d6b88252a1..e142e8ad5091bb52a9290e1694fbf0e6df6f000f 100644 (file)
@@ -770,6 +770,8 @@ impl ChannelManager {
 
        /// Call this upon creation of a funding transaction for the given channel.
        /// Panics if a funding transaction has already been provided for this channel.
+       /// May panic if the funding_txo is duplicative with some other channel (note that this should
+       /// be trivially prevented by using unique funding transaction keys per-channel).
        pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_txo: OutPoint) {
 
                macro_rules! add_pending_event {
@@ -812,7 +814,14 @@ impl ChannelManager {
                });
 
                let mut channel_state = self.channel_state.lock().unwrap();
-               channel_state.by_id.insert(chan.channel_id(), chan);
+               match channel_state.by_id.entry(chan.channel_id()) {
+                       hash_map::Entry::Occupied(_) => {
+                               panic!("Generated duplicate funding txid?");
+                       },
+                       hash_map::Entry::Vacant(e) => {
+                               e.insert(chan);
+                       }
+               }
        }
 
        fn get_announcement_sigs(&self, chan: &Channel) -> Result<Option<msgs::AnnouncementSignatures>, HandleError> {
@@ -1323,26 +1332,24 @@ impl ChannelMessageHandler for ChannelManager {
        }
 
        fn handle_funding_created(&self, their_node_id: &PublicKey, msg: &msgs::FundingCreated) -> Result<msgs::FundingSigned, HandleError> {
-               //TODO: broke this - a node shouldn't be able to get their channel removed by sending a
-               //funding_created a second time, or long after the first, or whatever (note this also
-               //leaves the short_to_id map in a busted state.
                let (chan, funding_msg, monitor_update) = {
                        let mut channel_state = self.channel_state.lock().unwrap();
-                       match channel_state.by_id.remove(&msg.temporary_channel_id) {
-                               Some(mut chan) => {
-                                       if chan.get_their_node_id() != *their_node_id {
+                       match channel_state.by_id.entry(msg.temporary_channel_id.clone()) {
+                               hash_map::Entry::Occupied(mut chan) => {
+                                       if chan.get().get_their_node_id() != *their_node_id {
                                                return Err(HandleError{err: "Got a message for a channel from the wrong node!", action: None})
                                        }
-                                       match chan.funding_created(msg) {
+                                       match chan.get_mut().funding_created(msg) {
                                                Ok((funding_msg, monitor_update)) => {
-                                                       (chan, funding_msg, monitor_update)
+                                                       (chan.remove(), funding_msg, monitor_update)
                                                },
                                                Err(e) => {
+                                                       //TODO: Possibly remove the channel depending on e.action
                                                        return Err(e);
                                                }
                                        }
                                },
-                               None => return Err(HandleError{err: "Failed to find corresponding channel", action: None})
+                               hash_map::Entry::Vacant(_) => return Err(HandleError{err: "Failed to find corresponding channel", action: None})
                        }
                }; // Release channel lock for install_watch_outpoint call,
                   // note that this means if the remote end is misbehaving and sends a message for the same
@@ -1352,7 +1359,17 @@ impl ChannelMessageHandler for ChannelManager {
                        unimplemented!();
                }
                let mut channel_state = self.channel_state.lock().unwrap();
-               channel_state.by_id.insert(funding_msg.channel_id, chan);
+               match channel_state.by_id.entry(funding_msg.channel_id) {
+                       hash_map::Entry::Occupied(_) => {
+                               return Err(HandleError {
+                                       err: "Duplicate channel_id!",
+                                       action: Some(msgs::ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id: funding_msg.channel_id, data: "Already had channel with the new channel_id".to_owned() } })
+                               });
+                       },
+                       hash_map::Entry::Vacant(e) => {
+                               e.insert(chan);
+                       }
+               }
                Ok(funding_msg)
        }
 
index 766557e3ca52805379e4b5618243ac4f06ed2a76..8edd5c7709ccd554cad50da3822209cf053a8475 100644 (file)
@@ -4,9 +4,13 @@ pub(crate) mod byte_utils;
 pub(crate) mod chacha20poly1305rfc;
 pub(crate) mod internal_traits;
 pub(crate) mod rng;
-pub(crate) mod sha2;
 pub(crate) mod transaction_utils;
 
+#[cfg(feature = "fuzztarget")]
+pub mod sha2;
+#[cfg(not(feature = "fuzztarget"))]
+pub(crate) mod sha2;
+
 #[cfg(feature = "fuzztarget")]
 pub use self::rng::reset_rng_state;