From f00cc23f671643446577ee9c0da3e5b9a266fbc0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 28 Sep 2022 14:19:37 +0930 Subject: [PATCH] sphinx: rename confusing functions, ensure valid payloads. "sphinx_add_hop" takes a literal hop to include, "sphinx_add_modern_hop" prepends the length. Now we always prepend a length, make it clear that the literal version is a shortcut: * sphinx_add_hop -> sphinx_add_hop_has_length * sphinx_add_modern_hop -> sphinx_add_hop In addition, we check that length is actually correct! This means `createonion` can no longer create legacy or otherwise-invalid onions: fix tests and update man page to remove legacy usage. Signed-off-by: Rusty Russell Changelog-Changed: JSON-RPC: `createonion` no longer allows non-TLV-style payloads. --- common/sphinx.c | 24 +++++++++++++++++----- common/sphinx.h | 12 ++++++----- common/test/run-blindedpath_onion.c | 3 +-- common/test/run-onion-test-vector.c | 2 +- common/test/run-sphinx-xor_cipher_stream.c | 3 +++ common/test/run-sphinx.c | 3 +++ devtools/onion.c | 24 +++++----------------- doc/lightning-createonion.7.md | 11 +++++----- lightningd/onion_message.c | 2 +- lightningd/pay.c | 12 +++++++---- tests/test_pay.py | 12 +++++------ 11 files changed, 59 insertions(+), 49 deletions(-) diff --git a/common/sphinx.c b/common/sphinx.c index bf25c01aa..fff2a2d29 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -4,6 +4,7 @@ #include #include #include +#include #include @@ -103,17 +104,29 @@ size_t sphinx_path_payloads_size(const struct sphinx_path *path) return size; } -void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey, - const u8 *payload TAKES) +bool sphinx_add_hop_has_length(struct sphinx_path *path, const struct pubkey *pubkey, + const u8 *payload TAKES) { struct sphinx_hop sp; + bigsize_t lenlen, prepended_len; + + /* You promised size was prepended! */ + if (tal_bytelen(payload) == 0) + return false; + lenlen = bigsize_get(payload, tal_bytelen(payload), &prepended_len); + if (add_overflows_u64(lenlen, prepended_len)) + return false; + if (lenlen + prepended_len != tal_bytelen(payload)) + return false; + sp.raw_payload = tal_dup_talarr(path, u8, payload); sp.pubkey = *pubkey; tal_arr_expand(&path->hops, sp); + return true; } -void sphinx_add_modern_hop(struct sphinx_path *path, const struct pubkey *pubkey, - const u8 *payload TAKES) +void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey, + const u8 *payload TAKES) { u8 *with_len = tal_arr(NULL, u8, 0); size_t len = tal_bytelen(payload); @@ -122,7 +135,8 @@ void sphinx_add_modern_hop(struct sphinx_path *path, const struct pubkey *pubkey if (taken(payload)) tal_free(payload); - sphinx_add_hop(path, pubkey, take(with_len)); + if (!sphinx_add_hop_has_length(path, pubkey, take(with_len))) + abort(); } /* Small helper to append data to a buffer and update the position diff --git a/common/sphinx.h b/common/sphinx.h index 142cb3d6f..6ba537cf3 100644 --- a/common/sphinx.h +++ b/common/sphinx.h @@ -202,17 +202,19 @@ struct sphinx_path *sphinx_path_new_with_key(const tal_t *ctx, const struct secret *session_key); /** - * Add a payload hop to the path. + * Add a payload hop to the path (already has length prepended). + * + * Fails if length actually isn't prepended! */ -void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey, - const u8 *payload TAKES); +bool sphinx_add_hop_has_length(struct sphinx_path *path, const struct pubkey *pubkey, + const u8 *payload TAKES); /** * Prepend length to payload and add: for onionmessage, any size is OK, * for HTLC onions tal_bytelen(payload) must be > 1. */ -void sphinx_add_modern_hop(struct sphinx_path *path, const struct pubkey *pubkey, - const u8 *payload TAKES); +void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey, + const u8 *payload TAKES); /** * Compute the size of the serialized payloads. diff --git a/common/test/run-blindedpath_onion.c b/common/test/run-blindedpath_onion.c index 3c3f44841..ef9711dca 100644 --- a/common/test/run-blindedpath_onion.c +++ b/common/test/run-blindedpath_onion.c @@ -204,8 +204,7 @@ int main(int argc, char *argv[]) payload->encrypted_data_tlv = enctlv[i]; onionmsg_payload[i] = tal_arr(tmpctx, u8, 0); towire_tlv_onionmsg_payload(&onionmsg_payload[i], payload); - sphinx_add_modern_hop(sphinx_path, &alias[i], - onionmsg_payload[i]); + sphinx_add_hop(sphinx_path, &alias[i], onionmsg_payload[i]); } op = create_onionpacket(tmpctx, sphinx_path, ROUTING_INFO_SIZE, &path_secrets); diff --git a/common/test/run-onion-test-vector.c b/common/test/run-onion-test-vector.c index 2595ee997..3effdb5b5 100644 --- a/common/test/run-onion-test-vector.c +++ b/common/test/run-onion-test-vector.c @@ -162,7 +162,7 @@ int main(int argc, char *argv[]) max = tal_bytelen(payloads[i]); len = fromwire_bigsize(&cursor, &max); assert(len == max); - sphinx_add_modern_hop(sp, &k, take(tal_dup_arr(NULL, u8, cursor, max, 0))); + sphinx_add_hop(sp, &k, take(tal_dup_arr(NULL, u8, cursor, max, 0))); } assert(i == ARRAY_SIZE(payloads)); diff --git a/common/test/run-sphinx-xor_cipher_stream.c b/common/test/run-sphinx-xor_cipher_stream.c index 4212fab5d..7875d5ed8 100644 --- a/common/test/run-sphinx-xor_cipher_stream.c +++ b/common/test/run-sphinx-xor_cipher_stream.c @@ -35,6 +35,9 @@ struct amount_asset amount_sat_to_asset(struct amount_sat *sat UNNEEDED, const u /* Generated stub for amount_tx_fee */ struct amount_sat amount_tx_fee(u32 fee_per_kw UNNEEDED, size_t weight UNNEEDED) { fprintf(stderr, "amount_tx_fee called!\n"); abort(); } +/* Generated stub for bigsize_get */ +size_t bigsize_get(const u8 *p UNNEEDED, size_t max UNNEEDED, bigsize_t *val UNNEEDED) +{ fprintf(stderr, "bigsize_get called!\n"); abort(); } /* Generated stub for fromwire */ const u8 *fromwire(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, void *copy UNNEEDED, size_t n UNNEEDED) { fprintf(stderr, "fromwire called!\n"); abort(); } diff --git a/common/test/run-sphinx.c b/common/test/run-sphinx.c index 7af35453b..8a1e07b44 100644 --- a/common/test/run-sphinx.c +++ b/common/test/run-sphinx.c @@ -45,6 +45,9 @@ struct amount_asset amount_sat_to_asset(struct amount_sat *sat UNNEEDED, const u /* Generated stub for amount_tx_fee */ struct amount_sat amount_tx_fee(u32 fee_per_kw UNNEEDED, size_t weight UNNEEDED) { fprintf(stderr, "amount_tx_fee called!\n"); abort(); } +/* Generated stub for bigsize_get */ +size_t bigsize_get(const u8 *p UNNEEDED, size_t max UNNEEDED, bigsize_t *val UNNEEDED) +{ fprintf(stderr, "bigsize_get called!\n"); abort(); } /* Generated stub for bigsize_put */ size_t bigsize_put(u8 buf[BIGSIZE_MAX_LEN] UNNEEDED, bigsize_t v UNNEEDED) { fprintf(stderr, "bigsize_put called!\n"); abort(); } diff --git a/devtools/onion.c b/devtools/onion.c index 31bbcc56e..58c76c851 100644 --- a/devtools/onion.c +++ b/devtools/onion.c @@ -67,7 +67,7 @@ static void do_generate(int argc, char **argv, if (!data) errx(1, "bad hex after / in %s", argv[1 + i]); - sphinx_add_hop(sp, &path[i], data); + sphinx_add_hop_has_length(sp, &path[i], data); } else { struct short_channel_id scid; struct amount_msat amt; @@ -76,13 +76,13 @@ static void do_generate(int argc, char **argv, memset(&scid, i, sizeof(scid)); amt = amount_msat(i); if (i == num_hops - 1) - sphinx_add_hop(sp, &path[i], + sphinx_add_hop_has_length(sp, &path[i], take(onion_final_hop(NULL, amt, i, amt, NULL, NULL, NULL, NULL))); else - sphinx_add_hop(sp, &path[i], + sphinx_add_hop_has_length(sp, &path[i], take(onion_nonfinal_hop(NULL, &scid, amt, i, @@ -225,27 +225,13 @@ static void runtest(const char *filename) /* Unpack the hops and build up the path */ hopstok = json_get_member(buffer, gentok, "hops"); json_for_each_arr(i, hop, hopstok) { - u8 *full; - size_t prepended; - payloadtok = json_get_member(buffer, hop, "payload"); typetok = json_get_member(buffer, hop, "type"); pubkeytok = json_get_member(buffer, hop, "pubkey"); payload = json_tok_bin_from_hex(ctx, buffer, payloadtok); json_to_pubkey(buffer, pubkeytok, &pubkey); - if (!typetok || json_tok_streq(buffer, typetok, "legacy")) { - /* Legacy has a single 0 prepended as "realm" byte */ - full = tal_arrz(ctx, u8, 33); - memcpy(full + 1, payload, tal_bytelen(payload)); - } else { - /* TLV has length prepended */ - full = tal_arr(ctx, u8, 0); - towire_bigsize(&full, tal_bytelen(payload)); - prepended = tal_bytelen(full); - tal_resize(&full, prepended + tal_bytelen(payload)); - memcpy(full + prepended, payload, tal_bytelen(payload)); - } - sphinx_add_hop(path, &pubkey, full); + assert(json_tok_streq(buffer, typetok, "tlv")); + sphinx_add_hop(path, &pubkey, take(payload)); } res = create_onionpacket(ctx, path, ROUTING_INFO_SIZE, &shared_secrets); serialized = serialize_onionpacket(ctx, res); diff --git a/doc/lightning-createonion.7.md b/doc/lightning-createonion.7.md index 75aa660cc..bbeace8f2 100644 --- a/doc/lightning-createonion.7.md +++ b/doc/lightning-createonion.7.md @@ -20,14 +20,13 @@ payload destined for that node. The following is an example of a 3 hop onion: [ { "pubkey": "022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59", - "payload": "00000067000001000100000000000003e90000007b000000000000000000000000000000000000000000000000" + "payload": "11020203e904017b06080000670000010001" }, { "pubkey": "035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d", - "payload": "00000067000003000100000000000003e800000075000000000000000000000000000000000000000000000000" + "payload": "11020203e804017506080000670000030001" }, { - "style": "legacy", "pubkey": "0382ce59ebf18be7d84677c2e35f23294b9992ceca95491fcf8a56c6cb2d9de199", - "payload": "00000067000003000100000000000003e800000075000000000000000000000000000000000000000000000000" + "payload": "07020203e8040175" } ] ``` @@ -63,8 +62,8 @@ which the above *hops* parameter was generated: ] ``` - - Notice that the payload in the *hops* parameter is the hex-encoded version - of the parameters in the `getroute` response. + - Notice that the payload in the *hops* parameter is the hex-encoded TLV + of the parameters in the `getroute` response, with length prepended as a `bigsize_t`. - Except for the pubkey, the values are shifted left by one, i.e., the 1st payload in `createonion` corresponds to the 2nd set of values from `getroute`. - The final payload is a copy of the last payload sans `channel` diff --git a/lightningd/onion_message.c b/lightningd/onion_message.c index 66cbafa70..310ae68f2 100644 --- a/lightningd/onion_message.c +++ b/lightningd/onion_message.c @@ -281,7 +281,7 @@ static struct command_result *json_sendonionmessage2(struct command *cmd, /* Create an onion which encodes this. */ sphinx_path = sphinx_path_new(cmd, NULL); for (size_t i = 0; i < tal_count(hops); i++) - sphinx_add_modern_hop(sphinx_path, &hops[i].node, hops[i].tlv); + sphinx_add_hop(sphinx_path, &hops[i].node, hops[i].tlv); /* BOLT-onion-message #4: * - SHOULD set `len` to 1366 or 32834. diff --git a/lightningd/pay.c b/lightningd/pay.c index 07b2d0982..e8fe8abee 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1168,7 +1168,7 @@ send_payment(struct lightningd *ld, ret = pubkey_from_node_id(&pubkey, &ids[i]); assert(ret); - sphinx_add_hop(path, &pubkey, + sphinx_add_hop_has_length(path, &pubkey, take(onion_nonfinal_hop(NULL, &route[i + 1].scid, route[i + 1].amount, @@ -1200,7 +1200,7 @@ send_payment(struct lightningd *ld, "Destination does not support" " payment_secret"); } - sphinx_add_hop(path, &pubkey, onion); + sphinx_add_hop_has_length(path, &pubkey, onion); /* Copy channels used along the route. */ channels = tal_arr(tmpctx, struct short_channel_id, n_hops); @@ -1760,8 +1760,12 @@ static struct command_result *json_createonion(struct command *cmd, else sp = sphinx_path_new_with_key(cmd, assocdata, session_key); - for (size_t i=0; i *packet_size) return command_fail( diff --git a/tests/test_pay.py b/tests/test_pay.py index c6cf0844e..f760b5f79 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -3297,19 +3297,19 @@ def test_createonion_limits(node_factory): hops = [{ # privkey: 41bfd2660762506c9933ade59f1debf7e6495b10c14a92dbcd2d623da2507d3d "pubkey": "0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518", - "payload": "00" * 228 + "payload": bytes([227] + [0] * 227).hex(), }, { "pubkey": "0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c", - "payload": "00" * 228 + "payload": bytes([227] + [0] * 227).hex(), }, { "pubkey": "027f31ebc5462c1fdce1b737ecff52d37d75dea43ce11c74d25aa297165faa2007", - "payload": "00" * 228 + "payload": bytes([227] + [0] * 227).hex(), }, { "pubkey": "032c0b7cf95324a07d05398b240174dc0c2be444d96b159aa6c7f7b1e668680991", - "payload": "00" * 228 + "payload": bytes([227] + [0] * 227).hex(), }, { "pubkey": "02edabbd16b41c8371b92ef2f04c1185b4f03b6dcd52ba9b78d9d7c89c8f221145", - "payload": "00" * 228 + "payload": bytes([227] + [0] * 227).hex(), }] # This should success since it's right at the edge @@ -3317,7 +3317,7 @@ def test_createonion_limits(node_factory): # This one should fail however with pytest.raises(RpcError, match=r'Payloads exceed maximum onion packet size.'): - hops[0]['payload'] += '01' + hops[0]['payload'] = bytes([228] + [0] * 228).hex() l1.rpc.createonion(hops=hops, assocdata="BB" * 32) # But with a larger onion, it will work!