lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.15k stars 363 forks source link

Authenticate use of offer blinded paths #3139

Closed jkczyz closed 2 months ago

jkczyz commented 3 months ago

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

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 95.57823% with 26 lines in your changes missing coverage. Please review.

Project coverage is 89.78%. Comparing base (3733103) to head (825bda0).

Files Patch % Lines
lightning/src/offers/signer.rs 54.54% 15 Missing :warning:
lightning/src/events/mod.rs 0.00% 4 Missing :warning:
lightning/src/ln/channelmanager.rs 93.75% 2 Missing and 2 partials :warning:
lightning/src/offers/nonce.rs 93.10% 1 Missing and 1 partial :warning:
lightning/src/offers/invoice.rs 97.22% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

TheBlueMatt commented 2 months ago

Chatted for a while offline, it looks like we need to remove the nonce from the metadata in invoice_requests.

jkczyz commented 2 months ago

Chatted for a while offline, it looks like we need to remove the nonce from the metadata in invoice_requests.

Yup, it was removed directly from derive_metadata_and_keys in 9f7fd0dfee0096972b05c807817273ef3eed29ab. It doesn't affect the offer metadata case since we already ignore the returned value there and set the metadata to None. With the change in that commit, the returned bytes is now the empty vec (since there is no payment id for offer metadata) instead of allocating a vec with the nonce bytes. And when used for payer metadata it is the encrypted payment id, as is desired.

TheBlueMatt commented 2 months ago

Needs rebase, it seems.

TheBlueMatt commented 2 months ago

Needs rebase again, sorry :(

TheBlueMatt commented 2 months ago

Also feel free to squash I think.

jkczyz commented 2 months ago

Squashed fixups and rebased.

jkczyz commented 2 months ago

Basically LGTM mod outstanding feedback!

@valentinewallace Could you take another look? A similar change was made to payer_metadata though instead of eliding it entirely we instead remove the nonce.

valentinewallace commented 2 months ago

Basically LGTM mod outstanding feedback!

@valentinewallace Could you take another look? A similar change was made to payer_metadata though instead of eliding it entirely we instead remove the nonce.

Will take another look today!

jkczyz commented 2 months ago

Last push just moves a commit since it didn't belong by the fixup.

valentinewallace commented 2 months ago

Needs rebase. I'm good with a squash.

jkczyz commented 2 months ago

Squashed and rebased.