bolt11: fix decoding and encoding of unknown fields.

Fixes: #2527
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

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
 

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)

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);

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