Skip to content

Fix various issues found in full_stack_target fuzzing #2808

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 8, 2024
Merged

Fix various issues found in full_stack_target fuzzing #2808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
df1f981
Fix `Feature` eq + hash to ignore excess zero bytes
TheBlueMatt Dec 24, 2023
ddb54fc
Stop including dust values in feerate affordability checks
TheBlueMatt Dec 24, 2023
c946edb
Fix `REVOKEABLE_REDEEMSCRIPT_MAX_LENGTH` for contest delays >0x7fff
TheBlueMatt Dec 24, 2023
5d8cd5a
Fix debug assertion on opening a channel with a disconnected peer
TheBlueMatt Dec 24, 2023
3b6a361
Fix dust buffer feerate calculation overflow
TheBlueMatt Dec 24, 2023
7f24e83
Fix reachable unwrap on non-channel_type manual channel acceptance
TheBlueMatt Dec 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lightning/src/ln/chan_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,9 +485,11 @@ impl TxCreationKeys {
}

/// The maximum length of a script returned by get_revokeable_redeemscript.
// Calculated as 6 bytes of opcodes, 1 byte push plus 2 bytes for contest_delay, and two public
// keys of 33 bytes (+ 1 push).
pub const REVOKEABLE_REDEEMSCRIPT_MAX_LENGTH: usize = 6 + 3 + 34*2;
// Calculated as 6 bytes of opcodes, 1 byte push plus 3 bytes for contest_delay, and two public
// keys of 33 bytes (+ 1 push). Generally, pushes are only 2 bytes (for values below 0x7fff, i.e.
// around 7 months), however, a 7 month contest delay shouldn't result in being unable to reclaim
// on-chain funds.
pub const REVOKEABLE_REDEEMSCRIPT_MAX_LENGTH: usize = 6 + 4 + 34*2;

/// A script either spendable by the revocation
/// key or the broadcaster_delayed_payment_key and satisfying the relative-locktime OP_CSV constrain.
Expand Down
77 changes: 42 additions & 35 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,8 +732,8 @@ struct CommitmentStats<'a> {
total_fee_sat: u64, // the total fee included in the transaction
num_nondust_htlcs: usize, // the number of HTLC outputs (dust HTLCs *non*-included)
htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, // the list of HTLCs (dust HTLCs *included*) which were not ignored when building the transaction
local_balance_msat: u64, // local balance before fees but considering dust limits
remote_balance_msat: u64, // remote balance before fees but considering dust limits
local_balance_msat: u64, // local balance before fees *not* considering dust limits
remote_balance_msat: u64, // remote balance before fees *not* considering dust limits
outbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful offered HTLCs since last commitment
inbound_htlc_preimages: Vec<PaymentPreimage>, // preimages for successful received HTLCs since last commitment
}
Expand Down Expand Up @@ -1728,13 +1728,13 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
}

let mut value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset;
let value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset;
assert!(value_to_self_msat >= 0);
// Note that in case they have several just-awaiting-last-RAA fulfills in-progress (ie
// AwaitingRemoteRevokeToRemove or AwaitingRemovedRemoteRevoke) we may have allowed them to
// "violate" their reserve value by couting those against it. Thus, we have to convert
// everything to i64 before subtracting as otherwise we can overflow.
let mut value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset;
let value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000) as i64 - (self.value_to_self_msat as i64) - (remote_htlc_total_msat as i64) - value_to_self_msat_offset;
assert!(value_to_remote_msat >= 0);

#[cfg(debug_assertions)]
Expand Down Expand Up @@ -1800,10 +1800,6 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
htlcs_included.sort_unstable_by_key(|h| h.0.transaction_output_index.unwrap());
htlcs_included.append(&mut included_dust_htlcs);

// For the stats, trimmed-to-0 the value in msats accordingly
value_to_self_msat = if (value_to_self_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_self_msat };
value_to_remote_msat = if (value_to_remote_msat * 1000) < broadcaster_dust_limit_satoshis as i64 { 0 } else { value_to_remote_msat };

CommitmentStats {
tx,
feerate_per_kw,
Expand Down Expand Up @@ -1876,7 +1872,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
if let Some(feerate) = outbound_feerate_update {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the description above it is written:

// may, at any point, increase _by_ at least 10 sat/vB (i.e 2530 sat/kWU) or 25%,
// whichever is higher.

which implies:

cmp::max(feerate_by_kw + 2530, feerate_plus_quarter)

Whereas, the line should read:

pub fn get_dust_buffer_feerate(&self, outbound_feerate_update: Option<u32>) -> u32 {
                // When calculating our exposure to dust HTLCs, we assume that the channel feerate
-               // may, at any point, increase by at least 10 sat/vB (i.e 2530 sat/kWU) or 25%,
+               // may, at any point, increase to at least 10 sat/vB (i.e 2530 sat/kWU) or by 25%,
                // whichever is higher. This ensures that we aren't suddenly exposed to significantly

Since we are touching this function in this PR. I believe we should also address this small but significant comment change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch! I'd like to actually address this by making the code match the comment, not the other way around, so I'll address this in a later PR - #2815

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

feerate_per_kw = cmp::max(feerate_per_kw, feerate);
}
cmp::max(2530, feerate_per_kw * 1250 / 1000)
let feerate_plus_quarter = feerate_per_kw.checked_mul(1250).map(|v| v / 1000);
cmp::max(2530, feerate_plus_quarter.unwrap_or(u32::max_value()))
}

/// Get forwarding information for the counterparty.
Expand Down Expand Up @@ -6848,6 +6845,41 @@ pub(super) struct InboundV1Channel<SP: Deref> where SP::Target: SignerProvider {
pub unfunded_context: UnfundedChannelContext,
}

/// Fetches the [`ChannelTypeFeatures`] that will be used for a channel built from a given
/// [`msgs::OpenChannel`].
pub(super) fn channel_type_from_open_channel(
msg: &msgs::OpenChannel, their_features: &InitFeatures,
our_supported_features: &ChannelTypeFeatures
) -> Result<ChannelTypeFeatures, ChannelError> {
if let Some(channel_type) = &msg.channel_type {
if channel_type.supports_any_optional_bits() {
return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".to_owned()));
}

// We only support the channel types defined by the `ChannelManager` in
// `provided_channel_type_features`. The channel type must always support
// `static_remote_key`.
if !channel_type.requires_static_remote_key() {
return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned()));
}
// Make sure we support all of the features behind the channel type.
if !channel_type.is_subset(our_supported_features) {
return Err(ChannelError::Close("Channel Type contains unsupported features".to_owned()));
}
let announced_channel = if (msg.channel_flags & 1) == 1 { true } else { false };
if channel_type.requires_scid_privacy() && announced_channel {
return Err(ChannelError::Close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned()));
}
Ok(channel_type.clone())
} else {
let channel_type = ChannelTypeFeatures::from_init(&their_features);
if channel_type != ChannelTypeFeatures::only_static_remote_key() {
return Err(ChannelError::Close("Only static_remote_key is supported for non-negotiated channel types".to_owned()));
}
Ok(channel_type)
}
}

impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
/// Creates a new channel from a remote sides' request for one.
/// Assumes chain_hash has already been checked and corresponds with what we expect!
Expand All @@ -6866,32 +6898,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {

// First check the channel type is known, failing before we do anything else if we don't
// support this channel type.
let channel_type = if let Some(channel_type) = &msg.channel_type {
if channel_type.supports_any_optional_bits() {
return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".to_owned()));
}

// We only support the channel types defined by the `ChannelManager` in
// `provided_channel_type_features`. The channel type must always support
// `static_remote_key`.
if !channel_type.requires_static_remote_key() {
return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned()));
}
// Make sure we support all of the features behind the channel type.
if !channel_type.is_subset(our_supported_features) {
return Err(ChannelError::Close("Channel Type contains unsupported features".to_owned()));
}
if channel_type.requires_scid_privacy() && announced_channel {
return Err(ChannelError::Close("SCID Alias/Privacy Channel Type cannot be set on a public channel".to_owned()));
}
channel_type.clone()
} else {
let channel_type = ChannelTypeFeatures::from_init(&their_features);
if channel_type != ChannelTypeFeatures::only_static_remote_key() {
return Err(ChannelError::Close("Only static_remote_key is supported for non-negotiated channel types".to_owned()));
}
channel_type
};
let channel_type = channel_type_from_open_channel(msg, their_features, our_supported_features)?;

let channel_keys_id = signer_provider.generate_channel_keys_id(true, msg.funding_satoshis, user_id);
let holder_signer = signer_provider.derive_channel_signer(msg.funding_satoshis, channel_keys_id);
Expand Down
17 changes: 8 additions & 9 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ use crate::events::{Event, EventHandler, EventsProvider, MessageSendEvent, Messa
// Since this struct is returned in `list_channels` methods, expose it here in case users want to
// construct one themselves.
use crate::ln::{inbound_payment, ChannelId, PaymentHash, PaymentPreimage, PaymentSecret};
use crate::ln::channel::{Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext};
use crate::ln::channel::{self, Channel, ChannelPhase, ChannelContext, ChannelError, ChannelUpdateStatus, ShutdownResult, UnfundedChannelContext, UpdateFulfillCommitFetch, OutboundV1Channel, InboundV1Channel, WithChannelContext};
use crate::ln::features::{Bolt12InvoiceFeatures, ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
#[cfg(any(feature = "_test_utils", test))]
use crate::ln::features::Bolt11InvoiceFeatures;
Expand Down Expand Up @@ -6170,13 +6170,18 @@ where

// If we're doing manual acceptance checks on the channel, then defer creation until we're sure we want to accept.
if self.default_configuration.manually_accept_inbound_channels {
let channel_type = channel::channel_type_from_open_channel(
&msg, &peer_state.latest_features, &self.channel_type_features()
).map_err(|e|
MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id)
)?;
let mut pending_events = self.pending_events.lock().unwrap();
pending_events.push_back((events::Event::OpenChannelRequest {
temporary_channel_id: msg.temporary_channel_id.clone(),
counterparty_node_id: counterparty_node_id.clone(),
funding_satoshis: msg.funding_satoshis,
push_msat: msg.push_msat,
channel_type: msg.channel_type.clone().unwrap(),
channel_type,
}, None));
peer_state.inbound_channel_request_by_id.insert(channel_id, InboundChannelRequest {
open_channel_msg: msg.clone(),
Expand Down Expand Up @@ -8979,13 +8984,7 @@ where
let pending_msg_events = &mut peer_state.pending_msg_events;

peer_state.channel_by_id.iter_mut().filter_map(|(_, phase)|
if let ChannelPhase::Funded(chan) = phase { Some(chan) } else {
// Since unfunded channel maps are cleared upon disconnecting a peer, and they're not persisted
// (so won't be recovered after a crash), they shouldn't exist here and we would never need to
// worry about closing and removing them.
debug_assert!(false);
None
}
if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
).for_each(|chan| {
let logger = WithChannelContext::from(&self.logger, &chan.context);
pending_msg_events.push(events::MessageSendEvent::SendChannelReestablish {
Expand Down
38 changes: 36 additions & 2 deletions lightning/src/ln/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,12 +469,24 @@ impl<T: sealed::Context> Clone for Features<T> {
}
impl<T: sealed::Context> Hash for Features<T> {
fn hash<H: Hasher>(&self, hasher: &mut H) {
self.flags.hash(hasher);
let mut nonzero_flags = &self.flags[..];
while nonzero_flags.last() == Some(&0) {
nonzero_flags = &nonzero_flags[..nonzero_flags.len() - 1];
}
nonzero_flags.hash(hasher);
}
}
impl<T: sealed::Context> PartialEq for Features<T> {
fn eq(&self, o: &Self) -> bool {
self.flags.eq(&o.flags)
let mut o_iter = o.flags.iter();
let mut self_iter = self.flags.iter();
loop {
match (o_iter.next(), self_iter.next()) {
(Some(o), Some(us)) => if o != us { return false },
(Some(b), None) | (None, Some(b)) => if *b != 0 { return false },
(None, None) => return true,
}
}
}
}
impl<T: sealed::Context> PartialOrd for Features<T> {
Expand Down Expand Up @@ -1215,4 +1227,26 @@ mod tests {
assert!(!converted_features.supports_any_optional_bits());
assert!(converted_features.requires_static_remote_key());
}

#[test]
#[cfg(feature = "std")]
fn test_excess_zero_bytes_ignored() {
// Checks that `Hash` and `PartialEq` ignore excess zero bytes, which may appear due to
// feature conversion or because a peer serialized their feature poorly.
use std::collections::hash_map::DefaultHasher;
use std::hash::{Hash, Hasher};

let mut zerod_features = InitFeatures::empty();
zerod_features.flags = vec![0];
let empty_features = InitFeatures::empty();
assert!(empty_features.flags.is_empty());

assert_eq!(zerod_features, empty_features);

let mut zerod_hash = DefaultHasher::new();
zerod_features.hash(&mut zerod_hash);
let mut empty_hash = DefaultHasher::new();
empty_features.hash(&mut empty_hash);
assert_eq!(zerod_hash.finish(), empty_hash.finish());
}
}