Pin admin responses to the stored peer key and request id (#11092)

* Pin admin responses to the stored peer key and request id

noteOutgoingAdminRequest derived its PKC pin from p.public_key, which nothing
populates on the outgoing path, so keyValid was false for every client request
and the pin never engaged. The accepted-response predicate reduced to an
unauthenticated from plus variant and subtype.

Resolve the destination key from NodeDB the way perhapsEncode does, and pin it
only when the request would actually be PKC-encrypted. Extract that condition
from perhapsEncode as wouldEncryptWithPKC so both use one predicate. Also
record the request's packet id and require the response to echo it.

* Treat a zero request id as no pairing token

src/mesh/Router.cpp

@@ -822,6 +822,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)
@@ -915,28 +943,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

@@ -1975,7 +1975,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.
@@ -1994,6 +2002,7 @@ 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
@@ -2001,7 +2010,7 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p)
                                  : 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);
@@ -2014,6 +2023,11 @@ 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. Id 0 is
+        // no token at all - an omitted request_id decodes to 0 - so such a slot never matches.
+        if (o.requestId == 0 || 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);
@@ -411,38 +453,104 @@ 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 request we could not bind to an id must not be answerable by a response that simply omits
+// request_id, which decodes to 0.
+void test_request_without_an_id_admits_nothing(void)
+{
+    meshtastic_MeshPacket req = makeOutgoingModuleConfigRequest(STRANGER_NODE);
+    req.id = 0;
+    admin->noteOutgoingAdminRequest(req);
+
+    meshtastic_AdminMessage m;
+    meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m);
+    mp.decoded.request_id = 0;
+
+    TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG),
+                              "a zero request id must not act as a matching token");
 }
 
 // A remote_hardware response must answer a request for that exact subtype, not just any module
@@ -542,6 +650,10 @@ 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_request_without_an_id_admits_nothing);
     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);