plugin: Do not send the internal framed message over the wire

Looks like ElementsProject#4394 treated a symptom but not the root cause. We were
actually sending the message framed with the WIRE_CUSTOMMSG_OUT and
the length prefix over the encrypted connection to the peer. It just
happened to be a valid custommsg...

This fixes the issue, and this time I made sure we actually send the
raw message over the wire. However for backward compatibility we
needed to imitate the faulty behavior which is 90% of this patch :-)

Changelog-Fixed: plugin: `dev-sendcustommsg` included the type and length prefix when sending a message.

channeld/channeld.c

@@ -2881,7 +2881,10 @@ static void handle_dev_memleak(struct peer *peer, const u8 *msg)
  * just forward it here. */
 static void channeld_send_custommsg(struct peer *peer, const u8 *msg)
 {
-	sync_crypto_write(peer->pps, take(msg));
+	u8 *inner;
+	if (!fromwire_custommsg_out(tmpctx, msg, &inner))
+		master_badmsg(WIRE_CUSTOMMSG_OUT, msg);
+	sync_crypto_write(peer->pps, take(inner));
 }
 #endif /* DEVELOPER */
 

lightningd/peer_control.c

@@ -2521,16 +2521,52 @@ static void custommsg_final(struct custommsg_payload *payload STEALS)
 static void custommsg_payload_serialize(struct custommsg_payload *payload,
 					struct json_stream *stream)
 {
+	/* Backward compat for broken custommsg: if we get a custommsg
+	 * from an old c-lightning node, then we must identify and
+	 * strip the prefix from the payload. If it's a new one, we
+	 * need to add the frame for the `message` for backward
+	 * compatibility. */
+	size_t msglen = tal_bytelen(payload->msg), framedlen, unframedlen, max;
+	const u8 *unframed, *framed, *p = payload->msg;
+	u8 *tmp;
+	max = msglen;
+
+	if (msglen >= 4 && fromwire_u16(&p, &max) == WIRE_CUSTOMMSG_OUT &&
+	    fromwire_u16(&p, &max) == msglen - 4 && deprecated_apis) {
+		/* This is from an old c-lightning implementation that
+		 * erroneously sent the framed message over the
+		 * connection. */
+		unframed = payload->msg + 4;
+		unframedlen = msglen - 4;
+		framed = payload->msg;
+		framedlen = msglen;
+	} else {
+		/* This is from a new c-lightning, which correctly
+		 * sent the raw custommsg without framing. We still
+		 * need to reconstruct the wrong message since plugins
+		 * may rely on it. */
+		if (deprecated_apis) {
+			tmp = tal_arr(tmpctx, u8, 0);
+			towire_u16(&tmp, WIRE_CUSTOMMSG_OUT);
+			towire_u16(&tmp, msglen);
+			towire(&tmp, payload->msg, msglen);
+			framedlen = msglen + 4;
+			framed = tmp;
+		}
+
+		unframed = payload->msg;
+		unframedlen = msglen;
+	}
+
 	if (deprecated_apis) {
-		json_add_hex_talarr(stream, "message", payload->msg);
+		json_add_hex(stream, "message", framed, framedlen);
 		json_add_string(
 		    stream, "warning",
 		    "The `message` field is deprecated and has been replaced "
 		    "with the payload` field which skips the internal type and "
 		    "the length prefix. Please update to use that instead.");
 	}
-	json_add_hex(stream, "payload", payload->msg + 4,
-		     tal_bytelen(payload->msg) - 4);
+	json_add_hex(stream, "payload", unframed, unframedlen);
 	json_add_node_id(stream, "peer_id", &payload->peer_id);
 }
 

openingd/dualopend.c

@@ -803,7 +803,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg)
  * just forward it here. */
 static void dualopend_send_custommsg(struct state *state, const u8 *msg)
 {
-	sync_crypto_write(state->pps, take(msg));
+	u8 *inner;
+	if (!fromwire_custommsg_out(tmpctx, msg, &inner))
+		master_badmsg(WIRE_CUSTOMMSG_OUT, msg);
+	sync_crypto_write(state->pps, take(inner));
 }
 #endif
 

openingd/openingd.c

@@ -1242,7 +1242,10 @@ static void handle_dev_memleak(struct state *state, const u8 *msg)
  * just forward it here. */
 static void openingd_send_custommsg(struct state *state, const u8 *msg)
 {
-	sync_crypto_write(state->pps, take(msg));
+	u8 *inner;
+	if (!fromwire_custommsg_out(tmpctx, msg, &inner))
+		master_badmsg(WIRE_CUSTOMMSG_OUT, msg);
+	sync_crypto_write(state->pps, take(inner));
 }
 #endif /* DEVELOPER */
 

tests/test_misc.py

@@ -2250,8 +2250,7 @@ def test_sendcustommsg(node_factory):
     node_factory.join_nodes([l1, l2, l3])
     l2.connect(l4)
     l3.stop()
-    msg = r'ff' * 32
-    serialized = r'04070020' + msg
+    msg = 'aa' + ('ff' * 30) + 'bb'
 
     # This address doesn't exist so we should get an error when we try sending
     # a message to it.
@@ -2279,11 +2278,11 @@ def test_sendcustommsg(node_factory):
     # This should work since the peer is currently owned by `channeld`
     l2.rpc.dev_sendcustommsg(l1.info['id'], msg)
     l2.daemon.wait_for_log(
-        r'{peer_id}-{owner}-chan#[0-9]: \[OUT\] {serialized}'.format(
-            owner='channeld', serialized=serialized, peer_id=l1.info['id']
+        r'{peer_id}-{owner}-chan#[0-9]: \[OUT\] {msg}'.format(
+            owner='channeld', msg=msg, peer_id=l1.info['id']
         )
     )
-    l1.daemon.wait_for_log(r'\[IN\] {}'.format(serialized))
+    l1.daemon.wait_for_log(r'\[IN\] {}'.format(msg))
     l1.daemon.wait_for_logs([
         r'Got custommessage_a {msg} from peer {peer_id}'.format(
             msg=msg, peer_id=l2.info['id']),
@@ -2294,11 +2293,11 @@ def test_sendcustommsg(node_factory):
     # This should work since the peer is currently owned by `openingd`
     l2.rpc.dev_sendcustommsg(l4.info['id'], msg)
     l2.daemon.wait_for_log(
-        r'{peer_id}-{owner}-chan#[0-9]: \[OUT\] {serialized}'.format(
-            owner='openingd', serialized=serialized, peer_id=l4.info['id']
+        r'{peer_id}-{owner}-chan#[0-9]: \[OUT\] {msg}'.format(
+            owner='openingd', msg=msg, peer_id=l4.info['id']
         )
     )
-    l4.daemon.wait_for_log(r'\[IN\] {}'.format(serialized))
+    l4.daemon.wait_for_log(r'\[IN\] {}'.format(msg))
     l4.daemon.wait_for_logs([
         r'Got custommessage_a {msg} from peer {peer_id}'.format(
             msg=msg, peer_id=l2.info['id']),