Pin admin responses to the stored peer key and request id

src/mesh/Router.cpp

@@ -750,6 +750,34 @@ static bool signedDataFits(meshtastic_Data *d)
 }
 #endif
 
+#if !(MESHTASTIC_EXCLUDE_PKI)
+bool wouldEncryptWithPKC(const meshtastic_MeshPacket *p, ChannelIndex chIndex, bool haveDestKey)
+{
+    // First, only PKC encrypt packets we are originating
+    return isFromUs(p) &&
+#if ARCH_PORTDUINO
+           // Sim radio via the cli flag skips PKC
+           !portduino_config.force_simradio &&
+#endif
+           // Don't use PKC with Ham mode
+           !owner.is_licensed &&
+           // Don't use PKC on 'serial' or 'gpio' channels unless explicitly requested
+           !(p->pki_encrypted != true && (strcasecmp(channels.getName(chIndex), Channels::serialChannel) == 0 ||
+                                          strcasecmp(channels.getName(chIndex), Channels::gpioChannel) == 0)) &&
+           // Check for valid keys and single node destination
+           config.security.private_key.size == 32 && !isBroadcast(p->to) &&
+           // Some portnums either make no sense to send with PKC
+           p->decoded.portnum != meshtastic_PortNum_TRACEROUTE_APP && p->decoded.portnum != meshtastic_PortNum_NODEINFO_APP &&
+           p->decoded.portnum != meshtastic_PortNum_ROUTING_APP && p->decoded.portnum != meshtastic_PortNum_POSITION_APP &&
+           // We allow Key Verification messages to be sent without a known destination key, since the point of those messages is
+           // to exchange keys. The first exchange (no usable key yet) falls through to channel encryption; the follow-on packet
+           // uses the pending key resolved into haveDestKey/destKey above.
+           // Though possible the first packet each direction should go non-pkc
+           // to handle the case where the remote node has our key, but we don't have theirs.
+           !(p->decoded.portnum == meshtastic_PortNum_KEY_VERIFICATION_APP && !haveDestKey);
+}
+#endif
+
 /** Return 0 for success or a Routing_Error code for failure
  */
 meshtastic_Routing_Error perhapsEncode(meshtastic_MeshPacket *p)
@@ -843,28 +871,7 @@ meshtastic_Routing_Error perhapsEncode(meshtastic_MeshPacket *p)
         }
         // We may want to retool things so we can send a PKC packet when the client specifies a key and nodenum, even if the node
         // is not in the local nodedb
-        // First, only PKC encrypt packets we are originating
-        if (isFromUs(p) &&
-#if ARCH_PORTDUINO
-            // Sim radio via the cli flag skips PKC
-            !portduino_config.force_simradio &&
-#endif
-            // Don't use PKC with Ham mode
-            !owner.is_licensed &&
-            // Don't use PKC on 'serial' or 'gpio' channels unless explicitly requested
-            !(p->pki_encrypted != true && (strcasecmp(channels.getName(chIndex), Channels::serialChannel) == 0 ||
-                                           strcasecmp(channels.getName(chIndex), Channels::gpioChannel) == 0)) &&
-            // Check for valid keys and single node destination
-            config.security.private_key.size == 32 && !isBroadcast(p->to) &&
-            // Some portnums either make no sense to send with PKC
-            p->decoded.portnum != meshtastic_PortNum_TRACEROUTE_APP && p->decoded.portnum != meshtastic_PortNum_NODEINFO_APP &&
-            p->decoded.portnum != meshtastic_PortNum_ROUTING_APP && p->decoded.portnum != meshtastic_PortNum_POSITION_APP &&
-            // We allow Key Verification messages to be sent without a known destination key, since the point of those messages is
-            // to exchange keys. The first exchange (no usable key yet) falls through to channel encryption; the follow-on packet
-            // uses the pending key resolved into haveDestKey/destKey above.
-            // Though possible the first packet each direction should go non-pkc
-            // to handle the case where the remote node has our key, but we don't have theirs.
-            !(p->decoded.portnum == meshtastic_PortNum_KEY_VERIFICATION_APP && !haveDestKey)) {
+        if (wouldEncryptWithPKC(p, chIndex, haveDestKey)) {
             LOG_DEBUG("Use PKI!");
             if (numbytes + MESHTASTIC_HEADER_LENGTH + MESHTASTIC_PKC_OVERHEAD > MAX_LORA_PAYLOAD_LEN)
                 return meshtastic_Routing_Error_TOO_LARGE;

src/mesh/Router.h

@@ -193,6 +193,18 @@ meshtastic_Routing_Error perhapsEncode(meshtastic_MeshPacket *p);
 bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p);
 #endif
 
+#if !(MESHTASTIC_EXCLUDE_PKI)
+/**
+ * Would perhapsEncode() PKC-encrypt this outgoing packet? Callers that must know the encryption a
+ * packet will get before it is encoded (e.g. pinning a peer key at request time) have to ask this
+ * rather than inspect p, whose pki_encrypted/public_key fields are only populated on the RX path.
+ *
+ * @param chIndex the channel index p carries before encoding rewrites it to a hash.
+ * @param haveDestKey whether a public key for p->to was resolvable.
+ */
+bool wouldEncryptWithPKC(const meshtastic_MeshPacket *p, ChannelIndex chIndex, bool haveDestKey);
+#endif
+
 extern Router *router;
 
 /// Generate a unique packet id

src/modules/AdminModule.cpp

@@ -1904,7 +1904,15 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p)
     if (!responseVariant)
         return; // not a getter whose response we can pair
 
-    const bool keyValid = p.pki_encrypted && p.public_key.size == 32;
+    // Pin the key perhapsEncode will actually encrypt to, resolved the same way it resolves it.
+    // p.public_key is NOT it: nothing populates that field on the outgoing path (only perhapsDecode
+    // sets it, on RX), so reading it here pinned nothing and left `from` as the sole check.
+    bool keyValid = false;
+    meshtastic_NodeInfoLite_public_key_t destKey = {0, {0}};
+#if !(MESHTASTIC_EXCLUDE_PKI)
+    const bool haveDestKey = nodeDB->copyPublicKey(p.to, destKey);
+    keyValid = haveDestKey && wouldEncryptWithPKC(&p, p.channel, haveDestKey);
+#endif
 
     // One entry per request (a client sends N indexed get_channel requests, each answered once, so
     // entries must not merge). Free slot, else evict the oldest by rollover-safe elapsed time.
@@ -1923,14 +1931,15 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p)
     }
 
     slot->to = p.to;
+    slot->requestId = p.id;
     slot->sentAtMs = millis();
     slot->expectedResponse = responseVariant;
     slot->moduleConfigType = admin.which_payload_variant == meshtastic_AdminMessage_get_module_config_request_tag
                                  ? (uint8_t)admin.get_module_config_request
                                  : 0;
     slot->keyValid = keyValid;
     if (keyValid)
-        memcpy(slot->key, p.public_key.bytes, 32);
+        memcpy(slot->key, destKey.bytes, 32);
     else
         memset(slot->key, 0, 32);
     LOG_DEBUG("Admin request sent to 0x%08x, expecting its response", p.to);
@@ -1943,6 +1952,10 @@ bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t
     for (auto &o : outstandingAdminRequests) {
         if (o.to != mp.from || o.expectedResponse != responseVariant)
             continue;
+        // mp.from is unauthenticated, so also require the response to echo our request's packet id
+        // (setReplyTo puts it in decoded.request_id). A blind injector must now guess it.
+        if (mp.decoded.request_id != o.requestId)
+            continue;
         if (!Throttle::isWithinTimespanMs(o.sentAtMs, kOutstandingAdminRequestMs)) {
             o.to = 0; // lapsed; free the slot and keep looking for another live match
             continue;

src/modules/AdminModule.h

@@ -90,10 +90,11 @@ class AdminModule : public ProtobufModule<meshtastic_AdminMessage>, public Obser
     static constexpr uint32_t kOutstandingAdminRequestMs = 300 * 1000; // same window as the session passkey
     struct OutstandingAdminRequest {
         NodeNum to;                 // 0 = free slot
+        uint32_t requestId;         // our request's packet id; the response must echo it as request_id
         uint32_t sentAtMs;          // millis() when this request went out
         pb_size_t expectedResponse; // the one response variant this request authorizes
         uint8_t moduleConfigType;   // for get_module_config_request: which ModuleConfigType we asked
-        uint8_t key[32];            // pinned destination key when the request went out over PKC
+        uint8_t key[32];            // pinned destination key when the request goes out over PKC
         bool keyValid;
     };
     OutstandingAdminRequest outstandingAdminRequests[kOutstandingAdminRequests] = {};

test/test_admin_session_repro/test_main.cpp

@@ -29,8 +29,43 @@ static const uint8_t ADMIN_KEY[32] = {0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77,
                                       0xcc, 0xdd, 0xee, 0xff, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
                                       0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x20};
 
+// MeshService assigns every outgoing packet an id before noteOutgoingAdminRequest sees it, and
+// setReplyTo echoes it back as decoded.request_id, so the pairing is keyed on it.
+static constexpr uint32_t REQUEST_ID = 0x5EED0001;
+static const uint8_t QUERIED_KEY[32] = {0xC1, 0xC2, 0xC3, 0xC4, 0xC5, 0xC6, 0xC7, 0xC8, 0xC9, 0xCa, 0xCb,
+                                        0xCc, 0xCd, 0xCe, 0xCf, 0xD0, 0xD1, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6,
+                                        0xD7, 0xD8, 0xD9, 0xDa, 0xDb, 0xDc, 0xDd, 0xDe, 0xDf, 0xE0};
+
+// NodeDB with injectable nodes, so a destination can have a stored public key to pin.
+class MockNodeDB : public NodeDB
+{
+  public:
+    void clearTestNodes()
+    {
+        testNodes.clear();
+        meshNodes = &testNodes;
+        numMeshNodes = 0;
+    }
+
+    void addNodeWithKey(NodeNum num, const uint8_t *key)
+    {
+        meshtastic_NodeInfoLite n = meshtastic_NodeInfoLite_init_zero;
+        n.num = num;
+        if (key) {
+            n.public_key.size = 32;
+            memcpy(n.public_key.bytes, key, 32);
+        }
+        testNodes.push_back(n);
+        meshNodes = &testNodes;
+        numMeshNodes = testNodes.size();
+    }
+
+    std::vector<meshtastic_NodeInfoLite> testNodes;
+};
+
 static MockMeshService *mockService = nullptr;
 static AdminModuleTestShim *admin = nullptr;
+static MockNodeDB *mockNodeDB = nullptr;
 
 // A remote, PKC-authorized set_owner. `session` (if non-empty) is the session_passkey the client presents.
 static meshtastic_MeshPacket makeRemoteSetOwner(const char *newLongName, const uint8_t *session, size_t sessionLen,
@@ -71,6 +106,7 @@ static meshtastic_MeshPacket makeModuleConfigResponse(NodeNum from, meshtastic_A
     mp.to = LOCAL_NODE;
     mp.channel = 0;
     mp.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
+    mp.decoded.request_id = REQUEST_ID; // setReplyTo echoes the request's packet id
     return mp;
 }
 
@@ -93,6 +129,7 @@ static meshtastic_MeshPacket makeOutgoingRequest(NodeNum to, const meshtastic_Ad
     p.to = to;
     p.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
     p.decoded.portnum = meshtastic_PortNum_ADMIN_APP;
+    p.id = REQUEST_ID;
     p.decoded.payload.size =
         pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), &meshtastic_AdminMessage_msg, &req);
     return p;
@@ -114,11 +151,16 @@ void setUp(void)
     admin = new AdminModuleTestShim();
     admin->deferSaves(); // no disk/reboot side effects when a setter is accepted
 
-    if (!nodeDB)
-        nodeDB = new NodeDB();
+    if (!mockNodeDB)
+        mockNodeDB = new MockNodeDB();
+    mockNodeDB->clearTestNodes();
+    nodeDB = mockNodeDB;
     myNodeInfo.my_node_num = LOCAL_NODE;
 
     config = meshtastic_LocalConfig_init_zero;
+    // A real device always holds a private key; without one perhapsEncode never picks PKC.
+    config.security.private_key.size = 32;
+    memset(config.security.private_key.bytes, 0xA5, 32);
     // Authorize ADMIN_NODE's key as an admin key so the PKC path accepts it and we reach the session gate.
     config.security.admin_key[0].size = 32;
     memcpy(config.security.admin_key[0].bytes, ADMIN_KEY, 32);
@@ -310,38 +352,88 @@ void test_response_variant_must_match_request(void)
 // must not relax the PKC pin of an earlier one (the old shared-slot model cleared it).
 void test_pinned_request_keeps_its_key_after_an_unpinned_request(void)
 {
-    uint8_t key[32];
-    memset(key, 0xC1, 32);
+    // QUERIED has a stored key, so a request to it will be PKC-encrypted and pins that key.
+    // STRANGER has none, so a request to it cannot be pinned.
+    mockNodeDB->addNodeWithKey(QUERIED_NODE, QUERIED_KEY);
+    mockNodeDB->addNodeWithKey(STRANGER_NODE, nullptr);
 
-    // A PKC-pinned get_config request to STRANGER (pins `key`).
     meshtastic_AdminMessage cfg = meshtastic_AdminMessage_init_zero;
     cfg.which_payload_variant = meshtastic_AdminMessage_get_config_request_tag;
-    meshtastic_MeshPacket pinned = makeOutgoingRequest(STRANGER_NODE, cfg);
-    pinned.pki_encrypted = true;
-    pinned.public_key.size = 32;
-    memcpy(pinned.public_key.bytes, key, 32);
-    admin->noteOutgoingAdminRequest(pinned);
+    admin->noteOutgoingAdminRequest(makeOutgoingRequest(QUERIED_NODE, cfg));
 
-    // A later, unpinned get_owner request to the same node.
     meshtastic_AdminMessage own = meshtastic_AdminMessage_init_zero;
     own.which_payload_variant = meshtastic_AdminMessage_get_owner_request_tag;
     admin->noteOutgoingAdminRequest(makeOutgoingRequest(STRANGER_NODE, own));
 
     meshtastic_AdminMessage m;
-    meshtastic_MeshPacket resp = makeModuleConfigResponse(STRANGER_NODE, m); // from set; pki off by default
+    meshtastic_MeshPacket resp = makeModuleConfigResponse(QUERIED_NODE, m); // pki off by default
 
-    // A plaintext get_config_response is still rejected: the config request was pinned.
     TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(resp, meshtastic_AdminMessage_get_config_response_tag, 0),
                               "an unpinned request must not relax an earlier request's key pin");
-    // Over PKC with the pinned key it is accepted.
+
     resp.pki_encrypted = true;
     resp.public_key.size = 32;
-    memcpy(resp.public_key.bytes, key, 32);
+    memcpy(resp.public_key.bytes, QUERIED_KEY, 32);
     TEST_ASSERT_TRUE(admin->responseIsSolicited(resp, meshtastic_AdminMessage_get_config_response_tag, 0));
-    // The unpinned get_owner_response is accepted in plaintext (its request carried no pin).
-    resp.pki_encrypted = false;
-    resp.public_key.size = 0;
-    TEST_ASSERT_TRUE(admin->responseIsSolicited(resp, meshtastic_AdminMessage_get_owner_response_tag, 0));
+}
+
+// The pin is taken from the destination's stored NodeDB key - the key perhapsEncode will encrypt
+// to. Reading the outgoing packet's public_key instead pinned nothing: nothing populates that
+// field before encryption, so every real request was unpinned and `from` alone admitted responses.
+void test_request_to_keyed_node_pins_the_stored_key(void)
+{
+    mockNodeDB->addNodeWithKey(QUERIED_NODE, QUERIED_KEY);
+    admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(QUERIED_NODE));
+
+    meshtastic_AdminMessage m;
+    meshtastic_MeshPacket plain = makeModuleConfigResponse(QUERIED_NODE, m);
+    TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(plain, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG),
+                              "a plaintext response must not answer a PKC-pinned request");
+
+    meshtastic_MeshPacket wrongKey = makeModuleConfigResponse(QUERIED_NODE, m);
+    wrongKey.pki_encrypted = true;
+    wrongKey.public_key.size = 32;
+    memset(wrongKey.public_key.bytes, 0x77, 32);
+    TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(wrongKey, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG),
+                              "a response under a different key must not be accepted");
+
+    meshtastic_MeshPacket good = makeModuleConfigResponse(QUERIED_NODE, m);
+    good.pki_encrypted = true;
+    good.public_key.size = 32;
+    memcpy(good.public_key.bytes, QUERIED_KEY, 32);
+    TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(good, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG),
+                             "the pinned key must still admit the genuine response");
+}
+
+// Ham mode never uses PKC, so pinning a key there would reject the legitimate plaintext response.
+void test_ham_mode_request_is_not_pinned(void)
+{
+    owner.is_licensed = true;
+    mockNodeDB->addNodeWithKey(QUERIED_NODE, QUERIED_KEY);
+    admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(QUERIED_NODE));
+
+    meshtastic_AdminMessage m;
+    meshtastic_MeshPacket mp = makeModuleConfigResponse(QUERIED_NODE, m);
+    TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG),
+                             "a request that could not have gone out over PKC must not be pinned");
+}
+
+// The response must echo our request's packet id, so an injector cannot answer a request it did
+// not see just by naming the right node and variant.
+void test_response_with_wrong_request_id_is_rejected(void)
+{
+    admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(STRANGER_NODE));
+
+    meshtastic_AdminMessage m;
+    meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m);
+    mp.decoded.request_id = REQUEST_ID ^ 0xFFFF; // answers some other request
+
+    TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG),
+                              "a response that does not echo our request id must be rejected");
+
+    mp.decoded.request_id = REQUEST_ID;
+    TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG),
+                             "the matching request id must still be accepted");
 }
 
 // A remote_hardware response must answer a request for that exact subtype, not just any module
@@ -436,6 +528,9 @@ void setup()
     RUN_TEST(test_request_to_one_node_does_not_admit_another);
     RUN_TEST(test_response_variant_must_match_request);
     RUN_TEST(test_pinned_request_keeps_its_key_after_an_unpinned_request);
+    RUN_TEST(test_request_to_keyed_node_pins_the_stored_key);
+    RUN_TEST(test_ham_mode_request_is_not_pinned);
+    RUN_TEST(test_response_with_wrong_request_id_is_rejected);
     RUN_TEST(test_module_config_subtype_must_match);
     RUN_TEST(test_response_is_consumed_no_replay);
     RUN_TEST(test_outgoing_setter_does_not_admit_responses);