From 7497ed2402aaf8b69b6d5c1a85dc99dba36a6ca3 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 14 Jul 2021 16:23:38 -0400 Subject: [PATCH 1/3] Fix crash due to index-out-of-bounds in feature translation This was reported by a user when trying to send a payment using the LDK sample (specifically during route generation when translating a Features from one context to another) The problem was we didn't check T::KNOWN_FEATURE_MASK vec length before indexing into it, due likely to the assumption that known feature vec lengths are the same across contexts, when they may not be --- lightning/src/ln/features.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index b459baf0658..9d865d15ec2 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -490,13 +490,14 @@ impl Features { /// Converts `Features` to `Features`. Only known `T` features relevant to context `C` are /// included in the result. fn to_context_internal(&self) -> Features { - let byte_count = C::KNOWN_FEATURE_MASK.len(); + let from_byte_count = T::KNOWN_FEATURE_MASK.len(); + let to_byte_count = C::KNOWN_FEATURE_MASK.len(); let mut flags = Vec::new(); for (i, byte) in self.flags.iter().enumerate() { - if i < byte_count { - let known_source_features = T::KNOWN_FEATURE_MASK[i]; - let known_target_features = C::KNOWN_FEATURE_MASK[i]; - flags.push(byte & known_source_features & known_target_features); + if i < from_byte_count && i < to_byte_count { + let from_known_features = T::KNOWN_FEATURE_MASK[i]; + let to_known_features = C::KNOWN_FEATURE_MASK[i]; + flags.push(byte & from_known_features & to_known_features); } } Features:: { flags, mark: PhantomData, } From 06ecbecd6d5944281bbe1f1407114af49eb293d3 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 14 Jul 2021 16:15:00 -0700 Subject: [PATCH 2/3] Remove unnecessary feature test-only methods --- lightning/src/ln/features.rs | 47 +++------------------------------ lightning/src/routing/router.rs | 3 +-- 2 files changed, 5 insertions(+), 45 deletions(-) diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index 9d865d15ec2..a9f5a10ee2d 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -301,27 +301,7 @@ mod sealed { set_shutdown_any_segwit_required); #[cfg(test)] - define_context!(TestingContext { - required_features: [ - // Byte 0 - , - // Byte 1 - , - // Byte 2 - UnknownFeature, - ], - optional_features: [ - // Byte 0 - , - // Byte 1 - , - // Byte 2 - , - ], - }); - - #[cfg(test)] - define_feature!(23, UnknownFeature, [TestingContext], + define_feature!(123456789, UnknownFeature, [NodeContext, ChannelContext], "Feature flags for an unknown feature used in testing.", set_unknown_feature_optional, set_unknown_feature_required); } @@ -553,21 +533,6 @@ impl Features { pub(crate) fn byte_count(&self) -> usize { self.flags.len() } - - #[cfg(test)] - pub(crate) fn set_required_unknown_bits(&mut self) { - ::set_required_bit(&mut self.flags); - } - - #[cfg(test)] - pub(crate) fn set_optional_unknown_bits(&mut self) { - ::set_optional_bit(&mut self.flags); - } - - #[cfg(test)] - pub(crate) fn clear_unknown_bits(&mut self) { - ::clear_bits(&mut self.flags); - } } impl Features { @@ -765,19 +730,15 @@ mod tests { #[test] fn sanity_test_unknown_bits() { - let mut features = ChannelFeatures::empty(); + let features = ChannelFeatures::empty(); assert!(!features.requires_unknown_bits()); assert!(!features.supports_unknown_bits()); - features.set_required_unknown_bits(); + let features = ChannelFeatures::empty().set_unknown_feature_required(); assert!(features.requires_unknown_bits()); assert!(features.supports_unknown_bits()); - features.clear_unknown_bits(); - assert!(!features.requires_unknown_bits()); - assert!(!features.supports_unknown_bits()); - - features.set_optional_unknown_bits(); + let features = ChannelFeatures::empty().set_unknown_feature_optional(); assert!(!features.requires_unknown_bits()); assert!(features.supports_unknown_bits()); } diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 6c14ebebd89..44b8c8a24ae 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -1992,8 +1992,7 @@ mod tests { let (_, our_id, privkeys, nodes) = get_nodes(&secp_ctx); // Disable nodes 1, 2, and 8 by requiring unknown feature bits - let mut unknown_features = NodeFeatures::known(); - unknown_features.set_required_unknown_bits(); + let unknown_features = NodeFeatures::known().set_unknown_feature_required(); add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[0], unknown_features.clone(), 1); add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[1], unknown_features.clone(), 1); add_or_update_node(&net_graph_msg_handler, &secp_ctx, &privkeys[7], unknown_features.clone(), 1); From 87233488ccfc0dad06ce712b9e594fc20e0e52f1 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 14 Jul 2021 16:18:33 -0700 Subject: [PATCH 3/3] Test index-out-of-bounds in Features::to_context When there are fewer known `from` feature bytes than known `to` feature bytes, an index-out-of-bounds error can occur if the `from` features have unknown features set in a byte past the greatest known `from` feature byte. --- lightning/src/ln/features.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index a9f5a10ee2d..5d908ee01a4 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -301,7 +301,7 @@ mod sealed { set_shutdown_any_segwit_required); #[cfg(test)] - define_feature!(123456789, UnknownFeature, [NodeContext, ChannelContext], + define_feature!(123456789, UnknownFeature, [NodeContext, ChannelContext, InvoiceContext], "Feature flags for an unknown feature used in testing.", set_unknown_feature_optional, set_unknown_feature_required); } @@ -774,6 +774,16 @@ mod tests { assert!(!init_features.supports_gossip_queries()); } + #[test] + fn convert_to_context_with_unknown_flags() { + // Ensure the `from` context has fewer known feature bytes than the `to` context. + assert!(InvoiceFeatures::known().byte_count() < NodeFeatures::known().byte_count()); + let invoice_features = InvoiceFeatures::known().set_unknown_feature_optional(); + assert!(invoice_features.supports_unknown_bits()); + let node_features: NodeFeatures = invoice_features.to_context(); + assert!(!node_features.supports_unknown_bits()); + } + #[test] fn set_feature_bits() { let features = InvoiceFeatures::empty()