-
Notifications
You must be signed in to change notification settings - Fork 367
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 use of offer blinded paths #3139
Authenticate use of offer blinded paths #3139
Conversation
cd9a904
to
e0fcca0
Compare
lightning/src/ln/channelmanager.rs
Outdated
OffersMessage::Invoice(invoice) => { | ||
let expected_payment_id = match context { | ||
OffersContext::OutboundPayment { payment_id } => payment_id, | ||
_ => return ResponseInstruction::NoResponse, | ||
}; | ||
|
||
let result = match invoice.verify(expanded_key, secp_ctx) { | ||
Ok(payment_id) => { | ||
if payment_id != expected_payment_id { | ||
return ResponseInstruction::NoResponse; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TheBlueMatt If we receive an invoice for a refund without blinded paths, can we actually authenticate it? With this change, we'd see OffersContext::Unknown
and so would return ResponseInstruction::NoResponse
.
But if we skip the check in that case, someone could send us an invoice for a refund using an unrelated blinded path created before this PR (i.e., one where we'd get OffersContext::Unknown
here). While this wouldn't de-anonymize us -- the refund contains our pubkey -- it could tie us to an old offer (or refund) that uses a transient pubkey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is also the case for an offer without blinded paths. Although in that case we can authenticate, but it could tie us to an older offer that uses a transient pubkey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO if we break refunds issued by old LDK that's okay at this point. Offers less so.
e0fcca0
to
ea192d7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3139 +/- ##
==========================================
+ Coverage 89.71% 89.78% +0.06%
==========================================
Files 121 122 +1
Lines 101074 101493 +419
Branches 101074 101493 +419
==========================================
+ Hits 90680 91126 +446
+ Misses 7701 7684 -17
+ Partials 2693 2683 -10 ☔ View full report in Codecov by Sentry. |
fdb819e
to
5cfad02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM, I think.
lightning/src/offers/signer.rs
Outdated
@@ -74,6 +80,7 @@ impl Metadata { | |||
// derived, as wouldn't be the case if a Metadata::Bytes with length PaymentId::LENGTH + | |||
// Nonce::LENGTH had been set explicitly. | |||
Metadata::Bytes(bytes) => bytes.len() == PaymentId::LENGTH + Nonce::LENGTH, | |||
Metadata::Nonce(_) => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not switch the payer keys to being derived in the same way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, yes. It's a little more complicated as the context would also need the Nonce
and we'd need another variant here that wraps a 48-byte array (so it could be used with as_ref
) containing the encrypted payment id and nonce. It would avoid increasing the size of a Refund
with a path. Though for an InvoiceRequest
the size savings isn't necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little more complicated as the context would also need the Nonce
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly for the reasons given in the rest of the sentence. :) Though, originally I thought the whole change would be more complicated until I switched to the additional enum variant approach.
Another consideration is we actually do need to set the payer metadata to something for both the InvoiceRequest
and Refund
cases according to the spec.
The writer:
- MUST set `invreq_metadata` to an unpredictable series of bytes.
The reader:
- MUST fail the request if `invreq_payer_id` or `invreq_metadata` are not present.
For the case where we include blinded paths, we could set only the 16-byte Nonce
instead? Or maybe the first 16 bytes of the encrypted PaymentId
to avoid allocating another Vec
? We wouldn't need it for anything ourself, as both the PaymentId
and Nonce
would come back in the BlindedPath
. But the spec wants it because:
- if `offer_node_id` is present, and `invreq_metadata` is identical to a previous `invoice_request`:
- MAY simply reply with the previous invoice.
And:
Note: the `invreq_metadata` is numbered 0 (not in the
80-159 range for other invreq fields) as this is the first
TLV element, which ensures payer-provided entropy is used in hashing
for [Signature Calculation](#signature-calculation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I made this change except I've left the payer_metadata
as is for now. When trying to change it to only include the nonce, ln::offers_tests::pays_bolt12_invoice_asynchronously
fails. Presumably because the code is trying to re-verify the invoice using payer_metadata
. I'll need to put some thought in if we could do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was confused about the rest of the sentence :)
Ok, I made this change except I've left the payer_metadata as is for now.
Doesn't this break our protection? We put the nonce in the blinded path so that no one but us knows it but then we copy it into the payer_metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this situation because the payment_id
is encrypted in the payer_metadata
. So an attacker wouldn't be able to include the unencrypted payment_id
in any blinded path that they composed unless they learned of it by some other means.
I think the solution for asynchronous handling is to either include the OfferContext
in Event::InvoiceReceived
or drop the verification check from ChannelManager::send_payment_for_bolt12_invoice
. Then we could remove the encrypted payment_id
from the payer_metadata
to save those 32 bytes, which is more important for a Refund
. We'd still need to include some random data as per the spec, so we'd leave the nonce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this situation because the
payment_id
is encrypted in thepayer_metadata
. So an attacker wouldn't be able to include the unencryptedpayment_id
in any blinded path that they composed unless they learned of it by some other means.
Discussed offline. Ended up dropping the nonce from the payer metadata when including blinded paths.
I think the solution for asynchronous handling is to either include the
OfferContext
inEvent::InvoiceReceived
or drop the verification check fromChannelManager::send_payment_for_bolt12_invoice
. Then we could remove the encryptedpayment_id
from thepayer_metadata
to save those 32 bytes, which is more important for aRefund
. We'd still need to include some random data as per the spec, so we'd leave the nonce.
Included the OffersContext
in Event::InvoiceReceived
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM mod outstanding feedback!
5cfad02
to
46bee8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to address Val's comments.
lightning/src/offers/signer.rs
Outdated
@@ -74,6 +80,7 @@ impl Metadata { | |||
// derived, as wouldn't be the case if a Metadata::Bytes with length PaymentId::LENGTH + | |||
// Nonce::LENGTH had been set explicitly. | |||
Metadata::Bytes(bytes) => bytes.len() == PaymentId::LENGTH + Nonce::LENGTH, | |||
Metadata::Nonce(_) => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I made this change except I've left the payer_metadata
as is for now. When trying to change it to only include the nonce, ln::offers_tests::pays_bolt12_invoice_asynchronously
fails. Presumably because the code is trying to re-verify the invoice using payer_metadata
. I'll need to put some thought in if we could do this.
c61e16d
to
9f7fd0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Great To Me modulo some nits! 🚀
Unknown {}, | ||
/// Represents an outbound BOLT12 payment context. | ||
/// Context used by a [`BlindedPath`] within an [`Offer`] for sending an [`InvoiceRequest`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we can improve this comment a bit to make the purpose of this more clear.
Maybe something like:
/// Context used by a [`BlindedPath`] within an [`Offer`] for sending an [`InvoiceRequest`]. | |
/// Context used by a [`BlindedPath`] within an [`Offer`] to be received with an [`InvoiceRequest`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good call. I was trying to cram too much in one sentence. Re-worded into two sentences where one describes which messages use the variant in their blinded paths and another on which messages are intended to be received with the context.
/// | ||
/// [`Bolt12Invoice`]: crate::offers::invoice::Bolt12Invoice | ||
/// [`InvoiceError`]: crate::offers::invoice_error::InvoiceError | ||
InboundPayment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointer for myself:
Looks like this variant is unused in this PR but will be used by #3163 when we start adding reply_path
with the sent BOLT12Invoice
@@ -667,9 +665,9 @@ impl Offer { | |||
|
|||
#[cfg(async_payments)] | |||
pub(super) fn verify<T: secp256k1::Signing>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since we are touching this function, shall we also add it's documentation?
/// Context used by a [`BlindedPath`] for replying to a [`Bolt12Invoice`] with an | ||
/// [`InvoiceError`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we can reword it a bit for better clarity!
/// Context used by a [`BlindedPath`] for replying to a [`Bolt12Invoice`] with an | |
/// [`InvoiceError`]. | |
/// Context used by a [`BlindedPath`] as a reply path for a [`Bolt12Invoice`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also updated this to align with the other variants, for consistency.
Chatted for a while offline, it looks like we need to remove the nonce from the metadata in |
Yup, it was removed directly from |
$self.offer.signing_pubkey = Some(keys.public_key()); | ||
match keys { | ||
Some(keys) => $self.offer.signing_pubkey = Some(keys.public_key()), | ||
None => $self.offer.metadata = Some(derived_metadata), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to elide the nonce
even when we're using non-random keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... None
here implies not only did we not derive keys but also that we did not include any blinded paths. So we need the nonce in the metadata in order to authenticate as we'll get OffersContext::Unknown
in the handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay, sorry, missed the "no blinded paths" implication, can you comment that? Also is that enforced by the builder? Should it be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments throughout this method explaining. Currently, the builder requires giving a node id / signing pubkey upon construction. Then here when building the offer we override it if any blinded paths are set. It happens when Metadata
transitions from DerivedSigningPubkey
to Derived
when calling Metadata::without_keys
above, and thus Metadata::derive_from
returns None
for the keys here.
let error = Bolt12SemanticError::InvalidMetadata; | ||
return responder.respond(OffersMessage::InvoiceError(error.into())); | ||
}, | ||
Err(()) => return ResponseInstruction::NoResponse, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I don't think there's anything wrong with ignoring the BOLTs here - as long as we handle "unsolicited invoice_request that is for an offer we didnt built" and "invoice_request that we received over the wrong path" the same we're fine. Not sure it matters much if we think this case isn't going to be a common one that we need to worry about making sure we get good errors back, but if we think it'd be helpful we should just do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this will only be triggered for older offers. When we add support for producing offers without blinded paths (i.e. with a public node id), it will also be hit. So at that point someone could de-anonymize the older offer by sending an invoice request for it to the node id given in the newer offer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm? Not sure I understand the scenario you're describing - are you saying if we issue two offers, one with a blinded path and one that publishes our node, someone can use the one with the blinded path and the one that publishes our node somehow to get different behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no, I was saying they could determine that they came from the same issuer. Though, that is actually a different problem than what you are asking about and one we probably can't solve.
Sorry, think I was confused about your comment. Were you saying we should continue to send an InvoiceError
with Bolt12SemanticError::InvalidMetadata
for both verification paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so? Like, if we think it doesn't matter if we pass back an error then no need, but if we think we may hit those cases and we should pass back an error then we should ignore the BOLTs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably not useful to send an InvoiceError
. The only time those should ever fail is if we are given an invoice request for an invalid offer or an invalid path id used. Older offers should still work.
lightning/src/ln/channelmanager.rs
Outdated
@@ -10682,8 +10682,14 @@ where | |||
} | |||
}, | |||
OffersMessage::InvoiceError(invoice_error) => { | |||
let payment_hash = match context { | |||
OffersContext::InboundPayment { payment_hash } => Some(payment_hash), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really confused. We never actually build this context, so how could we be expecting to receive it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we will only start creating invoice reply paths #3163. See #3163 (comment).
I can leave these commits to a follow-up if you'd prefer. I'll have another PR that authenticates the path for outbound payments. But for inbound payments, it's mostly to improve logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, lets leave this commit for there cause its very confusing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped from this PR.
InvoiceContents::ForRefund { refund, .. } => refund.derives_keys(), | ||
} | ||
signer::verify_payer_metadata( | ||
metadata.as_ref(), key, iv_bytes, payer_id, tlv_stream, secp_ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a pending release note that upgrades will break any pending Refund
s or, in rare cases, pending BOLT 12 payments depending on their exact state, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, good call. Will add after I finish addressing @shaavan's comments on updating the tests.
9f7fd0d
to
97c567c
Compare
Needs rebase, it seems. |
97c567c
to
d751845
Compare
To authenticate that an InvoiceRequest is for a valid Offer, include the nonce from the Offer::metadata in the Offer::paths. This can be used to prevent de-anonymization attacks where an attacker sends requests using self-constructed paths to nodes near the Offer::paths' introduction nodes.
When an InvoiceRequest is handled with an OfferContext, use the containing nonce to verify that it is for a valid Offer. Otherwise, fall back to using Offer::metadata, which also contains the nonce. The latter is useful for supporting offers without blinded paths or those created prior to including an OffersContext in their blinded paths.
When an Offer uses blinded paths, its metadata consists of a nonce used to derive its signing keys. Now that the blinded paths contain this nonce, elide the metadata as it is now redundant. This saves space and also makes it impossible to derive the signing keys if an invoice request is received with the incorrect nonce. The nonce shouldn't be revealed in this case either to prevent de-anonymization attacks.
When a Bolt12Invoice is handled with an OfferContext, use the containing payment_id to verify that it is for a pending outbound payment. Only invoices for refunds without any blinded paths can be verified without an OfferContext.
When an invoice or invoice request cannot be authenticated from an OffersContext, simply do not respond instead of sending an InvoiceError message. According to BOLT4, messages sent over a blinded path not intended for its use MUST be ignored.
When using InvoiceRequestBuilder::deriving_payer_id, the nonce generated needs to be the same one included in any reply path. This is because the nonce is used along with the invoice request TLVs to derive a payer id. While this data is also included in the payer_metadata, including it in the blinded path would allow reducing the amount of data needed there to just enough to provide entropy (i.e., 16 bytes). This is more important for Refund because it can be transmitted via a QR code. But using the same payer_metadata structure for both InvoiceRequest and Refund would be beneficial to avoid more code.
When using RefundBuilder::deriving_payer_id, the nonce generated needs to be the same one included in any RefundBuilder::paths. This is because the nonce is used along with the refund TLVs to derive a payer id and will soon be used to authenticate any invoices.
Invoices are authenticated by checking the payer metadata in the corresponding invoice request or refund. For all invoices requests and for refunds using blinded paths, this will be the encrypted payment id and a 128-bit nonce. Allows checking the unencrypted payment id and nonce explicitly instead of the payer metadata. This will be used by an upcoming change that includes the payment id and nonce in the invoice request's reply path and the refund's blinded paths instead of completely in the payer metadata, which mitigates de-anonymization attacks.
To authenticate that a Bolt12Invoice is for a valid InvoiceRequest or Refund, include the nonce from the payer_metadata in the InvoiceRequest reply path or Refund::paths, respectively. This can be used to prevent de-anonymization attacks where an attacker sends invoices using self-constructed paths to nodes near the blinded paths' introduction nodes.
When a Bolt12Invoice is handled with an OfferContext, use both the containing payment_id and nonce to verify that it is for a pending outbound payment. Previously, the nonce the payment_id were taken from the payer_metadata and the latter was compared against the payment_id in the OfferContext. The payer_metadata thus no longer needs to include either when a blinded path is used. However, some payer_metadata will still be needed as per the spec.
When authenticating that an invoice is for a valid invoice request, the payer metadata is needed. Some of this data will be removed in the next commit and instead be included in the message context of the request's reply path. Add this data to Event::InvoiceReceived so that asynchronous invoice handling can verify properly.
InvoiceRequest and Refund have payer_metadata which consists of an encrypted payment id and a nonce used to derive its signing keys and authenticate any corresponding invoices. Now that the blinded paths include this data in their OffersContext, remove the nonce as it is now redundant. Keep the encrypted payment id as some data is needed in the payer metadata according to the spec. This saves space and prevents de-anonymization attacks as along as the nonce isn't revealed.
41ef097
to
825bda0
Compare
Squashed and rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two quick followups, but otherwise LGTM. I do find the metadata code to be quite a maze, I think in large part due to the metadata field being both "dumb bytes" on the wire and post-read as well as carrying state (in additional to the "dumb bytes") tracking the type of metadata during creation/signing.
lightning/src/offers/offer.rs
Outdated
} | ||
|
||
/// Verifies that the offer metadata was produced from the offer in the TLV stream. | ||
fn verify_using_metadata<T: secp256k1::Signing>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd feel much better if the two verification methods used different IVs. I know it shouldn't matter cause the offer can't be changed, but I'd feel better about it.
/// sent through. | ||
pub fn verify_using_payer_data<T: secp256k1::Signing>( | ||
&self, payment_id: PaymentId, nonce: Nonce, key: &ExpandedKey, secp_ctx: &Secp256k1<T> | ||
) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verification functions really need to return a Result
to get the warnings if the response is ignored. I know we haven't always been 100% on this but we should be going forward.
Invoice requests are authenticated by checking the metadata in the corresponding offer. For offers using blinded paths, this will simply be a 128-bit nonce. Elided this metadata from the offer directly and instead include it in the offer's blinded paths. This prevents de-anonymization attacks by ensuring the blinded paths are used in the right context.
Fixes #3117