From 21355edc431319c2a5823f6b8ce3c22c9bd12efe Mon Sep 17 00:00:00 2001 From: Christian Decker Date: Fri, 5 Mar 2021 19:02:00 +0100 Subject: [PATCH] plugin: Do not send the internal framed message over the wire Looks like #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 | 5 ++++- lightningd/peer_control.c | 42 ++++++++++++++++++++++++++++++++++++--- openingd/dualopend.c | 5 ++++- openingd/openingd.c | 5 ++++- tests/test_misc.py | 15 +++++++------- 5 files changed, 58 insertions(+), 14 deletions(-) diff --git a/channeld/channeld.c b/channeld/channeld.c index 7b7849ec7..5bbcaeb39 100644 --- a/channeld/channeld.c +++ b/channeld/channeld.c @@ -2880,7 +2880,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 */ diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index b6b15d1b1..48a34b5e1 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -2568,16 +2568,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); } diff --git a/openingd/dualopend.c b/openingd/dualopend.c index 18ea4598e..c7ea5b448 100644 --- a/openingd/dualopend.c +++ b/openingd/dualopend.c @@ -795,7 +795,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 diff --git a/openingd/openingd.c b/openingd/openingd.c index ca0d7285e..1e55e834e 100644 --- a/openingd/openingd.c +++ b/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 */ diff --git a/tests/test_misc.py b/tests/test_misc.py index d6a3e02ea..dba3c0a30 100644 --- a/tests/test_misc.py +++ b/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']),