From 2189f0d77f54bf1c91cddedccea9f99b81be9eaf Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:38:14 -0700 Subject: [PATCH 1/7] fix(security): keep verified signer state packet-local Remove the process-global signer context and carry only the exact Router-verified key on the authenticated packet. Clear serialized authentication metadata before every untrusted decode, require both the verified-signature marker and packet-local key in licensed Admin authorization, and expand authorization/session/isolation coverage while cleaning routing test ownership. --- src/mesh/Router.cpp | 3 ++- src/modules/AdminModule.cpp | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 0d40ae715f9..d044b3682fe 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -816,6 +816,7 @@ RoutingAuthVerdict passesRoutingAuthGate(meshtastic_MeshPacket *p) // decryptor. Never trust serialized local authentication metadata on that boundary. authCandidate.pki_encrypted = false; authCandidate.public_key.size = 0; + authCandidate.xeddsa_signed = false; #if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA) concurrency::LockGuard g(cryptLock); if (!checkXeddsaReceivePolicy(&authCandidate)) { @@ -823,7 +824,6 @@ RoutingAuthVerdict passesRoutingAuthGate(meshtastic_MeshPacket *p) return RoutingAuthVerdict::REJECT; } #endif - p->xeddsa_signed = authCandidate.xeddsa_signed; wire = *p; storeRoutingAuthCache(wire, authCandidate); return RoutingAuthVerdict::ACCEPT; @@ -932,6 +932,7 @@ DecodeState perhapsDecode(meshtastic_MeshPacket *p) // Authentication metadata is local-only. Re-establish it below only after successful PKI decryption. p->pki_encrypted = false; p->public_key.size = 0; + p->xeddsa_signed = false; size_t rawSize = p->encrypted.size; if (rawSize > sizeof(bytes)) { diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index abc71034e6e..0fa11fd3d8d 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -146,6 +146,29 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta } #endif meshtastic_Channel *ch = &channels.getByIndex(mp.channel); + const bool licensedRemote = owner.is_licensed && mp.from != 0; + bool authorizedLicensedSigner = false; + if (licensedRemote) { + const bool directedAdmin = mp.to == nodeDB->getNodeNum() && !isBroadcast(mp.to) && + mp.decoded.portnum == meshtastic_PortNum_ADMIN_APP && !mp.pki_encrypted; + if (!directedAdmin || !mp.xeddsa_signed || mp.public_key.size != 32) { + LOG_INFO("Ignore licensed admin payload without a directed Router-verified signature"); + myReply = allocErrorResponse(meshtastic_Routing_Error_NOT_AUTHORIZED, &mp); + return handled; + } + for (const auto &adminKey : config.security.admin_key) { + if (adminKey.size == 32 && memcmp(mp.public_key.bytes, adminKey.bytes, 32) == 0) { + authorizedLicensedSigner = true; + break; + } + } + if (!messageIsResponse(r) && !authorizedLicensedSigner) { + LOG_INFO("Received signed licensed admin payload from a non-allowlisted key"); + myReply = allocErrorResponse(meshtastic_Routing_Error_ADMIN_PUBLIC_KEY_UNAUTHORIZED, &mp); + return handled; + } + LOG_INFO("Signed licensed admin payload with authorized Router-verified sender"); + } 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. From 9ead9a9634874bd16d99599708b96945d1ed0f19 Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Thu, 9 Jul 2026 21:46:48 -0700 Subject: [PATCH 2/7] fix: address packet auth static analysis Make the routing authentication gate input const and collapse licensed Admin authorization into a single branch so legacy response, local, channel, and PKI checks run only for non-licensed-remote traffic. --- src/mesh/FloodingRouter.cpp | 2 +- src/mesh/Router.cpp | 2 +- src/mesh/Router.h | 2 +- src/modules/AdminModule.cpp | 3 ++- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index 7048cd91873..5988bacac5d 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -71,7 +71,7 @@ bool FloodingRouter::perhapsHandleUpgradedPacket(const meshtastic_MeshPacket *p) // Verify the replacement before deleting the valid lower-hop copy waiting in the TX queue. // This is intentionally redundant with ReliableRouter's ingress gate: it keeps this helper // safe if another caller is introduced later. - if (passesRoutingAuthGate(const_cast(p)) != RoutingAuthVerdict::ACCEPT) + if (passesRoutingAuthGate(p) != RoutingAuthVerdict::ACCEPT) return true; // If we overhear a duplicate copy of the packet with more hops left than the one we are waiting to diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index d044b3682fe..d36aa482279 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -800,7 +800,7 @@ bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p) } #endif -RoutingAuthVerdict passesRoutingAuthGate(meshtastic_MeshPacket *p) +RoutingAuthVerdict passesRoutingAuthGate(const meshtastic_MeshPacket *p) { // Routing still needs the original encrypted representation for byte-for-byte relay and for // MQTT uplink. Authenticate a copy here; handleReceived() performs the normal in-place decode diff --git a/src/mesh/Router.h b/src/mesh/Router.h index d4ffe85c15b..3f753300cd7 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -265,7 +265,7 @@ enum class RoutingAuthVerdict { ACCEPT, OPAQUE_RELAY_ONLY, REJECT }; DecodeState perhapsDecode(meshtastic_MeshPacket *p); /** Apply receive authentication before routing state mutation; unknown-channel packets may remain opaque relay-only. */ -RoutingAuthVerdict passesRoutingAuthGate(meshtastic_MeshPacket *p); +RoutingAuthVerdict passesRoutingAuthGate(const meshtastic_MeshPacket *p); #ifdef PIO_UNIT_TESTING uint32_t routingAuthEvaluationCount(); void resetRoutingAuthEvaluationCount(); diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 0fa11fd3d8d..841fd68727c 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -148,6 +148,7 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta meshtastic_Channel *ch = &channels.getByIndex(mp.channel); const bool licensedRemote = owner.is_licensed && mp.from != 0; bool authorizedLicensedSigner = false; + // Could tighten responses further by tracking the last public key queried. if (licensedRemote) { const bool directedAdmin = mp.to == nodeDB->getNodeNum() && !isBroadcast(mp.to) && mp.decoded.portnum == meshtastic_PortNum_ADMIN_APP && !mp.pki_encrypted; @@ -167,7 +168,7 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta myReply = allocErrorResponse(meshtastic_Routing_Error_ADMIN_PUBLIC_KEY_UNAUTHORIZED, &mp); return handled; } - LOG_INFO("Signed licensed admin payload with authorized Router-verified sender"); + LOG_INFO("Signed licensed admin payload with Router-verified sender"); } if (messageIsResponse(r)) { // Only accept a response from a remote we sent the matching request to. from == 0 is a From ad24e277b2f5c9c63f4d721dd01dd182609be903 Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Thu, 9 Jul 2026 22:03:13 -0700 Subject: [PATCH 3/7] fix(security): reverify lost routing auth handoffs --- src/mesh/FloodingRouter.cpp | 4 +- src/mesh/Router.cpp | 74 ++++++++++++++++++++++--------------- src/mesh/Router.h | 8 +++- 3 files changed, 51 insertions(+), 35 deletions(-) diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index 5988bacac5d..1b2f05c978e 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -68,9 +68,7 @@ bool FloodingRouter::perhapsHandleUpgradedPacket(const meshtastic_MeshPacket *p) { // isRebroadcaster() is duplicated in perhapsRebroadcast(), but this avoids confusing log messages if (isRebroadcaster() && iface && p->hop_limit > 0) { - // Verify the replacement before deleting the valid lower-hop copy waiting in the TX queue. - // This is intentionally redundant with ReliableRouter's ingress gate: it keeps this helper - // safe if another caller is introduced later. + // Re-authenticate before replacing the queued lower-hop copy so future callers remain safe. if (passesRoutingAuthGate(p) != RoutingAuthVerdict::ACCEPT) return true; diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index d36aa482279..96bef7bf2ed 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -178,6 +178,8 @@ static bool routingAuthCacheMatches(const meshtastic_MeshPacket &packet) static void storeRoutingAuthCache(const meshtastic_MeshPacket &wire, const meshtastic_MeshPacket &authenticated) { + if (!routingAuthCacheLock) + return; concurrency::LockGuard guard(routingAuthCacheLock); routingAuthCache.wire = wire; routingAuthCache.authenticated = authenticated; @@ -800,61 +802,66 @@ bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p) } #endif -RoutingAuthVerdict passesRoutingAuthGate(const meshtastic_MeshPacket *p) +static RoutingAuthVerdict evaluateRoutingAuth(const meshtastic_MeshPacket &wire, meshtastic_MeshPacket &authenticated) { - // Routing still needs the original encrypted representation for byte-for-byte relay and for - // MQTT uplink. Authenticate a copy here; handleReceived() performs the normal in-place decode - // only after stateful routing filters have completed. - if (routingAuthCacheMatches(*p)) - return RoutingAuthVerdict::ACCEPT; - - meshtastic_MeshPacket wire = *p; - meshtastic_MeshPacket authCandidate = *p; + authenticated = wire; routingAuthEvaluations++; - if (authCandidate.which_payload_variant == meshtastic_MeshPacket_decoded_tag) { + if (authenticated.which_payload_variant == meshtastic_MeshPacket_decoded_tag) { // Already-decoded remote ingress (notably Portduino SimRadio) did not pass through a // decryptor. Never trust serialized local authentication metadata on that boundary. - authCandidate.pki_encrypted = false; - authCandidate.public_key.size = 0; - authCandidate.xeddsa_signed = false; + authenticated.pki_encrypted = false; + authenticated.public_key.size = 0; + authenticated.xeddsa_signed = false; #if !(MESHTASTIC_EXCLUDE_PKI) && !(MESHTASTIC_EXCLUDE_XEDDSA) concurrency::LockGuard g(cryptLock); - if (!checkXeddsaReceivePolicy(&authCandidate)) { - LOG_WARN("Already-decoded packet rejected by signature policy"); + if (!checkXeddsaReceivePolicy(&authenticated)) { + LOG_WARN("Already-decoded packet rejected by routing signature policy"); return RoutingAuthVerdict::REJECT; } #endif - wire = *p; - storeRoutingAuthCache(wire, authCandidate); return RoutingAuthVerdict::ACCEPT; } - const DecodeState state = perhapsDecode(&authCandidate); + const DecodeState state = perhapsDecode(&authenticated); if (state == DecodeState::DECODE_POLICY_REJECT) { - LOG_WARN("Packet rejected by signature policy"); + LOG_WARN("Packet rejected by routing signature policy"); return RoutingAuthVerdict::REJECT; } if (state == DecodeState::DECODE_FATAL) { - LOG_WARN("Fatal decode error, drop packet"); + LOG_WARN("Fatal decode error during routing authentication"); return RoutingAuthVerdict::REJECT; } if (state == DecodeState::DECODE_FAILURE) { // One-byte hash collisions are indistinguishable from tampering, so relay opaquely // instead of blackholing; isFromUs stays REJECT to keep forged senders off the ACK path. - if (!isToUs(p) && !isFromUs(p)) { + if (!isToUs(&wire) && !isFromUs(&wire)) { LOG_WARN("Decryptable packet failed decoding, relay opaquely"); return RoutingAuthVerdict::OPAQUE_RELAY_ONLY; } - LOG_WARN("Decryptable packet failed decoding, drop"); + LOG_WARN("Decryptable packet failed routing authentication"); return RoutingAuthVerdict::REJECT; } // Only an explicit unknown-channel result remains eligible for opaque relay. if (state == DecodeState::DECODE_OPAQUE) return RoutingAuthVerdict::OPAQUE_RELAY_ONLY; - storeRoutingAuthCache(wire, authCandidate); return RoutingAuthVerdict::ACCEPT; } +RoutingAuthVerdict passesRoutingAuthGate(const meshtastic_MeshPacket *p) +{ + // Routing still needs the original encrypted representation for byte-for-byte relay and for + // MQTT uplink. Authenticate a copy here; handleReceived() performs the normal in-place decode + // only after stateful routing filters have completed. + if (routingAuthCacheMatches(*p)) + return RoutingAuthVerdict::ACCEPT; + + meshtastic_MeshPacket authenticated = meshtastic_MeshPacket_init_zero; + const RoutingAuthVerdict verdict = evaluateRoutingAuth(*p, authenticated); + if (verdict == RoutingAuthVerdict::ACCEPT) + storeRoutingAuthCache(*p, authenticated); + return verdict; +} + #if !(MESHTASTIC_EXCLUDE_PKI) // The fallback costs three X25519 ops before the AEAD tag is checked. Budget is global because p->from is // attacker-controlled; successful runs refund, and their key is then persisted for the fast path. @@ -1424,7 +1431,7 @@ void Router::deliverLocal(meshtastic_MeshPacket *p, RxSource src) * Handle any packet that is received by an interface on this node. * Note: some packets may merely being passed through this node and will be forwarded elsewhere. */ -void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) +void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src, bool routingAuthRequired) { { concurrency::LockGuard g(&deferredLock); @@ -1435,7 +1442,7 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) #endif } - dispatchReceived(p, src); + dispatchReceived(p, src, routingAuthRequired); // Decide "am I the last frame" and drop the depth in one critical section. Splitting them lets // two frames both read the same pre-decrement value, skip the drain, and strand the ring. @@ -1456,12 +1463,12 @@ void Router::handleReceived(meshtastic_MeshPacket *p, RxSource src) } // Depth stays at 1 across the drain, so a loopback from these modules defers instead of // recursing, and dispatch runs outside the lock. - dispatchReceived(d.p, d.src); + dispatchReceived(d.p, d.src, false); packetPool.release(d.p); } } -void Router::dispatchReceived(meshtastic_MeshPacket *p, RxSource src) +void Router::dispatchReceived(meshtastic_MeshPacket *p, RxSource src, bool routingAuthRequired) { bool skipHandle = false; @@ -1477,8 +1484,15 @@ void Router::dispatchReceived(meshtastic_MeshPacket *p, RxSource src) // Consume the decoded/authenticated handoff after preserving the exact encrypted packet and // before mutating any packet fields that participate in the exact cache match. - if (src == RX_SRC_RADIO) - applyRoutingAuthCache(p); + if (routingAuthRequired && !applyRoutingAuthCache(p)) { + meshtastic_MeshPacket authenticated = meshtastic_MeshPacket_init_zero; + if (evaluateRoutingAuth(*p, authenticated) != RoutingAuthVerdict::ACCEPT) { + LOG_WARN("Routing authentication handoff was lost and packet re-verification failed"); + packetPool.release(p_encrypted); + return; + } + *p = authenticated; + } // Keep the decoded working packet and encrypted MQTT copy on the same local arrival timestamp. // See computeRxTimeStamp() for the placeholder/has_rx_time semantics. @@ -1711,6 +1725,6 @@ void Router::perhapsHandleReceived(meshtastic_MeshPacket *p) // Note: we avoid calling shouldFilterReceived if we are supposed to ignore certain nodes - because some overrides might // cache/learn of the existence of nodes (i.e. FloodRouter) that they should not - handleReceived(p); + handleReceived(p, RX_SRC_RADIO, true); packetPool.release(p); } diff --git a/src/mesh/Router.h b/src/mesh/Router.h index 3f753300cd7..36a843e7462 100644 --- a/src/mesh/Router.h +++ b/src/mesh/Router.h @@ -125,6 +125,10 @@ class Router : protected concurrency::OSThread, protected PacketHistory before us */ uint32_t rxDupe = 0, txRelayCanceled = 0; +#ifdef PIO_UNIT_TESTING + void handleReceivedAfterRoutingGateForTest(meshtastic_MeshPacket *p) { handleReceived(p, RX_SRC_RADIO, true); } +#endif + protected: friend class RoutingModule; @@ -188,14 +192,14 @@ class Router : protected concurrency::OSThread, protected PacketHistory * Called from perhapsHandleReceived() for radio ingress and from deliverLocal() for our own * loopback, so p may be locally generated. Does NOT free p; the caller still owns it. */ - void handleReceived(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO); + void handleReceived(meshtastic_MeshPacket *p, RxSource src = RX_SRC_RADIO, bool routingAuthRequired = false); /** * The body of handleReceived(): decode, run modules, publish to MQTT. Split out so the * depth-guarded drain in handleReceived() can process a deferred packet without re-entering * the drain (and without touching handleDepth) - keeping the stack flat. */ - void dispatchReceived(meshtastic_MeshPacket *p, RxSource src); + void dispatchReceived(meshtastic_MeshPacket *p, RxSource src, bool routingAuthRequired = false); /** * Route a packet addressed to us (or a local broadcast we loop back) into handleReceived(). From 2784c548b00afc0b2a2c6cb6caf6bf1e90bda488 Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Fri, 10 Jul 2026 21:42:26 -0700 Subject: [PATCH 4/7] test: stabilize signed admin native coverage --- src/modules/AdminModule.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 841fd68727c..ae526a0f5f2 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -230,7 +230,7 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta // that pointer is unrelated, so the path was unsafe.) // Automatically favorite the node that is using the admin key - auto remoteNode = nodeDB->getMeshNode(mp.from); + auto remoteNode = nodeDB ? nodeDB->getMeshNode(mp.from) : nullptr; if (remoteNode && !nodeInfoLiteIsFavorite(remoteNode)) { if (config.device.role == meshtastic_Config_DeviceConfig_Role_CLIENT_BASE) { // Special case for CLIENT_BASE: is_favorite has special meaning, and we don't want to automatically set it From 7119b8c787aac61dd9e1e51f874864403f190333 Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Sun, 12 Jul 2026 15:24:55 -0700 Subject: [PATCH 5/7] fix(mqtt): preserve signed packet metadata --- src/mqtt/MQTT.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index 6bd2f3688fa..3ab9e92ca9c 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -124,6 +124,9 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length) if (!pAck) return; pAck->transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT; + // sendLocal consumes packets sent to a live interface, but returns SHOULD_RELEASE + // when it handled a local delivery synchronously. Match MeshService::sendToMesh's + // ownership contract so the MQTT acknowledgement cannot leak on the local path. if (router->sendLocal(pAck) == ERRNO_SHOULD_RELEASE) packetPool.release(pAck); } else { @@ -160,6 +163,11 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length) if (shouldDropMqttDownlink(*p)) return; + // A decoded MQTT packet has already crossed the broker boundary, so authenticate it in place + // before handing it to the router. The routing-auth cache intentionally keeps an authenticated + // copy separate for encrypted packets, but a cache hit alone does not update this packet's + // xeddsa_signed marker (which is part of the downstream message metadata). + bool decodedAuthChecked = false; if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) { if (moduleConfig.mqtt.encryption_enabled) { @@ -176,12 +184,14 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length) // signature policy here: verify a carried signature and apply unsigned-downgrade // protection for known signers. Without this, a peer on a plaintext broker could // impersonate a signing node with unsigned broadcasts. Hold cryptLock like the RF path - // (perhapsDecode) does - checkXeddsaReceivePolicy -> xeddsa_verify mutates shared - // CryptoEngine cache state, and MQTT ingress can run on a different task. - if (passesRoutingAuthGate(p.get()) != RoutingAuthVerdict::ACCEPT) { + // CryptoEngine cache state, and MQTT ingress can run on a different task. The in-place + // call preserves the verified xeddsa_signed marker for downstream routing/UI consumers. + concurrency::LockGuard g(cryptLock); + if (!checkXeddsaReceivePolicy(p.get())) { LOG_INFO("Ignore decoded msg failing XEdDSA policy"); return; } + decodedAuthChecked = true; #endif } @@ -193,7 +203,7 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length) // likely they discovered each other via a channel we have downlink enabled for if (isToUs(p.get()) || (nodeInfoLiteHasUser(tx) && nodeInfoLiteHasUser(rx))) router->enqueueReceivedMessage(p.release()); - } else if (router && passesRoutingAuthGate(p.get()) == RoutingAuthVerdict::ACCEPT) + } else if (router && (decodedAuthChecked || passesRoutingAuthGate(p.get()) == RoutingAuthVerdict::ACCEPT)) router->enqueueReceivedMessage(p.release()); } From 201f5ee6d04f55a41860a64e76978737ae2961cd Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Thu, 23 Jul 2026 17:30:42 -0700 Subject: [PATCH 6/7] fix: accept signed licensed admin requests --- src/modules/AdminModule.cpp | 2 ++ src/mqtt/MQTT.cpp | 9 ++---- test/test_admin_session_repro/test_main.cpp | 32 +++++++++++++++++++++ test/test_mqtt/MQTT.cpp | 5 +--- 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index ae526a0f5f2..6fb8dd55587 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -203,6 +203,8 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta return handled; } #endif + } else if (authorizedLicensedSigner) { + // Router verified a plaintext licensed-mode signer against the admin allowlist above. } else if (strcasecmp(ch->settings.name, Channels::adminChannel) == 0) { if (!config.security.admin_channel_enabled) { LOG_INFO("Ignore admin channel, legacy admin disabled"); diff --git a/src/mqtt/MQTT.cpp b/src/mqtt/MQTT.cpp index 3ab9e92ca9c..eb598c42205 100644 --- a/src/mqtt/MQTT.cpp +++ b/src/mqtt/MQTT.cpp @@ -124,9 +124,7 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length) if (!pAck) return; pAck->transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_MQTT; - // sendLocal consumes packets sent to a live interface, but returns SHOULD_RELEASE - // when it handled a local delivery synchronously. Match MeshService::sendToMesh's - // ownership contract so the MQTT acknowledgement cannot leak on the local path. + // Release locally handled ACKs only when sendLocal requests it. if (router->sendLocal(pAck) == ERRNO_SHOULD_RELEASE) packetPool.release(pAck); } else { @@ -163,10 +161,7 @@ inline void onReceiveProto(char *topic, byte *payload, size_t length) if (shouldDropMqttDownlink(*p)) return; - // A decoded MQTT packet has already crossed the broker boundary, so authenticate it in place - // before handing it to the router. The routing-auth cache intentionally keeps an authenticated - // copy separate for encrypted packets, but a cache hit alone does not update this packet's - // xeddsa_signed marker (which is part of the downstream message metadata). + // Authenticate decoded MQTT packets in place so xeddsa_signed reaches consumers. bool decodedAuthChecked = false; if (p->which_payload_variant == meshtastic_MeshPacket_decoded_tag) { diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index 36efc9a41e3..e3a2d394fc3 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -17,6 +17,7 @@ #include "mesh/NodeDB.h" #include "mesh/mesh-pb-constants.h" #include "modules/AdminModule.h" +#include "modules/NodeInfoModule.h" #include "support/AdminModuleTestShim.h" #include "support/MockMeshService.h" #include @@ -89,6 +90,18 @@ static meshtastic_MeshPacket makeRemoteSetOwner(const char *newLongName, const u return mp; } +static meshtastic_MeshPacket makeLicensedRemoteSetOwner(const char *newLongName, const uint8_t *session, size_t sessionLen, + meshtastic_AdminMessage &out) +{ + auto mp = makeRemoteSetOwner(newLongName, session, sessionLen, out); + mp.to = LOCAL_NODE; + mp.pki_encrypted = false; + mp.xeddsa_signed = true; + mp.decoded.portnum = meshtastic_PortNum_ADMIN_APP; + out.set_owner.is_licensed = true; + 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. @@ -195,6 +208,24 @@ void test_remote_setter_without_session_is_rejected(void) TEST_ASSERT_EQUAL_STRING("Original", owner.long_name); } +void test_licensed_signed_setter_with_session_is_accepted(void) +{ + owner.is_licensed = true; + meshtastic_AdminMessage sessionResponse = meshtastic_AdminMessage_init_zero; + admin->setPassKey(&sessionResponse); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeLicensedRemoteSetOwner("LicensedAdmin", sessionResponse.session_passkey.bytes, + sessionResponse.session_passkey.size, m); + NodeInfoModule *savedNodeInfoModule = nodeInfoModule; + nodeInfoModule = reinterpret_cast(1); + admin->handleReceivedProtobuf(mp, &m); + nodeInfoModule = savedNodeInfoModule; + admin->drainReply(); + + TEST_ASSERT_EQUAL_STRING("LicensedAdmin", owner.long_name); +} + // The node's session key is minted only by setPassKey (which runs when it answers an admin GET), // so before any GET the expected key is all-zero and any presented key mismatches. void test_expected_session_key_is_zero_before_any_get(void) @@ -661,6 +692,7 @@ void setup() UNITY_BEGIN(); #if !(MESHTASTIC_EXCLUDE_PKI) RUN_TEST(test_remote_setter_without_session_is_rejected); + RUN_TEST(test_licensed_signed_setter_with_session_is_accepted); RUN_TEST(test_expected_session_key_is_zero_before_any_get); RUN_TEST(test_session_gate_accepts_key_from_a_get_response); RUN_TEST(test_remote_security_config_omits_private_key); diff --git a/test/test_mqtt/MQTT.cpp b/test/test_mqtt/MQTT.cpp index 4d2ea6c2687..54e7e33022c 100644 --- a/test/test_mqtt/MQTT.cpp +++ b/test/test_mqtt/MQTT.cpp @@ -60,10 +60,7 @@ class MockRouter : public Router class MockMeshService : public MeshService { public: - // No PhoneAPI reader exists in these tests, so packets the receive pipeline forwards to the phone - // (MeshService::sendToPhone enqueues pooled copies into toPhoneQueue) would leak at teardown. Drain - // the queue like the phone would. This surfaced once sendLocal() began dispatching local packets - // through handleReceived() directly rather than via the (mock-overridden) enqueueReceivedMessage(). + // These tests have no PhoneAPI reader, so drain queued packet copies before teardown. ~MockMeshService() { while (meshtastic_MeshPacket *p = getForPhone()) From eb16e085163648e307db28cba6f4153a242cbbef Mon Sep 17 00:00:00 2001 From: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:00:26 -0700 Subject: [PATCH 7/7] fix(security): propagate verified XEdDSA public key to packet for licensed admin --- src/mesh/Router.cpp | 11 ++-- src/modules/AdminModule.cpp | 2 +- test/test_admin_session_repro/test_main.cpp | 61 +++++++++++++++++++++ 3 files changed, 68 insertions(+), 6 deletions(-) diff --git a/src/mesh/Router.cpp b/src/mesh/Router.cpp index 96bef7bf2ed..70cd5c8e724 100644 --- a/src/mesh/Router.cpp +++ b/src/mesh/Router.cpp @@ -168,12 +168,9 @@ static bool routingAuthCacheMatches(const meshtastic_MeshPacket &packet) concurrency::LockGuard guard(routingAuthCacheLock); if (!routingAuthCache.valid) return false; - if (routingAuthCache.policy != config.security.packet_signature_policy || - memcmp(&routingAuthCache.wire, &packet, sizeof(packet)) != 0) { - routingAuthCache.valid = false; + if (routingAuthCache.policy != config.security.packet_signature_policy) return false; - } - return true; + return memcmp(&routingAuthCache.wire, &packet, sizeof(packet)) == 0; } static void storeRoutingAuthCache(const meshtastic_MeshPacket &wire, const meshtastic_MeshPacket &authenticated) @@ -722,6 +719,8 @@ static NodeInfoBootstrapResult verifyFirstContactNodeInfo(meshtastic_MeshPacket memcpy(node->public_key.bytes, user.public_key.bytes, user.public_key.size); nodeInfoLiteSetBit(node, NODEINFO_BITFIELD_HAS_XEDDSA_SIGNED_MASK, true); p->xeddsa_signed = true; + p->public_key.size = user.public_key.size; + memcpy(p->public_key.bytes, user.public_key.bytes, user.public_key.size); LOG_DEBUG("Verified first-contact XEdDSA NodeInfo from 0x%08x", p->from); return NodeInfoBootstrapResult::VERIFIED; } @@ -744,6 +743,8 @@ bool checkXeddsaReceivePolicy(meshtastic_MeshPacket *p) crypto->xeddsa_verify(senderKey.bytes, p->from, p->id, p->decoded.portnum, p->decoded.payload.bytes, p->decoded.payload.size, p->decoded.xeddsa_signature.bytes); if (p->xeddsa_signed) { + memcpy(p->public_key.bytes, senderKey.bytes, 32); + p->public_key.size = 32; // Learn this node as a signer, so a later unsigned signable broadcast from it is dropped // A warm-tier key must be re-admitted before setting the signer bit; otherwise Balanced // forgets downgrade protection as soon as the node is evicted from the hot store. diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 6fb8dd55587..7f920738344 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -150,7 +150,7 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta bool authorizedLicensedSigner = false; // Could tighten responses further by tracking the last public key queried. if (licensedRemote) { - const bool directedAdmin = mp.to == nodeDB->getNodeNum() && !isBroadcast(mp.to) && + const bool directedAdmin = nodeDB && mp.to == nodeDB->getNodeNum() && !isBroadcast(mp.to) && mp.decoded.portnum == meshtastic_PortNum_ADMIN_APP && !mp.pki_encrypted; if (!directedAdmin || !mp.xeddsa_signed || mp.public_key.size != 32) { LOG_INFO("Ignore licensed admin payload without a directed Router-verified signature"); diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index e3a2d394fc3..93b3a97d45b 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -226,6 +226,64 @@ void test_licensed_signed_setter_with_session_is_accepted(void) TEST_ASSERT_EQUAL_STRING("LicensedAdmin", owner.long_name); } +void test_licensed_signed_setter_unauthorized_signer_is_rejected(void) +{ + owner.is_licensed = true; + meshtastic_AdminMessage sessionResponse = meshtastic_AdminMessage_init_zero; + admin->setPassKey(&sessionResponse); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = + makeLicensedRemoteSetOwner("Attacker", sessionResponse.session_passkey.bytes, sessionResponse.session_passkey.size, m); + // Alter public key to stranger key not in config.security.admin_key + mp.public_key.bytes[0] ^= 0xFF; + + admin->handleReceivedProtobuf(mp, &m); + TEST_ASSERT_NOT_NULL(admin->reply()); + TEST_ASSERT_EQUAL(meshtastic_Routing_Error_ADMIN_PUBLIC_KEY_UNAUTHORIZED, admin->reply()->decoded.routing.error_reason); + admin->drainReply(); + + TEST_ASSERT_EQUAL_STRING("Original", owner.long_name); +} + +void test_licensed_signed_setter_broadcast_is_rejected(void) +{ + owner.is_licensed = true; + meshtastic_AdminMessage sessionResponse = meshtastic_AdminMessage_init_zero; + admin->setPassKey(&sessionResponse); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeLicensedRemoteSetOwner("BroadcastAttack", sessionResponse.session_passkey.bytes, + sessionResponse.session_passkey.size, m); + mp.to = NODENUM_BROADCAST; + + admin->handleReceivedProtobuf(mp, &m); + TEST_ASSERT_NOT_NULL(admin->reply()); + TEST_ASSERT_EQUAL(meshtastic_Routing_Error_NOT_AUTHORIZED, admin->reply()->decoded.routing.error_reason); + admin->drainReply(); + + TEST_ASSERT_EQUAL_STRING("Original", owner.long_name); +} + +void test_licensed_setter_unsigned_is_rejected(void) +{ + owner.is_licensed = true; + meshtastic_AdminMessage sessionResponse = meshtastic_AdminMessage_init_zero; + admin->setPassKey(&sessionResponse); + + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeLicensedRemoteSetOwner("UnsignedAttack", sessionResponse.session_passkey.bytes, + sessionResponse.session_passkey.size, m); + mp.xeddsa_signed = false; + + admin->handleReceivedProtobuf(mp, &m); + TEST_ASSERT_NOT_NULL(admin->reply()); + TEST_ASSERT_EQUAL(meshtastic_Routing_Error_NOT_AUTHORIZED, admin->reply()->decoded.routing.error_reason); + admin->drainReply(); + + TEST_ASSERT_EQUAL_STRING("Original", owner.long_name); +} + // The node's session key is minted only by setPassKey (which runs when it answers an admin GET), // so before any GET the expected key is all-zero and any presented key mismatches. void test_expected_session_key_is_zero_before_any_get(void) @@ -693,6 +751,9 @@ void setup() #if !(MESHTASTIC_EXCLUDE_PKI) RUN_TEST(test_remote_setter_without_session_is_rejected); RUN_TEST(test_licensed_signed_setter_with_session_is_accepted); + RUN_TEST(test_licensed_signed_setter_unauthorized_signer_is_rejected); + RUN_TEST(test_licensed_signed_setter_broadcast_is_rejected); + RUN_TEST(test_licensed_setter_unsigned_is_rejected); RUN_TEST(test_expected_session_key_is_zero_before_any_get); RUN_TEST(test_session_gate_accepts_key_from_a_get_response); RUN_TEST(test_remote_security_config_omits_private_key);