From 077c6e72f411d5b0ee31341a567175e954980d31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Tue, 21 Jul 2026 09:11:05 +0200 Subject: [PATCH] Corroborate traceroute next-hop updates against the relaying node (#11108) --- src/modules/TraceRouteModule.cpp | 9 ++ test/test_traceroute_nexthop/test_main.cpp | 162 +++++++++++++++++++++ 2 files changed, 171 insertions(+) create mode 100644 test/test_traceroute_nexthop/test_main.cpp diff --git a/src/modules/TraceRouteModule.cpp b/src/modules/TraceRouteModule.cpp index 11019dcf9..7cfe3259f 100644 --- a/src/modules/TraceRouteModule.cpp +++ b/src/modules/TraceRouteModule.cpp @@ -305,6 +305,15 @@ void TraceRouteModule::updateNextHops(const meshtastic_MeshPacket &p, meshtastic } uint8_t nextHopByte = nodeDB->getLastByteOfNodeNum(nextHop); + // The route array is unauthenticated payload, so only learn from it when the node it names as our + // next hop is the one that actually relayed this packet to us. Otherwise a forged response could + // point any node's next_hop anywhere. relay_node is 0 for MQTT-sourced packets, which cannot + // corroborate an RF route either. + if (p.relay_node == NO_RELAY_NODE || nextHopByte != p.relay_node) { + LOG_DEBUG("Ignore traceroute next-hop 0x%02x, packet was relayed by 0x%02x", nextHopByte, p.relay_node); + return; + } + // For the rest of the nodes in the route, set their next-hop // Note: if we are the last in the route, this loop will not run for (int8_t i = nextHopIndex; i < r->route_count; i++) { diff --git a/test/test_traceroute_nexthop/test_main.cpp b/test/test_traceroute_nexthop/test_main.cpp new file mode 100644 index 000000000..5f8d546db --- /dev/null +++ b/test/test_traceroute_nexthop/test_main.cpp @@ -0,0 +1,162 @@ +// Tests that TraceRouteModule only learns next_hop from a traceroute response when the route array +// agrees with the node that actually relayed the packet. The route is unauthenticated payload, so +// without that check a forged response could point any node's next_hop anywhere. + +#include "MeshTypes.h" // include BEFORE TestUtil.h +#include "TestUtil.h" +#include + +#include "mesh/NodeDB.h" +#include "modules/TraceRouteModule.h" +#include + +static constexpr NodeNum LOCAL_NODE = 0x0A0A0A0A; // us, the node that asked for the traceroute +static constexpr NodeNum RELAY_B = 0x0B0B0B0B; // the honest first hop back towards us +static constexpr NodeNum NODE_C = 0x0C0C0C0C; +static constexpr NodeNum NODE_D = 0x0D0D0D0D; // the traceroute target, which answers +static constexpr uint8_t ATTACKER_RELAY_BYTE = 0xEE; // some node that is not RELAY_B + +class MockNodeDB : public NodeDB +{ + public: + void clearTestNodes() + { + testNodes.clear(); + meshNodes = &testNodes; + numMeshNodes = 0; + } + + void addNode(NodeNum num) + { + meshtastic_NodeInfoLite node = meshtastic_NodeInfoLite_init_zero; + node.num = num; + testNodes.push_back(node); + meshNodes = &testNodes; + numMeshNodes = testNodes.size(); + } + + uint8_t nextHopOf(NodeNum num) + { + meshtastic_NodeInfoLite *n = getMeshNode(num); + TEST_ASSERT_NOT_NULL(n); + return n->next_hop; + } + + std::vector testNodes; +}; + +// alterReceivedProtobuf is the real entry point; updateNextHops is private behind it. +class TraceRouteModuleTestShim : public TraceRouteModule +{ + public: + using TraceRouteModule::alterReceivedProtobuf; +}; + +static MockNodeDB *mockNodeDB = nullptr; +static TraceRouteModuleTestShim *shim = nullptr; + +// A traceroute response addressed to us, claiming the forward route LOCAL -> RELAY_B -> NODE_C -> NODE_D, +// carried to us by whichever node `relayByte` names. +static meshtastic_MeshPacket makeResponse(uint8_t relayByte, meshtastic_RouteDiscovery *r) +{ + *r = meshtastic_RouteDiscovery_init_zero; + r->route_count = 3; + r->route[0] = RELAY_B; + r->route[1] = NODE_C; + r->route[2] = NODE_D; + + meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; + p.from = NODE_D; // the target answered + p.to = LOCAL_NODE; + p.id = 0x1234; + p.relay_node = relayByte; + p.hop_start = 3; + p.hop_limit = 1; + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_TRACEROUTE_APP; + p.decoded.request_id = 0x9999; // non-zero marks this a response, which is what drives updateNextHops + return p; +} + +void setUp(void) +{ + mockNodeDB = new MockNodeDB(); + mockNodeDB->clearTestNodes(); + nodeDB = mockNodeDB; + + config = meshtastic_LocalConfig_init_zero; + owner = meshtastic_User_init_zero; + myNodeInfo.my_node_num = LOCAL_NODE; // drives isToUs() + + mockNodeDB->addNode(LOCAL_NODE); + mockNodeDB->addNode(RELAY_B); + mockNodeDB->addNode(NODE_C); + mockNodeDB->addNode(NODE_D); + + shim = new TraceRouteModuleTestShim(); +} + +void tearDown(void) +{ + delete shim; + shim = nullptr; + delete mockNodeDB; + mockNodeDB = nullptr; + nodeDB = nullptr; +} + +// The honest case: the route names RELAY_B as our next hop, and RELAY_B is who handed us the packet. +void test_nexthop_learned_when_route_matches_relay(void) +{ + meshtastic_RouteDiscovery r; + meshtastic_MeshPacket p = makeResponse(nodeDB->getLastByteOfNodeNum(RELAY_B), &r); + + shim->alterReceivedProtobuf(p, &r); + + const uint8_t expected = nodeDB->getLastByteOfNodeNum(RELAY_B); + TEST_ASSERT_EQUAL_MESSAGE(expected, mockNodeDB->nextHopOf(RELAY_B), "next hop for the relay itself"); + TEST_ASSERT_EQUAL_MESSAGE(expected, mockNodeDB->nextHopOf(NODE_C), "next hop for a node beyond the relay"); + TEST_ASSERT_EQUAL_MESSAGE(expected, mockNodeDB->nextHopOf(NODE_D), "next hop for the target"); +} + +// A forged response: the attacker transmits it themselves, so relay_node is their byte, but the route +// claims RELAY_B. Believing the payload here would let them redirect traffic for every node listed. +void test_nexthop_ignored_when_route_contradicts_relay(void) +{ + meshtastic_RouteDiscovery r; + meshtastic_MeshPacket p = makeResponse(ATTACKER_RELAY_BYTE, &r); + + shim->alterReceivedProtobuf(p, &r); + + TEST_ASSERT_EQUAL_MESSAGE(0, mockNodeDB->nextHopOf(RELAY_B), "forged route must not set a next hop"); + TEST_ASSERT_EQUAL_MESSAGE(0, mockNodeDB->nextHopOf(NODE_C), "forged route must not set a next hop"); + TEST_ASSERT_EQUAL_MESSAGE(0, mockNodeDB->nextHopOf(NODE_D), "forged route must not set a next hop"); +} + +// MQTT-sourced packets carry relay_node 0, so nothing corroborates the route and we must not learn. +void test_nexthop_ignored_without_a_relay(void) +{ + meshtastic_RouteDiscovery r; + meshtastic_MeshPacket p = makeResponse(NO_RELAY_NODE, &r); + + shim->alterReceivedProtobuf(p, &r); + + TEST_ASSERT_EQUAL_MESSAGE(0, mockNodeDB->nextHopOf(RELAY_B), "no relay means no corroboration"); + TEST_ASSERT_EQUAL_MESSAGE(0, mockNodeDB->nextHopOf(NODE_C), "no relay means no corroboration"); + TEST_ASSERT_EQUAL_MESSAGE(0, mockNodeDB->nextHopOf(NODE_D), "no relay means no corroboration"); +} + +void setup() +{ + delay(10); + delay(2000); + + initializeTestEnvironment(); + UNITY_BEGIN(); + RUN_TEST(test_nexthop_learned_when_route_matches_relay); + RUN_TEST(test_nexthop_ignored_when_route_contradicts_relay); + RUN_TEST(test_nexthop_ignored_without_a_relay); + exit(UNITY_END()); +} + +void loop() {}