diff --git a/src/mesh/MeshModule.cpp b/src/mesh/MeshModule.cpp index 5dc7fba4a37..4b75917d8df 100644 --- a/src/mesh/MeshModule.cpp +++ b/src/mesh/MeshModule.cpp @@ -224,6 +224,11 @@ meshtastic_MeshPacket *MeshModule::allocReply() return r; } +uint8_t MeshModule::getResponseHopLimit(const meshtastic_MeshPacket &req) +{ + return routingModule->getHopLimitForResponse(req); +} + /** Messages can be received that have the want_response bit set. If set, this callback will be invoked * so that subclasses can (optionally) send a response back to the original sender. Implementing this method * is optional @@ -233,6 +238,7 @@ void MeshModule::sendResponse(const meshtastic_MeshPacket &req) auto r = allocReply(); if (r) { setReplyTo(r, req); + r->hop_limit = getResponseHopLimit(req); currentReply = r; } else { // Ignore - this is now expected behavior for routing module (because it ignores some replies) diff --git a/src/mesh/MeshModule.h b/src/mesh/MeshModule.h index 3dc6414a515..856720dc37b 100644 --- a/src/mesh/MeshModule.h +++ b/src/mesh/MeshModule.h @@ -231,6 +231,10 @@ class MeshModule friend class ReliableRouter; + protected: + virtual uint8_t getResponseHopLimit(const meshtastic_MeshPacket &req); + + private: /** Messages can be received that have the want_response bit set. If set, this callback will be invoked * so that subclasses can (optionally) send a response back to the original sender. This method calls allocReply() * to generate the reply message, and if !NULL that message will be delivered to whoever sent req diff --git a/src/modules/NodeInfoModule.cpp b/src/modules/NodeInfoModule.cpp index f7bfcf2fe83..41b00cc4833 100644 --- a/src/modules/NodeInfoModule.cpp +++ b/src/modules/NodeInfoModule.cpp @@ -30,8 +30,21 @@ bool NodeInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, mes auto p = *pptr; + if (p.is_licensed != owner.is_licensed) { + LOG_WARN("Invalid nodeInfo detected, is_licensed mismatch!"); + return true; + } + NodeNum sourceNum = getFrom(&mp); + const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(sourceNum); + // Broadcasts only: senders never sign unicast NodeInfo, so dropping it would break exchanges + // with signer nodes. Backstops ingress that skips Router's downgrade drop (e.g. decoded MQTT). + if (node && nodeInfoLiteHasXeddsaSigned(node) && !mp.xeddsa_signed && isBroadcast(mp.to)) { + LOG_WARN("Dropping unsigned NodeInfo broadcast from node 0x%08x that previously signed", sourceNum); + return true; + } + // Suppress replies to senders we've replied to recently (12H window) - if (mp.decoded.want_response && !isFromUs(&mp)) { + if (mp.decoded.want_response && !isFromUs(&mp) && (!isBroadcast(mp.to) || isDirectBroadcastDiscoveryRequest(mp))) { const NodeNum sender = getFrom(&mp); const uint32_t now = mp.rx_time ? mp.rx_time : getTime(); auto it = lastNodeInfoSeen.find(sender); @@ -45,19 +58,6 @@ bool NodeInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, mes pruneLastNodeInfoCache(); } - if (p.is_licensed != owner.is_licensed) { - LOG_WARN("Invalid nodeInfo detected, is_licensed mismatch!"); - return true; - } - NodeNum sourceNum = getFrom(&mp); - const meshtastic_NodeInfoLite *node = nodeDB->getMeshNode(sourceNum); - // Broadcasts only: senders never sign unicast NodeInfo, so dropping it would break exchanges - // with signer nodes. Backstops ingress that skips Router's downgrade drop (e.g. decoded MQTT). - if (node && nodeInfoLiteHasXeddsaSigned(node) && !mp.xeddsa_signed && isBroadcast(mp.to)) { - LOG_WARN("Dropping unsigned NodeInfo broadcast from node 0x%08x that previously signed", sourceNum); - return true; - } - // Coerce user.id to be derived from the node number snprintf(p.id, sizeof(p.id), "!%08x", getFrom(&mp)); @@ -113,6 +113,8 @@ void NodeInfoModule::sendOurNodeInfo(NodeNum dest, bool wantReplies, uint8_t cha wantReplies; p->decoded.want_response = requestWantResponse; + if (requestWantResponse && isBroadcast(dest)) + p->hop_limit = 0; if (_shorterTimeout) p->priority = meshtastic_MeshPacket_Priority_DEFAULT; else @@ -135,6 +137,21 @@ void NodeInfoModule::triggerImmediateNodeInfoCheck() setIntervalFromNow(0); } +bool NodeInfoModule::isDirectBroadcastDiscoveryRequest(const meshtastic_MeshPacket &request) +{ + return request.which_payload_variant == meshtastic_MeshPacket_decoded_tag && + request.decoded.portnum == meshtastic_PortNum_NODEINFO_APP && request.decoded.want_response && + isBroadcast(request.to) && getHopsAway(request) == 0; +} + +uint8_t NodeInfoModule::getResponseHopLimit(const meshtastic_MeshPacket &req) +{ + if (isDirectBroadcastDiscoveryRequest(req)) + return 0; + + return MeshModule::getResponseHopLimit(req); +} + meshtastic_MeshPacket *NodeInfoModule::allocReply() { // Only apply suppression when actually replying to someone else's request, not for periodic broadcasts. @@ -143,6 +160,13 @@ meshtastic_MeshPacket *NodeInfoModule::allocReply() currentRequest->decoded.portnum == meshtastic_PortNum_NODEINFO_APP && currentRequest->decoded.want_response && !isFromUs(currentRequest); + const bool isBroadcastRequest = isReplyingToExternalRequest && isBroadcast(currentRequest->to); + if (isBroadcastRequest && !isDirectBroadcastDiscoveryRequest(*currentRequest)) { + LOG_DEBUG("Skip NodeInfo response to non-direct broadcast discovery"); + ignoreRequest = true; + return NULL; + } + if (suppressReplyForCurrentRequest && isReplyingToExternalRequest) { LOG_DEBUG("Skip send NodeInfo since we heard the requester <12h ago"); ignoreRequest = true; @@ -234,4 +258,4 @@ int32_t NodeInfoModule::runOnce() sendOurNodeInfo(NODENUM_BROADCAST, requestReplies); // Send our info (don't request replies) } return Default::getConfiguredOrDefaultMs(config.device.node_info_broadcast_secs, default_node_info_broadcast_secs); -} \ No newline at end of file +} diff --git a/src/modules/NodeInfoModule.h b/src/modules/NodeInfoModule.h index 9b3b66caed4..f9fcc5cf6b8 100644 --- a/src/modules/NodeInfoModule.h +++ b/src/modules/NodeInfoModule.h @@ -31,6 +31,10 @@ class NodeInfoModule : public ProtobufModule, private concurren void triggerImmediateNodeInfoCheck(); protected: + static bool isDirectBroadcastDiscoveryRequest(const meshtastic_MeshPacket &request); + + virtual uint8_t getResponseHopLimit(const meshtastic_MeshPacket &req) override; + /** Called to handle a particular incoming message @return true if you've guaranteed you've handled this message and no other handlers should be considered for it diff --git a/test/test_mesh_module/test_main.cpp b/test/test_mesh_module/test_main.cpp index 6ba038af37a..5bd7f326d88 100644 --- a/test/test_mesh_module/test_main.cpp +++ b/test/test_mesh_module/test_main.cpp @@ -3,6 +3,7 @@ #include "TestUtil.h" #include +#include "airtime.h" #include "configuration.h" #include "mesh/CryptoEngine.h" #include "mesh/MeshService.h" @@ -10,6 +11,7 @@ #include "mesh/RadioInterface.h" #include "mesh/Router.h" #include "modules/NeighborInfoModule.h" +#include "modules/NodeInfoModule.h" #include "modules/RoutingModule.h" #include "support/MockMeshService.h" #include @@ -126,6 +128,34 @@ class SyntheticReplyModule : public MeshModule bool acceptsEveryPort; }; +class ZeroHopReplyModule : public SyntheticReplyModule +{ + public: + ZeroHopReplyModule() + : SyntheticReplyModule("zero-hop-reply", meshtastic_PortNum_TELEMETRY_APP, meshtastic_PortNum_TELEMETRY_APP) + { + } + + protected: + uint8_t getResponseHopLimit(const meshtastic_MeshPacket &req) override + { + (void)req; + return 0; + } +}; + +class NodeInfoPolicyShim : public NodeInfoModule +{ + public: + using NodeInfoModule::allocReply; + using NodeInfoModule::getResponseHopLimit; + using NodeInfoModule::handleReceivedProtobuf; + using NodeInfoModule::isDirectBroadcastDiscoveryRequest; + + MeshModule *asMeshModule() { return this; } + void setCurrentRequest(const meshtastic_MeshPacket *request) { currentRequest = request; } +}; + class ObservingIgnoreModule : public MeshModule { public: @@ -175,6 +205,7 @@ static MockMeshService *mockService; static MockRouter *mockRouter; static MockRoutingModule *mockRoutingModule; static NeighborInfoModule *realNeighborInfoModule; +static AirTime *testAirTime; static std::vector dispatchModules; template static T *registerDispatchModule(T *module) @@ -204,6 +235,11 @@ static void dispatch(meshtastic_PortNum port) MeshModule::callModules(request); } +static void dispatch(meshtastic_MeshPacket request) +{ + MeshModule::callModules(request); +} + } // namespace void setUp(void) @@ -214,6 +250,9 @@ void setUp(void) owner = meshtastic_User_init_zero; myNodeInfo.my_node_num = LOCAL_NODE; + testAirTime = new AirTime(); + airTime = testAirTime; + mockNodeDB = new MockNodeDB(); nodeDB = mockNodeDB; myNodeInfo.my_node_num = LOCAL_NODE; @@ -267,6 +306,10 @@ void tearDown(void) delete mockNodeDB; mockNodeDB = nullptr; nodeDB = nullptr; + + airTime = nullptr; + delete testAirTime; + testAirTime = nullptr; } // Zero-hop broadcast (hop_limit == hop_start): should be allowed @@ -418,6 +461,109 @@ static void test_dispatch_crossPortReplyUsesRequestOwner() TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size()); } +static void test_dispatch_moduleCanConstrainReplyHopLimit() +{ + registerDispatchModule(new ZeroHopReplyModule()); + + dispatch(meshtastic_PortNum_TELEMETRY_APP); + + TEST_ASSERT_EQUAL_UINT32(1, mockRouter->sentPackets.size()); + TEST_ASSERT_EQUAL_UINT8(0, mockRouter->sentPackets[0].hop_limit); +} + +static void test_nodeInfo_directBroadcastDiscoveryUsesZeroHopReply() +{ + NodeInfoPolicyShim nodeInfo; + meshtastic_MeshPacket request = makeRequest(meshtastic_PortNum_NODEINFO_APP); + request.to = NODENUM_BROADCAST; + request.hop_start = 3; + request.hop_limit = 3; + request.decoded.has_bitfield = true; + + TEST_ASSERT_TRUE(NodeInfoPolicyShim::isDirectBroadcastDiscoveryRequest(request)); + TEST_ASSERT_EQUAL_UINT8(0, nodeInfo.getResponseHopLimit(request)); +} + +static void test_nodeInfo_relayedAndUnknownBroadcastDiscoveryDoNotQualify() +{ + meshtastic_MeshPacket request = makeRequest(meshtastic_PortNum_NODEINFO_APP); + request.to = NODENUM_BROADCAST; + request.hop_start = 3; + request.hop_limit = 2; + request.decoded.has_bitfield = true; + + TEST_ASSERT_FALSE(NodeInfoPolicyShim::isDirectBroadcastDiscoveryRequest(request)); + + request.hop_start = 0; + request.hop_limit = 0; + request.decoded.has_bitfield = false; + + TEST_ASSERT_FALSE(NodeInfoPolicyShim::isDirectBroadcastDiscoveryRequest(request)); +} + +static void test_nodeInfo_unicastRequestRetainsRoutingHopLimit() +{ + NodeInfoPolicyShim nodeInfo; + meshtastic_MeshPacket request = makeRequest(meshtastic_PortNum_NODEINFO_APP); + + TEST_ASSERT_FALSE(NodeInfoPolicyShim::isDirectBroadcastDiscoveryRequest(request)); + TEST_ASSERT_EQUAL_UINT8(mockRoutingModule->getHopLimitForResponse(request), nodeInfo.getResponseHopLimit(request)); +} + +static void test_nodeInfo_rejectedBroadcastDoesNotSuppressDirectDiscovery() +{ + auto *nodeInfo = new NodeInfoPolicyShim(); + dispatchModules.push_back(nodeInfo->asMeshModule()); + meshtastic_MeshPacket request = makeRequest(meshtastic_PortNum_NODEINFO_APP); + request.to = NODENUM_BROADCAST; + request.decoded.has_bitfield = true; + request.hop_start = 3; + request.hop_limit = 2; + + dispatch(request); + TEST_ASSERT_EQUAL_UINT32(0, mockRouter->sentPackets.size()); + TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size()); + + request.hop_start = 0; + request.hop_limit = 0; + request.decoded.has_bitfield = false; + dispatch(request); + TEST_ASSERT_EQUAL_UINT32(0, mockRouter->sentPackets.size()); + TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size()); + + request.decoded.has_bitfield = true; + dispatch(request); + TEST_ASSERT_EQUAL_UINT32(1, mockRouter->sentPackets.size()); + TEST_ASSERT_EQUAL_UINT8(0, mockRouter->sentPackets[0].hop_limit); + TEST_ASSERT_EQUAL_UINT32(0, mockRoutingModule->ackNaks.size()); +} + +static void test_nodeInfo_rejectedDirectRequestDoesNotSuppressDiscovery() +{ + NodeInfoPolicyShim nodeInfo; + meshtastic_MeshPacket request = makeRequest(meshtastic_PortNum_NODEINFO_APP); + request.to = NODENUM_BROADCAST; + request.hop_start = 0; + request.hop_limit = 0; + request.decoded.has_bitfield = true; + + TEST_ASSERT_NOT_NULL(mockNodeDB->getOrCreateMeshNode(REMOTE_NODE)); + + meshtastic_User rejectedUser = meshtastic_User_init_zero; + rejectedUser.is_licensed = true; + TEST_ASSERT_TRUE(nodeInfo.handleReceivedProtobuf(request, &rejectedUser)); + + meshtastic_User validUser = meshtastic_User_init_zero; + TEST_ASSERT_FALSE(nodeInfo.handleReceivedProtobuf(request, &validUser)); + + nodeInfo.setCurrentRequest(&request); + meshtastic_MeshPacket *reply = nodeInfo.allocReply(); + nodeInfo.setCurrentRequest(nullptr); + + TEST_ASSERT_NOT_NULL(reply); + packetPool.release(reply); +} + static void test_dispatch_foreignPortObserverCanSuppressNak() { auto *observer = registerDispatchModule(new ObservingIgnoreModule()); @@ -491,6 +637,12 @@ void setup() RUN_TEST(test_dispatch_foreignPortOffenderCannotShadowOwner); RUN_TEST(test_dispatch_ownerPortStillReplies); RUN_TEST(test_dispatch_crossPortReplyUsesRequestOwner); + RUN_TEST(test_dispatch_moduleCanConstrainReplyHopLimit); + RUN_TEST(test_nodeInfo_directBroadcastDiscoveryUsesZeroHopReply); + RUN_TEST(test_nodeInfo_relayedAndUnknownBroadcastDiscoveryDoNotQualify); + RUN_TEST(test_nodeInfo_unicastRequestRetainsRoutingHopLimit); + RUN_TEST(test_nodeInfo_rejectedBroadcastDoesNotSuppressDirectDiscovery); + RUN_TEST(test_nodeInfo_rejectedDirectRequestDoesNotSuppressDiscovery); RUN_TEST(test_dispatch_foreignPortObserverCanSuppressNak); RUN_TEST(test_dispatch_noResponderSendsNak); RUN_TEST(test_dispatch_ignoreRequestIsClearedPerPacket);