SanitizedString struct to safely Display CounterpartyForceClosed peer_msg

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 62891cc415a32d45467253f7f0fc00b15a542f0e..34bd053ee5e14f0613754ba8a44abde0211a3f4e 100644 (file)
--- 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 4fe5d041028c4a0fefbf8c4ea3dc6e69bda2f88b..1410abc9310af0fd969ac0d777b4373df565fc04 100644 (file)
--- 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 15361b98ad71fd9ee1193891aa71f4d45ef5d9a3..29972c9015655d4e8ff288e05db997b1d4f79ddd 100644 (file)
--- 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 14deefd3f93828181f6cfad952d48913c8e25586..2380eed78a82f005cbc66c71741f6b0a9d367706 100644 (file)
--- 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 69e0c9afa9e49c056c7d081ef8e42913d7492d55..6af8acc1c05d5e38276d21105b99c425a045f87f 100644 (file)
--- 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 75eacb74bc2fe7352a99288e28fcef82eac4c81d..6616ed9bf482b0acb5814d809f48cd9620c2886d 100644 (file)
--- 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 8c981fd1c482db6b9c75323b639735e87d4bcac6..02e8e32af0bcd3fbe1e6cd95b04f83bb128fad8e 100644 (file)
--- 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 42eb96f5bf6076b471f0d72fc0ef66e4f4d0bafa..3e5942f6f04c13b77ceae9401355f86c249bb346 100644 (file)
--- 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<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+               self.0.write(w)
+       }
+}
+
+impl Readable for UntrustedString {
+       fn read<R: Read>(r: &mut R) -> Result<Self, msgs::DecodeError> {
+               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`].