Use KeysInterface::read_chan_signer for all channel keys deser

This drops any direct calls to a generic `ChannelKeys::read()` and
replaces it with the new `KeysInterface::read_chan_signer()`. Still,
under the hood all of our own `KeysInterface::read_chan_signer()`
implementations simply call out to a `Readable::read()` implemention.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -45,6 +45,7 @@ use lightning::util::logger::Logger;
 use lightning::util::config::UserConfig;
 use lightning::util::events::{EventsProvider, MessageSendEventsProvider};
 use lightning::util::ser::{Readable, ReadableArgs, Writeable, Writer};
+use lightning::util::test_utils::OnlyReadsKeysInterface;
 use lightning::routing::router::{Route, RouteHop};
 
 
@@ -128,7 +129,7 @@ impl chain::Watch for TestChainMonitor {
 			hash_map::Entry::Vacant(_) => panic!("Didn't have monitor on update call"),
 		};
 		let mut deserialized_monitor = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::
-			read(&mut Cursor::new(&map_entry.get().1)).unwrap().1;
+			read(&mut Cursor::new(&map_entry.get().1), &OnlyReadsKeysInterface {}).unwrap().1;
 		deserialized_monitor.update_monitor(&update, &&TestBroadcaster{}, &&FuzzEstimator{}, &self.logger).unwrap();
 		let mut ser = VecWriter(Vec::new());
 		deserialized_monitor.write(&mut ser).unwrap();
@@ -311,7 +312,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 			let mut monitors = HashMap::new();
 			let mut old_monitors = $old_monitors.latest_monitors.lock().unwrap();
 			for (outpoint, (update_id, monitor_ser)) in old_monitors.drain() {
-				monitors.insert(outpoint, <(BlockHash, ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&monitor_ser)).expect("Failed to read monitor").1);
+				monitors.insert(outpoint, <(BlockHash, ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&monitor_ser), &OnlyReadsKeysInterface {}).expect("Failed to read monitor").1);
 				chain_monitor.latest_monitors.lock().unwrap().insert(outpoint, (update_id, monitor_ser));
 			}
 			let mut monitor_refs = HashMap::new();

fuzz/src/chanmon_deser.rs

@@ -5,7 +5,8 @@ use bitcoin::hash_types::BlockHash;
 
 use lightning::chain::channelmonitor;
 use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
-use lightning::util::ser::{Readable, Writer, Writeable};
+use lightning::util::ser::{ReadableArgs, Writer, Writeable};
+use lightning::util::test_utils::OnlyReadsKeysInterface;
 
 use utils::test_logger;
 
@@ -24,10 +25,10 @@ impl Writer for VecWriter {
 
 #[inline]
 pub fn do_test<Out: test_logger::Output>(data: &[u8], _out: Out) {
-	if let Ok((latest_block_hash, monitor)) = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(data)) {
+	if let Ok((latest_block_hash, monitor)) = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(data), &OnlyReadsKeysInterface {}) {
 		let mut w = VecWriter(Vec::new());
 		monitor.write(&mut w).unwrap();
-		let deserialized_copy = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0)).unwrap();
+		let deserialized_copy = <(BlockHash, channelmonitor::ChannelMonitor<EnforcingChannelKeys>)>::read(&mut Cursor::new(&w.0), &OnlyReadsKeysInterface {}).unwrap();
 		assert!(latest_block_hash == deserialized_copy.0);
 		assert!(monitor == deserialized_copy.1);
 	}

lightning-persister/src/lib.rs

@@ -7,13 +7,15 @@ use lightning::chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, Cha
 use lightning::chain::channelmonitor;
 use lightning::chain::keysinterface::ChannelKeys;
 use lightning::chain::transaction::OutPoint;
-use lightning::util::ser::{Writeable, Readable};
+use lightning::util::ser::Writeable;
 use std::fs;
 use std::io::Error;
 use std::path::{Path, PathBuf};
 
 #[cfg(test)]
 use {
+	lightning::chain::keysinterface::KeysInterface,
+	lightning::util::ser::ReadableArgs,
 	bitcoin::{BlockHash, Txid},
 	bitcoin::hashes::hex::FromHex,
 	std::collections::HashMap,
@@ -94,8 +96,8 @@ impl FilesystemPersister {
 	}
 
 	#[cfg(test)]
-	fn load_channel_data<ChanSigner: ChannelKeys + Readable>(&self) ->
-		Result<HashMap<OutPoint, ChannelMonitor<ChanSigner>>, ChannelMonitorUpdateErr> {
+	fn load_channel_data<Keys: KeysInterface>(&self, keys: &Keys) ->
+		Result<HashMap<OutPoint, ChannelMonitor<Keys::ChanKeySigner>>, ChannelMonitorUpdateErr> {
 		if let Err(_) = fs::create_dir_all(&self.path_to_channel_data) {
 			return Err(ChannelMonitorUpdateErr::PermanentFailure);
 		}
@@ -118,7 +120,7 @@ impl FilesystemPersister {
 			if contents.is_err() { return Err(ChannelMonitorUpdateErr::PermanentFailure); }
 
 			if let Ok((_, loaded_monitor)) =
-				<(BlockHash, ChannelMonitor<ChanSigner>)>::read(&mut Cursor::new(&contents.unwrap())) {
+				<(BlockHash, ChannelMonitor<Keys::ChanKeySigner>)>::read(&mut Cursor::new(&contents.unwrap()), keys) {
 				res.insert(OutPoint { txid: txid.unwrap(), index: index.unwrap() }, loaded_monitor);
 			} else {
 				return Err(ChannelMonitorUpdateErr::PermanentFailure);
@@ -128,7 +130,7 @@ impl FilesystemPersister {
 	}
 }
 
-impl<ChanSigner: ChannelKeys + Readable + Send + Sync> channelmonitor::Persist<ChanSigner> for FilesystemPersister {
+impl<ChanSigner: ChannelKeys + Send + Sync> channelmonitor::Persist<ChanSigner> for FilesystemPersister {
 	fn persist_new_channel(&self, funding_txo: OutPoint, monitor: &ChannelMonitor<ChanSigner>) -> Result<(), ChannelMonitorUpdateErr> {
 		self.write_channel_data(funding_txo, monitor)
 		  .map_err(|_| ChannelMonitorUpdateErr::PermanentFailure)
@@ -168,7 +170,6 @@ mod tests {
 	use lightning::ln::features::InitFeatures;
 	use lightning::ln::functional_test_utils::*;
 	use lightning::ln::msgs::ErrorAction;
-	use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
 	use lightning::util::events::{MessageSendEventsProvider, MessageSendEvent};
 	use lightning::util::ser::Writer;
 	use lightning::util::test_utils;
@@ -206,20 +207,20 @@ mod tests {
 
 		// Check that the persisted channel data is empty before any channels are
 		// open.
-		let mut persisted_chan_data_0 = persister_0.load_channel_data::<EnforcingChannelKeys>().unwrap();
+		let mut persisted_chan_data_0 = persister_0.load_channel_data(nodes[0].keys_manager).unwrap();
 		assert_eq!(persisted_chan_data_0.keys().len(), 0);
-		let mut persisted_chan_data_1 = persister_1.load_channel_data::<EnforcingChannelKeys>().unwrap();
+		let mut persisted_chan_data_1 = persister_1.load_channel_data(nodes[1].keys_manager).unwrap();
 		assert_eq!(persisted_chan_data_1.keys().len(), 0);
 
 		// Helper to make sure the channel is on the expected update ID.
 		macro_rules! check_persisted_data {
 			($expected_update_id: expr) => {
-				persisted_chan_data_0 = persister_0.load_channel_data::<EnforcingChannelKeys>().unwrap();
+				persisted_chan_data_0 = persister_0.load_channel_data(nodes[0].keys_manager).unwrap();
 				assert_eq!(persisted_chan_data_0.keys().len(), 1);
 				for mon in persisted_chan_data_0.values() {
 					assert_eq!(mon.get_latest_update_id(), $expected_update_id);
 				}
-				persisted_chan_data_1 = persister_1.load_channel_data::<EnforcingChannelKeys>().unwrap();
+				persisted_chan_data_1 = persister_1.load_channel_data(nodes[1].keys_manager).unwrap();
 				assert_eq!(persisted_chan_data_1.keys().len(), 1);
 				for mon in persisted_chan_data_1.values() {
 					assert_eq!(mon.get_latest_update_id(), $expected_update_id);

lightning/src/chain/channelmonitor.rs

@@ -44,9 +44,9 @@ use ln::channelmanager::{HTLCSource, PaymentPreimage, PaymentHash};
 use ln::onchaintx::{OnchainTxHandler, InputDescriptors};
 use chain::chaininterface::{BroadcasterInterface, FeeEstimator};
 use chain::transaction::{OutPoint, TransactionData};
-use chain::keysinterface::{SpendableOutputDescriptor, ChannelKeys};
+use chain::keysinterface::{SpendableOutputDescriptor, ChannelKeys, KeysInterface};
 use util::logger::Logger;
-use util::ser::{Readable, MaybeReadable, Writer, Writeable, U48};
+use util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, U48};
 use util::byte_utils;
 use util::events::Event;
 
@@ -2289,8 +2289,9 @@ pub trait Persist<Keys: ChannelKeys>: Send + Sync {
 
 const MAX_ALLOC_SIZE: usize = 64*1024;
 
-impl<ChanSigner: ChannelKeys + Readable> Readable for (BlockHash, ChannelMonitor<ChanSigner>) {
-	fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+impl<'a, ChanSigner: ChannelKeys, K: KeysInterface<ChanKeySigner = ChanSigner>> ReadableArgs<&'a K>
+		for (BlockHash, ChannelMonitor<ChanSigner>) {
+	fn read<R: ::std::io::Read>(reader: &mut R, keys_manager: &'a K) -> Result<Self, DecodeError> {
 		macro_rules! unwrap_obj {
 			($key: expr) => {
 				match $key {
@@ -2524,7 +2525,7 @@ impl<ChanSigner: ChannelKeys + Readable> Readable for (BlockHash, ChannelMonitor
 				return Err(DecodeError::InvalidValue);
 			}
 		}
-		let onchain_tx_handler = Readable::read(reader)?;
+		let onchain_tx_handler = ReadableArgs::read(reader, keys_manager)?;
 
 		let lockdown_from_offchain = Readable::read(reader)?;
 		let holder_tx_signed = Readable::read(reader)?;

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -26,7 +26,7 @@ use routing::router::get_route;
 use util::enforcing_trait_impls::EnforcingChannelKeys;
 use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
 use util::errors::APIError;
-use util::ser::{Readable, Writeable};
+use util::ser::{ReadableArgs, Writeable};
 
 use bitcoin::hashes::sha256::Hash as Sha256;
 use bitcoin::hashes::Hash;
@@ -107,7 +107,7 @@ fn test_monitor_and_persister_update_fail() {
 		let mut w = test_utils::TestVecWriter(Vec::new());
 		monitor.write(&mut w).unwrap();
 		let new_monitor = <(BlockHash, ChannelMonitor<EnforcingChannelKeys>)>::read(
-			&mut ::std::io::Cursor::new(&w.0)).unwrap().1;
+			&mut ::std::io::Cursor::new(&w.0), &test_utils::OnlyReadsKeysInterface {}).unwrap().1;
 		assert!(new_monitor == *monitor);
 		let chain_mon = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister);
 		assert!(chain_mon.watch_channel(outpoint, new_monitor).is_ok());

lightning/src/ln/channel.rs

@@ -33,7 +33,7 @@ use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitor
 use chain::transaction::{OutPoint, TransactionData};
 use chain::keysinterface::{ChannelKeys, KeysInterface};
 use util::transaction_utils;
-use util::ser::{Readable, Writeable, Writer};
+use util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter};
 use util::logger::Logger;
 use util::errors::APIError;
 use util::config::{UserConfig,ChannelConfig};
@@ -4092,7 +4092,13 @@ impl<ChanSigner: ChannelKeys> Writeable for Channel<ChanSigner> {
 
 		self.latest_monitor_update_id.write(writer)?;
 
-		self.holder_keys.write(writer)?;
+		let mut key_data = VecWriter(Vec::new());
+		self.holder_keys.write(&mut key_data)?;
+		assert!(key_data.0.len() < std::usize::MAX);
+		assert!(key_data.0.len() < std::u32::MAX as usize);
+		(key_data.0.len() as u32).write(writer)?;
+		writer.write_all(&key_data.0[..])?;
+
 		self.shutdown_pubkey.write(writer)?;
 		self.destination_script.write(writer)?;
 
@@ -4260,8 +4266,10 @@ impl<ChanSigner: ChannelKeys> Writeable for Channel<ChanSigner> {
 	}
 }
 
-impl<ChanSigner: ChannelKeys + Readable> Readable for Channel<ChanSigner> {
-	fn read<R : ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+const MAX_ALLOC_SIZE: usize = 64*1024;
+impl<'a, ChanSigner: ChannelKeys, K: Deref> ReadableArgs<&'a K> for Channel<ChanSigner>
+		where K::Target: KeysInterface<ChanKeySigner = ChanSigner> {
+	fn read<R : ::std::io::Read>(reader: &mut R, keys_source: &'a K) -> Result<Self, DecodeError> {
 		let _ver: u8 = Readable::read(reader)?;
 		let min_ver: u8 = Readable::read(reader)?;
 		if min_ver > SERIALIZATION_VERSION {
@@ -4278,7 +4286,17 @@ impl<ChanSigner: ChannelKeys + Readable> Readable for Channel<ChanSigner> {
 
 		let latest_monitor_update_id = Readable::read(reader)?;
 
-		let holder_keys = Readable::read(reader)?;
+		let keys_len: u32 = Readable::read(reader)?;
+		let mut keys_data = Vec::with_capacity(cmp::min(keys_len as usize, MAX_ALLOC_SIZE));
+		while keys_data.len() != keys_len as usize {
+			// Read 1KB at a time to avoid accidentally allocating 4GB on corrupted channel keys
+			let mut data = [0; 1024];
+			let read_slice = &mut data[0..cmp::min(1024, keys_len as usize - keys_data.len())];
+			reader.read_exact(read_slice)?;
+			keys_data.extend_from_slice(read_slice);
+		}
+		let holder_keys = keys_source.read_chan_signer(&keys_data)?;
+
 		let shutdown_pubkey = Readable::read(reader)?;
 		let destination_script = Readable::read(reader)?;
 
@@ -4559,7 +4577,7 @@ mod tests {
 			self.chan_keys.clone()
 		}
 		fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] }
-		fn read_chan_signer(&self, data: &[u8]) -> Result<Self::ChanKeySigner, DecodeError> { panic!(); }
+		fn read_chan_signer(&self, _data: &[u8]) -> Result<Self::ChanKeySigner, DecodeError> { panic!(); }
 	}
 
 	fn public_from_secret_hex(secp_ctx: &Secp256k1<All>, hex: &str) -> PublicKey {

lightning/src/ln/channelmanager.rs

@@ -3782,7 +3782,8 @@ pub struct ChannelManagerReadArgs<'a, ChanSigner: 'a + ChannelKeys, M: Deref, T:
         L::Target: Logger,
 {
 	/// The keys provider which will give us relevant keys. Some keys will be loaded during
-	/// deserialization.
+	/// deserialization and KeysInterface::read_chan_signer will be used to read per-Channel
+	/// signing data.
 	pub keys_manager: K,
 
 	/// The fee_estimator for use in the ChannelManager in the future.
@@ -3844,7 +3845,7 @@ impl<'a, ChanSigner: 'a + ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L
 
 // Implement ReadableArgs for an Arc'd ChannelManager to make it a bit easier to work with the
 // SipmleArcChannelManager type:
-impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
+impl<'a, ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	ReadableArgs<ChannelManagerReadArgs<'a, ChanSigner, M, T, K, F, L>> for (BlockHash, Arc<ChannelManager<ChanSigner, M, T, K, F, L>>)
 	where M::Target: chain::Watch<Keys=ChanSigner>,
         T::Target: BroadcasterInterface,
@@ -3858,7 +3859,7 @@ impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: De
 	}
 }
 
-impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
+impl<'a, ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	ReadableArgs<ChannelManagerReadArgs<'a, ChanSigner, M, T, K, F, L>> for (BlockHash, ChannelManager<ChanSigner, M, T, K, F, L>)
 	where M::Target: chain::Watch<Keys=ChanSigner>,
         T::Target: BroadcasterInterface,
@@ -3884,7 +3885,7 @@ impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: De
 		let mut by_id = HashMap::with_capacity(cmp::min(channel_count as usize, 128));
 		let mut short_to_id = HashMap::with_capacity(cmp::min(channel_count as usize, 128));
 		for _ in 0..channel_count {
-			let mut channel: Channel<ChanSigner> = Readable::read(reader)?;
+			let mut channel: Channel<ChanSigner> = Channel::read(reader, &args.keys_manager)?;
 			if channel.last_block_connected != Default::default() && channel.last_block_connected != last_block_hash {
 				return Err(DecodeError::InvalidValue);
 			}

lightning/src/ln/functional_test_utils.rs

@@ -21,7 +21,7 @@ use ln::msgs;
 use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler};
 use util::enforcing_trait_impls::EnforcingChannelKeys;
 use util::test_utils;
-use util::test_utils::TestChainMonitor;
+use util::test_utils::{TestChainMonitor, OnlyReadsKeysInterface};
 use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider};
 use util::errors::APIError;
 use util::config::UserConfig;
@@ -172,7 +172,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> {
 					let mut w = test_utils::TestVecWriter(Vec::new());
 					old_monitor.write(&mut w).unwrap();
 					let (_, deserialized_monitor) = <(BlockHash, ChannelMonitor<EnforcingChannelKeys>)>::read(
-						&mut ::std::io::Cursor::new(&w.0)).unwrap();
+						&mut ::std::io::Cursor::new(&w.0), &OnlyReadsKeysInterface {}).unwrap();
 					deserialized_monitors.push(deserialized_monitor);
 				}
 			}