From 987ab9512ce1e98fbd8c99339515a3ac972ce231 Mon Sep 17 00:00:00 2001 From: Evan Feenstra Date: Fri, 17 Mar 2023 22:24:23 -0700 Subject: [PATCH] SanitizedString struct to safely Display CounterpartyForceClosed peer_msg --- lightning/src/ln/channelmanager.rs | 3 ++- lightning/src/ln/functional_tests.rs | 7 ++++--- lightning/src/ln/payment_tests.rs | 5 +++-- lightning/src/ln/reload_tests.rs | 3 ++- lightning/src/ln/reorg_tests.rs | 3 ++- lightning/src/ln/shutdown_tests.rs | 3 ++- lightning/src/util/events.rs | 12 +++++++----- lightning/src/util/string.rs | 27 +++++++++++++++++++++++++++ 8 files changed, 49 insertions(+), 14 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 62891cc4..34bd053e 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -59,6 +59,7 @@ use crate::util::events::{Event, EventHandler, EventsProvider, MessageSendEvent, use crate::util::events; use crate::util::wakers::{Future, Notifier}; use crate::util::scid_utils::fake_scid; +use crate::util::string::UntrustedString; use crate::util::ser::{BigSize, FixedLengthReader, Readable, ReadableArgs, MaybeReadable, Writeable, Writer, VecWriter}; use crate::util::logger::{Level, Logger}; use crate::util::errors::APIError; @@ -1990,7 +1991,7 @@ where let peer_state = &mut *peer_state_lock; if let hash_map::Entry::Occupied(chan) = peer_state.channel_by_id.entry(channel_id.clone()) { if let Some(peer_msg) = peer_msg { - self.issue_channel_close_events(chan.get(),ClosureReason::CounterpartyForceClosed { peer_msg: peer_msg.to_string() }); + self.issue_channel_close_events(chan.get(),ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(peer_msg.to_string()) }); } else { self.issue_channel_close_events(chan.get(),ClosureReason::HolderForceClosed); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 4fe5d041..1410abc9 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -34,6 +34,7 @@ use crate::util::test_utils; use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination}; use crate::util::errors::APIError; use crate::util::ser::{Writeable, ReadableArgs}; +use crate::util::string::UntrustedString; use crate::util::config::UserConfig; use bitcoin::hash_types::BlockHash; @@ -8344,7 +8345,7 @@ fn test_pre_lockin_no_chan_closed_update() { let channel_id = crate::chain::transaction::OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id(); nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id, data: "Hi".to_owned() }); assert!(nodes[0].chain_monitor.added_monitors.lock().unwrap().is_empty()); - check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: "Hi".to_string() }, true); + check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString("Hi".to_string()) }, true); } #[test] @@ -8802,7 +8803,7 @@ fn test_error_chans_closed() { nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: chan_2.2, data: "ERR".to_owned() }); check_added_monitors!(nodes[0], 1); check_closed_broadcast!(nodes[0], false); - check_closed_event!(nodes[0], 1, ClosureReason::CounterpartyForceClosed { peer_msg: "ERR".to_string() }); + check_closed_event!(nodes[0], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString("ERR".to_string()) }); assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1); assert_eq!(nodes[0].node.list_usable_channels().len(), 2); assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_1.2 || nodes[0].node.list_usable_channels()[1].channel_id == chan_1.2); @@ -8812,7 +8813,7 @@ fn test_error_chans_closed() { let _chan_4 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001); nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: [0; 32], data: "ERR".to_owned() }); check_added_monitors!(nodes[0], 2); - check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: "ERR".to_string() }); + check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString("ERR".to_string()) }); let events = nodes[0].node.get_and_clear_pending_msg_events(); assert_eq!(events.len(), 2); match events[0] { diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 15361b98..29972c90 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -28,6 +28,7 @@ use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEven use crate::util::test_utils; use crate::util::errors::APIError; use crate::util::ser::Writeable; +use crate::util::string::UntrustedString; use bitcoin::{Block, BlockHeader, TxMerkleNode}; use bitcoin::hashes::Hash; @@ -353,7 +354,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) { MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => { assert_eq!(node_id, nodes[1].node.get_our_node_id()); nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), msg); - check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id()) }); + check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id())) }); check_added_monitors!(nodes[1], 1); assert_eq!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1); }, @@ -518,7 +519,7 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) { MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => { assert_eq!(node_id, nodes[1].node.get_our_node_id()); nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), msg); - check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id()) }); + check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id())) }); check_added_monitors!(nodes[1], 1); bs_commitment_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0); }, diff --git a/lightning/src/ln/reload_tests.rs b/lightning/src/ln/reload_tests.rs index 14deefd3..2380eed7 100644 --- a/lightning/src/ln/reload_tests.rs +++ b/lightning/src/ln/reload_tests.rs @@ -23,6 +23,7 @@ use crate::util::errors::APIError; use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider}; use crate::util::ser::{Writeable, ReadableArgs}; use crate::util::config::UserConfig; +use crate::util::string::UntrustedString; use bitcoin::hash_types::BlockHash; @@ -566,7 +567,7 @@ fn do_test_data_loss_protect(reconnect_panicing: bool) { nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), &err_msgs_0[0]); assert!(nodes[1].node.list_usable_channels().is_empty()); check_added_monitors!(nodes[1], 1); - check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id()) }); + check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id())) }); check_closed_broadcast!(nodes[1], false); } diff --git a/lightning/src/ln/reorg_tests.rs b/lightning/src/ln/reorg_tests.rs index 69e0c9af..6af8acc1 100644 --- a/lightning/src/ln/reorg_tests.rs +++ b/lightning/src/ln/reorg_tests.rs @@ -17,6 +17,7 @@ use crate::ln::msgs::{ChannelMessageHandler, Init}; use crate::util::events::{Event, MessageSendEventsProvider, ClosureReason, HTLCDestination}; use crate::util::test_utils; use crate::util::ser::Writeable; +use crate::util::string::UntrustedString; use bitcoin::blockdata::block::{Block, BlockHeader}; use bitcoin::blockdata::script::Builder; @@ -368,7 +369,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_ nodes[0].node.test_process_background_events(); // Required to free the pending background monitor update check_added_monitors!(nodes[0], 1); let expected_err = "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs."; - check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: "Channel closed because of an exception: ".to_owned() + expected_err }); + check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(format!("Channel closed because of an exception: {}", expected_err)) }); check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: expected_err.to_owned() }); assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index 75eacb74..6616ed9b 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -21,6 +21,7 @@ use crate::util::test_utils::OnGetShutdownScriptpubkey; use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason}; use crate::util::errors::APIError; use crate::util::config::UserConfig; +use crate::util::string::UntrustedString; use bitcoin::blockdata::script::Builder; use bitcoin::blockdata::opcodes; @@ -380,7 +381,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) { // closing_signed so we do it ourselves check_closed_broadcast!(nodes[1], false); check_added_monitors!(nodes[1], 1); - check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id()) }); + check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: UntrustedString(format!("Got a message for a channel from the wrong node! No such channel for the passed counterparty_node_id {}", &nodes[1].node.get_our_node_id())) }); } assert!(nodes[0].node.list_channels().is_empty()); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 8c981fd1..02e8e32a 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -25,6 +25,7 @@ use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::routing::gossip::NetworkUpdate; use crate::util::errors::APIError; use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength}; +use crate::util::string::UntrustedString; use crate::routing::router::{RouteHop, RouteParameters}; use bitcoin::{PackedLockTime, Transaction}; @@ -125,10 +126,12 @@ pub enum ClosureReason { CounterpartyForceClosed { /// The error which the peer sent us. /// - /// The string should be sanitized before it is used (e.g emitted to logs - /// or printed to stdout). Otherwise, a well crafted error message may exploit + /// Be careful about printing the peer_msg, a well-crafted message could exploit /// a security vulnerability in the terminal emulator or the logging subsystem. - peer_msg: String, + /// To be safe, use `Display` on `UntrustedString` + /// + /// [`UntrustedString`]: crate::util::string::UntrustedString + peer_msg: UntrustedString, }, /// Closure generated from [`ChannelManager::force_close_channel`], called by the user. /// @@ -173,8 +176,7 @@ impl core::fmt::Display for ClosureReason { f.write_str("Channel closed because ")?; match self { ClosureReason::CounterpartyForceClosed { peer_msg } => { - f.write_str("counterparty force-closed with message ")?; - f.write_str(&peer_msg) + f.write_fmt(format_args!("counterparty force-closed with message: {}", peer_msg)) }, ClosureReason::HolderForceClosed => f.write_str("user manually force-closed the channel"), ClosureReason::CooperativeClosure => f.write_str("the channel was cooperatively closed"), diff --git a/lightning/src/util/string.rs b/lightning/src/util/string.rs index 42eb96f5..3e5942f6 100644 --- a/lightning/src/util/string.rs +++ b/lightning/src/util/string.rs @@ -9,7 +9,34 @@ //! Utilities for strings. +use alloc::string::String; use core::fmt; +use crate::io::{self, Read}; +use crate::ln::msgs; +use crate::util::ser::{Writeable, Writer, Readable}; + +/// Struct to `Display` fields in a safe way using `PrintableString` +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct UntrustedString(pub String); + +impl Writeable for UntrustedString { + fn write(&self, w: &mut W) -> Result<(), io::Error> { + self.0.write(w) + } +} + +impl Readable for UntrustedString { + fn read(r: &mut R) -> Result { + let s: String = Readable::read(r)?; + Ok(Self(s)) + } +} + +impl fmt::Display for UntrustedString { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + PrintableString(&self.0).fmt(f) + } +} /// A string that displays only printable characters, replacing control characters with /// [`core::char::REPLACEMENT_CHARACTER`]. -- 2.30.2