From a4c482dc072aa2905a6eca61679d613e3cb501e8 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 8 Nov 2022 11:48:12 +1030 Subject: [PATCH] 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; }