From 8969f133e1ee72ca963ba0360307a65141fd34d0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 8 Nov 2022 11:41:44 +1030 Subject: [PATCH 1/3] lightningd: --dev-onion-reply-length option. Signed-off-by: Rusty Russell --- common/sphinx.c | 11 +++++++++-- common/sphinx.h | 3 +++ lightningd/options.c | 5 +++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/common/sphinx.c b/common/sphinx.c index fff2a2d297b5..2fef1451f892 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -679,12 +679,19 @@ struct route_step *process_onionpacket( return step; } +#if DEVELOPER +unsigned dev_onion_reply_length = ONION_REPLY_SIZE; +#define OUR_ONION_REPLY_SIZE dev_onion_reply_length +#else +#define OUR_ONION_REPLY_SIZE ONION_REPLY_SIZE +#endif + struct onionreply *create_onionreply(const tal_t *ctx, const struct secret *shared_secret, const u8 *failure_msg) { size_t msglen = tal_count(failure_msg); - size_t padlen = ONION_REPLY_SIZE - msglen; + size_t padlen = OUR_ONION_REPLY_SIZE - msglen; struct onionreply *reply = tal(ctx, struct onionreply); u8 *payload = tal_arr(ctx, u8, 0); struct secret key; @@ -716,7 +723,7 @@ struct onionreply *create_onionreply(const tal_t *ctx, * - Note: this value is 118 bytes longer than the longest * currently-defined message. */ - assert(tal_count(payload) == ONION_REPLY_SIZE + 4); + assert(tal_count(payload) == OUR_ONION_REPLY_SIZE + 4); /* BOLT #4: * diff --git a/common/sphinx.h b/common/sphinx.h index 6ba537cf36d3..9b80c29b3d04 100644 --- a/common/sphinx.h +++ b/common/sphinx.h @@ -267,6 +267,9 @@ sphinx_compressed_onion_deserialize(const tal_t *ctx, const u8 *src); #if DEVELOPER /* Override to force us to reject valid onion packets */ extern bool dev_fail_process_onionpacket; + +/* Override to set custom onion error lengths. */ +extern unsigned dev_onion_reply_length; #endif #endif /* LIGHTNING_COMMON_SPHINX_H */ diff --git a/lightningd/options.c b/lightningd/options.c index 91f443643a08..07504bda942a 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -752,6 +752,11 @@ static void dev_register_opts(struct lightningd *ld) opt_register_noarg("--dev-no-ping-timer", opt_set_bool, &ld->dev_no_ping_timer, "Don't hang up if we don't get a ping response"); + opt_register_arg("--dev-onion-reply-length", + opt_set_uintval, + opt_show_uintval, + &dev_onion_reply_length, + "Send onion errors of custom length"); } #endif /* DEVELOPER */ From ccdb8b15e9ac408bb1f4fd753689913565700346 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 8 Nov 2022 11:42:17 +1030 Subject: [PATCH 2/3] pytest: add test for generating non-standard length onion errors. Signed-off-by: Rusty Russell --- tests/test_plugin.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index f75b4fcc1484..a8e02bbd5a51 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1042,7 +1042,8 @@ def test_channel_state_change_history(node_factory, bitcoind): assert(history[3]['message'] == "Closing complete") -@pytest.mark.developer("without DEVELOPER=1, gossip v slow") +@pytest.mark.xfail(strict=True) +@pytest.mark.developer("Gossip slow, and we test --dev-onion-reply-length") def test_htlc_accepted_hook_fail(node_factory): """Send payments from l1 to l2, but l2 just declines everything. @@ -1053,7 +1054,8 @@ def test_htlc_accepted_hook_fail(node_factory): """ l1, l2, l3 = node_factory.line_graph(3, opts=[ {}, - {'plugin': os.path.join(os.getcwd(), 'tests/plugins/fail_htlcs.py')}, + {'dev-onion-reply-length': 1111, + 'plugin': os.path.join(os.getcwd(), 'tests/plugins/fail_htlcs.py')}, {} ], wait_for_announce=True) From f15613ae18126770d7ced173d770254a8b79dd95 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 8 Nov 2022 11:48:12 +1030 Subject: [PATCH 3/3] common/sphinx: don't use fixed lengths anywhere. 1. Remove the very concept of ONION_REPLY_SIZE, instead make it a local variable in create_onionreply(). 2. Use the proper fromwire_ primitives in unwrap_onionreply() so we don't have to do explicit length checks. 3. Make fromwire_tal_arrn() return NULL if it fails to pull, instead of a zero-length allocation. Signed-off-by: Rusty Russell Changelog-Fixed: Protocol: we now correctly decrypt non-256-length onion errors (we always forwarded them fine, now we actually can parse them). --- common/sphinx.c | 74 +++++++++++++++++++++----------------------- tests/test_plugin.py | 1 - wire/fromwire.c | 2 ++ 3 files changed, 38 insertions(+), 39 deletions(-) diff --git a/common/sphinx.c b/common/sphinx.c index 2fef1451f892..4de8f23221ff 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -16,8 +16,6 @@ #define BLINDING_FACTOR_SIZE 32 -#define ONION_REPLY_SIZE 256 - #define RHO_KEYTYPE "rho" struct hop_params { @@ -680,10 +678,7 @@ struct route_step *process_onionpacket( } #if DEVELOPER -unsigned dev_onion_reply_length = ONION_REPLY_SIZE; -#define OUR_ONION_REPLY_SIZE dev_onion_reply_length -#else -#define OUR_ONION_REPLY_SIZE ONION_REPLY_SIZE +unsigned dev_onion_reply_length = 256; #endif struct onionreply *create_onionreply(const tal_t *ctx, @@ -691,12 +686,27 @@ struct onionreply *create_onionreply(const tal_t *ctx, const u8 *failure_msg) { size_t msglen = tal_count(failure_msg); - size_t padlen = OUR_ONION_REPLY_SIZE - msglen; + size_t padlen; struct onionreply *reply = tal(ctx, struct onionreply); u8 *payload = tal_arr(ctx, u8, 0); struct secret key; struct hmac hmac; + /* BOLT #4: + * The _erring node_: + * - SHOULD set `pad` such that the `failure_len` plus `pad_len` + * is equal to 256. + * - Note: this value is 118 bytes longer than the longest + * currently-defined message. + */ + const u16 onion_reply_size = IFDEV(dev_onion_reply_length, 256); + + /* We never do this currently, but could in future! */ + if (msglen > onion_reply_size) + padlen = 0; + else + padlen = onion_reply_size - msglen; + /* BOLT #4: * * The node generating the error message (_erring node_) builds a return @@ -715,15 +725,8 @@ struct onionreply *create_onionreply(const tal_t *ctx, towire_u16(&payload, padlen); towire_pad(&payload, padlen); - /* BOLT #4: - * - * The _erring node_: - * - SHOULD set `pad` such that the `failure_len` plus `pad_len` is - * equal to 256. - * - Note: this value is 118 bytes longer than the longest - * currently-defined message. - */ - assert(tal_count(payload) == OUR_ONION_REPLY_SIZE + 4); + /* Two bytes for each length: failure_len and pad_len */ + assert(tal_count(payload) == onion_reply_size + 4); /* BOLT #4: * @@ -770,21 +773,17 @@ u8 *unwrap_onionreply(const tal_t *ctx, int *origin_index) { struct onionreply *r; - struct secret key; - struct hmac hmac; const u8 *cursor; - u8 *final; size_t max; u16 msglen; - if (tal_count(reply->contents) != ONION_REPLY_SIZE + sizeof(hmac) + 4) { - return NULL; - } - r = new_onionreply(tmpctx, reply->contents); *origin_index = -1; for (int i = 0; i < numhops; i++) { + struct secret key; + struct hmac hmac, expected_hmac; + /* Since the encryption is just XORing with the cipher * stream encryption is identical to decryption */ r = wrap_onionreply(tmpctx, &shared_secrets[i], r); @@ -792,30 +791,29 @@ u8 *unwrap_onionreply(const tal_t *ctx, /* Check if the HMAC matches, this means that this is * the origin */ subkey_from_hmac("um", &shared_secrets[i], &key); - compute_hmac(&key, r->contents + sizeof(hmac.bytes), - tal_count(r->contents) - sizeof(hmac.bytes), - NULL, 0, &hmac); - if (memcmp(hmac.bytes, r->contents, sizeof(hmac.bytes)) == 0) { + + cursor = r->contents; + max = tal_count(r->contents); + + fromwire_hmac(&cursor, &max, &hmac); + /* Too short. */ + if (!cursor) + return NULL; + + compute_hmac(&key, cursor, max, NULL, 0, &expected_hmac); + if (hmac_eq(&hmac, &expected_hmac)) { *origin_index = i; break; } } + + /* Didn't find source, it's garbled */ if (*origin_index == -1) { return NULL; } - cursor = r->contents + sizeof(hmac); - max = tal_count(r->contents) - sizeof(hmac); msglen = fromwire_u16(&cursor, &max); - - if (msglen > ONION_REPLY_SIZE) { - return NULL; - } - - final = tal_arr(ctx, u8, msglen); - if (!fromwire(&cursor, &max, final, msglen)) - return tal_free(final); - return final; + return fromwire_tal_arrn(ctx, &cursor, &max, msglen); } struct onionpacket *sphinx_decompress(const tal_t *ctx, diff --git a/tests/test_plugin.py b/tests/test_plugin.py index a8e02bbd5a51..4e67b5b76d06 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1042,7 +1042,6 @@ def test_channel_state_change_history(node_factory, bitcoind): assert(history[3]['message'] == "Closing complete") -@pytest.mark.xfail(strict=True) @pytest.mark.developer("Gossip slow, and we test --dev-onion-reply-length") def test_htlc_accepted_hook_fail(node_factory): """Send payments from l1 to l2, but l2 just declines everything. diff --git a/wire/fromwire.c b/wire/fromwire.c index 97909534db50..69139ca3b918 100644 --- a/wire/fromwire.c +++ b/wire/fromwire.c @@ -223,6 +223,8 @@ u8 *fromwire_tal_arrn(const tal_t *ctx, arr = tal_arr(ctx, u8, num); fromwire_u8_array(cursor, max, arr, num); + if (!*cursor) + return tal_free(arr); return arr; }