lightningd: don't assume peer existrs in peer_connected_serialize.
What the developer wrote
Authored by Rusty Russell
lightningd: don't assume peer existrs in peer_connected_serialize.
It's always true for the first hook invocation, but if there is more
than one plugin, it could vanish between the two! In the default configuration, this can't happen.
This bug has been around since v23.02.
Note: we always tell all the plugins about the peer, even if it's
already gone.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Fixed: lightningd: possible crash when peers disconnected if there was more than one plugin servicing the `peer_connected` hook.
Reported-by: https://github.com/santyr
Fixes: https://github.com/ElementsProject/lightning/issues/8858
What changed, and why it matters
This commit fixes a crash bug in Core Lightning's lightningd daemon. When a peer disconnects between two plugin hook invocations, the code could try to read from a peer object that no longer exists, causing a use-after-free style crash. The fix copies the needed peer feature data into the hook payload so it remains valid even if the peer disappears. By default this cannot happen because only one plugin handles the hook, but it becomes possible when more than one plugin is registered.
Upgrade to a Core Lightning release containing this commit if you run multiple plugins that service the peer_connected hook. As a workaround, avoid chaining multiple plugins on the peer_connected hook. No immediate remote exploitability is evident, but the crash is a denial-of-service risk for affected node operators.
Security signals we found
use-after-free / dangling pointer risk in peer_connected_serialize
peer object dereferenced after potential free between plugin hook callbacks
crash on peer disconnection with multiple peer_connected hook plugins
Changelog-Fixed labels this as a possible crash
Evidence from the diff
In peer_connected_serialize(), the code previously called peer_by_id() and dereferenced the returned peer to access their_features. Between multiple plugin hook invocations, the peer could be freed, leading to a dangling pointer dereference. The fix adds a their_features field to struct peer_connected_hook_payload, copies the features into the payload during message parsing, and serializes from the payload instead of looking up the peer. The test test_plugin_connected_hook_disconnect_crash is also re-enabled (xfail marker removed).
Changed components
lightningd/peer_control.cstruct peer_connected_hook_payloadpeer_connected_serialize()handle_peer_connected()tests/test_plugin.py::test_plugin_connected_hook_disconnect_crashInspect captured patch +8 / −10
diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c
index 2ec8bea3..6c121690 100644
--- a/lightningd/peer_control.c
+++ b/lightningd/peer_control.c
@@ -1309,6 +1309,7 @@ struct peer_connected_hook_payload {
struct lightningd *ld;
struct wireaddr_internal addr;
struct wireaddr *remote_addr;
+ u8 *their_features;
bool incoming;
/* We don't keep a pointer to peer: it might be freed! */
struct node_id peer_id;
@@ -1327,10 +1328,7 @@ peer_connected_serialize(struct peer_connected_hook_payload *payload,
if (payload->remote_addr)
json_add_string(stream, "remote_addr",
fmt_wireaddr(tmpctx, payload->remote_addr));
- /* Since this is start of hook, peer is always in table! */
- json_add_hex_talarr(stream, "features",
- peer_by_id(payload->ld, &payload->peer_id)
- ->their_features);
+ json_add_hex_talarr(stream, "features", payload->their_features);
json_object_end(stream); /* .peer */
}
@@ -1759,7 +1757,6 @@ REGISTER_PLUGIN_HOOK(peer_connected,
void handle_peer_connected(struct lightningd *ld, const u8 *msg)
{
struct node_id id;
- u8 *their_features;
struct peer *peer;
struct peer_connected_hook_payload *hook_payload;
u64 connectd_counter;
@@ -1776,7 +1773,7 @@ void handle_peer_connected(struct lightningd *ld, const u8 *msg)
&hook_payload->addr,
&hook_payload->remote_addr,
&hook_payload->incoming,
- &their_features,
+ &hook_payload->their_features,
&connect_reason,
&connect_nsec)) {
u64 prev_connectd_counter, connected_time_nsec;
@@ -1786,7 +1783,7 @@ void handle_peer_connected(struct lightningd *ld, const u8 *msg)
&hook_payload->addr,
&hook_payload->remote_addr,
&hook_payload->incoming,
- &their_features,
+ &hook_payload->their_features,
&connected_time_nsec)) {
fatal("Connectd gave bad CONNECT_PEER_(RE)CONNECTED message %s",
tal_hex(msg, msg));
@@ -1830,10 +1827,12 @@ void handle_peer_connected(struct lightningd *ld, const u8 *msg)
/* If we connected to them, we know this is a good address. */
peer = new_peer(ld, 0, &id, &hook_payload->addr,
last_known_addr,
- take(their_features), hook_payload->incoming);
+ hook_payload->their_features,
+ hook_payload->incoming);
} else {
tal_free(peer->their_features);
- peer->their_features = tal_steal(peer, their_features);
+ peer->their_features = tal_dup_talarr(peer, u8,
+ hook_payload->their_features);
/* Update known address. */
tal_free(peer->last_known_addr);
diff --git a/tests/test_plugin.py b/tests/test_plugin.py
index b40cb515..b61d1cd4 100644
--- a/tests/test_plugin.py
+++ b/tests/test_plugin.py
@@ -514,7 +514,6 @@ def test_plugin_connected_hook_chaining(node_factory):
assert not l1.daemon.is_in_log(f"peer_connected_logger_b {l3id}")
-@pytest.mark.xfail(strict=True)
def test_plugin_connected_hook_disconnect_crash(node_factory, executor):
"""A peer disconnnects between plugin hook invocations"""
opts = [{},
Why this scored 43/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.