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: check route+hop hints while decoding #3505

Merged

Conversation

Crypt-iQ
Copy link
Collaborator

@Crypt-iQ Crypt-iQ commented Sep 14, 2019

This commit checks that there are not greater than 20 route hints while decoding or more than 20 hop hints for a single route hint. This mitigates a potential DoS vector where an attacker could craft a very large bech32 invoice string containing an absurd amount of route and/or hop hints. If sent to an application that processes payment requests, this would allocate a burdensome amount of memory due to the public key parsing for each route/hop hint.

For a 1.7MB payment request, this yielded about 38MB in allocations from just parsing public keys (bech32VerifyChecksum allocated another 25MB):

   45.51MB  7.31% 92.07%    45.51MB  7.31%  math/big.nat.make
   25.50MB  4.09% 96.16%    25.50MB  4.09%  github.com/lightningnetwork/lnd/zpay32.bech32VerifyChecksum
       1MB  0.16% 96.32%    39.50MB  6.34%  github.com/lightningnetwork/lnd/zpay32.parseRouteHint
       1MB  0.16% 96.48%    33.50MB  5.38%  github.com/btcsuite/btcd/btcec.decompressPoint
    0.50MB  0.08% 96.56%     7.50MB  1.20%  crypto/elliptic.(*CurveParams).doubleJacobian
    0.50MB  0.08% 96.64%       38MB  6.10%  github.com/btcsuite/btcd/btcec.ParsePubKey
         0     0% 96.64%       12MB  1.93%  crypto/ecdsa.Verify
         0     0% 96.64%        8MB  1.28%  crypto/elliptic.(*CurveParams).ScalarBaseMult
         0     0% 96.64%       12MB  1.93%  crypto/elliptic.(*CurveParams).ScalarMult

With this change, memory usage will be far lower as decoding will exit early with an error if too many route/hop hints are found.

EDIT:
With btcsuite/btcd#1429, the memory allocation from parsing pubkeys occupies about ~6.5MB which is significant improvement. The bech32VerifyChecksum still allocates a ridiculous amount of memory. Another fix would be to limit the size of invoices before attempting any decoding. In conjunction with the commits in this PR, it should limit any potential sources of memory blow-up.

@Crypt-iQ Crypt-iQ changed the title zpay: check route+hop hints while decoding zpay32: check route+hop hints while decoding Sep 14, 2019
@wpaulino wpaulino added dos/hardening Related to the resilience of LND against denial of service or other related attacks enhancement Improvements to existing features / behaviour v0.8.0 labels Sep 14, 2019
@wpaulino wpaulino added this to the 0.8.0 milestone Sep 14, 2019
zpay32/invoice.go Outdated Show resolved Hide resolved
@wpaulino wpaulino removed the request for review from Roasbeef September 17, 2019 23:21
@Crypt-iQ
Copy link
Collaborator Author

Crypt-iQ commented Sep 18, 2019

I'd also like to include a length limit on the invoices that'll be checked before starting the decoding process. Ideas @wpaulino ?

Another reason to limit the length besides the QR code argument below is that you can also make invoices huge by including unknown fields that'll be skipped over while decoding. So limiting route hints doesn't solve the memory blow up issues entirely.

I'm by no means in expert in QR codes or anything, but from here https://en.wikipedia.org/wiki/QR_code#Storage it seems that len(invoice) <= 7089 for it to fit into a QR code. I think this might even be implementation depending on the app used. However, if each field can have a maximum of 1024 bytes and there can be 20 route hints, this would give 20480 bytes, not counting the other data in the invoice. With the current code, it's legal for an invoice to not fit into a QR code which I think was one of the points of bolt 11?

@Crypt-iQ Crypt-iQ force-pushed the invoice-param-checks-0913 branch 2 times, most recently from e077e3d to 1574ad6 Compare September 18, 2019 13:25
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Changes look good to me so far!

Agree that a max invoice length would make sense also. The max-length-that-fits-in-a-qr-code argument sounds fine to me.

zpay32/invoice.go Outdated Show resolved Hide resolved
zpay32/invoice.go Show resolved Hide resolved
zpay32/invoice_test.go Outdated Show resolved Hide resolved
This commit checks that the size of the bech32 encoded invoice is not
greater than 7092 bytes, which is the maximum number of bytes that can
fit into a QR code. This mitigates a potential DoS vector where an attacker
could craft a very large bech32 invoice string containing an absurd amount
of route and/or hop hints. If sent to an application that processes
payment requests, this would allocate a burdensome amount of memory
due to the public key parsing for each route/hop hint.

For a 1.7MB payment request, this yielded about 38MB in allocations
from just parsing public keys:

```
   45.51MB  7.31% 92.07%    45.51MB  7.31%  math/big.nat.make
   25.50MB  4.09% 96.16%    25.50MB  4.09%  github.com/lightningnetwork/lnd/zpay32.bech32VerifyChecksum
       1MB  0.16% 96.32%    39.50MB  6.34%  github.com/lightningnetwork/lnd/zpay32.parseRouteHint
       1MB  0.16% 96.48%    33.50MB  5.38%  github.com/btcsuite/btcd/btcec.decompressPoint
    0.50MB  0.08% 96.56%     7.50MB  1.20%  crypto/elliptic.(*CurveParams).doubleJacobian
    0.50MB  0.08% 96.64%       38MB  6.10%  github.com/btcsuite/btcd/btcec.ParsePubKey
         0     0% 96.64%       12MB  1.93%  crypto/ecdsa.Verify
         0     0% 96.64%        8MB  1.28%  crypto/elliptic.(*CurveParams).ScalarBaseMult
         0     0% 96.64%       12MB  1.93%  crypto/elliptic.(*CurveParams).ScalarMult
```

With this change, memory usage will be far lower as decoding will exit
early with an error if the invoice is too large.
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM now 👍

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

LGTM 🔒

@wpaulino wpaulino merged commit 811c2df into lightningnetwork:master Sep 24, 2019
@Crypt-iQ Crypt-iQ deleted the invoice-param-checks-0913 branch September 25, 2019 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dos/hardening Related to the resilience of LND against denial of service or other related attacks enhancement Improvements to existing features / behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants