From 2ab3f1d9427b943766e8de8242581c3b3d367350 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 16 Jul 2026 17:30:51 +0200 Subject: [PATCH 1/4] AdminModule: only accept admin responses to requests we sent An admin *_response short-circuited the auth and session-passkey checks that gate every other admin message, so any node could deliver one. On a channel the module listens to unauthenticated, a get_module_config_response drives the remote-hardware pin handler with attacker-supplied values. Track the destination of outgoing admin requests (per remote, with the pinned PKC key when there is one) and accept a response only from a node with a matching outstanding request, inside the same window as the session passkey. Local (from == 0) admin is unchanged; PhoneAPI already gates it. Also fix the response dispatch: get_module_config_response.which_payload_variant is a ModuleConfig oneof tag, but it was compared against the AdminMessage ModuleConfigType enum (different numbering), so the handler never ran. Compare against the oneof tag. --- src/mesh/MeshService.cpp | 9 ++ src/modules/AdminModule.cpp | 73 +++++++++- src/modules/AdminModule.h | 19 +++ test/support/AdminModuleTestShim.h | 1 + test/test_admin_session_repro/test_main.cpp | 146 +++++++++++++++++++- 5 files changed, 242 insertions(+), 6 deletions(-) diff --git a/src/mesh/MeshService.cpp b/src/mesh/MeshService.cpp index ca062abb984..ce356d7cdc4 100644 --- a/src/mesh/MeshService.cpp +++ b/src/mesh/MeshService.cpp @@ -17,6 +17,7 @@ #include "main.h" #include "mesh-pb-constants.h" #include "meshUtils.h" +#include "modules/AdminModule.h" #include "modules/NodeInfoModule.h" #include "modules/PositionModule.h" #include "modules/RoutingModule.h" @@ -259,6 +260,14 @@ void MeshService::handleToRadio(meshtastic_MeshPacket &p) const StoredMessage &sm = messageStore.addFromPacket(p); graphics::MessageRenderer::handleNewMessage(nullptr, sm, p); // notify UI }) +#if !MESHTASTIC_EXCLUDE_ADMIN + // Note admin requests on their way out: AdminModule only accepts a response from a remote we + // actually asked. Runs before encryption, while the payload is still readable. + if (adminModule && p.which_payload_variant == meshtastic_MeshPacket_decoded_tag && + p.decoded.portnum == meshtastic_PortNum_ADMIN_APP) + adminModule->noteOutgoingAdminRequest(p); +#endif + // Send the packet into the mesh DEBUG_HEAP_BEFORE; auto a = packetPool.allocCopy(p); diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index b98df39cdc8..afb9bd4a52b 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -125,9 +125,13 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta } #endif meshtastic_Channel *ch = &channels.getByIndex(mp.channel); - // Could tighten this up further by tracking the last public_key we went an AdminMessage request to - // and only allowing responses from that remote. if (messageIsResponse(r)) { + // Only accept a response from a remote we sent the matching request to. from == 0 is a + // local client, which PhoneAPI has already gated. + if (mp.from != 0 && !responseIsSolicited(mp)) { + LOG_INFO("Ignore admin response from 0x%08x, no outstanding request", mp.from); + return handled; + } LOG_DEBUG("Allow admin response message"); } else if (mp.from == 0) { // Local admin from a BLE/USB/TCP client. from == 0 cannot arrive from the @@ -472,8 +476,9 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta } case meshtastic_AdminMessage_get_module_config_response_tag: { LOG_INFO("Client received a get_module_config response"); - if (fromOthers && r->get_module_config_response.which_payload_variant == - meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG) { + // which_payload_variant is the ModuleConfig oneof tag, so compare against that tag, not the + // AdminMessage ModuleConfigType enum (whose REMOTEHARDWARE value is a different number). + if (fromOthers && r->get_module_config_response.which_payload_variant == meshtastic_ModuleConfig_remote_hardware_tag) { handleGetModuleConfigResponse(mp, r); } break; @@ -1821,6 +1826,66 @@ bool AdminModule::messageIsResponse(const meshtastic_AdminMessage *r) return false; } +void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) +{ + if (p.which_payload_variant != meshtastic_MeshPacket_decoded_tag || p.decoded.portnum != meshtastic_PortNum_ADMIN_APP) + return; + // Local admin is answered in-process, and a broadcast is never a request to one remote. + if (p.to == 0 || isBroadcast(p.to) || p.to == nodeDB->getNodeNum()) + return; + + meshtastic_AdminMessage admin = meshtastic_AdminMessage_init_zero; + if (!pb_decode_from_bytes(p.decoded.payload.bytes, p.decoded.payload.size, &meshtastic_AdminMessage_msg, &admin)) + return; + if (!messageIsRequest(&admin)) + return; + + // Reuse this remote's slot if it has one, else a free slot, else the oldest. + OutstandingAdminRequest *slot = nullptr; + for (auto &o : outstandingAdminRequests) + if (o.to == p.to) + slot = &o; + if (!slot) + for (auto &o : outstandingAdminRequests) + if (o.to == 0) { + slot = &o; + break; + } + if (!slot) { + slot = &outstandingAdminRequests[0]; + for (auto &o : outstandingAdminRequests) + if (o.sentAtSecs < slot->sentAtSecs) + slot = &o; + } + + slot->to = p.to; + slot->sentAtSecs = millis() / 1000; + slot->keyValid = p.pki_encrypted && p.public_key.size == 32; + if (slot->keyValid) + memcpy(slot->key, p.public_key.bytes, 32); + else + memset(slot->key, 0, 32); + LOG_DEBUG("Admin request sent to 0x%08x, expecting its response", p.to); +} + +bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp) +{ + const uint32_t now = millis() / 1000; + for (auto &o : outstandingAdminRequests) { + if (o.to == 0 || o.to != mp.from) + continue; + if (now > o.sentAtSecs + kOutstandingAdminRequestSecs) { + o.to = 0; + return false; + } + // A request the client pinned to a key must be answered over PKC by that same key. + if (o.keyValid && (!mp.pki_encrypted || mp.public_key.size != 32 || memcmp(mp.public_key.bytes, o.key, 32) != 0)) + return false; + return true; + } + return false; +} + bool AdminModule::messageIsRequest(const meshtastic_AdminMessage *r) { if (r->which_payload_variant == meshtastic_AdminMessage_get_channel_request_tag || diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index 15923988bc9..7a927d1ef63 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -77,7 +77,26 @@ class AdminModule : public ProtobufModule, public Obser public: void handleSetHamMode(const meshtastic_HamParameters &req); + /// Note an admin request leaving this node for a remote, so that remote's response is + /// accepted. Called from the client-to-mesh path (MeshService::handleToRadio). + void noteOutgoingAdminRequest(const meshtastic_MeshPacket &p); + private: + // An admin response carries no session passkey and its sender is not an admin-key holder, so + // the only thing vouching for one is a request we sent. Track those per remote: a client may + // have several nodes in flight, and each keeps answering until the window lapses. + static constexpr size_t kOutstandingAdminRequests = 3; + static constexpr uint32_t kOutstandingAdminRequestSecs = 300; // same window as the session passkey + struct OutstandingAdminRequest { + NodeNum to; // 0 = free slot + uint32_t sentAtSecs; // millis()/1000 when the request went out + uint8_t key[32]; // destination key, when the client pinned one + bool keyValid; + }; + OutstandingAdminRequest outstandingAdminRequests[kOutstandingAdminRequests] = {}; + + /// Whether mp answers a request we actually sent to mp.from. + bool responseIsSolicited(const meshtastic_MeshPacket &mp); void handleStoreDeviceUIConfig(const meshtastic_DeviceUIConfig &uicfg); void handleSendInputEvent(const meshtastic_AdminMessage_InputEvent &inputEvent); void reboot(int32_t seconds); diff --git a/test/support/AdminModuleTestShim.h b/test/support/AdminModuleTestShim.h index 6d9c3f1a8ec..c189bee84cc 100644 --- a/test/support/AdminModuleTestShim.h +++ b/test/support/AdminModuleTestShim.h @@ -12,6 +12,7 @@ class AdminModuleTestShim : public AdminModule using AdminModule::handleReceivedProtobuf; using AdminModule::handleSetConfig; using AdminModule::handleSetModuleConfig; + using AdminModule::responseIsSolicited; // request/response pairing gate using AdminModule::setPassKey; // Peek at the reply a handler queued, before drainReply() releases it. diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index 6502ab3c4b4..1af60e18986 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -1,6 +1,7 @@ // Deterministic reproduction of the admin session-key behavior discussed on PR #10669 // (ndoo's "Admin message without session_key!" report), plus the remote-vs-local redaction of -// secret material in admin GET responses. +// secret material in admin GET responses, plus the request/response pairing that decides which +// admin responses are accepted at all. // // Drives the REAL incoming-admin path (AdminModule::handleReceivedProtobuf) with a remote // (from != 0) PKC-authorized set_owner, exercising the exact checkPassKey/setPassKey gate. @@ -21,7 +22,9 @@ #include static constexpr NodeNum LOCAL_NODE = 0x0A0A0A0A; -static constexpr NodeNum ADMIN_NODE = 0x0B0B0B0B; // authorized admin, sends remote admin to us +static constexpr NodeNum ADMIN_NODE = 0x0B0B0B0B; // authorized admin, sends remote admin to us +static constexpr NodeNum QUERIED_NODE = 0x0C0C0C0C; // a remote we send admin requests to +static constexpr NodeNum STRANGER_NODE = 0x0D0D0D0D; static const uint8_t ADMIN_KEY[32] = {0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x20}; @@ -51,6 +54,54 @@ static meshtastic_MeshPacket makeRemoteSetOwner(const char *newLongName, const u return mp; } +// A get_module_config_response carrying a remote_hardware pin list, as a remote would answer. +// This is the class of message that short-circuited auth: no session passkey, sender need not +// hold an admin key. handleGetModuleConfigResponse() stamps mp.from into the pin table. +static meshtastic_MeshPacket makeModuleConfigResponse(NodeNum from, meshtastic_AdminMessage &out) +{ + out = meshtastic_AdminMessage_init_zero; + out.which_payload_variant = meshtastic_AdminMessage_get_module_config_response_tag; + out.get_module_config_response.which_payload_variant = meshtastic_ModuleConfig_remote_hardware_tag; + out.get_module_config_response.payload_variant.remote_hardware.enabled = true; + out.get_module_config_response.payload_variant.remote_hardware.available_pins_count = 1; + out.get_module_config_response.payload_variant.remote_hardware.available_pins[0].gpio_pin = 17; + + meshtastic_MeshPacket mp = meshtastic_MeshPacket_init_zero; + mp.from = from; + mp.to = LOCAL_NODE; + mp.channel = 0; + mp.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + return mp; +} + +// One known pin entry, so a response that reaches handleGetModuleConfigResponse rewrites its owner. +static void seedRemoteHardwarePin(NodeNum owner) +{ + devicestate.node_remote_hardware_pins_count = 1; + devicestate.node_remote_hardware_pins[0] = meshtastic_NodeRemoteHardwarePin_init_zero; + devicestate.node_remote_hardware_pins[0].node_num = owner; + devicestate.node_remote_hardware_pins[0].has_pin = true; + devicestate.node_remote_hardware_pins[0].pin.gpio_pin = 4; +} + +// The outgoing request a local client would send to `to`, as MeshService sees it (from == 0, +// ADMIN_APP, payload still plaintext). +static meshtastic_MeshPacket makeOutgoingModuleConfigRequest(NodeNum to) +{ + meshtastic_AdminMessage req = meshtastic_AdminMessage_init_zero; + req.which_payload_variant = meshtastic_AdminMessage_get_module_config_request_tag; + req.get_module_config_request = meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG; + + meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; + p.from = 0; + p.to = to; + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_ADMIN_APP; + p.decoded.payload.size = + pb_encode_to_bytes(p.decoded.payload.bytes, sizeof(p.decoded.payload.bytes), &meshtastic_AdminMessage_msg, &req); + return p; +} + void setUp(void) { mockService = new MockMeshService(); @@ -194,6 +245,91 @@ void test_local_security_config_keeps_private_key(void) admin->drainReply(); } +// An admin response carries no session passkey and its sender is not an admin-key holder, so a +// request we sent is the only thing vouching for it. A get_module_config_response from a node we +// never queried is not. +void test_unsolicited_response_is_not_solicited(void) +{ + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); + + TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp), "a response nobody asked for must not be accepted"); +} + +// Control: once the client has sent that node a request, its response is accepted. Without this, +// the test above would also pass if responseIsSolicited() simply always said no. +void test_response_after_our_request_is_solicited(void) +{ + admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(STRANGER_NODE)); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); + + TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(mp), "the answer to our own request must be accepted"); +} + +// A request to one remote does not vouch for a different remote's response. +void test_request_to_one_node_does_not_admit_another(void) +{ + admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(QUERIED_NODE)); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); // answered by someone else + + TEST_ASSERT_FALSE(admin->responseIsSolicited(mp)); +} + +// Only requests arm the gate: sending a setter to a node must not make it a trusted responder. +void test_outgoing_setter_does_not_admit_responses(void) +{ + meshtastic_AdminMessage setter = meshtastic_AdminMessage_init_zero; + setter.which_payload_variant = meshtastic_AdminMessage_set_owner_tag; + meshtastic_MeshPacket out = meshtastic_MeshPacket_init_zero; + out.to = STRANGER_NODE; + out.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + out.decoded.portnum = meshtastic_PortNum_ADMIN_APP; + out.decoded.payload.size = + pb_encode_to_bytes(out.decoded.payload.bytes, sizeof(out.decoded.payload.bytes), &meshtastic_AdminMessage_msg, &setter); + admin->noteOutgoingAdminRequest(out); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); + + TEST_ASSERT_FALSE(admin->responseIsSolicited(mp)); +} + +// End to end through the real handler: an unsolicited get_module_config_response must not reach +// handleGetModuleConfigResponse, so the remote_hardware pin table keeps its owner. +void test_unsolicited_response_does_not_poison_pins(void) +{ + seedRemoteHardwarePin(QUERIED_NODE); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); + admin->handleReceivedProtobuf(mp, &m); + admin->drainReply(); + + TEST_ASSERT_EQUAL_UINT32_MESSAGE(QUERIED_NODE, devicestate.node_remote_hardware_pins[0].node_num, + "unsolicited response must not rewrite the pin owner"); +} + +// Control: once we have requested it, the same response is handled and updates the pin table. This +// proves the assertion above is the auth gate firing, not the handler being dead. (It also exercises +// the dispatch tag fixed in this change - without it, the handler never runs for either node.) +void test_solicited_response_updates_pins(void) +{ + seedRemoteHardwarePin(QUERIED_NODE); + admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(STRANGER_NODE)); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); + admin->handleReceivedProtobuf(mp, &m); + admin->drainReply(); + + TEST_ASSERT_EQUAL_UINT32_MESSAGE(STRANGER_NODE, devicestate.node_remote_hardware_pins[0].node_num, + "a response we asked for must reach the handler"); +} + #endif // !(MESHTASTIC_EXCLUDE_PKI) void setup() @@ -207,6 +343,12 @@ void setup() RUN_TEST(test_session_gate_accepts_key_from_a_get_response); RUN_TEST(test_remote_security_config_omits_private_key); RUN_TEST(test_local_security_config_keeps_private_key); + RUN_TEST(test_unsolicited_response_is_not_solicited); + RUN_TEST(test_response_after_our_request_is_solicited); + RUN_TEST(test_request_to_one_node_does_not_admit_another); + RUN_TEST(test_outgoing_setter_does_not_admit_responses); + RUN_TEST(test_unsolicited_response_does_not_poison_pins); + RUN_TEST(test_solicited_response_updates_pins); #endif exit(UNITY_END()); } From 74c85cc1e8c2f0653d6c86bc44ad87ac5aff420b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 16 Jul 2026 21:08:50 +0200 Subject: [PATCH 2/4] AdminModule: rollover-safe request window, bind response to request type Two review refinements to the request/response pairing: Use Throttle::isWithinTimespanMs for the outstanding-request expiry instead of comparing millis()/1000 sums, which mis-expired across the millis() rollover. Bind each accepted response to a request type actually sent to that node. Each outstanding record now carries a bitmask of the response variants its requests authorize, so a get_owner request no longer admits a get_module_config response. The mask accumulates per remote, so a client may still pipeline several request types to one node and have every answer accepted. --- src/modules/AdminModule.cpp | 83 ++++++++++++++++++--- src/modules/AdminModule.h | 13 ++-- test/test_admin_session_repro/test_main.cpp | 54 +++++++++----- 3 files changed, 118 insertions(+), 32 deletions(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index afb9bd4a52b..36fe1f7f7e6 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -10,6 +10,7 @@ #include "input/InputBroker.h" #include "meshUtils.h" #include +#include #include // for better whitespace handling #if defined(ARCH_ESP32) && !MESHTASTIC_EXCLUDE_WIFI #include "MeshtasticOTA.h" @@ -128,7 +129,7 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta if (messageIsResponse(r)) { // Only accept a response from a remote we sent the matching request to. from == 0 is a // local client, which PhoneAPI has already gated. - if (mp.from != 0 && !responseIsSolicited(mp)) { + if (mp.from != 0 && !responseIsSolicited(mp, r->which_payload_variant)) { LOG_INFO("Ignore admin response from 0x%08x, no outstanding request", mp.from); return handled; } @@ -1826,6 +1827,64 @@ bool AdminModule::messageIsResponse(const meshtastic_AdminMessage *r) return false; } +// The response variant that answers a getter request, or 0 if the request has none. +static pb_size_t adminResponseForRequest(pb_size_t requestVariant) +{ + switch (requestVariant) { + case meshtastic_AdminMessage_get_channel_request_tag: + return meshtastic_AdminMessage_get_channel_response_tag; + case meshtastic_AdminMessage_get_owner_request_tag: + return meshtastic_AdminMessage_get_owner_response_tag; + case meshtastic_AdminMessage_get_config_request_tag: + return meshtastic_AdminMessage_get_config_response_tag; + case meshtastic_AdminMessage_get_module_config_request_tag: + return meshtastic_AdminMessage_get_module_config_response_tag; + case meshtastic_AdminMessage_get_canned_message_module_messages_request_tag: + return meshtastic_AdminMessage_get_canned_message_module_messages_response_tag; + case meshtastic_AdminMessage_get_device_metadata_request_tag: + return meshtastic_AdminMessage_get_device_metadata_response_tag; + case meshtastic_AdminMessage_get_ringtone_request_tag: + return meshtastic_AdminMessage_get_ringtone_response_tag; + case meshtastic_AdminMessage_get_device_connection_status_request_tag: + return meshtastic_AdminMessage_get_device_connection_status_response_tag; + case meshtastic_AdminMessage_get_node_remote_hardware_pins_request_tag: + return meshtastic_AdminMessage_get_node_remote_hardware_pins_response_tag; + case meshtastic_AdminMessage_get_ui_config_request_tag: + return meshtastic_AdminMessage_get_ui_config_response_tag; + default: + return 0; + } +} + +// A one-bit identifier for a response variant, so several expected responses pack into a mask. +static uint32_t adminResponseBit(pb_size_t responseVariant) +{ + switch (responseVariant) { + case meshtastic_AdminMessage_get_channel_response_tag: + return 1u << 0; + case meshtastic_AdminMessage_get_owner_response_tag: + return 1u << 1; + case meshtastic_AdminMessage_get_config_response_tag: + return 1u << 2; + case meshtastic_AdminMessage_get_module_config_response_tag: + return 1u << 3; + case meshtastic_AdminMessage_get_canned_message_module_messages_response_tag: + return 1u << 4; + case meshtastic_AdminMessage_get_device_metadata_response_tag: + return 1u << 5; + case meshtastic_AdminMessage_get_ringtone_response_tag: + return 1u << 6; + case meshtastic_AdminMessage_get_device_connection_status_response_tag: + return 1u << 7; + case meshtastic_AdminMessage_get_node_remote_hardware_pins_response_tag: + return 1u << 8; + case meshtastic_AdminMessage_get_ui_config_response_tag: + return 1u << 9; + default: + return 0; + } +} + void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) { if (p.which_payload_variant != meshtastic_MeshPacket_decoded_tag || p.decoded.portnum != meshtastic_PortNum_ADMIN_APP) @@ -1837,14 +1896,16 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) meshtastic_AdminMessage admin = meshtastic_AdminMessage_init_zero; if (!pb_decode_from_bytes(p.decoded.payload.bytes, p.decoded.payload.size, &meshtastic_AdminMessage_msg, &admin)) return; - if (!messageIsRequest(&admin)) - return; + const uint32_t bit = adminResponseBit(adminResponseForRequest(admin.which_payload_variant)); + if (!bit) + return; // not a getter whose response we can pair // Reuse this remote's slot if it has one, else a free slot, else the oldest. OutstandingAdminRequest *slot = nullptr; for (auto &o : outstandingAdminRequests) if (o.to == p.to) slot = &o; + const bool sameNode = slot != nullptr; if (!slot) for (auto &o : outstandingAdminRequests) if (o.to == 0) { @@ -1854,12 +1915,14 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) if (!slot) { slot = &outstandingAdminRequests[0]; for (auto &o : outstandingAdminRequests) - if (o.sentAtSecs < slot->sentAtSecs) + if (o.sentAtMs < slot->sentAtMs) slot = &o; } slot->to = p.to; - slot->sentAtSecs = millis() / 1000; + slot->sentAtMs = millis(); + // Keep the types already pending for this node so a client may pipeline several to it. + slot->expectedResponses = sameNode ? (slot->expectedResponses | bit) : bit; slot->keyValid = p.pki_encrypted && p.public_key.size == 32; if (slot->keyValid) memcpy(slot->key, p.public_key.bytes, 32); @@ -1868,16 +1931,18 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) LOG_DEBUG("Admin request sent to 0x%08x, expecting its response", p.to); } -bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp) +bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant) { - const uint32_t now = millis() / 1000; + const uint32_t bit = adminResponseBit(responseVariant); for (auto &o : outstandingAdminRequests) { if (o.to == 0 || o.to != mp.from) continue; - if (now > o.sentAtSecs + kOutstandingAdminRequestSecs) { - o.to = 0; + if (!Throttle::isWithinTimespanMs(o.sentAtMs, kOutstandingAdminRequestMs)) { + o.to = 0; // lapsed; free the slot return false; } + if (!(o.expectedResponses & bit)) + return false; // we queried this node, but not for this // A request the client pinned to a key must be answered over PKC by that same key. if (o.keyValid && (!mp.pki_encrypted || mp.public_key.size != 32 || memcmp(mp.public_key.bytes, o.key, 32) != 0)) return false; diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index 7a927d1ef63..cd808942db0 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -86,17 +86,18 @@ class AdminModule : public ProtobufModule, public Obser // the only thing vouching for one is a request we sent. Track those per remote: a client may // have several nodes in flight, and each keeps answering until the window lapses. static constexpr size_t kOutstandingAdminRequests = 3; - static constexpr uint32_t kOutstandingAdminRequestSecs = 300; // same window as the session passkey + static constexpr uint32_t kOutstandingAdminRequestMs = 300 * 1000; // same window as the session passkey struct OutstandingAdminRequest { - NodeNum to; // 0 = free slot - uint32_t sentAtSecs; // millis()/1000 when the request went out - uint8_t key[32]; // destination key, when the client pinned one + NodeNum to; // 0 = free slot + uint32_t sentAtMs; // millis() when the most recent request to this node went out + uint32_t expectedResponses; // bitmask of response variants these requests authorize + uint8_t key[32]; // destination key, when the client pinned one bool keyValid; }; OutstandingAdminRequest outstandingAdminRequests[kOutstandingAdminRequests] = {}; - /// Whether mp answers a request we actually sent to mp.from. - bool responseIsSolicited(const meshtastic_MeshPacket &mp); + /// Whether a response of variant responseVariant from mp.from answers a request we sent it. + bool responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant); void handleStoreDeviceUIConfig(const meshtastic_DeviceUIConfig &uicfg); void handleSendInputEvent(const meshtastic_AdminMessage_InputEvent &inputEvent); void reboot(int32_t seconds); diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index 1af60e18986..5e49f9f7960 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -84,14 +84,10 @@ static void seedRemoteHardwarePin(NodeNum owner) devicestate.node_remote_hardware_pins[0].pin.gpio_pin = 4; } -// The outgoing request a local client would send to `to`, as MeshService sees it (from == 0, +// The outgoing request `req` a local client would send to `to`, as MeshService sees it (from == 0, // ADMIN_APP, payload still plaintext). -static meshtastic_MeshPacket makeOutgoingModuleConfigRequest(NodeNum to) +static meshtastic_MeshPacket makeOutgoingRequest(NodeNum to, const meshtastic_AdminMessage &req) { - meshtastic_AdminMessage req = meshtastic_AdminMessage_init_zero; - req.which_payload_variant = meshtastic_AdminMessage_get_module_config_request_tag; - req.get_module_config_request = meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG; - meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; p.from = 0; p.to = to; @@ -102,6 +98,14 @@ static meshtastic_MeshPacket makeOutgoingModuleConfigRequest(NodeNum to) return p; } +static meshtastic_MeshPacket makeOutgoingModuleConfigRequest(NodeNum to) +{ + meshtastic_AdminMessage req = meshtastic_AdminMessage_init_zero; + req.which_payload_variant = meshtastic_AdminMessage_get_module_config_request_tag; + req.get_module_config_request = meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG; + return makeOutgoingRequest(to, req); +} + void setUp(void) { mockService = new MockMeshService(); @@ -248,12 +252,15 @@ void test_local_security_config_keeps_private_key(void) // An admin response carries no session passkey and its sender is not an admin-key holder, so a // request we sent is the only thing vouching for it. A get_module_config_response from a node we // never queried is not. +static constexpr pb_size_t MODULE_CONFIG_RESPONSE = meshtastic_AdminMessage_get_module_config_response_tag; + void test_unsolicited_response_is_not_solicited(void) { meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); - TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp), "a response nobody asked for must not be accepted"); + TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE), + "a response nobody asked for must not be accepted"); } // Control: once the client has sent that node a request, its response is accepted. Without this, @@ -265,7 +272,8 @@ void test_response_after_our_request_is_solicited(void) meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); - TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(mp), "the answer to our own request must be accepted"); + TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE), + "the answer to our own request must be accepted"); } // A request to one remote does not vouch for a different remote's response. @@ -276,7 +284,24 @@ void test_request_to_one_node_does_not_admit_another(void) meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); // answered by someone else - TEST_ASSERT_FALSE(admin->responseIsSolicited(mp)); + TEST_ASSERT_FALSE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE)); +} + +// A response only answers its own request type: a get_owner_request does not admit a +// get_module_config_response from the same node. +void test_response_variant_must_match_request(void) +{ + meshtastic_AdminMessage owner_req = meshtastic_AdminMessage_init_zero; + owner_req.which_payload_variant = meshtastic_AdminMessage_get_owner_request_tag; + admin->noteOutgoingAdminRequest(makeOutgoingRequest(STRANGER_NODE, owner_req)); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); // wrong type for the request + + TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE), + "a get_owner request must not admit a get_module_config response"); + // ...but the response it actually asked for is still accepted from the same slot. + TEST_ASSERT_TRUE(admin->responseIsSolicited(mp, meshtastic_AdminMessage_get_owner_response_tag)); } // Only requests arm the gate: sending a setter to a node must not make it a trusted responder. @@ -284,18 +309,12 @@ void test_outgoing_setter_does_not_admit_responses(void) { meshtastic_AdminMessage setter = meshtastic_AdminMessage_init_zero; setter.which_payload_variant = meshtastic_AdminMessage_set_owner_tag; - meshtastic_MeshPacket out = meshtastic_MeshPacket_init_zero; - out.to = STRANGER_NODE; - out.which_payload_variant = meshtastic_MeshPacket_decoded_tag; - out.decoded.portnum = meshtastic_PortNum_ADMIN_APP; - out.decoded.payload.size = - pb_encode_to_bytes(out.decoded.payload.bytes, sizeof(out.decoded.payload.bytes), &meshtastic_AdminMessage_msg, &setter); - admin->noteOutgoingAdminRequest(out); + admin->noteOutgoingAdminRequest(makeOutgoingRequest(STRANGER_NODE, setter)); meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); - TEST_ASSERT_FALSE(admin->responseIsSolicited(mp)); + TEST_ASSERT_FALSE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE)); } // End to end through the real handler: an unsolicited get_module_config_response must not reach @@ -346,6 +365,7 @@ void setup() RUN_TEST(test_unsolicited_response_is_not_solicited); RUN_TEST(test_response_after_our_request_is_solicited); RUN_TEST(test_request_to_one_node_does_not_admit_another); + RUN_TEST(test_response_variant_must_match_request); RUN_TEST(test_outgoing_setter_does_not_admit_responses); RUN_TEST(test_unsolicited_response_does_not_poison_pins); RUN_TEST(test_solicited_response_updates_pins); From d8e5dd43ce0f983befdcca04e6621e95e883cac2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Fri, 17 Jul 2026 09:16:49 +0200 Subject: [PATCH 3/4] AdminModule: track admin requests per-request, not per-node Reworks the outstanding-request table so each request is its own entry with its own expiry window and pinned key, replacing the per-node bitmask that shared one timestamp and one key across every response variant. That sharing let a later request to the same node extend an earlier one's window and, worse, clear its PKC pin: an unpinned request cleared keyValid, so a plaintext response to an earlier PKC-pinned request was then accepted. Per-request entries keep each pin intact. Identical requests are de-duplicated (a client may fetch several config subtypes, all answered by one response variant) and eviction compares elapsed time, which is rollover-safe. Test: a pinned request's response still requires its key after an unpinned request to the same node. --- src/modules/AdminModule.cpp | 70 +++++++-------------- src/modules/AdminModule.h | 15 ++--- test/test_admin_session_repro/test_main.cpp | 39 ++++++++++++ 3 files changed, 70 insertions(+), 54 deletions(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 36fe1f7f7e6..0ce95df1f9a 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -1856,35 +1856,6 @@ static pb_size_t adminResponseForRequest(pb_size_t requestVariant) } } -// A one-bit identifier for a response variant, so several expected responses pack into a mask. -static uint32_t adminResponseBit(pb_size_t responseVariant) -{ - switch (responseVariant) { - case meshtastic_AdminMessage_get_channel_response_tag: - return 1u << 0; - case meshtastic_AdminMessage_get_owner_response_tag: - return 1u << 1; - case meshtastic_AdminMessage_get_config_response_tag: - return 1u << 2; - case meshtastic_AdminMessage_get_module_config_response_tag: - return 1u << 3; - case meshtastic_AdminMessage_get_canned_message_module_messages_response_tag: - return 1u << 4; - case meshtastic_AdminMessage_get_device_metadata_response_tag: - return 1u << 5; - case meshtastic_AdminMessage_get_ringtone_response_tag: - return 1u << 6; - case meshtastic_AdminMessage_get_device_connection_status_response_tag: - return 1u << 7; - case meshtastic_AdminMessage_get_node_remote_hardware_pins_response_tag: - return 1u << 8; - case meshtastic_AdminMessage_get_ui_config_response_tag: - return 1u << 9; - default: - return 0; - } -} - void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) { if (p.which_payload_variant != meshtastic_MeshPacket_decoded_tag || p.decoded.portnum != meshtastic_PortNum_ADMIN_APP) @@ -1896,16 +1867,22 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) meshtastic_AdminMessage admin = meshtastic_AdminMessage_init_zero; if (!pb_decode_from_bytes(p.decoded.payload.bytes, p.decoded.payload.size, &meshtastic_AdminMessage_msg, &admin)) return; - const uint32_t bit = adminResponseBit(adminResponseForRequest(admin.which_payload_variant)); - if (!bit) + const pb_size_t responseVariant = adminResponseForRequest(admin.which_payload_variant); + if (!responseVariant) return; // not a getter whose response we can pair - // Reuse this remote's slot if it has one, else a free slot, else the oldest. + const bool keyValid = p.pki_encrypted && p.public_key.size == 32; + + // One entry per (node, response variant, pinning). Refresh an identical outstanding request + // rather than duplicate it (a client may fetch several config subtypes, all answered by one + // response variant); else take a free slot, else evict the oldest (rollover-safe elapsed). OutstandingAdminRequest *slot = nullptr; for (auto &o : outstandingAdminRequests) - if (o.to == p.to) + if (o.to == p.to && o.expectedResponse == responseVariant && o.keyValid == keyValid && + (!keyValid || memcmp(o.key, p.public_key.bytes, 32) == 0)) { slot = &o; - const bool sameNode = slot != nullptr; + break; + } if (!slot) for (auto &o : outstandingAdminRequests) if (o.to == 0) { @@ -1913,18 +1890,18 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) break; } if (!slot) { + const uint32_t now = millis(); slot = &outstandingAdminRequests[0]; for (auto &o : outstandingAdminRequests) - if (o.sentAtMs < slot->sentAtMs) + if ((uint32_t)(now - o.sentAtMs) > (uint32_t)(now - slot->sentAtMs)) slot = &o; } slot->to = p.to; slot->sentAtMs = millis(); - // Keep the types already pending for this node so a client may pipeline several to it. - slot->expectedResponses = sameNode ? (slot->expectedResponses | bit) : bit; - slot->keyValid = p.pki_encrypted && p.public_key.size == 32; - if (slot->keyValid) + slot->expectedResponse = responseVariant; + slot->keyValid = keyValid; + if (keyValid) memcpy(slot->key, p.public_key.bytes, 32); else memset(slot->key, 0, 32); @@ -1933,19 +1910,18 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant) { - const uint32_t bit = adminResponseBit(responseVariant); + // Scan every entry: several requests for the same variant may differ only in pinning, and an + // unpinned one still authorizes the response even if a pinned one does not. for (auto &o : outstandingAdminRequests) { - if (o.to == 0 || o.to != mp.from) + if (o.to != mp.from || o.expectedResponse != responseVariant) continue; if (!Throttle::isWithinTimespanMs(o.sentAtMs, kOutstandingAdminRequestMs)) { - o.to = 0; // lapsed; free the slot - return false; + o.to = 0; // lapsed; free the slot and keep looking for another live match + continue; } - if (!(o.expectedResponses & bit)) - return false; // we queried this node, but not for this - // A request the client pinned to a key must be answered over PKC by that same key. + // A request pinned to a PKC key must be answered over PKC by that same key. if (o.keyValid && (!mp.pki_encrypted || mp.public_key.size != 32 || memcmp(mp.public_key.bytes, o.key, 32) != 0)) - return false; + continue; return true; } return false; diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index cd808942db0..136966c7e1f 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -82,16 +82,17 @@ class AdminModule : public ProtobufModule, public Obser void noteOutgoingAdminRequest(const meshtastic_MeshPacket &p); private: - // An admin response carries no session passkey and its sender is not an admin-key holder, so - // the only thing vouching for one is a request we sent. Track those per remote: a client may - // have several nodes in flight, and each keeps answering until the window lapses. - static constexpr size_t kOutstandingAdminRequests = 3; + // An admin response carries no session passkey and its sender is not an admin-key holder, so a + // request we sent is the only thing vouching for it. Track each request independently - its own + // expiry window and pinned key - so a later request to the same node can neither extend an + // earlier one's window nor relax its PKC pin. + static constexpr size_t kOutstandingAdminRequests = 8; static constexpr uint32_t kOutstandingAdminRequestMs = 300 * 1000; // same window as the session passkey struct OutstandingAdminRequest { NodeNum to; // 0 = free slot - uint32_t sentAtMs; // millis() when the most recent request to this node went out - uint32_t expectedResponses; // bitmask of response variants these requests authorize - uint8_t key[32]; // destination key, when the client pinned one + uint32_t sentAtMs; // millis() when this request went out + pb_size_t expectedResponse; // the one response variant this request authorizes + uint8_t key[32]; // pinned destination key when the request went 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 5e49f9f7960..92cb640c4b7 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -304,6 +304,44 @@ void test_response_variant_must_match_request(void) TEST_ASSERT_TRUE(admin->responseIsSolicited(mp, meshtastic_AdminMessage_get_owner_response_tag)); } +// Regression: each request keeps its own pinned key. A later unpinned request to the same node +// 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); + + // 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); + + // 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 + + // 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), + "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); + TEST_ASSERT_TRUE(admin->responseIsSolicited(resp, meshtastic_AdminMessage_get_config_response_tag)); + // 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)); +} + // Only requests arm the gate: sending a setter to a node must not make it a trusted responder. void test_outgoing_setter_does_not_admit_responses(void) { @@ -366,6 +404,7 @@ void setup() RUN_TEST(test_response_after_our_request_is_solicited); 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_outgoing_setter_does_not_admit_responses); RUN_TEST(test_unsolicited_response_does_not_poison_pins); RUN_TEST(test_solicited_response_updates_pins); From 8b73e68a614bcc2cb50a5ba402b0ea223eec52f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Fri, 17 Jul 2026 09:39:23 +0200 Subject: [PATCH 4/4] AdminModule: match module-config subtype and consume answered requests Two refinements to the request/response pairing: Only remote_hardware get_module_config_response mutates state (the pin table), so it must answer a request for that exact ModuleConfigType, not just any module-config request. Each entry records the requested subtype and the gate checks it. A matched request is now consumed on accept, so a node cannot replay a state-mutating response within the window. Because one request yields one response, request de-dup is dropped (a client's N indexed get_channel requests are N entries, each consumed once). Tests: a non-remote-hardware request does not admit a remote_hardware response, and a second copy of an answered response is rejected. --- src/modules/AdminModule.cpp | 30 ++++++----- src/modules/AdminModule.h | 13 ++--- test/test_admin_session_repro/test_main.cpp | 55 ++++++++++++++++----- 3 files changed, 68 insertions(+), 30 deletions(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 0ce95df1f9a..36a7d6fe97e 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -129,7 +129,10 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta if (messageIsResponse(r)) { // Only accept a response from a remote we sent the matching request to. from == 0 is a // local client, which PhoneAPI has already gated. - if (mp.from != 0 && !responseIsSolicited(mp, r->which_payload_variant)) { + const pb_size_t moduleConfigTag = r->which_payload_variant == meshtastic_AdminMessage_get_module_config_response_tag + ? r->get_module_config_response.which_payload_variant + : 0; + if (mp.from != 0 && !responseIsSolicited(mp, r->which_payload_variant, moduleConfigTag)) { LOG_INFO("Ignore admin response from 0x%08x, no outstanding request", mp.from); return handled; } @@ -1873,22 +1876,14 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) const bool keyValid = p.pki_encrypted && p.public_key.size == 32; - // One entry per (node, response variant, pinning). Refresh an identical outstanding request - // rather than duplicate it (a client may fetch several config subtypes, all answered by one - // response variant); else take a free slot, else evict the oldest (rollover-safe elapsed). + // 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. OutstandingAdminRequest *slot = nullptr; for (auto &o : outstandingAdminRequests) - if (o.to == p.to && o.expectedResponse == responseVariant && o.keyValid == keyValid && - (!keyValid || memcmp(o.key, p.public_key.bytes, 32) == 0)) { + if (o.to == 0) { slot = &o; break; } - if (!slot) - for (auto &o : outstandingAdminRequests) - if (o.to == 0) { - slot = &o; - break; - } if (!slot) { const uint32_t now = millis(); slot = &outstandingAdminRequests[0]; @@ -1900,6 +1895,9 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) slot->to = p.to; 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); @@ -1908,7 +1906,7 @@ void AdminModule::noteOutgoingAdminRequest(const meshtastic_MeshPacket &p) LOG_DEBUG("Admin request sent to 0x%08x, expecting its response", p.to); } -bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant) +bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant, pb_size_t moduleConfigTag) { // Scan every entry: several requests for the same variant may differ only in pinning, and an // unpinned one still authorizes the response even if a pinned one does not. @@ -1922,6 +1920,12 @@ bool AdminModule::responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t // A request pinned to a PKC key must be answered over PKC by that same key. if (o.keyValid && (!mp.pki_encrypted || mp.public_key.size != 32 || memcmp(mp.public_key.bytes, o.key, 32) != 0)) continue; + // remote_hardware is the only module config that mutates state (the pin table), so require + // it to answer a request for that exact subtype, not just any get_module_config_request. + if (moduleConfigTag == meshtastic_ModuleConfig_remote_hardware_tag && + o.moduleConfigType != meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG) + continue; + o.to = 0; // consume: one request authorizes one response, no replay within the window return true; } return false; diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index 136966c7e1f..bfc3035b511 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -82,23 +82,24 @@ class AdminModule : public ProtobufModule, public Obser void noteOutgoingAdminRequest(const meshtastic_MeshPacket &p); private: - // An admin response carries no session passkey and its sender is not an admin-key holder, so a - // request we sent is the only thing vouching for it. Track each request independently - its own - // expiry window and pinned key - so a later request to the same node can neither extend an - // earlier one's window nor relax its PKC pin. + // An admin response has no session passkey and its sender need not hold an admin key, so a + // request we sent is the only thing vouching for it. Track each request independently (its own + // expiry and pinned key) so a later one can't extend or relax an earlier one. static constexpr size_t kOutstandingAdminRequests = 8; static constexpr uint32_t kOutstandingAdminRequestMs = 300 * 1000; // same window as the session passkey struct OutstandingAdminRequest { NodeNum to; // 0 = free slot 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 bool keyValid; }; OutstandingAdminRequest outstandingAdminRequests[kOutstandingAdminRequests] = {}; - /// Whether a response of variant responseVariant from mp.from answers a request we sent it. - bool responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant); + /// Whether a response (variant responseVariant, module-config subtype moduleConfigTag or 0) + /// from mp.from answers a request we sent; consumes the matched request so it can't be replayed. + bool responseIsSolicited(const meshtastic_MeshPacket &mp, pb_size_t responseVariant, pb_size_t moduleConfigTag); void handleStoreDeviceUIConfig(const meshtastic_DeviceUIConfig &uicfg); void handleSendInputEvent(const meshtastic_AdminMessage_InputEvent &inputEvent); void reboot(int32_t seconds); diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index 92cb640c4b7..2a4cdda8d3d 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -98,11 +98,12 @@ static meshtastic_MeshPacket makeOutgoingRequest(NodeNum to, const meshtastic_Ad return p; } -static meshtastic_MeshPacket makeOutgoingModuleConfigRequest(NodeNum to) +static meshtastic_MeshPacket makeOutgoingModuleConfigRequest( + NodeNum to, meshtastic_AdminMessage_ModuleConfigType type = meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG) { meshtastic_AdminMessage req = meshtastic_AdminMessage_init_zero; req.which_payload_variant = meshtastic_AdminMessage_get_module_config_request_tag; - req.get_module_config_request = meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG; + req.get_module_config_request = type; return makeOutgoingRequest(to, req); } @@ -253,13 +254,14 @@ void test_local_security_config_keeps_private_key(void) // request we sent is the only thing vouching for it. A get_module_config_response from a node we // never queried is not. static constexpr pb_size_t MODULE_CONFIG_RESPONSE = meshtastic_AdminMessage_get_module_config_response_tag; +static constexpr pb_size_t REMOTE_HW_TAG = meshtastic_ModuleConfig_remote_hardware_tag; // makeModuleConfigResponse subtype void test_unsolicited_response_is_not_solicited(void) { meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); - TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE), + TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG), "a response nobody asked for must not be accepted"); } @@ -272,7 +274,7 @@ void test_response_after_our_request_is_solicited(void) meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); - TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE), + TEST_ASSERT_TRUE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG), "the answer to our own request must be accepted"); } @@ -284,7 +286,7 @@ void test_request_to_one_node_does_not_admit_another(void) meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); // answered by someone else - TEST_ASSERT_FALSE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE)); + TEST_ASSERT_FALSE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG)); } // A response only answers its own request type: a get_owner_request does not admit a @@ -298,10 +300,10 @@ void test_response_variant_must_match_request(void) meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); // wrong type for the request - TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE), + TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG), "a get_owner request must not admit a get_module_config response"); // ...but the response it actually asked for is still accepted from the same slot. - TEST_ASSERT_TRUE(admin->responseIsSolicited(mp, meshtastic_AdminMessage_get_owner_response_tag)); + TEST_ASSERT_TRUE(admin->responseIsSolicited(mp, meshtastic_AdminMessage_get_owner_response_tag, 0)); } // Regression: each request keeps its own pinned key. A later unpinned request to the same node @@ -329,17 +331,46 @@ void test_pinned_request_keeps_its_key_after_an_unpinned_request(void) meshtastic_MeshPacket resp = makeModuleConfigResponse(STRANGER_NODE, m); // from set; 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), + 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); - TEST_ASSERT_TRUE(admin->responseIsSolicited(resp, meshtastic_AdminMessage_get_config_response_tag)); + 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)); + TEST_ASSERT_TRUE(admin->responseIsSolicited(resp, meshtastic_AdminMessage_get_owner_response_tag, 0)); +} + +// A remote_hardware response must answer a request for that exact subtype, not just any module +// config - else an MQTT-config request could authorize a pin-table update. +void test_module_config_subtype_must_match(void) +{ + admin->noteOutgoingAdminRequest( + makeOutgoingModuleConfigRequest(STRANGER_NODE, meshtastic_AdminMessage_ModuleConfigType_MQTT_CONFIG)); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); // remote_hardware subtype + TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG), + "a non-remote-hardware request must not admit a remote_hardware response"); + + // Control: a request for the matching subtype does admit it. + admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(STRANGER_NODE)); + TEST_ASSERT_TRUE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG)); +} + +// A matched request is consumed, so a node cannot replay a state-mutating response within the window. +void test_response_is_consumed_no_replay(void) +{ + admin->noteOutgoingAdminRequest(makeOutgoingModuleConfigRequest(STRANGER_NODE)); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); + TEST_ASSERT_TRUE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG)); + TEST_ASSERT_FALSE_MESSAGE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG), + "a second copy of an already-answered response must be rejected"); } // Only requests arm the gate: sending a setter to a node must not make it a trusted responder. @@ -352,7 +383,7 @@ void test_outgoing_setter_does_not_admit_responses(void) meshtastic_AdminMessage m; meshtastic_MeshPacket mp = makeModuleConfigResponse(STRANGER_NODE, m); - TEST_ASSERT_FALSE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE)); + TEST_ASSERT_FALSE(admin->responseIsSolicited(mp, MODULE_CONFIG_RESPONSE, REMOTE_HW_TAG)); } // End to end through the real handler: an unsolicited get_module_config_response must not reach @@ -405,6 +436,8 @@ 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_module_config_subtype_must_match); + RUN_TEST(test_response_is_consumed_no_replay); RUN_TEST(test_outgoing_setter_does_not_admit_responses); RUN_TEST(test_unsolicited_response_does_not_poison_pins); RUN_TEST(test_solicited_response_updates_pins);