Split the TLV payload processing into parsing and validation by cdecker · Pull Request #3278 · ElementsProject/lightning

cdecker · 2019-11-19T20:33:12Z

In order to be useful #3260 requires that we allow TLV payloads with types that we may not know how to handle. In this case the plugin needs to terminate the onion and handle it by either forwarding manually using sendonion or terminate it either successfully or fail it.

This PR splits the parsing of the TLV payload from the validation. Parsing just stashes the entire payload in a tlv_payload instance, with typed pointers to fields that we know how to parse and handle. Validation then goes through and checks that there are no TLV fields that we don't know how to handle, that the types are in the correct order and that we have all the fields that we need in order to forward. The validity is then only used if lightningd isn't explicitly told how to handle the HTLC and to forward or terminate with an internal invoice.

rustyrussell

Nice cleanup, minor nits mainly, but please convert the TLV parsing tests in wire/test-run-tlvstream.c to ensure these new functions are correct.

rustyrussell · 2019-11-19T23:32:28Z

+		}
+		/* We've read bytes in ->fromwire, so update max */
+		*max -= field.length;
+		tal_expand(&record->fields, &field, 1);


tal_arr_expand() in utils.h does this a bit more compactly, BTW.

Ah, was looking for that but couldn't find it so I thought I was misremembering the name xD

rustyrussell · 2019-11-19T23:34:10Z

+		*max -= field.length;
+		tal_expand(&record->fields, &field, 1);
+	}
+	assert(tal_count(record->fields) > 0);


If we ever hand a zero-length field in here, this will assert, right? *max == 0? That seems wrong.

This one slipped in, it checks whether we had any TLV fields,not whether one of the fields had zero-length. However the check is wrong since an empty TLV is valid per spec, even though not very sensible.

rustyrussell · 2019-11-19T23:37:07Z

 	fromwire_fail(cursor, max);
 	return false;
 }
+bool ${tlv.name}_is_valid(const struct ${tlv.struct_name()} *record)


Please have this return the problematic offset within the TLV? There's a FIXME for this requirement in the onion parsing code, where we're supposed to return the offset which was the problem in the error msg.

We can return the exact offset of the offending field in fromwire_tlv_payload which has the locations, while here we would have to track the offset by accumulating the varint lengths and field lengths while verifying. I can add the offset in the fromwire code and just add the field index that we get upset about if you'd like :-)

Might make more sense anyway, since we can then say things like "Unknown even type x in field y" instead of saying I got upset somewhere at byte z.

rustyrussell · 2019-11-19T23:38:15Z

+	u64 prev_type = 0;
+	for (size_t i=0; i<numfields; i++) {
+		struct tlv_field *f = &record->fields[i];
+		if (f->numtype % 2 == 0 && f->meta == 0) {


f->meta == NULL please.

Ouch, sorry. Copy pasted it without checking.

rustyrussell · 2019-11-19T23:39:35Z

+				SUPERVERBOSE("invalid ordering");
+			return false;
+		}
+		first = false;


This code doesn't actually work; we don't update prev_type!

We had tests for the generic code which I think we should replicate here which should have caught this?

I'll migrate the run-tlvstream tests to use the typesafe wrapper and remove the old ones. 👍

rustyrussell · 2019-11-19T23:41:00Z

+ */
+static void route_step_decode(struct route_step *rs)
+{
+	if (rs->type == SPHINX_V0_PAYLOAD) {


Make this a switch() to catch if we ever add another type?

rustyrussell · 2019-11-19T23:42:05Z


+	/* In order to handle it internally the payload needs to have at least
+	 * the `outgoing_cltv_value`, the `amt_to_forward` and the
+	 * `short_channel_id` if we are forwarding. */


Please quote the BOLT directly instead, so we know when this requirement changes (which it will!).

niftynei · 2019-11-21T01:04:06Z

+	 *  - No unknown even types
+	 *  - Types in monotonical non-repeating order
+	 */
+	valid = valid && (rs->type == SPHINX_V0_PAYLOAD || tlv_payload_is_valid(rs->payload.tlv));


seems like it'd be nicer to have a separate method for validity check, instead of making it all inline here.

Added a htlc_accepted_can_continue function which tells us whether we can continue with the route-step if we're told to by the hook.

niftynei · 2019-11-21T01:04:26Z

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


you could just call the valid() method here

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.

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.

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.

We'll need to pass them around anyway, so just make them easier to access by doing a bit more to `process_onionpacket`.

We make the fields in `htlc_accepted_payload` optional (NULL if not present in the payload) and defer validation till after the hook call.

This now enforces all rules for validity, both for the TLV format and checking that the required fields have been provided.

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.

@niftynei

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>

cdecker · 2019-11-21T16:39:23Z

Updated the PR to address the feedback:

@rustyrussell here's the range-diff since your review
@niftynei here's the range-diff since your review

Notice that there are two new commit that will not be shown in the range-diff: 02fb929 and df4c187 😉

rustyrussell · 2019-11-22T04:39:33Z

Ack df4c187

Note: Travis picked up an unrelated race, causing #3286 ...

cdecker added this to the 0.7.4 milestone Nov 19, 2019

cdecker self-assigned this Nov 19, 2019

cdecker force-pushed the tlv-validation-split branch from a827c84 to 78ca90e Compare November 19, 2019 20:39

rustyrussell requested changes Nov 19, 2019

View reviewed changes

cdecker force-pushed the tlv-validation-split branch from 78ca90e to 08fc431 Compare November 20, 2019 13:13

cdecker mentioned this pull request Nov 20, 2019

Add createonion and sendonion RPC commands #3260

Merged

niftynei reviewed Nov 21, 2019

View reviewed changes

rustyrussell mentioned this pull request Nov 21, 2019

Payment secret support #3259

Merged

tlv: Add raw fields so we can store unknown fields as well

6ce68c6

cdecker force-pushed the tlv-validation-split branch 2 times, most recently from a8fbe3e to aecdf78 Compare November 21, 2019 16:33

cdecker requested review from niftynei and rustyrussell November 21, 2019 16:35

cdecker added 8 commits November 21, 2019 17:35

sphinx: Decode payload and place shortcuts in the route-step

af37d6e

We'll need to pass them around anyway, so just make them easier to access by doing a bit more to `process_onionpacket`.

htlcs: Make necessary payload fields optional and derfer validation

cc484fa

We make the fields in `htlc_accepted_payload` optional (NULL if not present in the payload) and defer validation till after the hook call.

htlc: Consolidate validation after the htlc_accepted hook returns

98e7c0f

This now enforces all rules for validity, both for the TLV format and checking that the required fields have been provided.

sphinx: Cleanup route_step_decode_* functions

2feb28e

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.

cdecker force-pushed the tlv-validation-split branch from aecdf78 to df4c187 Compare November 21, 2019 16:35

rustyrussell approved these changes Nov 22, 2019

View reviewed changes

rustyrussell merged commit d1df4d6 into ElementsProject:master Nov 22, 2019

cdecker mentioned this pull request Nov 26, 2019

TLV generation cleanup and testing #3297

Merged

Conversation

cdecker commented Nov 19, 2019

Uh oh!

rustyrussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdecker commented Nov 21, 2019

Uh oh!

rustyrussell commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants