diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 97da794ba4..95fe0e1369 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -136,7 +136,7 @@ Meshtastic packets on the air are typically encrypted one of two ways: the **per Implemented in `src/modules/AdminModule.cpp` → `handleReceivedProtobuf`. The authorization check runs in this order: 1. **Response messages** - if `messageIsResponse(r)` is true (the payload is a response to one of our earlier admin requests), it's accepted without any further check. The in-file comment flags this as a known-untightened gap: a stricter implementation would remember which `public_key` we last queried and reject responses that don't match. -2. **Local admin** - `mp.from == 0` (phone app over BLE, serial CLI, internal module); never travels over the air. **Rejected** if `config.security.is_managed` is true, because managed devices expect admin to arrive over the air through an authorized remote path. +2. **Local admin** - `mp.from == 0` (phone app over BLE, serial CLI, internal module); never travels over the air. RF, MQTT and UDP ingress drop `from == 0`, and the handler accepts it only while `MeshModule::callModules()` is dispatching that packet with `MeshModule::currentRequestSource` set to `RX_SRC_USER` or `RX_SRC_LOCAL`; anything else (radio source, or no dispatch context) is dropped before this ladder. **Rejected** if `config.security.is_managed` is true, because managed devices expect admin to arrive over the air through an authorized remote path. 3. **Legacy admin channel (deprecated)** - the packet arrived on a channel named literally `"admin"`. Gated by `config.security.admin_channel_enabled`; returns `NOT_AUTHORIZED` if the flag is false. Kept for backward compatibility; new deployments should use PKI admin. 4. **PKI admin (preferred for remote)** - `mp.pki_encrypted == true` AND `AdminKeys::isAuthorized(mp.public_key.bytes)`. That is `config.security.admin_key[0..2]` (up to three authorized 32-byte Curve25519 public keys, typically copied from the admin node's own `user.public_key`), plus, on meshtasticd, the keys `Security.AdminKeys` in `config.yaml` carried past those three. The same list backs `DMShell::isAuthorizedPacket`, the `is_managed` guard, and the admin-key trial decrypt in `Router::perhapsDecode` - add a key source there, not at a call site. On meshtasticd the file wins: `AdminKeys::applyHostKeys()` re-asserts it at boot and after every remote write of the security config. 5. **Fallthrough** → `NOT_AUTHORIZED`. diff --git a/src/mesh/MeshModule.cpp b/src/mesh/MeshModule.cpp index 0186c4d8e7..5cc1760af1 100644 --- a/src/mesh/MeshModule.cpp +++ b/src/mesh/MeshModule.cpp @@ -11,6 +11,7 @@ std::vector *MeshModule::modules; const meshtastic_MeshPacket *MeshModule::currentRequest; +RxSource MeshModule::currentRequestSource = RX_SRC_RADIO; uint8_t MeshModule::numPeriodicModules = 0; /** @@ -133,6 +134,7 @@ void MeshModule::callModules(meshtastic_MeshPacket &mp, RxSource src) auto &pi = **i; pi.currentRequest = ∓ + pi.currentRequestSource = src; pi.ignoreRequest = false; /// We only call modules that are interested in the packet (and the message is destined to us or we are promiscious) diff --git a/src/mesh/MeshModule.h b/src/mesh/MeshModule.h index 6a3c510670..fc337932db 100644 --- a/src/mesh/MeshModule.h +++ b/src/mesh/MeshModule.h @@ -138,6 +138,9 @@ class MeshModule */ static const meshtastic_MeshPacket *currentRequest; + /// Where currentRequest came from. Only meaningful while currentRequest points at the packet being handled. + static RxSource currentRequestSource; + // We keep track of the number of modules that send a periodic broadcast to schedule them spaced out over time static uint8_t numPeriodicModules; diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 3b5ab8d021..4a8249a2ce 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -135,6 +135,12 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta if (mp.which_payload_variant != meshtastic_MeshPacket_decoded_tag) { return handled; } + // from == 0 is trusted as a local client below, so require callModules() to vouch for its origin. + if (mp.from == 0 && + !(currentRequest == &mp && (currentRequestSource == RX_SRC_USER || currentRequestSource == RX_SRC_LOCAL))) { + LOG_WARN("Drop admin payload with from=0 that did not arrive from a local client"); + return handled; + } #ifdef ARCH_PORTDUINO // Simulator only: honor exit_simulator unconditionally for the local client (from==0). // The from==0 branch below now covers pki_encrypted local packets too, but is_managed @@ -161,8 +167,8 @@ bool AdminModule::handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshta LOG_TRACE("Allow admin response message"); } else if (mp.from == 0) { // Local admin from a BLE/USB/TCP client. from == 0 cannot arrive from the - // mesh: RF drops packets without a sender (RadioLibInterface) and MQTT treats - // from == 0 as our own downlink and ignores it. Clients may set pki_encrypted + // mesh: RF, MQTT and UDP ingress drop it, and the source gate above rejects + // any from == 0 not dispatched as RX_SRC_USER/LOCAL. Clients may set pki_encrypted // on self-addressed admin (the python CLI does), so don't use it to reroute // local packets into the remote-PKC key check. // diff --git a/test/support/AdminModuleTestShim.h b/test/support/AdminModuleTestShim.h index 15fe9e782e..81824331db 100644 --- a/test/support/AdminModuleTestShim.h +++ b/test/support/AdminModuleTestShim.h @@ -10,13 +10,35 @@ class AdminModuleTestShim : public AdminModule using AdminModule::checkPassKey; // session-key gate seam (see test_admin_session_repro) using AdminModule::handleGetConfig; using AdminModule::handleGetModuleConfig; - using AdminModule::handleReceivedProtobuf; using AdminModule::handleSetConfig; using AdminModule::handleSetModuleConfig; using AdminModule::handleSetOwner; using AdminModule::responseIsSolicited; // request/response pairing gate using AdminModule::setPassKey; + // Run the handler as MeshModule::callModules() would for a packet that arrived via `src`. + // AdminModule trusts from == 0 only when the dispatcher reports a local origin, so the default is + // RX_SRC_USER: the phone/serial client every from == 0 caller in these suites stands in for. + bool handleReceivedProtobuf(const meshtastic_MeshPacket &mp, meshtastic_AdminMessage *r, RxSource src = RX_SRC_USER) + { + return handleWithDispatchContext(&mp, src, mp, r); + } + + // Run the handler while callModules() is dispatching `dispatched` from `src`. Pass nullptr for a + // caller outside callModules(), or a different packet for a context that does not belong to `mp`. + bool handleWithDispatchContext(const meshtastic_MeshPacket *dispatched, RxSource src, const meshtastic_MeshPacket &mp, + meshtastic_AdminMessage *r) + { + const meshtastic_MeshPacket *savedRequest = currentRequest; + const RxSource savedSource = currentRequestSource; + currentRequest = dispatched; + currentRequestSource = src; + const bool handled = AdminModule::handleReceivedProtobuf(mp, r); + currentRequest = savedRequest; + currentRequestSource = savedSource; + return handled; + } + // Peek at the reply a handler queued, before drainReply() releases it. meshtastic_MeshPacket *reply() { return myReply; } diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index 36efc9a41e..873efbeae0 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -1,11 +1,18 @@ // 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, plus the request/response pairing that decides which -// admin responses are accepted at all. +// admin responses are accepted at all, plus the origin check on local (from == 0) admin. // // Drives the REAL incoming-admin path (AdminModule::handleReceivedProtobuf) with a remote // (from != 0) PKC-authorized set_owner, exercising the exact checkPassKey/setPassKey gate. // A local (from == 0) admin bypasses that gate, so the bug only reproduces from != 0. +// +// Because from == 0 skips the admin_key and session checks, AdminModule accepts it only when +// MeshModule::callModules() is dispatching that exact packet with currentRequestSource +// RX_SRC_USER or RX_SRC_LOCAL. Ingress (RadioLibInterface, MQTT, UDP) already drops from == 0; +// this gate keeps a forged one from becoming unauthenticated remote admin if a path ever misses +// that drop (the external from == 0 report). Relaxing it to a bare `mp.from == 0` check brings +// back that report's scenario: any setter, up to factory_reset, accepted without credentials. #include "MeshTypes.h" // include BEFORE TestUtil.h #include "TestUtil.h" @@ -652,6 +659,102 @@ void test_solicited_response_updates_pins(void) "a response we asked for must reach the handler"); } +// ---- from == 0 is trusted only when callModules() reports a local origin ---- + +// The packet shape from the external from == 0 report: addressed to us, no PKI, no session passkey, +// payload encoded so it can also go through MeshModule::callModules(). It carries a set_config +// (network.wifi_ssid) rather than the report's set_owner: the gate runs before the switch, so any +// setter proves it, and set_owner's reloadOwner() needs a NodeInfoModule this fixture lacks. +static meshtastic_MeshPacket makeFrom0SetSsid(const char *ssid, meshtastic_AdminMessage &out) +{ + out = meshtastic_AdminMessage_init_zero; + out.which_payload_variant = meshtastic_AdminMessage_set_config_tag; + out.set_config.which_payload_variant = meshtastic_Config_network_tag; + out.set_config.payload_variant.network = config.network; + strncpy(out.set_config.payload_variant.network.wifi_ssid, ssid, sizeof(out.set_config.payload_variant.network.wifi_ssid) - 1); + + meshtastic_MeshPacket mp = meshtastic_MeshPacket_init_zero; + mp.from = 0; + mp.to = LOCAL_NODE; + mp.id = 0x0F0F0001; + mp.channel = 0; + mp.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + mp.decoded.portnum = meshtastic_PortNum_ADMIN_APP; + mp.decoded.payload.size = + pb_encode_to_bytes(mp.decoded.payload.bytes, sizeof(mp.decoded.payload.bytes), &meshtastic_AdminMessage_msg, &out); + return mp; +} + +// Control: a phone/serial client (RX_SRC_USER) keeps full local admin. Without this, the +// rejections below could pass because the setter never applies in this fixture at all. +void test_from0_admin_from_phone_client_is_applied(void) +{ + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeFrom0SetSsid("Phone", m); + admin->handleReceivedProtobuf(mp, &m, RX_SRC_USER); + admin->drainReply(); + + TEST_ASSERT_EQUAL_STRING("Phone", config.network.wifi_ssid); +} + +// The reported attack: the same packet tagged as radio ingress must not take the local-client +// branch, which would skip both the admin_key check and the session passkey. +void test_from0_admin_from_radio_is_dropped(void) +{ + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeFrom0SetSsid("Hijacked", m); + admin->handleReceivedProtobuf(mp, &m, RX_SRC_RADIO); + + TEST_ASSERT_EQUAL_STRING("", config.network.wifi_ssid); + TEST_ASSERT_NULL_MESSAGE(admin->reply(), "a forged local packet is dropped silently, not answered"); +} + +// Fail closed: a caller that reaches the handler without going through callModules() has no +// source to vouch for it, so from == 0 alone must not be enough. +void test_from0_admin_without_dispatch_context_is_dropped(void) +{ + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeFrom0SetSsid("Hijacked", m); + admin->handleWithDispatchContext(nullptr, RX_SRC_USER, mp, &m); + admin->drainReply(); + + TEST_ASSERT_EQUAL_STRING("", config.network.wifi_ssid); +} + +// The source belongs to the packet callModules() is dispatching. A local source recorded for some +// other packet must not vouch for this one. +void test_from0_admin_with_another_packets_context_is_dropped(void) +{ + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeFrom0SetSsid("Hijacked", m); + meshtastic_MeshPacket other = mp; + admin->handleWithDispatchContext(&other, RX_SRC_USER, mp, &m); + admin->drainReply(); + + TEST_ASSERT_EQUAL_STRING("", config.network.wifi_ssid); +} + +// End to end through the real dispatcher, as the report's "callModules(mp, RX_SRC_RADIO)" variant: +// callModules() must hand AdminModule the source it was given. +void test_from0_admin_via_callModules_from_radio_is_dropped(void) +{ + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeFrom0SetSsid("Hijacked", m); + MeshModule::callModules(mp, RX_SRC_RADIO); + + TEST_ASSERT_EQUAL_STRING("", config.network.wifi_ssid); +} + +// Control for the above: the same dispatch from a phone client still applies the setter. +void test_from0_admin_via_callModules_from_phone_client_is_applied(void) +{ + meshtastic_AdminMessage m; + meshtastic_MeshPacket mp = makeFrom0SetSsid("Phone", m); + MeshModule::callModules(mp, RX_SRC_USER); + + TEST_ASSERT_EQUAL_STRING("Phone", config.network.wifi_ssid); +} + #endif // !(MESHTASTIC_EXCLUDE_PKI) void setup() @@ -685,6 +788,12 @@ void setup() 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); + RUN_TEST(test_from0_admin_from_phone_client_is_applied); + RUN_TEST(test_from0_admin_from_radio_is_dropped); + RUN_TEST(test_from0_admin_without_dispatch_context_is_dropped); + RUN_TEST(test_from0_admin_with_another_packets_context_is_dropped); + RUN_TEST(test_from0_admin_via_callModules_from_radio_is_dropped); + RUN_TEST(test_from0_admin_via_callModules_from_phone_client_is_applied); #endif exit(UNITY_END()); }