From 6ce68c6534582b511a6d104643cd626430740790 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 18 Nov 2019 15:53:27 +0100 Subject: [PATCH 1/9] tlv: Add raw fields so we can store unknown fields as well --- tools/gen/header_template | 5 +++++ tools/gen/impl_template | 6 +++++- wire/tlvstream.h | 13 +++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/tools/gen/header_template b/tools/gen/header_template index b1e492fb7c80..019b2b288638 100644 --- a/tools/gen/header_template +++ b/tools/gen/header_template @@ -53,6 +53,11 @@ struct ${struct.struct_name()} { ## Structs for TLVs % for tlv in tlvs.values(): struct ${tlv.struct_name()} { + /* Raw fields including unknown ones. */ + struct tlv_field *fields; + + /* TODO The following explicit fields could just point into the + * tlv_field entries above to save on memory. */ % for msg in tlv.messages.values(): struct ${msg.struct_name()} *${msg.name}; % endfor diff --git a/tools/gen/impl_template b/tools/gen/impl_template index 169f0332d7e5..eefdd5aced28 100644 --- a/tools/gen/impl_template +++ b/tools/gen/impl_template @@ -157,7 +157,11 @@ fromwire_${type_}(${'ctx, ' if f.needs_context() else ''}&cursor, &plen, ${'*' i ${tlv.type_name()} *${tlv.struct_name()}_new(const tal_t *ctx) { /* Initialize everything to NULL. (Quiet, C pedants!) */ - return talz(ctx, struct ${tlv.struct_name()}); + ${tlv.type_name()} *inst = talz(ctx, struct ${tlv.struct_name()}); + + /* Initialized the fields to an empty array. */ + inst->fields = tal_arr(inst, struct tlv_field, 0); + return inst; } % for msg in tlv.messages.values(): diff --git a/wire/tlvstream.h b/wire/tlvstream.h index c7f033f0bc8d..f36d42c23f7b 100644 --- a/wire/tlvstream.h +++ b/wire/tlvstream.h @@ -15,6 +15,19 @@ struct tlv_record_type { void (*fromwire)(const u8 **cursor, size_t *max, void *record); }; +/* A single TLV field, consisting of the data and its associated metadata. */ +struct tlv_field { + /* If this is a type that is known to c-lightning we have a pointer to + * the metadata. */ + const struct tlv_record_type *meta; + + /* In any case we'll have the numeric type, even if we don't have a + * name that we can call it. */ + u64 numtype; + size_t length; + u8 *value; +}; + /* Pull all tlvs from a stream. Return false and calls fromwire_fail() on * error. */ bool fromwire_tlvs(const u8 **cursor, size_t *max, From 482f91478baaabeb3c79404be4ce1920e71851e6 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 18 Nov 2019 19:52:20 +0100 Subject: [PATCH 2/9] tlv: Add typesafe fromwire codegen for TLV namespaces We were weaving in and out of generic code through `fromwire_tlvs` with custom parameters defining the types in that namespace. This hard-wires the parser with the namespace's types. Slowly trying to deprecate `fromwire_tlvs` in favor of this typesafe variant. --- tools/gen/header_template | 1 + tools/gen/impl_template | 93 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/tools/gen/header_template b/tools/gen/header_template index 019b2b288638..56dbd3c1846a 100644 --- a/tools/gen/header_template +++ b/tools/gen/header_template @@ -66,6 +66,7 @@ struct ${tlv.struct_name()} { % for tlv in tlvs.values(): struct ${tlv.struct_name()} *${tlv.struct_name()}_new(const tal_t *ctx); +bool fromwire_${tlv.name}(const u8 **cursor, size_t *max, struct ${tlv.struct_name()} *record); % if tlv.name in options.expose_tlv_type: #define TLVS_${tlv.name.upper()}_ARRAY_SIZE ${len(tlv.messages)} diff --git a/tools/gen/impl_template b/tools/gen/impl_template index eefdd5aced28..d559ea782bff 100644 --- a/tools/gen/impl_template +++ b/tools/gen/impl_template @@ -3,10 +3,16 @@ /* Original template can be found at tools/gen/impl_template */ #include <${header_filename}> +#include #include #include #include #include + +#ifndef SUPERVERBOSE +#define SUPERVERBOSE(...) +#endif + % for comment in top_comments: /*${comment} */ % endfor @@ -211,6 +217,93 @@ ${static}const struct tlv_record_type tlvs_${tlv.name}[] = { { ${msg.number}, towire_${msg.struct_name()}, fromwire_${msg.struct_name()} }, % endfor }; + +bool fromwire_${tlv.name}(const u8 **cursor, size_t *max, struct ${tlv.struct_name()} *record) +{ + size_t num_types = ${len(tlv.messages)}; + const struct tlv_record_type *types = tlvs_${tlv.name}; + while (*max > 0) { + struct tlv_field field; + + /* BOLT #1: + * + * A `varint` is a variable-length, unsigned integer encoding + * using the [BigSize](#appendix-a-bigsize-test-vectors) + * format + */ + field.numtype = fromwire_bigsize(cursor, max); + + /* BOLT #1: + * - if a `type` or `length` is not minimally encoded: + * - MUST fail to parse the `tlv_stream`. + */ + if (!*cursor) { + SUPERVERBOSE("type"); + goto fail; + } + field.length = fromwire_bigsize(cursor, max); + + /* BOLT #1: + * - if a `type` or `length` is not minimally encoded: + * - MUST fail to parse the `tlv_stream`. + */ + if (!*cursor) { + SUPERVERBOSE("length"); + goto fail; + } + + /* BOLT #1: + * - if `length` exceeds the number of bytes remaining in the + * message: + * - MUST fail to parse the `tlv_stream`. + */ + if (field.length > *max) { + SUPERVERBOSE("value"); + goto fail; + } + field.value = tal_dup_arr(record, u8, *cursor, field.length, 0); + + /* BOLT #1: + * - if `type` is known: + * - MUST decode the next `length` bytes using the known + * encoding for `type`. + */ + field.meta = NULL; + for (size_t i = 0; i < num_types; i++) + if (types[i].type == field.numtype) + field.meta = &types[i]; + + if (field.meta) { + /* Length of message can't exceed 16 bits anyway. */ + size_t tlvlen = field.length; + field.meta->fromwire(cursor, &tlvlen, record); + + if (!*cursor) + goto fail; + + /* BOLT #1: + * - if `length` is not exactly equal to that required + * for the known encoding for `type`: + * - MUST fail to parse the `tlv_stream`. + */ + if (tlvlen != 0) { + SUPERVERBOSE("greater than encoding length"); + goto fail; + } + } else { + /* We didn't read from *cursor through a fromwire, so + * update manually. */ + *cursor += field.length; + } + /* We've read bytes in ->fromwire, so update max */ + *max -= field.length; + tal_arr_expand(&record->fields, field); + } + return true; +fail: + fromwire_fail(cursor, max); + return false; +} % endfor ## END TLV's % for msg in messages: ## START Wire Messages From 02fb929c3a9da0d1dea89b12c2ba37a8dbabb23d Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 18 Nov 2019 20:02:55 +0100 Subject: [PATCH 3/9] tlv: Add validity check codegen for the tlv namespaces Since the parser itself just parses and doesn't include validation anymore we need to put that functionality somewhere. The validation consists of enforcing that the types are in monotonically increasing order without duplicates and that for the even types we know how to handle it. --- tools/gen/header_template | 12 ++++++++++++ tools/gen/impl_template | 35 +++++++++++++++++++++++++++++++++++ wallet/test/run-wallet.c | 3 +++ 3 files changed, 50 insertions(+) diff --git a/tools/gen/header_template b/tools/gen/header_template index 56dbd3c1846a..2cd7ad50c55a 100644 --- a/tools/gen/header_template +++ b/tools/gen/header_template @@ -68,6 +68,18 @@ struct ${tlv.struct_name()} { struct ${tlv.struct_name()} *${tlv.struct_name()}_new(const tal_t *ctx); bool fromwire_${tlv.name}(const u8 **cursor, size_t *max, struct ${tlv.struct_name()} *record); +/** + * Check that the TLV stream is valid. + * + * Enforces the followin validity rules: + * - Types must be in monotonic non-repeating order + * - We must understand all even types + * + * Returns the index of the field that was invalid, or -1 if the stream is + * valid. + */ +int ${tlv.name}_is_valid(const struct ${tlv.struct_name()} *record); + % if tlv.name in options.expose_tlv_type: #define TLVS_${tlv.name.upper()}_ARRAY_SIZE ${len(tlv.messages)} extern const struct tlv_record_type tlvs_${tlv.name}[]; diff --git a/tools/gen/impl_template b/tools/gen/impl_template index d559ea782bff..296328ce0f4e 100644 --- a/tools/gen/impl_template +++ b/tools/gen/impl_template @@ -304,6 +304,41 @@ fail: fromwire_fail(cursor, max); return false; } + +int ${tlv.name}_is_valid(const struct ${tlv.struct_name()} *record) +{ + size_t numfields = tal_count(record->fields); + bool first = true; + u64 prev_type = 0; + for (int i=0; ifields[i]; + if (f->numtype % 2 == 0 && f->meta == NULL) { + /* BOLT #1: + * - otherwise, if `type` is unknown: + * - if `type` is even: + * - MUST fail to parse the `tlv_stream`. + * - otherwise, if `type` is odd: + * - MUST discard the next `length` bytes. + */ + SUPERVERBOSE("Unknown even type in TLV"); + return i; + } else if (!first && f->numtype <= prev_type) { + /* BOLT #1: + * - if decoded `type`s are not monotonically-increasing: + * - MUST fail to parse the `tlv_stream`. + */ + if (f->numtype == prev_type) + SUPERVERBOSE("duplicate tlv type"); + else + SUPERVERBOSE("invalid ordering"); + return i; + } + first = false; + prev_type = f->numtype; + } + return -1; +} + % endfor ## END TLV's % for msg in messages: ## START Wire Messages diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index c3e0ae6abfef..6854b38e2b80 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -554,6 +554,9 @@ void subd_req_(const tal_t *ctx UNNEEDED, /* Generated stub for subd_send_msg */ void subd_send_msg(struct subd *sd UNNEEDED, const u8 *msg_out UNNEEDED) { fprintf(stderr, "subd_send_msg called!\n"); abort(); } +/* Generated stub for tlv_payload_is_valid */ +int tlv_payload_is_valid(const struct tlv_tlv_payload *record UNNEEDED) +{ fprintf(stderr, "tlv_payload_is_valid called!\n"); abort(); } /* Generated stub for topology_add_sync_waiter_ */ void topology_add_sync_waiter_(const tal_t *ctx UNNEEDED, struct chain_topology *topo UNNEEDED, From 0799d4a1c51b0754ea1a709dea595b7302cea05f Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Mon, 18 Nov 2019 20:10:58 +0100 Subject: [PATCH 4/9] sphinx: Use the new `fromwire_tlv_payload` function We wire in the code-generated function, which removes the upfront validation and add the validation back after the `htlc_accepted` hook returns. If a plugin wanted to handle the onion in a special way it'll not have told us to just continue. --- common/sphinx.c | 10 +++++++--- common/test/run-sphinx.c | 6 ------ lightningd/peer_htlcs.c | 6 +++++- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/common/sphinx.c b/common/sphinx.c index 872169bb1131..dd242a91b19d 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -591,9 +591,13 @@ static void sphinx_parse_payload(struct route_step *step, const u8 *src) const u8 *tlv = step->raw_payload; size_t max = tal_bytelen(tlv); step->payload.tlv = tlv_tlv_payload_new(step); - if (!fromwire_tlvs(&tlv, &max, tlvs_tlv_payload, - TLVS_TLV_PAYLOAD_ARRAY_SIZE, - step->payload.tlv)) { + + /* The raw payload includes the length / realm prefix, Consume + * the length off of the payload, so the decoding can strat + * correctly. */ + fromwire_varint(&tlv, &max); + + if (!fromwire_tlv_payload(&tlv, &max, step->payload.tlv)) { /* FIXME: record offset of violation for error! */ step->type = SPHINX_INVALID_PAYLOAD; return; diff --git a/common/test/run-sphinx.c b/common/test/run-sphinx.c index 41f9c44a0517..71b93dc2a7bf 100644 --- a/common/test/run-sphinx.c +++ b/common/test/run-sphinx.c @@ -56,12 +56,6 @@ void fromwire_sha256(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct sh void fromwire_short_channel_id(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct short_channel_id *short_channel_id UNNEEDED) { fprintf(stderr, "fromwire_short_channel_id called!\n"); abort(); } -/* Generated stub for fromwire_tlvs */ -bool fromwire_tlvs(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, - const struct tlv_record_type types[] UNNEEDED, - size_t num_types UNNEEDED, - void *record UNNEEDED) -{ fprintf(stderr, "fromwire_tlvs called!\n"); abort(); } /* Generated stub for fromwire_tu32 */ u32 fromwire_tu32(const u8 **cursor UNNEEDED, size_t *max UNNEEDED) { fprintf(stderr, "fromwire_tu32 called!\n"); abort(); } diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 1b4394071b53..9943bf3f7be9 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -781,7 +781,11 @@ htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request, switch (result) { case htlc_accepted_continue: - if (rs->nextcase == ONION_FORWARD) { + if (rs->type == SPHINX_TLV_PAYLOAD && !tlv_payload_is_valid(rs->payload.tlv)) { + log_debug(channel->log, "Failing HTLC because of an invalid TLV payload"); + failure_code = WIRE_INVALID_ONION_PAYLOAD; + fail_in_htlc(hin, failure_code, NULL, request->short_channel_id); + }else if (rs->nextcase == ONION_FORWARD) { struct gossip_resolve *gr = tal(ld, struct gossip_resolve); gr->next_onion = serialize_onionpacket(gr, rs->next); From af37d6e93ebfa3789b88b537c5fa7f47ab875e4d Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 19 Nov 2019 12:51:24 +0100 Subject: [PATCH 5/9] sphinx: Decode payload and place shortcuts in the route-step We'll need to pass them around anyway, so just make them easier to access by doing a bit more to `process_onionpacket`. --- common/sphinx.c | 48 ++++++++++++++++++++++++++++++++++++++++ common/sphinx.h | 5 +++++ common/test/run-sphinx.c | 3 +++ 3 files changed, 56 insertions(+) diff --git a/common/sphinx.c b/common/sphinx.c index dd242a91b19d..9d6bf341dffd 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -683,6 +683,52 @@ struct onionpacket *create_onionpacket( return packet; } +/** + * Helper to extract fields from the legacy or tlv payload into the top-level + * struct. + */ +static void route_step_decode(struct route_step *rs) +{ + switch (rs->type) { + case SPHINX_V0_PAYLOAD: + rs->amt_to_forward = &rs->payload.v0.amt_forward; + rs->outgoing_cltv = &rs->payload.v0.outgoing_cltv; + if (rs->nextcase == ONION_FORWARD) { + rs->forward_channel = &rs->payload.v0.channel_id; + } else { + rs->forward_channel = NULL; + } + break; + case SPHINX_TLV_PAYLOAD: + if (rs->payload.tlv->amt_to_forward) { + rs->amt_to_forward = tal(rs, struct amount_msat); + amount_msat_from_u64( + rs->amt_to_forward, + rs->payload.tlv->amt_to_forward->amt_to_forward); + } else { + rs->amt_to_forward = NULL; + } + + if (rs->payload.tlv->outgoing_cltv_value) { + rs->outgoing_cltv = + &rs->payload.tlv->outgoing_cltv_value + ->outgoing_cltv_value; + } else { + rs->outgoing_cltv = NULL; + } + + if (rs->payload.tlv->short_channel_id) + rs->forward_channel = &rs->payload.tlv->short_channel_id + ->short_channel_id; + else + rs->forward_channel = NULL; + break; + case SPHINX_INVALID_PAYLOAD: + case SPHINX_RAW_PAYLOAD: + abort(); + } +} + /* * Given an onionpacket msg extract the information for the current * node and unwrap the remainder so that the node can forward it. @@ -754,6 +800,8 @@ struct route_step *process_onionpacket( step->nextcase = ONION_FORWARD; } + route_step_decode(step); + return step; } diff --git a/common/sphinx.h b/common/sphinx.h index e0478f93f64e..ad43be561e34 100644 --- a/common/sphinx.h +++ b/common/sphinx.h @@ -84,6 +84,11 @@ struct route_step { struct tlv_tlv_payload *tlv; } payload; u8 *raw_payload; + + /* Quick access for internal use. */ + struct amount_msat *amt_to_forward; + u32 *outgoing_cltv; + struct short_channel_id *forward_channel; }; /** diff --git a/common/test/run-sphinx.c b/common/test/run-sphinx.c index 71b93dc2a7bf..7d1fe48507ca 100644 --- a/common/test/run-sphinx.c +++ b/common/test/run-sphinx.c @@ -18,6 +18,9 @@ bool amount_asset_is_main(struct amount_asset *asset UNNEEDED) /* Generated stub for amount_asset_to_sat */ struct amount_sat amount_asset_to_sat(struct amount_asset *asset UNNEEDED) { fprintf(stderr, "amount_asset_to_sat called!\n"); abort(); } +/* Generated stub for amount_msat_from_u64 */ +void amount_msat_from_u64(struct amount_msat *msat UNNEEDED, u64 millisatoshis UNNEEDED) +{ fprintf(stderr, "amount_msat_from_u64 called!\n"); abort(); } /* Generated stub for amount_sat_add */ bool amount_sat_add(struct amount_sat *val UNNEEDED, struct amount_sat a UNNEEDED, From cc484fa3e58d8a706d6f4abcef8c7268e8825ed6 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 19 Nov 2019 14:27:24 +0100 Subject: [PATCH 6/9] htlcs: Make necessary payload fields optional and derfer validation We make the fields in `htlc_accepted_payload` optional (NULL if not present in the payload) and defer validation till after the hook call. --- lightningd/peer_htlcs.c | 40 +++++++++++++--------------------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 9943bf3f7be9..5484e4e97d5d 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -637,8 +637,8 @@ struct htlc_accepted_hook_payload { struct htlc_in *hin; struct channel *channel; struct lightningd *ld; - struct amount_msat amt_to_forward; - u32 outgoing_cltv_value; + struct amount_msat *amt_to_forward; + u32 *outgoing_cltv_value; /* NULL if this is node is final */ struct short_channel_id *short_channel_id; u8 *next_onion; @@ -747,8 +747,10 @@ static void htlc_accepted_hook_serialize(struct htlc_accepted_hook_payload *p, if (p->short_channel_id) json_add_short_channel_id(s, "short_channel_id", p->short_channel_id); - json_add_amount_msat_only(s, "forward_amount", p->amt_to_forward); - json_add_u32(s, "outgoing_cltv_value", p->outgoing_cltv_value); + if (p->amt_to_forward) + json_add_amount_msat_only(s, "forward_amount", *p->amt_to_forward); + if (p->outgoing_cltv_value) + json_add_u32(s, "outgoing_cltv_value", *p->outgoing_cltv_value); json_add_hex_talarr(s, "next_onion", p->next_onion); json_add_secret(s, "shared_secret", hin->shared_secret); json_object_end(s); @@ -790,8 +792,8 @@ htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request, gr->next_onion = serialize_onionpacket(gr, rs->next); gr->next_channel = *request->short_channel_id; - gr->amt_to_forward = request->amt_to_forward; - gr->outgoing_cltv_value = request->outgoing_cltv_value; + gr->amt_to_forward = *request->amt_to_forward; + gr->outgoing_cltv_value = *request->outgoing_cltv_value; gr->hin = hin; req = towire_gossip_get_channel_peer(tmpctx, &gr->next_channel); @@ -802,8 +804,8 @@ htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request, channel_resolve_reply, gr); } else handle_localpay(hin, hin->cltv_expiry, &hin->payment_hash, - request->amt_to_forward, - request->outgoing_cltv_value); + *request->amt_to_forward, + *request->outgoing_cltv_value); break; case htlc_accepted_fail: log_debug(channel->log, @@ -927,29 +929,13 @@ static bool peer_accepted_htlc(struct channel *channel, u64 id, hook_payload = tal(hin, struct htlc_accepted_hook_payload); - if (rs->nextcase == ONION_END) { - if (!route_step_decode_end(rs, &hook_payload->amt_to_forward, - &hook_payload->outgoing_cltv_value)) { - *failcode = WIRE_INVALID_ONION_PAYLOAD; - goto out; - } - hook_payload->short_channel_id = NULL; - } else { - hook_payload->short_channel_id - = tal(hook_payload, struct short_channel_id); - if (!route_step_decode_forward(rs, - &hook_payload->amt_to_forward, - &hook_payload->outgoing_cltv_value, - hook_payload->short_channel_id)) { - *failcode = WIRE_INVALID_ONION_PAYLOAD; - goto out; - } - } - hook_payload->route_step = tal_steal(hook_payload, rs); hook_payload->ld = ld; hook_payload->hin = hin; hook_payload->channel = channel; + hook_payload->amt_to_forward = rs->amt_to_forward; + hook_payload->outgoing_cltv_value = rs->outgoing_cltv; + hook_payload->short_channel_id = rs->forward_channel; hook_payload->next_onion = serialize_onionpacket(hook_payload, rs->next); plugin_hook_call_htlc_accepted(ld, hook_payload, hook_payload); From 98e7c0fca0dc0384e37561210b088fde7869cfa1 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 19 Nov 2019 14:32:08 +0100 Subject: [PATCH 7/9] htlc: Consolidate validation after the htlc_accepted hook returns This now enforces all rules for validity, both for the TLV format and checking that the required fields have been provided. --- lightningd/peer_htlcs.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 5484e4e97d5d..0157ee61c586 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -779,14 +779,35 @@ htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request, enum htlc_accepted_result result; enum onion_type failure_code; u8 *channel_update; + bool valid; result = htlc_accepted_hook_deserialize(buffer, toks, &payment_preimage, &failure_code, &channel_update); + /* BOLT #4: + * + * The writer: + * - MUST include `amt_to_forward` and `outgoing_cltv_value` for every node. + * - MUST include `short_channel_id` for every non-final node. + * - MUST NOT include `short_channel_id` for the final node. + * + * The reader: + * - MUST return an error if `amt_to_forward` or `outgoing_cltv_value` are not present. + */ + valid = rs->amt_to_forward != NULL && rs->outgoing_cltv != NULL && + (rs->nextcase == ONION_END || + (rs->nextcase == ONION_FORWARD && rs->forward_channel != NULL)); + + /* In addition we also enforce the TLV validity rules: + * - No unknown even types + * - Types in monotonical non-repeating order + */ + valid = valid && (rs->type == SPHINX_V0_PAYLOAD || tlv_payload_is_valid(rs->payload.tlv)); + switch (result) { case htlc_accepted_continue: - if (rs->type == SPHINX_TLV_PAYLOAD && !tlv_payload_is_valid(rs->payload.tlv)) { - log_debug(channel->log, "Failing HTLC because of an invalid TLV payload"); + if (!valid) { + log_debug(channel->log, "Failing HTLC because of an invalid payload"); failure_code = WIRE_INVALID_ONION_PAYLOAD; - fail_in_htlc(hin, failure_code, NULL, request->short_channel_id); + fail_in_htlc(hin, failure_code, NULL, NULL); }else if (rs->nextcase == ONION_FORWARD) { struct gossip_resolve *gr = tal(ld, struct gossip_resolve); From 2feb28e5409f4ce3b3b6a5c58bda20680d1fe4f3 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Tue, 19 Nov 2019 14:33:35 +0100 Subject: [PATCH 8/9] sphinx: Cleanup route_step_decode_* functions We have consolidated the two functions into a single `route_step_decode` function, and made it static since we call it in the `process_onionpacket` function. We remove the two exposed functions since they're no longer useful. --- common/sphinx.c | 71 ---------------------------------------- common/sphinx.h | 14 -------- wallet/test/run-wallet.c | 11 ------- 3 files changed, 96 deletions(-) diff --git a/common/sphinx.c b/common/sphinx.c index 9d6bf341dffd..2d21c72c81a2 100644 --- a/common/sphinx.c +++ b/common/sphinx.c @@ -932,74 +932,3 @@ struct onionreply *unwrap_onionreply(const tal_t *ctx, return oreply; } - -/** - * Helper to extract fields from ONION_END. - */ -bool route_step_decode_end(const struct route_step *rs, - struct amount_msat *amt_forward, - u32 *outgoing_cltv) -{ - assert(rs->nextcase == ONION_END); - - switch (rs->type) { - case SPHINX_V0_PAYLOAD: - *amt_forward = rs->payload.v0.amt_forward; - *outgoing_cltv = rs->payload.v0.outgoing_cltv; - return true; - case SPHINX_TLV_PAYLOAD: - if (!rs->payload.tlv->amt_to_forward) - return false; - if (!rs->payload.tlv->outgoing_cltv_value) - return false; - amt_forward->millisatoshis /* Raw: tu64 -> millisatoshis */ - = rs->payload.tlv->amt_to_forward->amt_to_forward; - *outgoing_cltv = rs->payload.tlv->outgoing_cltv_value->outgoing_cltv_value; - return true; - case SPHINX_INVALID_PAYLOAD: - return false; - - /* This should probably be removed, as it's just for testing */ - case SPHINX_RAW_PAYLOAD: - abort(); - } - abort(); -} - -/** - * Helper to extract fields from ONION_FORWARD. - */ -bool route_step_decode_forward(const struct route_step *rs, - struct amount_msat *amt_forward, - u32 *outgoing_cltv, - struct short_channel_id *scid) -{ - assert(rs->nextcase == ONION_FORWARD); - - switch (rs->type) { - case SPHINX_V0_PAYLOAD: - *amt_forward = rs->payload.v0.amt_forward; - *outgoing_cltv = rs->payload.v0.outgoing_cltv; - *scid = rs->payload.v0.channel_id; - return true; - case SPHINX_TLV_PAYLOAD: - if (!rs->payload.tlv->amt_to_forward) - return false; - amt_forward->millisatoshis /* Raw: tu64 -> millisatoshis */ - = rs->payload.tlv->amt_to_forward->amt_to_forward; - if (!rs->payload.tlv->outgoing_cltv_value) - return false; - *outgoing_cltv = rs->payload.tlv->outgoing_cltv_value->outgoing_cltv_value; - if (!rs->payload.tlv->short_channel_id) - return false; - *scid = rs->payload.tlv->short_channel_id->short_channel_id; - return true; - case SPHINX_INVALID_PAYLOAD: - return false; - - /* This should probably be removed, as it's just for testing */ - case SPHINX_RAW_PAYLOAD: - abort(); - } - abort(); -} diff --git a/common/sphinx.h b/common/sphinx.h index ad43be561e34..59f27b7a6a52 100644 --- a/common/sphinx.h +++ b/common/sphinx.h @@ -249,18 +249,4 @@ void sphinx_add_final_hop(struct sphinx_path *path, struct amount_msat forward, u32 outgoing_cltv); -/** - * Helper to extract fields from ONION_END. - */ -bool route_step_decode_end(const struct route_step *rs, - struct amount_msat *amt_forward, - u32 *outgoing_cltv); - -/** - * Helper to extract fields from ONION_FORWARD. - */ -bool route_step_decode_forward(const struct route_step *rs, - struct amount_msat *amt_forward, - u32 *outgoing_cltv, - struct short_channel_id *scid); #endif /* LIGHTNING_COMMON_SPHINX_H */ diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 6854b38e2b80..db41d83baa79 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -524,17 +524,6 @@ struct route_step *process_onionpacket( const size_t assocdatalen ) { fprintf(stderr, "process_onionpacket called!\n"); abort(); } -/* Generated stub for route_step_decode_end */ -bool route_step_decode_end(const struct route_step *rs UNNEEDED, - struct amount_msat *amt_forward UNNEEDED, - u32 *outgoing_cltv UNNEEDED) -{ fprintf(stderr, "route_step_decode_end called!\n"); abort(); } -/* Generated stub for route_step_decode_forward */ -bool route_step_decode_forward(const struct route_step *rs UNNEEDED, - struct amount_msat *amt_forward UNNEEDED, - u32 *outgoing_cltv UNNEEDED, - struct short_channel_id *scid UNNEEDED) -{ fprintf(stderr, "route_step_decode_forward called!\n"); abort(); } /* Generated stub for serialize_onionpacket */ u8 *serialize_onionpacket( const tal_t *ctx UNNEEDED, From df4c187953fdabe70934528dbb530c59792c7425 Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Thu, 21 Nov 2019 17:26:30 +0100 Subject: [PATCH 9/9] htlc: Add a checker function tellung us whether we can continue This function ensures we have all the infos we need to continue if the htlc_accepted hook tells us to. It also enforces well-formedness of the TLV payload if we have a TLV payload. Suggested-by: List Neigut <@niftynei> Signed-off-by: Christian Decker <@cdecker> --- lightningd/peer_htlcs.c | 65 +++++++++++++++++++++++++++-------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index 0157ee61c586..a0e7ece18db6 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -31,6 +31,10 @@ #include #include +#ifndef SUPERVERBOSE +#define SUPERVERBOSE(...) +#endif + static bool state_update_ok(struct channel *channel, enum htlc_state oldstate, enum htlc_state newstate, u64 htlc_id, const char *dir) @@ -763,6 +767,44 @@ static void htlc_accepted_hook_serialize(struct htlc_accepted_hook_payload *p, json_object_end(s); } +/* Make sure that we can continue with a default action if the htlc_accepted + * hook tells us to. This means enforcing that we have the necessary + * information to forward, fail or accept, and that the TVL payload is encoded + * correctly. */ +static bool htlc_accepted_can_continue(struct route_step *rs) +{ + if (rs->type == SPHINX_TLV_PAYLOAD && !tlv_payload_is_valid(rs->payload.tlv)) { + SUPERVERBOSE("Encoding of TLV payload is invalid"); + return false; + } + + /* BOLT #4: + * + * The writer: + * - MUST include `amt_to_forward` and `outgoing_cltv_value` for every node. + * - MUST include `short_channel_id` for every non-final node. + * - MUST NOT include `short_channel_id` for the final node. + * + * The reader: + * - MUST return an error if `amt_to_forward` or `outgoing_cltv_value` are not present. + */ + if (rs->amt_to_forward == NULL) { + SUPERVERBOSE("Missing amt_to_forward in payload"); + return false; + } + + if (rs->outgoing_cltv == NULL) { + SUPERVERBOSE("Missing outgoing_cltv_value in payload"); + return false; + } + + if (rs->nextcase && rs->forward_channel == NULL) { + SUPERVERBOSE("Missing short_channel_id in payload"); + return false; + } + return true; +} + /** * Callback when a plugin answers to the htlc_accepted hook */ @@ -779,32 +821,11 @@ htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request, enum htlc_accepted_result result; enum onion_type failure_code; u8 *channel_update; - bool valid; result = htlc_accepted_hook_deserialize(buffer, toks, &payment_preimage, &failure_code, &channel_update); - /* BOLT #4: - * - * The writer: - * - MUST include `amt_to_forward` and `outgoing_cltv_value` for every node. - * - MUST include `short_channel_id` for every non-final node. - * - MUST NOT include `short_channel_id` for the final node. - * - * The reader: - * - MUST return an error if `amt_to_forward` or `outgoing_cltv_value` are not present. - */ - valid = rs->amt_to_forward != NULL && rs->outgoing_cltv != NULL && - (rs->nextcase == ONION_END || - (rs->nextcase == ONION_FORWARD && rs->forward_channel != NULL)); - - /* In addition we also enforce the TLV validity rules: - * - No unknown even types - * - Types in monotonical non-repeating order - */ - valid = valid && (rs->type == SPHINX_V0_PAYLOAD || tlv_payload_is_valid(rs->payload.tlv)); - switch (result) { case htlc_accepted_continue: - if (!valid) { + if (!htlc_accepted_can_continue(rs)) { log_debug(channel->log, "Failing HTLC because of an invalid payload"); failure_code = WIRE_INVALID_ONION_PAYLOAD; fail_in_htlc(hin, failure_code, NULL, NULL);