Merge branch 'master' into dont-process-mac-mismatch

src/mesh/NodeDB.cpp

@@ -985,9 +985,8 @@ void NodeDB::resetNodes()
 
 void NodeDB::removeNodeByNum(NodeNum nodeNum)
 {
-    // Don't remove the own node at position 0
-    int newPos = 1, removed = 0;
-    for (int i = 1; i < numMeshNodes; i++) {
+    int newPos = 0, removed = 0;
+    for (int i = 0; i < numMeshNodes; i++) {
         if (meshNodes->at(i).num != nodeNum)
             meshNodes->at(newPos++) = meshNodes->at(i);
         else
@@ -1083,16 +1082,18 @@ void NodeDB::pickNewNodeNum()
     }
 
     meshtastic_NodeInfoLite *found;
-    if (((found = getMeshNode(nodeNum)) && memcmp(found->user.macaddr, ourMacAddr, sizeof(ourMacAddr)) != 0) ||
-        (nodeNum == NODENUM_BROADCAST || nodeNum < NUM_RESERVED)) {
-        NodeNum newNodeNum = (ourMacAddr[2] << 24) | (ourMacAddr[3] << 16) | (ourMacAddr[4] << 8) | ourMacAddr[5];
-        LOG_WARN("NOTE! Our saved nodenum 0x%x is invalid or in use. Using 0x%x", nodeNum, newNodeNum);
-        nodeNum = newNodeNum;
+    while (((found = getMeshNode(nodeNum)) && memcmp(found->user.macaddr, ourMacAddr, sizeof(ourMacAddr)) != 0) ||
+           (nodeNum == NODENUM_BROADCAST || nodeNum < NUM_RESERVED)) {
+        NodeNum candidate = random(NUM_RESERVED, LONG_MAX); // try a new random choice
+        if (found)
+            LOG_WARN("NOTE! Our desired nodenum 0x%x is invalid or in use, by MAC ending in 0x%02x%02x vs our 0x%02x%02x, so "
+                     "trying for 0x%x",
+                     nodeNum, found->user.macaddr[4], found->user.macaddr[5], ourMacAddr[4], ourMacAddr[5], candidate);
+        nodeNum = candidate;
     }
     LOG_DEBUG("Use nodenum 0x%x ", nodeNum);
 
     myNodeInfo.my_node_num = nodeNum;
-    removeNodeByNum(nodeNum); // Since we skip 0, this should only ever remove outside matches.
 }
 
 /** Load a protobuf from a file, return LoadFileResult */
@@ -1688,10 +1689,10 @@ bool NodeDB::updateUser(uint32_t nodeId, meshtastic_User &p, uint8_t channelInde
 /// we updateGUI and updateGUIforNode if we think our this change is big enough for a redraw
 void NodeDB::updateFrom(const meshtastic_MeshPacket &mp)
 {
-    if (mp.transport_mechanism != meshtastic_MeshPacket_TransportMechanism_TRANSPORT_API && mp.from == getNodeNum()) {
-        LOG_DEBUG("Ignore update from self");
-        return;
-    }
+    // if (mp.from == getNodeNum()) {
+    //     LOG_DEBUG("Ignore update from self");
+    //     return;
+    // }
     if (mp.which_payload_variant == meshtastic_MeshPacket_decoded_tag && mp.from) {
         LOG_DEBUG("Update DB node 0x%x, rx_time=%u", mp.from, mp.rx_time);
 

src/modules/NodeInfoModule.cpp

@@ -6,11 +6,45 @@
 #include "Router.h"
 #include "configuration.h"
 #include "main.h"
+#include "mesh/mesh-pb-constants.h"
 #include <Throttle.h>
-#include <pb_encode.h>
 
 NodeInfoModule *nodeInfoModule;
 
+ProcessMessage NodeInfoModule::handleReceived(const meshtastic_MeshPacket &mp)
+{
+    if (mp.which_payload_variant != meshtastic_MeshPacket_decoded_tag || mp.decoded.portnum != ourPortNum) {
+        // Not for us, or not decoded, so skip
+        return ProcessMessage::CONTINUE;
+    }
+    // Decode the protobuf payload into a meshtastic_User
+    meshtastic_User p = meshtastic_User_init_default;
+    if (!pb_decode_from_bytes(mp.decoded.payload.bytes, mp.decoded.payload.size, &meshtastic_User_msg, &p)) {
+        LOG_ERROR("Failed to decode NodeInfo protobuf. Ignoring packet.");
+        return ProcessMessage::STOP;
+    }
+    // Validate that the nodenum matches the user's macaddr
+    NodeNum senderNodeNum = getFrom(&mp);
+
+    if (senderNodeNum != nodeDB->getNodeNum()) {
+        // Calculate expected nodenum from macaddr (same logic as pickNewNodeNum)
+        NodeNum expectedNodeNum = (p.macaddr[2] << 24) | (p.macaddr[3] << 16) | (p.macaddr[4] << 8) | p.macaddr[5];
+
+        if (senderNodeNum != expectedNodeNum) {
+            LOG_WARN(
+                "Dropping NodeInfo packet: nodenum 0x%08x doesn't match expected 0x%08x from MAC %02x:%02x:%02x:%02x:%02x:%02x",
+                senderNodeNum, expectedNodeNum, p.macaddr[0], p.macaddr[1], p.macaddr[2], p.macaddr[3], p.macaddr[4],
+                p.macaddr[5]);
+            // Cancel the sending of this packet and stop processing it
+            if (router)
+                router->cancelSending(mp.from, mp.id);
+            return ProcessMessage::STOP;
+        }
+    }
+
+    return ProcessMessage::CONTINUE;
+}
+
 bool NodeInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshtastic_User *pptr)
 {
     auto p = *pptr;
@@ -20,43 +54,6 @@ bool NodeInfoModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, mes
         return true;
     }
 
-    // Only send response if SNR is excellent (very close) and it's a direct neighbor
-    if (mp.rx_snr < 5) {
-        LOG_DEBUG("Skip poisoning NodeInfo, SNR too low: %.2f", mp.rx_snr);
-        return false;
-    }
-
-    // Check if it's a direct neighbor (zero hops away)
-    uint8_t hops_away = mp.hop_start - mp.hop_limit;
-    if (hops_away != 0) {
-        LOG_DEBUG("Skip poisoning NodeInfo, not a direct neighbor (hops: %d)", hops_away);
-        return false;
-    }
-    delay(1500);
-
-    LOG_INFO("Excellent SNR (%.2f) and direct neighbor detected, sending poisoned NodeInfo", mp.rx_snr);
-
-    strncpy(p.long_name, "New Node Name🥷", sizeof(p.long_name));
-    p.long_name[sizeof(p.long_name) - 1] = '\0';
-
-    meshtastic_MeshPacket *packet = router->allocForSending();
-    packet->from = mp.from;
-    packet->to = mp.to;
-    packet->channel = mp.channel;
-    packet->which_payload_variant = meshtastic_MeshPacket_decoded_tag;
-    packet->decoded.portnum = meshtastic_PortNum_NODEINFO_APP;
-
-    // Encode the modified User data into the packet payload
-    packet->decoded.payload.size =
-        pb_encode_to_bytes(packet->decoded.payload.bytes, sizeof(packet->decoded.payload.bytes), &meshtastic_User_msg, &p);
-
-    auto encodeResult = perhapsEncode(packet);
-
-    // Send raw packet directly via router interface
-    ErrorCode res = router->rawSend(packet);
-    LOG_INFO("Raw nodeinfo sent with result: %d", res);
-    return res == ERRNO_OK;
-
     // Coerce user.id to be derived from the node number
     snprintf(p.id, sizeof(p.id), "!%08x", getFrom(&mp));
 

src/modules/NodeInfoModule.h

@@ -25,11 +25,16 @@ class NodeInfoModule : public ProtobufModule<meshtastic_User>, private concurren
 
   protected:
     /** 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
     */
     virtual bool handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshtastic_User *p) override;
 
+    /** Called to handle a particular incoming message and continue or stop processing
+     * @return ProcessMessage::STOP if you've guaranteed you've handled this message and no other handlers should be considered
+     * for it
+     */
+    virtual ProcessMessage handleReceived(const meshtastic_MeshPacket &mp) override;
+
     /** 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.  */
     virtual meshtastic_MeshPacket *allocReply() override;

test/test_security/test_packet_handling.cpp

@@ -1,159 +0,0 @@
-#include "DebugConfiguration.h"
-#include "TestUtil.h"
-#include <unity.h>
-
-#ifdef ARCH_PORTDUINO
-#include "mesh/NodeDB.h"
-#include "mesh/generated/meshtastic/mesh.pb.h"
-#include "modules/NodeInfoModule.h"
-
-// Mock NodeDB that tracks when updateUser would be called - follows MQTT test pattern
-class MockNodeDB : public NodeDB
-{
-  public:
-    MockNodeDB()
-    {
-        updateUserCallCount = 0;
-        lastUpdatedNodeNum = 0;
-    }
-
-    // Override virtual getMeshNode method (same as MQTT test pattern)
-    meshtastic_NodeInfoLite *getMeshNode(NodeNum n) override { return &emptyNode; }
-
-    // Track calls that would go to updateUser (we'll check this in the test)
-    // Since updateUser is not virtual, we override a method that's called during the process
-    meshtastic_NodeInfoLite *getMeshNodeForUpdate(NodeNum n)
-    {
-        updateUserCallCount++;
-        lastUpdatedNodeNum = n;
-        return &emptyNode;
-    }
-
-    int updateUserCallCount;
-    NodeNum lastUpdatedNodeNum;
-    meshtastic_NodeInfoLite emptyNode = {};
-};
-
-// Testable version of NodeInfoModule that exposes protected methods
-class TestableNodeInfoModule : public NodeInfoModule
-{
-  public:
-    bool testHandleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshtastic_User *user)
-    {
-        return handleReceivedProtobuf(mp, user);
-    }
-};
-
-void test_nodeinfo_spoofing_vulnerability()
-{
-    // Create mock NodeDB and assign to global pointer like MQTT test
-    const std::unique_ptr<MockNodeDB> mockNodeDB(new MockNodeDB());
-    nodeDB = mockNodeDB.get();
-
-    // Set our node number (simulating what happens in real startup)
-    myNodeInfo.my_node_num = 0x12345678;
-
-    // Create a test NodeInfoModule
-    TestableNodeInfoModule testModule;
-
-    // Create a spoofed packet claiming to be from our own node
-    meshtastic_MeshPacket spoofedPacket = meshtastic_MeshPacket_init_default;
-    spoofedPacket.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA;
-    spoofedPacket.from = 0x12345678; // VULNERABILITY: Same as our node number
-    spoofedPacket.to = NODENUM_BROADCAST;
-    spoofedPacket.channel = 0;
-    spoofedPacket.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
-    spoofedPacket.decoded.portnum = meshtastic_PortNum_NODEINFO_APP;
-
-    // Create malicious User data that an attacker wants to inject
-    meshtastic_User maliciousUser = meshtastic_User_init_default;
-    strcpy(maliciousUser.long_name, "HACKED_NODE");
-    strcpy(maliciousUser.short_name, "HAK");
-    strcpy(maliciousUser.id, "!87654321"); // Attacker's fake ID
-    maliciousUser.is_licensed = true;      // Try to make us appear licensed when we're not
-
-    // Test the vulnerability: handleReceivedProtobuf should reject spoofed packets claiming to be from our own node
-    // but currently it processes them, calling updateUser with our own node number
-    bool result = testModule.testHandleReceivedProtobuf(spoofedPacket, &maliciousUser);
-
-    // The vulnerability is demonstrated by the function NOT rejecting the spoofed packet
-    // In a secure implementation, packets claiming to be from our own node should be rejected
-    // and the function should return true (meaning "I handled this, don't process further")
-
-    // Currently this will FAIL because the vulnerability exists:
-    // - The function returns false (allowing further processing)
-    // - It calls updateUser with the spoofed node number (our own number)
-    // - This allows an attacker to modify our node information
-
-    TEST_ASSERT_FALSE_MESSAGE(result,
-                              "VULNERABILITY CONFIRMED: handleReceivedProtobuf processes spoofed packets from our own node.\n"
-                              "Expected: Function should return true (reject spoofed packet)\n"
-                              "Actual: Function returned false (processed spoofed packet)\n"
-                              "This allows attackers to spoof our node number and modify our NodeInfo.");
-
-    printf("\n=== SECURITY TEST RESULTS ===\n");
-    printf("✗ Vulnerability exists: NodeInfoModule processes spoofed packets from our own node\n");
-    printf("✗ Attack vector: Attacker can spoof packets with from=our_node_number\n");
-    printf("==============================\n\n");
-}
-
-void test_legitimate_packet_processing()
-{
-    // Test that legitimate packets from OTHER nodes are processed correctly
-    const std::unique_ptr<MockNodeDB> mockNodeDB(new MockNodeDB());
-    nodeDB = mockNodeDB.get();
-
-    myNodeInfo.my_node_num = 0x12345678;
-    TestableNodeInfoModule testModule;
-
-    // Create a legitimate packet from a DIFFERENT node
-    meshtastic_MeshPacket legitimatePacket = meshtastic_MeshPacket_init_default;
-    legitimatePacket.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA;
-    legitimatePacket.from = 0x87654321; // Different node number - this is legitimate
-    legitimatePacket.to = NODENUM_BROADCAST;
-    legitimatePacket.channel = 0;
-    legitimatePacket.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
-    legitimatePacket.decoded.portnum = meshtastic_PortNum_NODEINFO_APP;
-
-    meshtastic_User legitimateUser = meshtastic_User_init_default;
-    strcpy(legitimateUser.long_name, "Legitimate User");
-    strcpy(legitimateUser.short_name, "LEG");
-
-    bool result = testModule.testHandleReceivedProtobuf(legitimatePacket, &legitimateUser);
-
-    // Legitimate packets should be processed normally (return false for further processing)
-    TEST_ASSERT_FALSE_MESSAGE(result, "Legitimate packets from other nodes should be processed normally");
-
-    printf("✓ Legitimate packet processing works correctly\n");
-}
-
-void setUp()
-{
-    // Required by Unity
-}
-
-void tearDown()
-{
-    // Required by Unity
-}
-
-void setup()
-{
-    // Initialize test environment like MQTT test
-    initializeTestEnvironment();
-
-    UNITY_BEGIN();
-
-    printf("\n=== NodeInfo Spoofing Security Test ===\n");
-    printf("Testing vulnerability in NodeInfoModule::handleReceivedProtobuf()\n");
-    printf("Issue: Function doesn't check if packet claims to be from our own node\n\n");
-
-    RUN_TEST(test_nodeinfo_spoofing_vulnerability);
-    RUN_TEST(test_legitimate_packet_processing);
-
-    UNITY_END();
-}
-
-void loop() {}
-
-#endif