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

zpay32: Fix broken last tagged field #3767

Merged
merged 3 commits into from Dec 5, 2019

Conversation

@matheusd
Copy link
Contributor

matheusd commented Nov 26, 2019

This fixes an issue with bech32 decoding where inserting a specific character in the checksum might generate a valid signature albeit with a different destination node than the original invoice.

This fixes the issue by ensuring that the last tagged field has all the required data instead of silently discarding a partial field, such as a field with only a type element but no length elements.

I tested this behavior in other implementations. c-lightning and lightning-payencode (python) fail to decode invoices with a partial field. Bolt11 (node) accepts it.

This should probably be clarified in BOLT-11 so that the behavior is consistent across implementations.

For context: sipa/bech32#51

matheusd added 2 commits Nov 25, 2019
This fixes an issue where the last tagged field of an invoice could get
broken due to the malleability of bech32 checksums.

The addition of a specific character in the second to last position of
the checksum could cause the previous signature field to mutate and thus
point to a different public node.
This switches the applicable error to use an exported sentinel error so
that it is more testable.
@matheusd matheusd requested review from halseth and Roasbeef as code owners Nov 26, 2019
This adds tests for checksum malleability issue of bech32 strings as
used by LN invoices.
@matheusd matheusd force-pushed the matheusdtech:fix-zpay32 branch from 9f01dd1 to cf6ae06 Nov 26, 2019
Copy link
Collaborator

halseth left a comment

Excellent PR! LGTM 👍

It's an interesting observation, that the signature can be changed this way while keeping the invoice valid. We could start adding the tagged field for pubkey by default to mitigate, but if someone can modify an invoice like this to change the destination, they probably can also just resign the invoice.

@matheusd

This comment has been minimized.

Copy link
Contributor Author

matheusd commented Nov 29, 2019

We could start adding the tagged field for pubkey by default to mitigate

Other options (but these involves changing BOLT-11) would be to include the length of tagged data as a prefixed field (along with the timestamp) or as a new tagged field. These have the advantage of being smaller than the pubkey.

if someone can modify an invoice like this to change the destination, they probably can also just resign the invoice.

Indeed.

Copy link
Collaborator

cfromknecht left a comment

@matheusd nice work! one small comment on the checksum unit test

zpay32/invoice_test.go Show resolved Hide resolved
@halseth halseth merged commit 509d0fb into lightningnetwork:master Dec 5, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.05%) to 62.713%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matheusd matheusd deleted the matheusdtech:fix-zpay32 branch Dec 13, 2019
@Christewart Christewart mentioned this pull request Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.