diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index a31c1d97e..bf02d8e7b 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -1013,6 +1013,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) @@ -1106,28 +1134,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; diff --git a/src/mesh/Router.h b/src/mesh/Router.h index 680d3ae8d..d1c368785 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -191,6 +191,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 diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index cd33f34aa..0edbf99bd 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -1985,7 +1985,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. @@ -2004,6 +2012,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 @@ -2011,7 +2020,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); @@ -2024,6 +2033,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; diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index 5c1a6ef58..1bf98be10 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -90,10 +90,11 @@ class AdminModule : public ProtobufModule, 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] = {}; diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index 890a9079e..00c90e381 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/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 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);