From 10847170ee0ae67f4d38ca043f8708d63345b128 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 29 Mar 2018 16:23:47 -0700 Subject: [PATCH 1/2] zpay32: adjust uint64 encoding to account for math.MaxUnit64 In this commit, we fix a logic error in our routine for converting a uint64 to/from base32. Before this commit, we assumed that the max number of groups was 12. However, the math.MaxUint64 (1<<64 - 1) can actually consume more than 12 groups with an extra set of bits. Before this commit, we would panic when attempting to parse an invoice generated like so: * addinvoice --amt 1337000 --expiry 99999999999999999 To fix this issue, we modify our logic to expect at most 13 groups. Additionally, we've added a new test that would panic before applying this commit. Fixes #972. --- zpay32/invoice.go | 10 ++++---- zpay32/invoice_internal_test.go | 42 +++++++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/zpay32/invoice.go b/zpay32/invoice.go index 4dbbfeec..091ca30b 100644 --- a/zpay32/invoice.go +++ b/zpay32/invoice.go @@ -1056,8 +1056,8 @@ func writeTaggedField(bufferBase32 *bytes.Buffer, dataType byte, data []byte) er // base32ToUint64 converts a base32 encoded number to uint64. func base32ToUint64(data []byte) (uint64, error) { - // Maximum that fits in uint64 is 64 / 5 = 12 groups. - if len(data) > 12 { + // Maximum that fits in uint64 is ceil(64 / 5) = 12 groups. + if len(data) > 13 { return 0, fmt.Errorf("cannot parse data of length %d as uint64", len(data)) } @@ -1077,9 +1077,9 @@ func uint64ToBase32(num uint64) []byte { return []byte{0} } - // To fit an uint64, we need at most is 64 / 5 = 12 groups. - arr := make([]byte, 12) - i := 12 + // To fit an uint64, we need at most is ceil(64 / 5) = 13 groups. + arr := make([]byte, 13) + i := 13 for num > 0 { i-- arr[i] = byte(num & uint64(31)) // 0b11111 in binary diff --git a/zpay32/invoice_internal_test.go b/zpay32/invoice_internal_test.go index 50596baf..613c3a13 100644 --- a/zpay32/invoice_internal_test.go +++ b/zpay32/invoice_internal_test.go @@ -2,6 +2,7 @@ package zpay32 import ( "encoding/binary" + "math" "reflect" "testing" "time" @@ -527,7 +528,11 @@ func TestParseExpiry(t *testing.T) { result: &testExpiry60, }, { - data: []byte{0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, 0xc}, + data: []byte{ + 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, + 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, + 0xc, 0x3, + }, valid: false, // data too long }, } @@ -547,7 +552,8 @@ func TestParseExpiry(t *testing.T) { } } -// TestParseMinFinalCLTVExpiry checks that the minFinalCLTVExpiry is properly parsed. +// TestParseMinFinalCLTVExpiry checks that the minFinalCLTVExpiry is properly +// parsed. func TestParseMinFinalCLTVExpiry(t *testing.T) { t.Parallel() @@ -567,12 +573,20 @@ func TestParseMinFinalCLTVExpiry(t *testing.T) { result: 60, }, { - data: []byte{0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, 0xc}, + data: []byte{ + 0x1, 0x2, 0x3, 0x4, 0x5, + 0x6, 0x7, 0x8, 0x9, 0xa, + 0xb, 0xc, + }, valid: true, result: 38390726480144748, }, { - data: []byte{0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, 0xc}, + data: []byte{ + 0x0, 0x1, 0x2, 0x3, 0x4, 0x5, + 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, + 0xc, 0x94, + }, valid: false, // data too long }, } @@ -592,6 +606,26 @@ func TestParseMinFinalCLTVExpiry(t *testing.T) { } } +// TestParseMinFinalCLTVExpiry tests that were able to properly encode/decode +// the math.MaxUint64 integer without panicking. +func TestParseMaxUint64Expiry(t *testing.T) { + t.Parallel() + + expiry := uint64(math.MaxUint64) + + expiryBytes := uint64ToBase32(expiry) + + expiryReParse, err := base32ToUint64(expiryBytes) + if err != nil { + t.Fatalf("unable to parse uint64: %v", err) + } + + if expiryReParse != expiry { + t.Fatalf("wrong expiry: expected %v got %v", expiry, + expiryReParse) + } +} + // TestParseFallbackAddr checks that the fallback address is properly parsed. func TestParseFallbackAddr(t *testing.T) { t.Parallel() From ef4512d1d88c4368e676f4fade81a645e85b8000 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 29 Mar 2018 16:25:03 -0700 Subject: [PATCH 2/2] rpc: limit the larger invoice expiry to 1 year This is a follow up to the prior commit. In order to add an additional layer of defense, we'll reject any expiry greater than 1 year. --- rpcserver.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 244795d8..2e90ff79 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -2122,8 +2122,21 @@ func (r *rpcServer) AddInvoice(ctx context.Context, // will be explicitly added to this payment request, which will imply // the default 3600 seconds. if invoice.Expiry > 0 { - exp := time.Duration(invoice.Expiry) * time.Second - options = append(options, zpay32.Expiry(exp)) + + // We'll ensure that the specified expiry is restricted to sane + // number of seconds. As a result, we'll reject an invoice with + // an expiry greater than 1 year. + maxExpiry := time.Hour * 24 * 365 + expSeconds := invoice.Expiry + + if float64(expSeconds) > maxExpiry.Seconds() { + return nil, fmt.Errorf("expiry of %v seconds "+ + "greater than max expiry of %v seconds", + float64(expSeconds), maxExpiry.Seconds()) + } + + expiry := time.Duration(invoice.Expiry) * time.Second + options = append(options, zpay32.Expiry(expiry)) } // If the description hash is set, then we add it do the list of options.