From 2a2341c56caa753a263415bdac426f721c11957a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 10 May 2019 10:47:49 +0930 Subject: [PATCH] bolt11: fix decoding and encoding of unknown fields. Fixes: #2527 Signed-off-by: Rusty Russell --- CHANGELOG.md | 1 + common/bolt11.c | 16 ++++++++++------ common/test/run-bolt11.c | 39 +++++++++++++++++++++++++++++++++++++-- tests/test_invoices.py | 19 +++++++++++++++++++ 4 files changed, 67 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b16c8a66e..3e481ee73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ changes. - protocol: handle lnd sending more messages before `reestablish`; don't fail channel, and handle older lnd's spurious empty commitments. - Fixed `fundchannel` crash when we have many UTXOs and we skip unconfirmed ones. - lightningd: fixed occasional hang on `connect` when peer had sent error. +- JSON RPC: `decodeinvoice` and `pay` now handle unknown invoice fields properly. ### Security diff --git a/common/bolt11.c b/common/bolt11.c index 17788e7f9..930962b09 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -129,9 +129,7 @@ static char *unknown_field(struct bolt11 *b11, extra->data = tal_dup_arr(extra, u5, *data, length, 0); list_add_tail(&b11->extra_fields, &extra->list); - pull_bits_certain(hu5, data, data_len, u8data, length, false); - (*data) += length; - (*data_len) -= length; + pull_bits_certain(hu5, data, data_len, u8data, length * 5, true); return NULL; } @@ -851,17 +849,22 @@ static void encode_r(u5 **data, const struct route_info *r) tal_free(rinfo); } -static void encode_extra(u5 **data, const struct bolt11_field *extra) +static bool encode_extra(u5 **data, const struct bolt11_field *extra) { size_t len; - push_varlen_uint(data, extra->tag, 5); + /* Can't encode an invalid tag. */ + if (bech32_charset_rev[(unsigned char)extra->tag] == -1) + return false; + + push_varlen_uint(data, bech32_charset_rev[(unsigned char)extra->tag], 5); push_varlen_uint(data, tal_count(extra->data), 10); /* extra->data is already u5s, so do this raw. */ len = tal_count(*data); tal_resize(data, len + tal_count(extra->data)); memcpy(*data + len, extra->data, tal_count(extra->data)); + return true; } /* Encodes, even if it's nonsense. */ @@ -950,7 +953,8 @@ char *bolt11_encode_(const tal_t *ctx, encode_r(&data, b11->routes[i]); list_for_each(&b11->extra_fields, extra, list) - encode_extra(&data, extra); + if (!encode_extra(&data, extra)) + return NULL; /* FIXME: towire_ should check this? */ if (tal_count(data) > 65535) diff --git a/common/test/run-bolt11.c b/common/test/run-bolt11.c index 97ecfe971..b7b19901b 100644 --- a/common/test/run-bolt11.c +++ b/common/test/run-bolt11.c @@ -78,6 +78,7 @@ static void test_b11(const char *b11str, struct bolt11 *b11; char *fail; char *reproduce; + struct bolt11_field *b11_extra, *expect_extra; b11 = bolt11_decode(tmpctx, b11str, hashed_desc, &fail); if (!b11) @@ -107,8 +108,16 @@ static void test_b11(const char *b11str, /* FIXME: compare routes. */ assert(tal_count(b11->routes) == tal_count(expect_b11->routes)); - /* FIXME: compare extra fields. */ - assert(list_empty(&b11->extra_fields) == list_empty(&expect_b11->extra_fields)); + expect_extra = list_top(&expect_b11->extra_fields, struct bolt11_field, + list); + list_for_each(&b11->extra_fields, b11_extra, list) { + assert(expect_extra->tag == b11_extra->tag); + assert(memeq(expect_extra->data, tal_bytelen(expect_extra->data), + b11_extra->data, tal_bytelen(b11_extra->data))); + expect_extra = list_next(&expect_b11->extra_fields, + expect_extra, list); + } + assert(!expect_extra); /* Re-encode to check */ reproduce = bolt11_encode(tmpctx, b11, false, test_sign, NULL); @@ -128,6 +137,7 @@ int main(void) struct node_id node; struct amount_msat msatoshi; const char *badstr; + struct bolt11_field *extra; wally_init(0); secp256k1_ctx = wally_get_secp_context(); @@ -271,6 +281,31 @@ int main(void) b11->expiry = 60; test_b11("LNBC2500U1PVJLUEZPP5QQQSYQCYQ5RQWZQFQQQSYQCYQ5RQWZQFQQQSYQCYQ5RQWZQFQYPQDQ5XYSXXATSYP3K7ENXV4JSXQZPUAZTRNWNGZN3KDZW5HYDLZF03QDGM2HDQ27CQV3AGM2AWHZ5SE903VRUATFHQ77W3LS4EVS3CH9ZW97J25EMUDUPQ63NYW24CG27H2RSPFJ9SRP", b11, NULL); + + /* Unknown field handling */ + if (!node_id_from_hexstr("02330d13587b67a85c0a36ea001c4dba14bcd48dda8988f7303275b040bffb6abd", strlen("02330d13587b67a85c0a36ea001c4dba14bcd48dda8988f7303275b040bffb6abd"), &node)) + abort(); + msatoshi = AMOUNT_MSAT(3000000000); + b11 = new_bolt11(tmpctx, &msatoshi); + b11->chain = chainparams_for_network("testnet"); + b11->timestamp = 1554294928; + if (!hex_decode("850aeaf5f69670e8889936fc2e0cff3ceb0c3b5eab8f04ae57767118db673a91", + strlen("850aeaf5f69670e8889936fc2e0cff3ceb0c3b5eab8f04ae57767118db673a91"), + &b11->payment_hash, sizeof(b11->payment_hash))) + abort(); + b11->min_final_cltv_expiry = 9; + b11->receiver_id = node; + b11->description = "Payment request with multipart support"; + b11->expiry = 28800; + extra = tal(b11, struct bolt11_field); + extra->tag = 'v'; + extra->data = tal_arr(extra, u5, 77); + for (size_t i = 0; i < 77; i++) + extra->data[i] = bech32_charset_rev[(u8)"dp68gup69uhnzwfj9cejuvf3xshrwde68qcrswf0d46kcarfwpshyaplw3skw0tdw4k8g6tsv9e8g"[i]]; + list_add(&b11->extra_fields, &extra->list); + + test_b11("lntb30m1pw2f2yspp5s59w4a0kjecw3zyexm7zur8l8n4scw674w8sftjhwec33km882gsdpa2pshjmt9de6zqun9w96k2um5ypmkjargypkh2mr5d9cxzun5ypeh2ursdae8gxqruyqvzddp68gup69uhnzwfj9cejuvf3xshrwde68qcrswf0d46kcarfwpshyaplw3skw0tdw4k8g6tsv9e8g4a3hx0v945csrmpm7yxyaamgt2xu7mu4xyt3vp7045n4k4czxf9kj0vw0m8dr5t3pjxuek04rtgyy8uzss5eet5gcyekd6m7u0mzv5sp7mdsag", b11, NULL); + /* FIXME: Test the others! */ wally_cleanup(0); tal_free(tmpctx); diff --git a/tests/test_invoices.py b/tests/test_invoices.py index b550f5005..bf39032a9 100644 --- a/tests/test_invoices.py +++ b/tests/test_invoices.py @@ -469,3 +469,22 @@ def test_autocleaninvoice(node_factory): # Everything deleted assert len(l1.rpc.listinvoices('inv1')['invoices']) == 0 assert len(l1.rpc.listinvoices('inv2')['invoices']) == 0 + + +def test_decode_unknown(node_factory): + l1 = node_factory.get_node() + + b11 = l1.rpc.decodepay('lntb30m1pw2f2yspp5s59w4a0kjecw3zyexm7zur8l8n4scw674w8sftjhwec33km882gsdpa2pshjmt9de6zqun9w96k2um5ypmkjargypkh2mr5d9cxzun5ypeh2ursdae8gxqruyqvzddp68gup69uhnzwfj9cejuvf3xshrwde68qcrswf0d46kcarfwpshyaplw3skw0tdw4k8g6tsv9e8gu2etcvsym36pdjpz04wm9nn96f9ntc3t3h5r08pe9d62p3js5wt5rkurqnrl7zkj2fjpvl3rmn7wwazt80letwxlm22hngu8n88g7hsp542qpl') + assert b11['currency'] == 'tb' + assert b11['created_at'] == 1554294928 + assert b11['payment_hash'] == '850aeaf5f69670e8889936fc2e0cff3ceb0c3b5eab8f04ae57767118db673a91' + assert b11['description'] == 'Payment request with multipart support' + assert b11['expiry'] == 28800 + assert b11['payee'] == '02330d13587b67a85c0a36ea001c4dba14bcd48dda8988f7303275b040bffb6abd' + assert b11['min_final_cltv_expiry'] == 9 + extra = only_one(b11['extra']) + assert extra['tag'] == 'v' + assert extra['data'] == 'dp68gup69uhnzwfj9cejuvf3xshrwde68qcrswf0d46kcarfwpshyaplw3skw0tdw4k8g6tsv9e8g' + assert b11['signature'] == '3045022100e2b2bc3204dc7416c8227d5db2ce65d24b35e22b8de8379c392b74a0c650a397022041db8304c7ff0ad25264167e23dcfce7744b3bff95b8dfda9579a38799ce8f5e' + assert 'fallbacks' not in b11 + assert 'routes' not in b11