sphinx: rename confusing functions, ensure valid payloads.

"sphinx_add_hop" takes a literal hop to include,
"sphinx_add_modern_hop" prepends the length.  Now we always prepend a
length, make it clear that the literal version is a shortcut:

* sphinx_add_hop -> sphinx_add_hop_has_length
* sphinx_add_modern_hop -> sphinx_add_hop

In addition, we check that length is actually correct!  This means
`createonion` can no longer create legacy or otherwise-invalid onions:
fix tests and update man page to remove legacy usage.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: JSON-RPC: `createonion` no longer allows non-TLV-style payloads.

common/sphinx.c

@@ -4,6 +4,7 @@
 #include <ccan/mem/mem.h>
 #include <common/onion.h>
 #include <common/onionreply.h>
+#include <common/overflows.h>
 #include <common/sphinx.h>
 
 
@@ -103,17 +104,29 @@ size_t sphinx_path_payloads_size(const struct sphinx_path *path)
 	return size;
 }
 
-void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey,
+bool sphinx_add_hop_has_length(struct sphinx_path *path, const struct pubkey *pubkey,
-		    const u8 *payload TAKES)
+			       const u8 *payload TAKES)
 {
 	struct sphinx_hop sp;
+	bigsize_t lenlen, prepended_len;
+
+	/* You promised size was prepended! */
+	if (tal_bytelen(payload) == 0)
+		return false;
+	lenlen = bigsize_get(payload, tal_bytelen(payload), &prepended_len);
+	if (add_overflows_u64(lenlen, prepended_len))
+		return false;
+	if (lenlen + prepended_len != tal_bytelen(payload))
+		return false;
+
 	sp.raw_payload = tal_dup_talarr(path, u8, payload);
 	sp.pubkey = *pubkey;
 	tal_arr_expand(&path->hops, sp);
+	return true;
 }
 
-void sphinx_add_modern_hop(struct sphinx_path *path, const struct pubkey *pubkey,
+void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey,
-			   const u8 *payload TAKES)
+		    const u8 *payload TAKES)
 {
 	u8 *with_len = tal_arr(NULL, u8, 0);
 	size_t len = tal_bytelen(payload);
@@ -122,7 +135,8 @@ void sphinx_add_modern_hop(struct sphinx_path *path, const struct pubkey *pubkey
 	if (taken(payload))
 		tal_free(payload);
 
-	sphinx_add_hop(path, pubkey, take(with_len));
+	if (!sphinx_add_hop_has_length(path, pubkey, take(with_len)))
+		abort();
 }
 
 /* Small helper to append data to a buffer and update the position

common/sphinx.h

@@ -202,17 +202,19 @@ struct sphinx_path *sphinx_path_new_with_key(const tal_t *ctx,
 					     const struct secret *session_key);
 
 /**
- * Add a payload hop to the path.
+ * Add a payload hop to the path (already has length prepended).
+ *
+ * Fails if length actually isn't prepended!
  */
-void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey,
+bool sphinx_add_hop_has_length(struct sphinx_path *path, const struct pubkey *pubkey,
-		    const u8 *payload TAKES);
+			       const u8 *payload TAKES);
 
 /**
  * Prepend length to payload and add: for onionmessage, any size is OK,
  * for HTLC onions tal_bytelen(payload) must be > 1.
  */
-void sphinx_add_modern_hop(struct sphinx_path *path, const struct pubkey *pubkey,
+void sphinx_add_hop(struct sphinx_path *path, const struct pubkey *pubkey,
-			   const u8 *payload TAKES);
+		    const u8 *payload TAKES);
 
 /**
  * Compute the size of the serialized payloads.

common/test/run-blindedpath_onion.c

@@ -204,8 +204,7 @@ int main(int argc, char *argv[])
 		payload->encrypted_data_tlv = enctlv[i];
 		onionmsg_payload[i] = tal_arr(tmpctx, u8, 0);
 		towire_tlv_onionmsg_payload(&onionmsg_payload[i], payload);
-		sphinx_add_modern_hop(sphinx_path, &alias[i],
+		sphinx_add_hop(sphinx_path, &alias[i], onionmsg_payload[i]);
-				      onionmsg_payload[i]);
 	}
 	op = create_onionpacket(tmpctx, sphinx_path, ROUTING_INFO_SIZE,
 				&path_secrets);

common/test/run-onion-test-vector.c

@@ -162,7 +162,7 @@ int main(int argc, char *argv[])
 		max = tal_bytelen(payloads[i]);
 		len = fromwire_bigsize(&cursor, &max);
 		assert(len == max);
-		sphinx_add_modern_hop(sp, &k, take(tal_dup_arr(NULL, u8, cursor, max, 0)));
+		sphinx_add_hop(sp, &k, take(tal_dup_arr(NULL, u8, cursor, max, 0)));
 	}
 	assert(i == ARRAY_SIZE(payloads));
 

common/test/run-sphinx-xor_cipher_stream.c

@@ -35,6 +35,9 @@ struct amount_asset amount_sat_to_asset(struct amount_sat *sat UNNEEDED, const u
 /* Generated stub for amount_tx_fee */
 struct amount_sat amount_tx_fee(u32 fee_per_kw UNNEEDED, size_t weight UNNEEDED)
 { fprintf(stderr, "amount_tx_fee called!\n"); abort(); }
+/* Generated stub for bigsize_get */
+size_t bigsize_get(const u8 *p UNNEEDED, size_t max UNNEEDED, bigsize_t *val UNNEEDED)
+{ fprintf(stderr, "bigsize_get called!\n"); abort(); }
 /* Generated stub for fromwire */
 const u8 *fromwire(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, void *copy UNNEEDED, size_t n UNNEEDED)
 { fprintf(stderr, "fromwire called!\n"); abort(); }

common/test/run-sphinx.c

@@ -45,6 +45,9 @@ struct amount_asset amount_sat_to_asset(struct amount_sat *sat UNNEEDED, const u
 /* Generated stub for amount_tx_fee */
 struct amount_sat amount_tx_fee(u32 fee_per_kw UNNEEDED, size_t weight UNNEEDED)
 { fprintf(stderr, "amount_tx_fee called!\n"); abort(); }
+/* Generated stub for bigsize_get */
+size_t bigsize_get(const u8 *p UNNEEDED, size_t max UNNEEDED, bigsize_t *val UNNEEDED)
+{ fprintf(stderr, "bigsize_get called!\n"); abort(); }
 /* Generated stub for bigsize_put */
 size_t bigsize_put(u8 buf[BIGSIZE_MAX_LEN] UNNEEDED, bigsize_t v UNNEEDED)
 { fprintf(stderr, "bigsize_put called!\n"); abort(); }

devtools/onion.c

@@ -67,7 +67,7 @@ static void do_generate(int argc, char **argv,
 
 			if (!data)
 				errx(1, "bad hex after / in %s", argv[1 + i]);
-			sphinx_add_hop(sp, &path[i], data);
+			sphinx_add_hop_has_length(sp, &path[i], data);
 		} else {
 			struct short_channel_id scid;
 			struct amount_msat amt;
@@ -76,13 +76,13 @@ static void do_generate(int argc, char **argv,
 			memset(&scid, i, sizeof(scid));
 			amt = amount_msat(i);
 			if (i == num_hops - 1)
-				sphinx_add_hop(sp, &path[i],
+				sphinx_add_hop_has_length(sp, &path[i],
 					       take(onion_final_hop(NULL,
 								    amt, i, amt,
 								    NULL, NULL,
 								    NULL, NULL)));
 			else
-				sphinx_add_hop(sp, &path[i],
+				sphinx_add_hop_has_length(sp, &path[i],
 					       take(onion_nonfinal_hop(NULL,
 								       &scid,
 								       amt, i,
@@ -225,27 +225,13 @@ static void runtest(const char *filename)
 	/* Unpack the hops and build up the path */
 	hopstok = json_get_member(buffer, gentok, "hops");
 	json_for_each_arr(i, hop, hopstok) {
-		u8 *full;
-		size_t prepended;
-
 		payloadtok = json_get_member(buffer, hop, "payload");
 		typetok = json_get_member(buffer, hop, "type");
 		pubkeytok = json_get_member(buffer, hop, "pubkey");
 		payload = json_tok_bin_from_hex(ctx, buffer, payloadtok);
 		json_to_pubkey(buffer, pubkeytok, &pubkey);
-		if (!typetok || json_tok_streq(buffer, typetok, "legacy")) {
+		assert(json_tok_streq(buffer, typetok, "tlv"));
-			/* Legacy has a single 0 prepended as "realm" byte */
+		sphinx_add_hop(path, &pubkey, take(payload));
-			full = tal_arrz(ctx, u8, 33);
-			memcpy(full + 1, payload, tal_bytelen(payload));
-		} else {
-			/* TLV has length prepended */
-			full = tal_arr(ctx, u8, 0);
-			towire_bigsize(&full, tal_bytelen(payload));
-			prepended = tal_bytelen(full);
-			tal_resize(&full, prepended + tal_bytelen(payload));
-			memcpy(full + prepended, payload, tal_bytelen(payload));
-		}
-		sphinx_add_hop(path, &pubkey, full);
 	}
 	res = create_onionpacket(ctx, path, ROUTING_INFO_SIZE, &shared_secrets);
 	serialized = serialize_onionpacket(ctx, res);

doc/lightning-createonion.7.md

@@ -20,14 +20,13 @@ payload destined for that node. The following is an example of a 3 hop onion:
 [
 	{
 		"pubkey": "022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59",
-		"payload": "00000067000001000100000000000003e90000007b000000000000000000000000000000000000000000000000"
+		"payload": "11020203e904017b06080000670000010001"
 	}, {
 		"pubkey": "035d2b1192dfba134e10e540875d366ebc8bc353d5aa766b80c090b39c3a5d885d",
-		"payload": "00000067000003000100000000000003e800000075000000000000000000000000000000000000000000000000"
+		"payload": "11020203e804017506080000670000030001"
 	}, {
-		"style": "legacy",
 		"pubkey": "0382ce59ebf18be7d84677c2e35f23294b9992ceca95491fcf8a56c6cb2d9de199",
-		"payload": "00000067000003000100000000000003e800000075000000000000000000000000000000000000000000000000"
+		"payload": "07020203e8040175"
 	}
 ]
 ```
@@ -63,8 +62,8 @@ which the above *hops* parameter was generated:
 ]
 ```
 
- - Notice that the payload in the *hops* parameter is the hex-encoded version
+ - Notice that the payload in the *hops* parameter is the hex-encoded TLV
-   of the parameters in the `getroute` response.
+   of the parameters in the `getroute` response, with length prepended as a `bigsize_t`.
  - Except for the pubkey, the values are shifted left by one, i.e., the 1st
    payload in `createonion` corresponds to the 2nd set of values from `getroute`.
  - The final payload is a copy of the last payload sans `channel`

lightningd/onion_message.c

@@ -281,7 +281,7 @@ static struct command_result *json_sendonionmessage2(struct command *cmd,
 	/* Create an onion which encodes this. */
 	sphinx_path = sphinx_path_new(cmd, NULL);
 	for (size_t i = 0; i < tal_count(hops); i++)
-		sphinx_add_modern_hop(sphinx_path, &hops[i].node, hops[i].tlv);
+		sphinx_add_hop(sphinx_path, &hops[i].node, hops[i].tlv);
 
 	/* BOLT-onion-message #4:
 	 * - SHOULD set `len` to 1366 or 32834.

lightningd/pay.c

@@ -1168,7 +1168,7 @@ send_payment(struct lightningd *ld,
 		ret = pubkey_from_node_id(&pubkey, &ids[i]);
 		assert(ret);
 
-		sphinx_add_hop(path, &pubkey,
+		sphinx_add_hop_has_length(path, &pubkey,
 			       take(onion_nonfinal_hop(NULL,
 					&route[i + 1].scid,
 					route[i + 1].amount,
@@ -1200,7 +1200,7 @@ send_payment(struct lightningd *ld,
 				    "Destination does not support"
 				    " payment_secret");
 	}
-	sphinx_add_hop(path, &pubkey, onion);
+	sphinx_add_hop_has_length(path, &pubkey, onion);
 
 	/* Copy channels used along the route. */
 	channels = tal_arr(tmpctx, struct short_channel_id, n_hops);
@@ -1760,8 +1760,12 @@ static struct command_result *json_createonion(struct command *cmd,
 	else
 		sp = sphinx_path_new_with_key(cmd, assocdata, session_key);
 
-	for (size_t i=0; i<tal_count(hops); i++)
+	for (size_t i=0; i<tal_count(hops); i++) {
-		sphinx_add_hop(sp, &hops[i].pubkey, hops[i].raw_payload);
+		if (!sphinx_add_hop_has_length(sp, &hops[i].pubkey, hops[i].raw_payload))
+			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+					    "hops[%zi] payload is not prefixed with length!",
+					    i);
+	}
 
 	if (sphinx_path_payloads_size(sp) > *packet_size)
 		return command_fail(

tests/test_pay.py

@@ -3297,19 +3297,19 @@ def test_createonion_limits(node_factory):
     hops = [{
         # privkey: 41bfd2660762506c9933ade59f1debf7e6495b10c14a92dbcd2d623da2507d3d
         "pubkey": "0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518",
-        "payload": "00" * 228
+        "payload": bytes([227] + [0] * 227).hex(),
     }, {
         "pubkey": "0324653eac434488002cc06bbfb7f10fe18991e35f9fe4302dbea6d2353dc0ab1c",
-        "payload": "00" * 228
+        "payload": bytes([227] + [0] * 227).hex(),
     }, {
         "pubkey": "027f31ebc5462c1fdce1b737ecff52d37d75dea43ce11c74d25aa297165faa2007",
-        "payload": "00" * 228
+        "payload": bytes([227] + [0] * 227).hex(),
     }, {
         "pubkey": "032c0b7cf95324a07d05398b240174dc0c2be444d96b159aa6c7f7b1e668680991",
-        "payload": "00" * 228
+        "payload": bytes([227] + [0] * 227).hex(),
     }, {
         "pubkey": "02edabbd16b41c8371b92ef2f04c1185b4f03b6dcd52ba9b78d9d7c89c8f221145",
-        "payload": "00" * 228
+        "payload": bytes([227] + [0] * 227).hex(),
     }]
 
     # This should success since it's right at the edge
@@ -3317,7 +3317,7 @@ def test_createonion_limits(node_factory):
 
     # This one should fail however
     with pytest.raises(RpcError, match=r'Payloads exceed maximum onion packet size.'):
-        hops[0]['payload'] += '01'
+        hops[0]['payload'] = bytes([228] + [0] * 228).hex()
         l1.rpc.createonion(hops=hops, assocdata="BB" * 32)
 
     # But with a larger onion, it will work!