Skip to content

Commit f06b59d

Browse files
EvanfeenstraEvanfeenstra
committed
refactor to SanitizedString, make sure ClosureReason::Display uses the sanitzed string
1 parent 4dce6b4 commit f06b59d

File tree

8 files changed

+52
-46
lines changed

8 files changed

+52
-46
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,11 @@ use crate::ln::outbound_payment::{OutboundPayments, PaymentAttempts, PendingOutb
5555
use crate::ln::wire::Encode;
5656
use crate::chain::keysinterface::{EntropySource, KeysManager, NodeSigner, Recipient, SignerProvider, ChannelSigner, WriteableEcdsaChannelSigner};
5757
use crate::util::config::{UserConfig, ChannelConfig};
58-
use crate::util::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, CounterpartyForceClosedPeerMsg, HTLCDestination};
58+
use crate::util::events::{Event, EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason, HTLCDestination};
5959
use crate::util::events;
6060
use crate::util::wakers::{Future, Notifier};
6161
use crate::util::scid_utils::fake_scid;
62+
use crate::util::string::SanitizedString;
6263
use crate::util::ser::{BigSize, FixedLengthReader, Readable, ReadableArgs, MaybeReadable, Writeable, Writer, VecWriter};
6364
use crate::util::logger::{Level, Logger};
6465
use crate::util::errors::APIError;
@@ -1990,7 +1991,7 @@ where
19901991
let peer_state = &mut *peer_state_lock;
19911992
if let hash_map::Entry::Occupied(chan) = peer_state.channel_by_id.entry(channel_id.clone()) {
19921993
if let Some(peer_msg) = peer_msg {
1993-
self.issue_channel_close_events(chan.get(),ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg(peer_msg.to_string()) });
1994+
self.issue_channel_close_events(chan.get(),ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString(peer_msg.to_string()) });
19941995
} else {
19951996
self.issue_channel_close_events(chan.get(),ClosureReason::HolderForceClosed);
19961997
}

‎lightning/src/ln/functional_tests.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,10 @@ use crate::ln::msgs;
3131
use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
3232
use crate::util::enforcing_trait_impls::EnforcingSigner;
3333
use crate::util::test_utils;
34-
use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, CounterpartyForceClosedPeerMsg, HTLCDestination};
34+
use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentPurpose, ClosureReason, HTLCDestination};
3535
use crate::util::errors::APIError;
3636
use crate::util::ser::{Writeable, ReadableArgs};
37+
use crate::util::string::SanitizedString;
3738
use crate::util::config::UserConfig;
3839

3940
use bitcoin::hash_types::BlockHash;
@@ -8344,7 +8345,7 @@ fn test_pre_lockin_no_chan_closed_update() {
83448345
let channel_id = crate::chain::transaction::OutPoint { txid: funding_created_msg.funding_txid, index: funding_created_msg.funding_output_index }.to_channel_id();
83458346
nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id, data: "Hi".to_owned() });
83468347
assert!(nodes[0].chain_monitor.added_monitors.lock().unwrap().is_empty());
8347-
check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg("Hi".to_string()) }, true);
8348+
check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString("Hi".to_string()) }, true);
83488349
}
83498350

83508351
#[test]
@@ -8802,7 +8803,7 @@ fn test_error_chans_closed() {
88028803
nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: chan_2.2, data: "ERR".to_owned() });
88038804
check_added_monitors!(nodes[0], 1);
88048805
check_closed_broadcast!(nodes[0], false);
8805-
check_closed_event!(nodes[0], 1, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg("ERR".to_string()) });
8806+
check_closed_event!(nodes[0], 1, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString("ERR".to_string()) });
88068807
assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1);
88078808
assert_eq!(nodes[0].node.list_usable_channels().len(), 2);
88088809
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() {
88128813
let _chan_4 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001);
88138814
nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: [0; 32], data: "ERR".to_owned() });
88148815
check_added_monitors!(nodes[0], 2);
8815-
check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg("ERR".to_string()) });
8816+
check_closed_event!(nodes[0], 2, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString("ERR".to_string()) });
88168817
let events = nodes[0].node.get_and_clear_pending_msg_events();
88178818
assert_eq!(events.len(), 2);
88188819
match events[0] {

‎lightning/src/ln/payment_tests.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,11 @@ use crate::ln::outbound_payment::Retry;
2424
use crate::routing::gossip::{EffectiveCapacity, RoutingFees};
2525
use crate::routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RouteParameters};
2626
use crate::routing::scoring::ChannelUsage;
27-
use crate::util::events::{ClosureReason, CounterpartyForceClosedPeerMsg, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure};
27+
use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure};
2828
use crate::util::test_utils;
2929
use crate::util::errors::APIError;
3030
use crate::util::ser::Writeable;
31+
use crate::util::string::SanitizedString;
3132

3233
use bitcoin::{Block, BlockHeader, TxMerkleNode};
3334
use bitcoin::hashes::Hash;
@@ -353,7 +354,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
353354
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
354355
assert_eq!(node_id, nodes[1].node.get_our_node_id());
355356
nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), msg);
356-
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg(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())) });
357+
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString(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())) });
357358
check_added_monitors!(nodes[1], 1);
358359
assert_eq!(nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0).len(), 1);
359360
},
@@ -518,7 +519,7 @@ fn do_test_completed_payment_not_retryable_on_reload(use_dust: bool) {
518519
MessageSendEvent::HandleError { node_id, action: msgs::ErrorAction::SendErrorMessage { ref msg } } => {
519520
assert_eq!(node_id, nodes[1].node.get_our_node_id());
520521
nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), msg);
521-
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg(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())) });
522+
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString(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())) });
522523
check_added_monitors!(nodes[1], 1);
523524
bs_commitment_tx = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().split_off(0);
524525
},

‎lightning/src/ln/reload_tests.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,10 @@ use crate::ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction}
2020
use crate::util::enforcing_trait_impls::EnforcingSigner;
2121
use crate::util::test_utils;
2222
use crate::util::errors::APIError;
23-
use crate::util::events::{ClosureReason, CounterpartyForceClosedPeerMsg, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
23+
use crate::util::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
2424
use crate::util::ser::{Writeable, ReadableArgs};
2525
use crate::util::config::UserConfig;
26+
use crate::util::string::SanitizedString;
2627

2728
use bitcoin::hash_types::BlockHash;
2829

@@ -566,7 +567,7 @@ fn do_test_data_loss_protect(reconnect_panicing: bool) {
566567
nodes[1].node.handle_error(&nodes[0].node.get_our_node_id(), &err_msgs_0[0]);
567568
assert!(nodes[1].node.list_usable_channels().is_empty());
568569
check_added_monitors!(nodes[1], 1);
569-
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg(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())) });
570+
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString(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())) });
570571
check_closed_broadcast!(nodes[1], false);
571572
}
572573

‎lightning/src/ln/reorg_tests.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,10 @@ use crate::chain::transaction::OutPoint;
1414
use crate::chain::Confirm;
1515
use crate::ln::channelmanager::ChannelManager;
1616
use crate::ln::msgs::{ChannelMessageHandler, Init};
17-
use crate::util::events::{Event, MessageSendEventsProvider, ClosureReason, CounterpartyForceClosedPeerMsg, HTLCDestination};
17+
use crate::util::events::{Event, MessageSendEventsProvider, ClosureReason, HTLCDestination};
1818
use crate::util::test_utils;
1919
use crate::util::ser::Writeable;
20+
use crate::util::string::SanitizedString;
2021

2122
use bitcoin::blockdata::block::{Block, BlockHeader};
2223
use bitcoin::blockdata::script::Builder;
@@ -368,7 +369,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
368369
nodes[0].node.test_process_background_events(); // Required to free the pending background monitor update
369370
check_added_monitors!(nodes[0], 1);
370371
let expected_err = "Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.";
371-
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg(format!("Channel closed because of an exception: {}", expected_err)) });
372+
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString(format!("Channel closed because of an exception: {}", expected_err)) });
372373
check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: expected_err.to_owned() });
373374
assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1);
374375
nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear();

‎lightning/src/ln/shutdown_tests.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,10 @@ use crate::ln::msgs::{ChannelMessageHandler, ErrorAction};
1818
use crate::ln::script::ShutdownScript;
1919
use crate::util::test_utils;
2020
use crate::util::test_utils::OnGetShutdownScriptpubkey;
21-
use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason, CounterpartyForceClosedPeerMsg};
21+
use crate::util::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
2222
use crate::util::errors::APIError;
2323
use crate::util::config::UserConfig;
24+
use crate::util::string::SanitizedString;
2425

2526
use bitcoin::blockdata::script::Builder;
2627
use bitcoin::blockdata::opcodes;
@@ -380,7 +381,7 @@ fn do_test_shutdown_rebroadcast(recv_count: u8) {
380381
// closing_signed so we do it ourselves
381382
check_closed_broadcast!(nodes[1], false);
382383
check_added_monitors!(nodes[1], 1);
383-
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: CounterpartyForceClosedPeerMsg(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())) });
384+
check_closed_event!(nodes[1], 1, ClosureReason::CounterpartyForceClosed { peer_msg: SanitizedString(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())) });
384385
}
385386

386387
assert!(nodes[0].node.list_channels().is_empty());

‎lightning/src/util/events.rs

Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
//! few other things.
1616
1717
use crate::chain::keysinterface::SpendableOutputDescriptor;
18-
use crate::io::Read;
1918
#[cfg(anchors)]
2019
use crate::ln::chan_utils::{self, ChannelTransactionParameters, HTLCOutputInCommitment};
2120
use crate::ln::channelmanager::{InterceptId, PaymentId};
@@ -26,7 +25,7 @@ use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
2625
use crate::routing::gossip::NetworkUpdate;
2726
use crate::util::errors::APIError;
2827
use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength};
29-
use crate::util::string::PrintableString;
28+
use crate::util::string::SanitizedString;
3029
use crate::routing::router::{RouteHop, RouteParameters};
3130

3231
use bitcoin::{PackedLockTime, Transaction};
@@ -42,7 +41,6 @@ use bitcoin::secp256k1::{self, Secp256k1};
4241
use bitcoin::secp256k1::ecdsa::Signature;
4342
use crate::io;
4443
use crate::prelude::*;
45-
use core::fmt;
4644
use core::time::Duration;
4745
use core::ops::Deref;
4846
use crate::sync::Arc;
@@ -130,10 +128,10 @@ pub enum ClosureReason {
130128
///
131129
/// Be careful about printing the peer_msg, a well-crafted message could exploit
132130
/// a security vulnerability in the terminal emulator or the logging subsystem.
133-
/// To be safe, use `Display` on `CounterpartyForceClosedPeerMsg` (it is sanitized)
131+
/// To be safe, use `Display` on `SanitizedString`
134132
///
135-
/// [`CounterpartyForceClosedPeerMsg`]: CounterpartyForceClosedPeerMsg
136-
peer_msg: CounterpartyForceClosedPeerMsg,
133+
/// [`SanitizedString`]: crate::util::string::SanitizedString
134+
peer_msg: SanitizedString,
137135
},
138136
/// Closure generated from [`ChannelManager::force_close_channel`], called by the user.
139137
///
@@ -179,7 +177,7 @@ impl core::fmt::Display for ClosureReason {
179177
match self {
180178
ClosureReason::CounterpartyForceClosed { peer_msg } => {
181179
f.write_str("counterparty force-closed with message ")?;
182-
f.write_str(&peer_msg.0)
180+
f.write_str(&format!("{}", peer_msg))
183181
},
184182
ClosureReason::HolderForceClosed => f.write_str("user manually force-closed the channel"),
185183
ClosureReason::CooperativeClosure => f.write_str("the channel was cooperatively closed"),
@@ -206,30 +204,6 @@ impl_writeable_tlv_based_enum_upgradable!(ClosureReason,
206204
(12, OutdatedChannelManager) => {},
207205
);
208206

209-
/// Struct to `Display` `CounterpartyForceClosed::peer_msg` using `PrintableString`
210-
/// Print the peer_msg using Display to sanitize it (remove control characters)
211-
#[derive(Clone, Debug, PartialEq, Eq)]
212-
pub struct CounterpartyForceClosedPeerMsg(pub String);
213-
214-
impl Writeable for CounterpartyForceClosedPeerMsg {
215-
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
216-
self.0.write(w)
217-
}
218-
}
219-
220-
impl Readable for CounterpartyForceClosedPeerMsg {
221-
fn read<R: Read>(r: &mut R) -> Result<Self, msgs::DecodeError> {
222-
let s: String = Readable::read(r)?;
223-
Ok(Self(s))
224-
}
225-
}
226-
227-
impl fmt::Display for CounterpartyForceClosedPeerMsg {
228-
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
229-
PrintableString(&self.0).fmt(f)
230-
}
231-
}
232-
233207
/// Intended destination of a failed HTLC as indicated in [`Event::HTLCHandlingFailed`].
234208
#[derive(Clone, Debug, PartialEq, Eq)]
235209
pub enum HTLCDestination {

‎lightning/src/util/string.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010
//! Utilities for strings.
1111
1212
use core::fmt;
13+
use crate::util::ser::{Writeable, Writer, Readable};
14+
use crate::io::{self, Read};
15+
use crate::ln::msgs;
1316

1417
/// A string that displays only printable characters, replacing control characters with
1518
/// [`core::char::REPLACEMENT_CHARACTER`].
@@ -40,3 +43,26 @@ mod tests {
4043
);
4144
}
4245
}
46+
47+
/// Struct to `Display` fields in a safe way using `PrintableString`
48+
#[derive(Clone, Debug, PartialEq, Eq)]
49+
pub struct SanitizedString(pub String);
50+
51+
impl Writeable for SanitizedString {
52+
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
53+
self.0.write(w)
54+
}
55+
}
56+
57+
impl Readable for SanitizedString {
58+
fn read<R: Read>(r: &mut R) -> Result<Self, msgs::DecodeError> {
59+
let s: String = Readable::read(r)?;
60+
Ok(Self(s))
61+
}
62+
}
63+
64+
impl fmt::Display for SanitizedString {
65+
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
66+
PrintableString(&self.0).fmt(f)
67+
}
68+
}

0 commit comments

Comments
 (0)