Merge df4c187 into 654faa6

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;
@@ -679,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.
@@ -750,6 +800,8 @@ struct route_step *process_onionpacket(
 		step->nextcase = ONION_FORWARD;
 	}
 
+	route_step_decode(step);
+
 	return step;
 }
 
@@ -880,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();
-}

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;
 };
 
 /**
@@ -244,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 */

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,
@@ -56,12 +59,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(); }

lightningd/peer_htlcs.c

@@ -31,6 +31,10 @@
 #include <wallet/wallet.h>
 #include <wire/gen_onion_wire.h>
 
+#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)
@@ -637,8 +641,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 +751,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);
@@ -761,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
  */
@@ -781,13 +825,17 @@ htlc_accepted_hook_callback(struct htlc_accepted_hook_payload *request,
 
 	switch (result) {
 	case htlc_accepted_continue:
-		if (rs->nextcase == ONION_FORWARD) {
+		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);
+		}else if (rs->nextcase == ONION_FORWARD) {
 			struct gossip_resolve *gr = tal(ld, struct gossip_resolve);
 
 			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);
@@ -798,8 +846,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,
@@ -923,29 +971,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);

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
@@ -61,6 +66,19 @@ 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);
+
+/**
+ * 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)}