Skip to content
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.

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

Authenticate InvoiceError messages #3192

Merged
merged 19 commits into from
Aug 14, 2024

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Jul 18, 2024

Before abandoning a payment when receiving an InvoiceError, verify that the PaymentId included in the OffersContext with the included HMAC. This prevents a malicious actor sending an InvoiceError with a known payment id from abandoning our payment.

Based on #3202

Fixes #3166
Fixes #3167

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 85.86572% with 40 lines in your changes missing coverage. Please review.

Project coverage is 89.74%. Comparing base (5ab40b2) to head (075a2e3).
Report is 20 commits behind head on main.

Files Patch % Lines
lightning/src/events/mod.rs 0.00% 22 Missing ⚠️
lightning/src/ln/channelmanager.rs 82.50% 4 Missing and 3 partials ⚠️
lightning/src/ln/offers_tests.rs 95.65% 5 Missing ⚠️
lightning/src/ln/outbound_payment.rs 94.23% 0 Missing and 3 partials ⚠️
lightning/src/ln/functional_test_utils.rs 0.00% 0 Missing and 2 partials ⚠️
lightning/src/util/ser.rs 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3192      +/-   ##
==========================================
- Coverage   89.75%   89.74%   -0.02%     
==========================================
  Files         123      123              
  Lines      102158   102330     +172     
  Branches   102158   102330     +172     
==========================================
+ Hits        91695    91833     +138     
- Misses       7773     7802      +29     
- Partials     2690     2695       +5     

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

@jkczyz jkczyz force-pushed the 2024-07-invoice-error-auth branch 2 times, most recently from fd7da7f to cde9852 Compare July 30, 2024 21:23
@jkczyz
Copy link
Contributor Author

jkczyz commented Jul 30, 2024

@TheBlueMatt @valentinewallace Need your opinions on Event::InvoiceRequestFailed. A payment in state AwaitingInvoice, may be abandoned for the following reasons:

  • Timeout waiting on the invoice
  • Explicitly abandoned by the user
  • Invoice contains unknown required features
  • Recipient sends back InvoiceError

Currently, the first two generate Event::InvoiceRequestFailed.

In this PR, we transition to state InvoiceReceived before checking the features, so that it will instead generate an Event::PaymentFailed. Note, we can only use Event::PaymentFailed when we have a PaymentHash.

Abandoning for the InvoiceError case requires us to give a PaymentFailureReason. But the reason in practice is ignored by PendingOutboundPayments::abandon_payment when in state AwaitingInvoice.

Do we care that reason is ignored and never finds it way to the user in Event::InvoiceRequestFailed? Should we add a reason to that event? And if so, should it be PaymentFailureReason or a dedicated type? If a dedicated type, I'm not sure how PendingOutboundPayments::abandon_payment would work when called for a payment in the wrong state. But re-using PaymentFailureReason instead means that some reasons aren't applicable for either event.

Or maybe we just add an Option<InvoiceError> to Event::InvoiceRequestFailed? Though when None that doesn't differentiate timeout from user-abandoned, which is currently the case.

Thoughts?

@jkczyz jkczyz force-pushed the 2024-07-invoice-error-auth branch from d68a1e0 to d6b6ae6 Compare August 2, 2024 15:00
@jkczyz jkczyz marked this pull request as ready for review August 2, 2024 15:01
@TheBlueMatt
Copy link
Collaborator

Need your opinions on Event::InvoiceRequestFailed

I mean the only reason for the distinction between the two events is that we need a PaymentHash for the payment failed one, so I don't feel super strongly about making sure there's too much of a logical distinction between the two - we expect users to largely handle them the same.

Do we care that reason is ignored and never finds it way to the user in Event::InvoiceRequestFailed? Should we add a reason to that event? And if so, should it be PaymentFailureReason or a dedicated type? If a dedicated type, I'm not sure how PendingOutboundPayments::abandon_payment would work when called for a payment in the wrong state. But re-using PaymentFailureReason instead means that some reasons aren't applicable for either event.

I'm starting to think we should make the hash optional in PaymentFailed and stick with that, but absent that, it seems like we could give users the same error type so that they could delegate to some common method to handle both cases? I'm not sure its that critical, though, honestly - there's not gonna be much in the way of programatic differences in how these events are handled, its really just them giving some kind of string to the operator/log. As far as the UX cares, the payment failed, and the reason is some technical garbage that isn't really useful to a human.

@jkczyz
Copy link
Contributor Author

jkczyz commented Aug 6, 2024

I'm starting to think we should make the hash optional in PaymentFailed and stick with that, but absent that, it seems like we could give users the same error type so that they could delegate to some common method to handle both cases?

Ended up making the payment hash optional in PaymentFailed and removing InvoiceRequestFailed. I agree that this approach is best.

@@ -882,6 +885,7 @@ pub enum Event {
payment_hash: PaymentHash,
/// The reason the payment failed. This is only `None` for events generated or serialized
/// by versions prior to 0.0.115.
/// TODO: Will downgrading cause this to be None for UnknownRequiredFeatures?
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, it'll cause reading the event to fail with DecodeError::InvalidValue :(. We should have done impl_writeable_tlv_based_enum_upgradable for PaymentFailureReason (we still should for the next added reason, but it won't help downgrades to 123).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and changed this to impl_writeable_tlv_based_enum_upgradable.

@@ -1555,7 +1557,7 @@ impl Writeable for Event {
write_tlv_fields!(writer, {
(0, payment_id, required),
(1, reason, option),
(2, payment_hash, required),
(2, payment_hash, option),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty meh on breaking downgrades for payment failures here (causing DecodeError::InvalidValue). Instead, maybe we write 0s (and drop all-0s on the read end, and also then reject sending payments with all-0 payment hashes in ChannelManager/outbound_payments, or write some other "no payment_hash" flag) and document it in the release notes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@valentinewallace
Copy link
Contributor

Needs rebase

@jkczyz jkczyz force-pushed the 2024-07-invoice-error-auth branch from d2df48a to 0552a46 Compare August 7, 2024 22:30
@jkczyz
Copy link
Contributor Author

jkczyz commented Aug 7, 2024

Feedback addressed.

Needs rebase

@TheBlueMatt Will rebase whenever you are good.

@TheBlueMatt
Copy link
Collaborator

Feel free to rebase!

@jkczyz jkczyz force-pushed the 2024-07-invoice-error-auth branch from 0552a46 to be5d04f Compare August 8, 2024 15:45
Comment on lines +532 to +547
UnknownRequiredFeatures,
/// A [`Bolt12Invoice`] was not received in a reasonable amount of time.
InvoiceRequestExpired,
/// An [`InvoiceRequest`] for the payment was rejected by the recipient.
///
/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
InvoiceRequestRejected,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty meh on the idea that users might upgrade, use bolt12, and suddenly be unable to downgrade even one version. Not entirely sure what to do about it, though - we could delay this part of the patch for a release, or we could write the PaymentFailureReason twice, once with these versions in a new TLV and once without, mapping them to UnexpectedError or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and wrote the reason twice. UnexpectedError would be a misleading, IMO, though. Used None instead, but let me know if there was some other alternative you were thinking about.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I suppose None is fine, but its a bit annoying to suddenly have no reason when the docs on 123 say that its "only None for events generated by versions prior to 0.0.115". I guess for InvoiceRequestRejected we could easily just say RecipientRejected, but InvoiceRequestExpired is tougher...maybe RetriesExhausted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... UnknownRequiredFeatures also needs to map to something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

RecipientRejected, I assume - they would have rejected a payment that has unknown features in the onion so...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it is us -- the payer -- rejecting an invoice for an outbound payment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea but the principle is the same - we have some issue causing us to be unable to talk to the intended recipient so cannot pay. RecipientRejected makes sense imo as it's also the most final - "don't bother trying this again cause they already said they won't accept it"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Updated now.

@@ -36,6 +36,9 @@ const DERIVED_METADATA_AND_KEYS_HMAC_INPUT: &[u8; 16] = &[2; 16];
const WITHOUT_ENCRYPTED_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[3; 16];
const WITH_ENCRYPTED_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[4; 16];

// HMAC input for a `PaymentId`. The HMAC is used in `OffersContext::OutboundPayment`.
const PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[5; 16];
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you help me understand why we need this input in addition to IV_BYTES in hmac_for_payment_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May not be necessary actually. Though, if in the future we hmac some other type of data, then we may want to differentiate it with another input. Happy to drop it if you and @TheBlueMatt don't think it is necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No reason not to have it IMO 🤷

write_tlv_fields!(writer, {
(0, payment_id, required),
(1, reason, option),
(2, payment_hash, required),
(3, invoice_received, required),
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like if someone downgraded after writing a zeroed out payment hash, then upgraded again, their event would then contain a payment_hash: Some(PaymentHash([0; 32])), IIUC. I think we could prevent that by omitting this invoice_received field and explicitly detecting a zeroed out payment hash instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, not sure how concerning that is but went ahead with your alternative.

@TheBlueMatt I'm not checking in ChannelManager for such a payment hash when sending. Seems in such a rare scenario it would be better to check the reason to determine whether the 0-payment_hash should be included in the event when reading. Maybe there some obscure upgrade-downgrade-upgrade scenario I'm not thinking about. But I'm not sure if it's worth adding some complex checking if the worse case is dropping the 0-payment_hash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we keep the old code writing the extra flag but then drop 0 payment hash if its missing? While people shouldn't be paying 0 payment hashes, its a bit weird that the payment hash will dissapear on you if you try to pay one because your counterparty made one up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated accordingly, IIUC.

@jkczyz jkczyz force-pushed the 2024-07-invoice-error-auth branch from be5d04f to 98dadb2 Compare August 9, 2024 16:09
Comment on lines +532 to +547
UnknownRequiredFeatures,
/// A [`Bolt12Invoice`] was not received in a reasonable amount of time.
InvoiceRequestExpired,
/// An [`InvoiceRequest`] for the payment was rejected by the recipient.
///
/// [`InvoiceRequest`]: crate::offers::invoice_request::InvoiceRequest
InvoiceRequestRejected,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and wrote the reason twice. UnexpectedError would be a misleading, IMO, though. Used None instead, but let me know if there was some other alternative you were thinking about.

write_tlv_fields!(writer, {
(0, payment_id, required),
(1, reason, option),
(2, payment_hash, required),
(3, invoice_received, required),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, not sure how concerning that is but went ahead with your alternative.

@TheBlueMatt I'm not checking in ChannelManager for such a payment hash when sending. Seems in such a rare scenario it would be better to check the reason to determine whether the 0-payment_hash should be included in the event when reading. Maybe there some obscure upgrade-downgrade-upgrade scenario I'm not thinking about. But I'm not sure if it's worth adding some complex checking if the worse case is dropping the 0-payment_hash.

Comment on lines +9027 to +9028
let hmac = signer::hmac_for_payment_id(payment_id, nonce, expanded_key);
let context = OffersContext::OutboundPayment { payment_id, nonce, hmac: Some(hmac) };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheBlueMatt This nonce is used both for the hmac and in the payer metadata. Is this ok?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's okay, they both use different IVs (and different algorithms) so it should be okay.

@@ -36,6 +36,9 @@ const DERIVED_METADATA_AND_KEYS_HMAC_INPUT: &[u8; 16] = &[2; 16];
const WITHOUT_ENCRYPTED_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[3; 16];
const WITH_ENCRYPTED_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[4; 16];

// HMAC input for a `PaymentId`. The HMAC is used in `OffersContext::OutboundPayment`.
const PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[5; 16];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

May not be necessary actually. Though, if in the future we hmac some other type of data, then we may want to differentiate it with another input. Happy to drop it if you and @TheBlueMatt don't think it is necessary.

@jkczyz jkczyz force-pushed the 2024-07-invoice-error-auth branch from 98dadb2 to 739832e Compare August 9, 2024 16:12
/// The reason the payment failed. This is only `None` for events generated or serialized
/// by versions prior to 0.0.115.
/// by versions prior to 0.0.115, when deserializing an `Event::InvoiceRequestFailed`, which
/// was removed in 0.0.124, or when downgrading to 0.0.124 or later with a reason that was
Copy link
Collaborator

Choose a reason for hiding this comment

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

technically with the current implementation if you downgrade to pre-0.0.124 with a reason that was added after you also get None.

Suggested change
/// was removed in 0.0.124, or when downgrading to 0.0.124 or later with a reason that was
/// or when downgrading with a reason that was

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, removed "to 0.0.124 or later".

However, the first part, "which was removed in 0.0.124", is a non-restrictive clause; that is, the sentence has the same meaning with or without it. It's simply used to note when the event was removed from the codebase. Updated to use a parenthetical to make it clearer.

write_tlv_fields!(writer, {
(0, payment_id, required),
(1, reason, option),
(2, payment_hash, required),
(3, invoice_received, required),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we keep the old code writing the extra flag but then drop 0 payment hash if its missing? While people shouldn't be paying 0 payment hashes, its a bit weird that the payment hash will dissapear on you if you try to pay one because your counterparty made one up.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM

(2, payment_hash, required),
(3, invoice_received, option),
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't matter much, but it would make more sense to write this as required since we always write it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option let's us handle the scenario described here: #3192 (comment). If it's required, then we would fail when re-upgrading.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should def make it Optional on read, but on writing it's fine to make it required since we always write it (there's no difference between required and an option where we always write).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Done.

@jkczyz jkczyz force-pushed the 2024-07-invoice-error-auth branch 2 times, most recently from 74c0ec3 to e09b191 Compare August 14, 2024 04:33
@jkczyz
Copy link
Contributor Author

jkczyz commented Aug 14, 2024

Squashed

@TheBlueMatt
Copy link
Collaborator

Ugh, needs a somewhat trivial rebase cause features.rs moved (and I guess git is dumb :( )

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Okay, one more nit to just fix (+ squash) on rebase, otherwise LGTM!

payment_id: payment_id.0.unwrap(),
payment_hash: None,
reason: None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we set this to Some(InvoiceRequestExpired)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@valentinewallace
Copy link
Contributor

LGTM after rebase

When receiving an InvoiceError in response to an InvoiceRequest, the
corresponding payment should be abandoned. Add functions for
constructing and verifying an HMAC over a Payment ID to allow for this.
An HMAC needs to be included in OffersContext::OutboundPayment to
authenticate the included PaymentId. Implement Readable and Writeable to
allow for this.
When receiving an InvoiceError in response to an InvoiceRequest, the
corresponding payment should be abandoned. Add an HMAC to
OffersContext::OutboundPayment such that the payment ID can be
authenticated prior to abandoning the payment.
Before abandoning a payment when receiving an InvoiceError, verify that
the PaymentId included in the OffersContext with the included HMAC. This
prevents a malicious actor sending an InvoiceError with a known payment
id from abandoning our payment.
When making an outbound BOLT12 payment, multiple invoices may be
received for the same payment id. Instead of abandoning the payment when
a duplicate invoice received, simply ignore it without responding with
an InvoiceError. This prevents abandoning in-progress payments and
sending unnecessary onion messages.
A BOLT12 payment may be abandoned when handling the invoice or when
receiving an InvoiceError message. When abandoning the payment, don't
use UserAbandoned as the reason since that is meant for when the user
calls ChannelManager::abandon_payment.
Refunds are typically communicated via QR code, where a smaller size is
desirable. Make the HMAC in OutboundPayment data optional such that it
is elided from blinded paths used in refunds. This prevents abandoning
refunds if the reader sends an invoice_error instead of an invoice
message. However, this use case isn't necessary as the corresponding
outbound payment will either timeout when the refund expires or can be
explicitly abandoned by the creator.
When handling a BOLT12 invoice, and invoice error is sent if the invoice
contains unknown required features. However, since the payment is still
in state AwaitingInvoice, abandoning it results in losing the reason
since an InvoiceRequestFailed event would be generated. Move the check
to PendingOutboundPayments such that the payment is first moved to state
InvoiceReceived so that a PaymentFailed event is generated instead.
When testing Bolt12Invoice unknown require feature handling, a large
feature bit can cause SendError::TooBigPacket when creating an onion
message. Use a smaller feature bit for UnknownFeature, which also has
the added benefit of reducing test output.
In order to test handling of unknown required features in a
Bolt12Invoice, add a test-only function to allow setting arbitrary
feature bits.
When abandoning a BOLT12 payment before a Bolt12Invoice is received, an
Event::InvoiceRequestFailed is generated and the abandonment reason is
lost. Make payment_hash optional in Event::PaymentFailed so that
Event::InvoiceRequestFailed can be removed in favor of it.
Now that Event::PaymentFailed has an option payment_hash, it can be used
in replace of Event::InvoiceRequestFailed. This allows for including a
reason when abandoning a payment before an invoice is received.
Now that Event::PaymentFailed is generated when an InvoiceRequest times
out, define a new PaymentFailureReason for this situation.
Instead of re-using PaymentFailureReason::RecipientRejected, define a
new InvoiceRequestRejected variant for when an InvoiceError is received
instead of a Bolt12Invoice. This allows user to differentiate the cause
of the failure.
This allows downgrading to version 0.0.124 or later and using None for a
PaymentFailureReason that was added after.
The PaymentFailureReason variants for invoice request failures will
cause downgrades to break. Instead, use a new TLV for the reason and
continue to write the old TLV, only use None for the new reasons.
@jkczyz
Copy link
Contributor Author

jkczyz commented Aug 14, 2024

Rebased

@TheBlueMatt TheBlueMatt merged commit 33e6995 into lightningdevkit:main Aug 14, 2024
18 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants