Add DecodeError::DangerousValue for decoding invalid channel managers #2974

benthecarman · 2024-03-27T18:44:14Z

This would help distinguish different types of errors when deserialzing a channel manager. InvalidValue was used previously but this could be because it is an old serialization format, whereas DangerousValue is a lot more clear on why the deserialization failed.

TheBlueMatt

Sure, seems reasonable. Minor gripe in your PR description - we dont fail to read old serialization formats, at least not any version of LDK in the past few years, InvalidValue indicates a bug or corrupt data.

TheBlueMatt · 2024-03-27T18:54:13Z

lightning/src/ln/msgs.rs

@@ -91,6 +91,11 @@ pub enum DecodeError {
 	Io(io::ErrorKind),
 	/// The message included zlib-compressed values, which we don't support.
 	UnsupportedCompression,
+	/// Value is validly encoded but is dangerous to use.
+	/// This is used for things like [`crate::ln::channelmanager::ChannelManager`] deserialization
+	/// where we want to ensure that we don't use a [`crate::ln::channelmanager::ChannelManager`]


We should be a bit more forceful here, its not just that its "in the past", but that it is corrupt or out of sync with the ChannelMonitors in such a way that indicates that the lightning node being operated has a critical bug in its storage layer and using this lightning node is critically unsafe.

Update docs

lightning/src/ln/msgs.rs

TheBlueMatt · 2024-03-27T20:35:56Z

lightning/src/ln/channelmanager.rs

 					}
 				} else {
 					// We shouldn't have persisted (or read) any unfunded channel types so none should have been
 					// created in this `channel_by_id` map.
 					debug_assert!(false);
-					return Err(DecodeError::InvalidValue);
+					return Err(DecodeError::DangerousValue);


This is really invalid/corrupt, not really "something is horribly wrong"

okay switched back

TheBlueMatt · 2024-03-27T20:36:38Z

Otherwise LGTM

TheBlueMatt · 2024-03-27T22:46:04Z

Oops, looks like the fuzzer doesn't build because of the changes here.

benthecarman · 2024-03-27T22:51:18Z

woops fixed

codecov-commenter · 2024-03-27T23:28:44Z

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 89.43%. Comparing base (5e41425) to head (dc856e3).
Report is 25 commits behind head on main.

❗ Current head dc856e3 differs from pull request most recent head 1a8ae9d. Consider uploading reports for the commit 1a8ae9d to get more accurate results

Files	Patch %	Lines
lightning/src/ln/peer_handler.rs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2974      +/-   ##
==========================================
+ Coverage   89.26%   89.43%   +0.17%     
==========================================
  Files         118      117       -1     
  Lines       96534    95191    -1343     
  Branches    96534    95191    -1343     
==========================================
- Hits        86167    85138    -1029     
+ Misses       7872     7824      -48     
+ Partials     2495     2229     -266

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz · 2024-03-28T13:17:42Z

I don't think this is generally the approach we should take, though maybe it is fine for the short term. I'd much rather decouple deserialization from sanity checks using some intermediary representation or enum that has state transitions. Possibly folding it into something like this: #2819 (comment). But that's a larger refactor.

TheBlueMatt · 2024-03-28T13:57:20Z

I feel like #2819 (comment) is unrelated. While I'm definitely a fan of cleaning up the code by separating concerns, I'm not sure why the public API has to expose that concept. Ignoring #2819 (comment) that would just mean additional code on the client's side for no reason.

tnull

Change looks good by itself, mod two nits.

Also not 100% sure if introducing a custom serialization error variant for this single use case is the way to go. So I'll give @jkczyz the chance to respond.

lightning/src/ln/msgs.rs

lightning/src/ln/channelmanager.rs

jkczyz · 2024-03-28T15:40:53Z

Also not 100% sure if introducing a custom serialization error variant for this single use case is the way to go. So I'll give @jkczyz the chance to respond.

That's my feeling too, but I'm ok with having it for now until we do a larger refactor.

jkczyz · 2024-03-28T15:49:37Z

I feel like #2819 (comment) is unrelated. While I'm definitely a fan of cleaning up the code by separating concerns, I'm not sure why the public API has to expose that concept. Ignoring #2819 (comment) that would just mean additional code on the client's side for no reason.

I'm not necessarily saying the transition from deserialized to uninitialized needs to be exposed as part of the API, but the error type in the API would need to differentiate between a DecodeError a DangerousState, likely by using an enum with different variants. Similar to what we do when parsing offer messages.

benthecarman · 2024-03-28T16:06:38Z

fixed @tnull's comments

benthecarman · 2024-03-28T16:07:59Z

I agree with @jkczyz this isn't the cleanest way to do this, but would like to add error handling for this and there really isn't a way to correctly match on this specific error that should be handled differently than a normal decode error

This would help distinguish different types of errors when deserialzing a channel manager. InvalidValue was used previously but this could be because it is an old serialization format, whereas DangerousValue is a lot more clear on why the deserialization failed.

TheBlueMatt

Gonna go ahead and land, it seems like we're all on the same page that a more general API overhaul for ChannelManager deser would be nice, but this is at least useful information for users for now.

v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping" API Updates =========== * To reduce risk of force-closures and improve HTLC reliability the default dust exposure limit has been increased to `MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing channels might want to consider using `ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045). * `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to remove from memory `ChannelMonitor`s that have been fully resolved on-chain and are now not needed. It uses the new `Persist::archive_persisted_channel` to inform the storage layer that such a monitor should be archived (lightningdevkit#2964). * An `OutputSweeper` is now provided which will automatically sweep `SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825). * After initiating an outbound channel, a peer disconnection no longer results in immediate channel closure. Rather, if the peer is reconnected before the channel times out LDK will automatically retry opening it (lightningdevkit#2725). * `PaymentPurpose` now has separate variants for BOLT12 payments, which include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970). * `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442). * `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858). * The `hashbrown` dependency has been upgraded and the use of `ahash` as the no-std hash table hash function has been removed. As a consequence, LDK's `Hash{Map,Set}`s no longer feature several constructors when LDK is built with no-std; see the `util::hash_tables` module instead. On platforms that `getrandom` supports, setting the `possiblyrandom/getrandom` feature flag will ensure hash tables are resistant to HashDoS attacks, though the `possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891). * `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816). * `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script` where available as well as new proprietary data which can be used to re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004). * `OutPoint::to_channel_id` has been removed in favor of `ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a different `ChannelId` derivation scheme (lightningdevkit#2797). * `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and `peer_by_node_id`, which provide more details (lightningdevkit#2905). * `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909). * `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847). * `ClosureReason::HTLCsTimedOut` has been separated out from `ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887). * `ClosureReason::CooperativeClosure` is now split into `{Counterparty,Locally}Initiated` variants (lightningdevkit#2863). * `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872). * `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924). * Channel init messages have been refactored towards V2 channels (lightningdevkit#2871). * `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873). * `util::scid_utils` is now public, with some trivial utilities to examine short channel ids (lightningdevkit#2694). * `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870). * Bounds in `lightning-background-processor` were simplified by using `AChannelManager` (lightningdevkit#2963). * The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976). * `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918). * `NodeId::from_slice` is now provided (lightningdevkit#2942). * `ChannelManager` deserialization may now fail with `DangerousValue` when LDK's persistence API was violated (lightningdevkit#2974). Bug Fixes ========= * Excess fees on counterparty commitment transactions are now included in the dust exposure calculation. This lines behavior up with some cases where transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045). * `Future`s used as an `std::...::Future` could grow in size unbounded if it was never woken. For those not using async persistence and using the async `lightning-background-processor`, this could cause a memory leak in the `ChainMonitor` (lightningdevkit#2894). * Inbound channel requests that fail in `ChannelManager::accept_inbound_channel` would previously have stalled from the peer's perspective as no `error` message was sent (lightningdevkit#2953). * Blinded path construction has been tuned to select paths more likely to succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912). * After a reorg, `lightning-transaction-sync` could have failed to follow a transaction that LDK needed information about (lightningdevkit#2946). * `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when paying with blinded paths (lightningdevkit#2975). * `Event::ChannelClosed` is now properly generated and peers are properly notified for all channels that as a part of a batch channel open fail to be funded (lightningdevkit#3029). * In cases where user event processing is substantially delayed such that we complete multiple round-trips with our peers before a `PaymentSent` event is handled and then restart without persisting the `ChannelManager` after having persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to deserialize the `ChannelManager` (lightningdevkit#3021). * If a peer has relatively high latency, `PeerManager` may have failed to establish a connection (lightningdevkit#2993). * `ChannelUpdate` messages broadcasted for our own channel closures are now slightly more robust (lightningdevkit#2731). * Deserializing malformed BOLT11 invoices may have resulted in an integer overflow panic in debug builds (lightningdevkit#3032). * In exceedingly rare cases (no cases of this are known), LDK may have created an invalid serialization for a `ChannelManager` (lightningdevkit#2998). * Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881). * Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033). Node Compatibility ================== * LDK's blinded paths were inconsistent with other implementations in several ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945). * LDK's messaging blinded paths now support the latest features which some nodes may begin relying on soon (lightningdevkit#2961). * LDK's BOLT12 structs have been updated to support some last-minute changes to the spec (lightningdevkit#3017, lightningdevkit#3018). * CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK by default does not set it for those not using a `P2PGossipSync` (e.g. those using RGS). This change was reverted in CLN v24.02.2 however for now LDK always sets the `gossip_queries` feature. This change is expected to be reverted in a future LDK release (lightningdevkit#2959). Security ======== 0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII characters. * BOLT11 invoices with non-ASCII characters in the human-readable-part may cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all BOLT11 invoices containing non-ASCII characters are invalid. In total, this release features 150 files changed, 19307 insertions, 6306 deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order: * Arik Sosman * Duncan Dean * Elias Rohrer * Evan Feenstra * Jeffrey Czyz * Keyue Bao * Matt Corallo * Orbital * Sergi Delgado Segura * Valentine Wallace * Willem Van Lint * Wilmer Paulino * benthecarman * jbesraa * olegkubrakov * optout * shaavan

benthecarman force-pushed the dang-value branch from 414e543 to db33ab4 Compare March 27, 2024 18:55

TheBlueMatt reviewed Mar 27, 2024

View reviewed changes

benthecarman force-pushed the dang-value branch 2 times, most recently from f4225f8 to f4e4bd1 Compare March 27, 2024 19:11

TheBlueMatt reviewed Mar 27, 2024

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

benthecarman force-pushed the dang-value branch from f4e4bd1 to 307e4c2 Compare March 27, 2024 19:15

TheBlueMatt reviewed Mar 27, 2024

View reviewed changes

benthecarman force-pushed the dang-value branch from 307e4c2 to a80a298 Compare March 27, 2024 20:53

TheBlueMatt previously approved these changes Mar 27, 2024

View reviewed changes

benthecarman dismissed TheBlueMatt’s stale review via dc856e3 March 27, 2024 22:51

benthecarman force-pushed the dang-value branch from a80a298 to dc856e3 Compare March 27, 2024 22:51

TheBlueMatt previously approved these changes Mar 27, 2024

View reviewed changes

tnull reviewed Mar 28, 2024

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Show resolved Hide resolved

benthecarman dismissed TheBlueMatt’s stale review via 1a8ae9d March 28, 2024 16:06

benthecarman force-pushed the dang-value branch from dc856e3 to 1a8ae9d Compare March 28, 2024 16:06

benthecarman force-pushed the dang-value branch from 1a8ae9d to 712d97d Compare March 28, 2024 17:30

TheBlueMatt approved these changes Mar 28, 2024

View reviewed changes

TheBlueMatt merged commit 6159325 into lightningdevkit:main Mar 28, 2024
13 of 16 checks passed

benthecarman deleted the dang-value branch March 28, 2024 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DecodeError::DangerousValue for decoding invalid channel managers #2974

Add DecodeError::DangerousValue for decoding invalid channel managers #2974

benthecarman commented Mar 27, 2024

TheBlueMatt left a comment

TheBlueMatt Mar 27, 2024

benthecarman Mar 27, 2024

TheBlueMatt Mar 27, 2024

benthecarman Mar 27, 2024

TheBlueMatt commented Mar 27, 2024

TheBlueMatt commented Mar 27, 2024

benthecarman commented Mar 27, 2024

codecov-commenter commented Mar 27, 2024 •

edited

Loading

jkczyz commented Mar 28, 2024

TheBlueMatt commented Mar 28, 2024

tnull left a comment

jkczyz commented Mar 28, 2024

jkczyz commented Mar 28, 2024

benthecarman commented Mar 28, 2024

benthecarman commented Mar 28, 2024

TheBlueMatt left a comment

Add DecodeError::DangerousValue for decoding invalid channel managers #2974

Add DecodeError::DangerousValue for decoding invalid channel managers #2974

Conversation

benthecarman commented Mar 27, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Mar 27, 2024

Choose a reason for hiding this comment

benthecarman Mar 27, 2024

Choose a reason for hiding this comment

TheBlueMatt Mar 27, 2024

Choose a reason for hiding this comment

benthecarman Mar 27, 2024

Choose a reason for hiding this comment

TheBlueMatt commented Mar 27, 2024

TheBlueMatt commented Mar 27, 2024

benthecarman commented Mar 27, 2024

codecov-commenter commented Mar 27, 2024 • edited Loading

Codecov Report

jkczyz commented Mar 28, 2024

TheBlueMatt commented Mar 28, 2024

tnull left a comment

Choose a reason for hiding this comment

jkczyz commented Mar 28, 2024

jkczyz commented Mar 28, 2024

benthecarman commented Mar 28, 2024

benthecarman commented Mar 28, 2024

TheBlueMatt left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 27, 2024 •

edited

Loading