From 5ded0ec8c5e4b8c7676571f2e28167afe537861d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Mon, 20 Jul 2026 11:56:42 +0200 Subject: [PATCH] Gate identity learning on signature in NodeDB::updateUser (#11084) --- src/mesh/NodeDB.cpp | 9 ++- src/mesh/NodeDB.h | 6 +- src/modules/NodeInfoModule.cpp | 4 +- src/modules/TrafficManagementModule.cpp | 2 +- test/test_packet_signing/test_main.cpp | 82 ++++++++++++++++++++++++- 5 files changed, 96 insertions(+), 7 deletions(-) diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index a33f08389..ef8b3c9f6 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -3310,13 +3310,20 @@ void NodeDB::addFromContact(meshtastic_SharedContact contact) /** Update user info and channel for this node based on received user data */ -bool NodeDB::updateUser(uint32_t nodeId, meshtastic_User &p, uint8_t channelIndex) +bool NodeDB::updateUser(uint32_t nodeId, meshtastic_User &p, uint8_t channelIndex, bool xeddsaSigned) { meshtastic_NodeInfoLite *info = getOrCreateMeshNode(nodeId); if (!info) { return false; } + // Once a node has proven it signs, only a signed update may change its identity. The public-key guard + // below is no help - an attacker can replay the victim's real (public) key. Our own record is exempt. + if (nodeId != getNodeNum() && nodeInfoLiteHasXeddsaSigned(info) && !xeddsaSigned) { + LOG_WARN("Refusing unsigned identity update for node 0x%08x that previously signed", nodeId); + return false; + } + #if !(MESHTASTIC_EXCLUDE_PKI) if (p.public_key.size == 32 && nodeId != nodeDB->getNodeNum()) { printBytes("Incoming Pubkey: ", p.public_key.bytes, 32); diff --git a/src/mesh/NodeDB.h b/src/mesh/NodeDB.h index f7494811f..fafe95e56 100644 --- a/src/mesh/NodeDB.h +++ b/src/mesh/NodeDB.h @@ -255,9 +255,9 @@ class NodeDB */ void updateTelemetry(uint32_t nodeId, const meshtastic_Telemetry &t, RxSource src = RX_SRC_RADIO); - /** Update user info and channel for this node based on received user data - */ - bool updateUser(uint32_t nodeId, meshtastic_User &p, uint8_t channelIndex = 0); + /** Update user info and channel for this node based on received user data. + * A known signer's identity is only learned when xeddsaSigned; defaults false so callers fail closed. */ + bool updateUser(uint32_t nodeId, meshtastic_User &p, uint8_t channelIndex = 0, bool xeddsaSigned = false); /* * Sets a node either favorite or unfavorite. Returns true if the node ends diff --git a/src/modules/NodeInfoModule.cpp b/src/modules/NodeInfoModule.cpp index 0e40daf10..f7bfcf2fe 100644 --- a/src/modules/NodeInfoModule.cpp +++ b/src/modules/NodeInfoModule.cpp @@ -61,7 +61,9 @@ bool NodeInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, mes // Coerce user.id to be derived from the node number snprintf(p.id, sizeof(p.id), "!%08x", getFrom(&mp)); - bool hasChanged = nodeDB->updateUser(getFrom(&mp), p, mp.channel); + // updateUser() refuses the identity write for a known signer sending unsigned (all unicast + // NodeInfo), so the exchange above still proceeds but cannot spoof the stored name. + bool hasChanged = nodeDB->updateUser(getFrom(&mp), p, mp.channel, mp.xeddsa_signed); bool wasBroadcast = isBroadcast(mp.to); diff --git a/src/modules/TrafficManagementModule.cpp b/src/modules/TrafficManagementModule.cpp index fcda98980..ea5054ded 100644 --- a/src/modules/TrafficManagementModule.cpp +++ b/src/modules/TrafficManagementModule.cpp @@ -734,7 +734,7 @@ ProcessMessage TrafficManagementModule::handleReceived(const meshtastic_MeshPack meshtastic_User requester = meshtastic_User_init_zero; if (!unauthenticatedSigner && pb_decode_from_bytes(mp.decoded.payload.bytes, mp.decoded.payload.size, &meshtastic_User_msg, &requester)) { - nodeDB->updateUser(getFrom(&mp), requester, mp.channel); + nodeDB->updateUser(getFrom(&mp), requester, mp.channel, mp.xeddsa_signed); } logAction("respond", &mp, "nodeinfo-cache"); incrementStat(&stats.nodeinfo_cache_hits); diff --git a/test/test_packet_signing/test_main.cpp b/test/test_packet_signing/test_main.cpp index 3bd6f93fc..5ae49a462 100644 --- a/test/test_packet_signing/test_main.cpp +++ b/test/test_packet_signing/test_main.cpp @@ -81,6 +81,21 @@ class MockNodeDB : public NodeDB nodeInfoLiteSetBit(n, NODEINFO_BITFIELD_HAS_XEDDSA_SIGNED_MASK, value); } + void setLongName(NodeNum num, const char *name) + { + meshtastic_NodeInfoLite *n = getMeshNode(num); + TEST_ASSERT_NOT_NULL(n); + strncpy(n->long_name, name, sizeof(n->long_name) - 1); + n->long_name[sizeof(n->long_name) - 1] = '\0'; + } + + const char *longName(NodeNum num) + { + meshtastic_NodeInfoLite *n = getMeshNode(num); + TEST_ASSERT_NOT_NULL(n); + return n->long_name; + } + std::vector testNodes; }; @@ -543,7 +558,8 @@ void test_B6_rich_shape_sweep_no_deadband(void) // =========================================================================== // Group C - NodeInfoModule downgrade drop (broadcast-only backstop for ingress paths that skip -// Router's check; unicast NodeInfo is never signed by senders, so it is exempt - see C4) +// Router's check; unicast NodeInfo is never signed by senders, so it is exempt - see C4), plus the +// identity-learning gate in NodeDB::updateUser that keeps that exemption from being a spoofing hole // =========================================================================== class NodeInfoTestShim : public NodeInfoModule { @@ -618,6 +634,67 @@ void test_C4_unsigned_unicast_nodeinfo_from_signer_accepted(void) "unsigned unicast NodeInfo from a signer must not be dropped"); } +// C5: the packet survives (C4) but the identity claim inside it must not land - the pubkey guard +// can't tell a signer from an impersonator replaying its (public) key. Only the write is refused. +void test_C5_unsigned_unicast_nodeinfo_from_signer_does_not_change_name(void) +{ + mockNodeDB->addNode(REMOTE_NODE); + mockNodeDB->setSignerBit(REMOTE_NODE, true); + mockNodeDB->setLongName(REMOTE_NODE, "Genuine"); + + NodeInfoTestShim shim; + meshtastic_MeshPacket mp = makeDecoded(REMOTE_NODE, LOCAL_NODE, meshtastic_PortNum_NODEINFO_APP, SMALL_PAYLOAD); + mp.xeddsa_signed = false; + meshtastic_User user = meshtastic_User_init_zero; + user.is_licensed = owner.is_licensed; + strcpy(user.long_name, "Spoofed"); + strcpy(user.short_name, "SPF"); + + TEST_ASSERT_FALSE_MESSAGE(shim.handleReceivedProtobuf(mp, &user), "the packet itself must still be accepted"); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Genuine", mockNodeDB->longName(REMOTE_NODE), + "unsigned unicast NodeInfo from a signer must not rewrite its stored name"); +} + +// C6: the same exchange signed - the update is authenticated and must land, pinning C5 as a +// targeted refusal rather than a blanket block on unicast NodeInfo from signers. +void test_C6_signed_unicast_nodeinfo_from_signer_changes_name(void) +{ + mockNodeDB->addNode(REMOTE_NODE); + mockNodeDB->setSignerBit(REMOTE_NODE, true); + mockNodeDB->setLongName(REMOTE_NODE, "Genuine"); + + NodeInfoTestShim shim; + meshtastic_MeshPacket mp = makeDecoded(REMOTE_NODE, LOCAL_NODE, meshtastic_PortNum_NODEINFO_APP, SMALL_PAYLOAD); + mp.xeddsa_signed = true; + meshtastic_User user = meshtastic_User_init_zero; + user.is_licensed = owner.is_licensed; + strcpy(user.long_name, "Renamed"); + strcpy(user.short_name, "RNM"); + + TEST_ASSERT_FALSE(shim.handleReceivedProtobuf(mp, &user)); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Renamed", mockNodeDB->longName(REMOTE_NODE), + "a signed update from a signer must still be learned"); +} + +// C7: a node that has never signed is unaffected - the ordinary case for most of the mesh. +void test_C7_unsigned_unicast_nodeinfo_from_nonsigner_changes_name(void) +{ + mockNodeDB->addNode(REMOTE_NODE); // signer bit clear + mockNodeDB->setLongName(REMOTE_NODE, "Genuine"); + + NodeInfoTestShim shim; + meshtastic_MeshPacket mp = makeDecoded(REMOTE_NODE, LOCAL_NODE, meshtastic_PortNum_NODEINFO_APP, SMALL_PAYLOAD); + mp.xeddsa_signed = false; + meshtastic_User user = meshtastic_User_init_zero; + user.is_licensed = owner.is_licensed; + strcpy(user.long_name, "Renamed"); + strcpy(user.short_name, "RNM"); + + TEST_ASSERT_FALSE(shim.handleReceivedProtobuf(mp, &user)); + TEST_ASSERT_EQUAL_STRING_MESSAGE("Renamed", mockNodeDB->longName(REMOTE_NODE), + "non-signer identity learning must be unaffected"); +} + // =========================================================================== // Group D - encoding invariants the routing gates depend on // =========================================================================== @@ -900,6 +977,9 @@ void setup() RUN_TEST(test_C2_signed_nodeinfo_from_signer_not_dropped); RUN_TEST(test_C3_unsigned_nodeinfo_from_nonsigner_not_dropped); RUN_TEST(test_C4_unsigned_unicast_nodeinfo_from_signer_accepted); + RUN_TEST(test_C5_unsigned_unicast_nodeinfo_from_signer_does_not_change_name); + RUN_TEST(test_C6_signed_unicast_nodeinfo_from_signer_changes_name); + RUN_TEST(test_C7_unsigned_unicast_nodeinfo_from_nonsigner_changes_name); printf("\n=== Group D: encoding invariants ===\n"); RUN_TEST(test_D1_signature_field_overhead_exact);