From bff0ba898bde787eca54ffa0e1c4f36aacb2ec24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 16 Jul 2026 22:23:59 +0200 Subject: [PATCH 1/3] PhoneAPI: gate local admin on the connection, not the wire from The lockdown admin check in handleToRadioPacket only ran when p.from == 0. from is a client-supplied wire field, and MeshService::handleToRadio rewrites it to 0 before AdminModule sees the packet. A client could therefore set from != 0 to skip the !getAdminAuthorized() drop, then have the packet normalized back to a local-admin identity and executed - unauthorized admin from an unauthorized connection. Every packet in handleToRadioPacket already comes from the local connection, so locality is a property of the connection, not of from. Move the decision into classifyLocalAdminPacket(), which ignores from and keys only on the admin variant and the connection's authorization: lockdown_auth is delivered inline, any other admin from an unauthorized connection is dropped, authorized admin passes through. The classifier is compiled unconditionally and unit-tested; the guarded caller (built only in the nRF52 lockdown config) calls it. Test: an unauthorized connection's ADMIN_APP packet with from != 0 is classified DropUnauthorized. --- src/mesh/PhoneAPI.cpp | 51 +++++++++++++++++++----------- src/mesh/PhoneAPI.h | 17 ++++++++++ test/test_stream_api/test_main.cpp | 46 +++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 18 deletions(-) diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index 4c9697114ab..53d53e4e388 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/src/mesh/PhoneAPI.cpp @@ -1697,6 +1697,19 @@ bool PhoneAPI::wasSeenRecently(uint32_t id) return false; } +PhoneAPI::LocalAdminGate PhoneAPI::classifyLocalAdminPacket(const meshtastic_MeshPacket &p, bool adminAuthorized, + meshtastic_AdminMessage &outAdmin) +{ + if (p.which_payload_variant != meshtastic_MeshPacket_decoded_tag || p.decoded.portnum != meshtastic_PortNum_ADMIN_APP) + return LocalAdminGate::NotAdmin; + outAdmin = meshtastic_AdminMessage_init_zero; + if (!pb_decode_from_bytes(p.decoded.payload.bytes, p.decoded.payload.size, &meshtastic_AdminMessage_msg, &outAdmin)) + return LocalAdminGate::NotAdmin; // undecodable: let the normal reject path respond + if (outAdmin.which_payload_variant == meshtastic_AdminMessage_lockdown_auth_tag) + return LocalAdminGate::LockdownAuth; // the passphrase itself; authenticate regardless of prior state + return adminAuthorized ? LocalAdminGate::AuthorizedPassThrough : LocalAdminGate::DropUnauthorized; +} + /** * Handle a packet that the phone wants us to send. It is our responsibility to free the packet to the pool */ @@ -1721,26 +1734,28 @@ bool PhoneAPI::handleToRadioPacket(meshtastic_MeshPacket &p) // the gate here closes that race and covers H6/H7 from the // audit: get_config_request and set_config from unauthed // clients no longer reach AdminModule at all. - if (p.from == 0 && p.which_payload_variant == meshtastic_MeshPacket_decoded_tag && - p.decoded.portnum == meshtastic_PortNum_ADMIN_APP) { + // Gate on the connection, not the wire `from`. `from` is attacker-controlled and MeshService + // rewrites it to 0 downstream, so keying this on `from == 0` let a client bypass the check by + // sending `from != 0` and have AdminModule treat it as authorized local admin. + { meshtastic_AdminMessage admin = meshtastic_AdminMessage_init_zero; - if (pb_decode_from_bytes(p.decoded.payload.bytes, p.decoded.payload.size, &meshtastic_AdminMessage_msg, &admin)) { - if (admin.which_payload_variant == meshtastic_AdminMessage_lockdown_auth_tag) { - handleLockdownAuthInline(admin.lockdown_auth); - // Wipe the decoded passphrase scratch - the byte array in - // p.decoded.payload.bytes is wiped by handleLockdownAuthInline. - volatile uint8_t *adminVol = const_cast(admin.lockdown_auth.passphrase.bytes); - for (size_t i = 0; i < sizeof(admin.lockdown_auth.passphrase.bytes); i++) - adminVol[i] = 0; - return true; - } - if (!getAdminAuthorized()) { - LOG_WARN("Lockdown: dropping admin payload variant=%d from unauthorized connection", admin.which_payload_variant); - return false; - } + switch (classifyLocalAdminPacket(p, getAdminAuthorized(), admin)) { + case LocalAdminGate::LockdownAuth: { + handleLockdownAuthInline(admin.lockdown_auth); + // Wipe the decoded passphrase scratch - the byte array in + // p.decoded.payload.bytes is wiped by handleLockdownAuthInline. + volatile uint8_t *adminVol = const_cast(admin.lockdown_auth.passphrase.bytes); + for (size_t i = 0; i < sizeof(admin.lockdown_auth.passphrase.bytes); i++) + adminVol[i] = 0; + return true; + } + case LocalAdminGate::DropUnauthorized: + LOG_WARN("Lockdown: dropping admin payload variant=%d from unauthorized connection", admin.which_payload_variant); + return false; + case LocalAdminGate::NotAdmin: + case LocalAdminGate::AuthorizedPassThrough: + break; // normal handling } - // pb_decode failure: fall through to normal handling so the - // regular Router/AdminModule reject path can respond. } #endif diff --git a/src/mesh/PhoneAPI.h b/src/mesh/PhoneAPI.h index ba7b1356777..8dcfdd7501d 100644 --- a/src/mesh/PhoneAPI.h +++ b/src/mesh/PhoneAPI.h @@ -319,4 +319,21 @@ class PhoneAPI /// If the mesh service tells us fromNum has changed, tell the phone virtual int onNotify(uint32_t newValue) override; + + public: + /// How the lockdown admin gate should treat a phone->radio packet. + enum class LocalAdminGate { + NotAdmin, ///< Not a decodable ADMIN_APP payload; normal handling. + LockdownAuth, ///< A lockdown_auth payload; authenticate the connection inline. + DropUnauthorized, ///< Admin payload from a connection that has not authenticated; drop. + AuthorizedPassThrough, ///< Admin payload from an authorized connection; normal handling. + }; + + /// Classify a phone->radio packet for the lockdown admin gate. Deliberately independent of the + /// wire `from` field: every packet here originates from the local connection and the phone does + /// not own `from` (MeshService rewrites it), so keying the gate on `from` lets a client bypass + /// it by setting `from != 0`. Static and free of connection state so it is unit-testable; the + /// caller supplies the connection's authorization and consumes `outAdmin` for the lockdown case. + static LocalAdminGate classifyLocalAdminPacket(const meshtastic_MeshPacket &p, bool adminAuthorized, + meshtastic_AdminMessage &outAdmin); }; diff --git a/test/test_stream_api/test_main.cpp b/test/test_stream_api/test_main.cpp index 84613cf328a..fe82b38afb5 100644 --- a/test/test_stream_api/test_main.cpp +++ b/test/test_stream_api/test_main.cpp @@ -408,6 +408,51 @@ void test_serial_console_suppresses_raw_output_in_protobuf_mode() TEST_ASSERT_TRUE(emptyAfterProtobuf); } +// Build a phone->radio ADMIN_APP packet carrying `admin`, with an arbitrary wire `from`. +static meshtastic_MeshPacket makeAdminPacket(NodeNum from, const meshtastic_AdminMessage &admin) +{ + meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; + p.from = from; + 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, &admin); + return p; +} + +// The lockdown admin gate must decide on the connection's authorization, not the wire `from`. A +// client that sets from != 0 previously skipped the gate, so an unauthorized connection could run +// admin. classifyLocalAdminPacket ignores `from`, so the same spoofed packet is still dropped. +static void test_lockdown_admin_gate_ignores_wire_from(void) +{ + meshtastic_AdminMessage setter = meshtastic_AdminMessage_init_zero; + setter.which_payload_variant = meshtastic_AdminMessage_set_owner_tag; + meshtastic_MeshPacket spoofed = makeAdminPacket(0x12345678, setter); // from != 0, the bypass + + meshtastic_AdminMessage out; + TEST_ASSERT_EQUAL_MESSAGE((int)PhoneAPI::LocalAdminGate::DropUnauthorized, + (int)PhoneAPI::classifyLocalAdminPacket(spoofed, /*adminAuthorized=*/false, out), + "unauthorized admin with from != 0 must still be dropped"); + // Control: an authorized connection's identical packet passes through. + TEST_ASSERT_EQUAL_MESSAGE((int)PhoneAPI::LocalAdminGate::AuthorizedPassThrough, + (int)PhoneAPI::classifyLocalAdminPacket(spoofed, /*adminAuthorized=*/true, out), + "authorized admin must not be dropped"); + + // lockdown_auth is the authentication itself, so it is delivered inline regardless of from/auth. + meshtastic_AdminMessage la = meshtastic_AdminMessage_init_zero; + la.which_payload_variant = meshtastic_AdminMessage_lockdown_auth_tag; + meshtastic_MeshPacket authPkt = makeAdminPacket(0x99, la); + TEST_ASSERT_EQUAL((int)PhoneAPI::LocalAdminGate::LockdownAuth, + (int)PhoneAPI::classifyLocalAdminPacket(authPkt, /*adminAuthorized=*/false, out)); + + // A non-admin packet is outside the gate entirely. + meshtastic_MeshPacket text = meshtastic_MeshPacket_init_zero; + text.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + text.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP; + TEST_ASSERT_EQUAL((int)PhoneAPI::LocalAdminGate::NotAdmin, + (int)PhoneAPI::classifyLocalAdminPacket(text, /*adminAuthorized=*/false, out)); +} + /// Unity per-test setup; fixtures are local to each test. void setUp(void) {} /// Unity per-test teardown; fixtures clean themselves up. @@ -427,6 +472,7 @@ void setup() RUN_TEST(test_stream_api_short_write_reports_failure_without_flush); RUN_TEST(test_stream_api_finishes_pending_before_advancing_phone_api); RUN_TEST(test_stream_api_gates_logs_and_marks_them_best_effort); + RUN_TEST(test_lockdown_admin_gate_ignores_wire_from); // usingProtobufs intentionally has no reset path, so this must run last. RUN_TEST(test_serial_console_suppresses_raw_output_in_protobuf_mode); exit(UNITY_END()); From 6bcc69945caeadf728cc284ee9bfc60385d0dbd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 16 Jul 2026 22:52:33 +0200 Subject: [PATCH 2/3] PhoneAPI: wipe the encoded lockdown passphrase, shorten comments --- src/mesh/PhoneAPI.cpp | 12 +++++++----- src/mesh/PhoneAPI.h | 7 ++----- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index 53d53e4e388..b5bbaa88453 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/src/mesh/PhoneAPI.cpp @@ -1734,19 +1734,21 @@ bool PhoneAPI::handleToRadioPacket(meshtastic_MeshPacket &p) // the gate here closes that race and covers H6/H7 from the // audit: get_config_request and set_config from unauthed // clients no longer reach AdminModule at all. - // Gate on the connection, not the wire `from`. `from` is attacker-controlled and MeshService - // rewrites it to 0 downstream, so keying this on `from == 0` let a client bypass the check by - // sending `from != 0` and have AdminModule treat it as authorized local admin. + // Gate on the connection, not the wire `from`, which a client can forge to a non-zero value to + // bypass the check before MeshService normalizes it back to a local identity. { meshtastic_AdminMessage admin = meshtastic_AdminMessage_init_zero; switch (classifyLocalAdminPacket(p, getAdminAuthorized(), admin)) { case LocalAdminGate::LockdownAuth: { handleLockdownAuthInline(admin.lockdown_auth); - // Wipe the decoded passphrase scratch - the byte array in - // p.decoded.payload.bytes is wiped by handleLockdownAuthInline. + // Wipe both copies of the passphrase: the decoded scratch and the still-encoded bytes in + // the packet buffer (handleLockdownAuthInline only clears the decoded copy). volatile uint8_t *adminVol = const_cast(admin.lockdown_auth.passphrase.bytes); for (size_t i = 0; i < sizeof(admin.lockdown_auth.passphrase.bytes); i++) adminVol[i] = 0; + volatile uint8_t *encodedVol = const_cast(p.decoded.payload.bytes); + for (size_t i = 0; i < sizeof(p.decoded.payload.bytes); i++) + encodedVol[i] = 0; return true; } case LocalAdminGate::DropUnauthorized: diff --git a/src/mesh/PhoneAPI.h b/src/mesh/PhoneAPI.h index 8dcfdd7501d..ab04c178b0b 100644 --- a/src/mesh/PhoneAPI.h +++ b/src/mesh/PhoneAPI.h @@ -329,11 +329,8 @@ class PhoneAPI AuthorizedPassThrough, ///< Admin payload from an authorized connection; normal handling. }; - /// Classify a phone->radio packet for the lockdown admin gate. Deliberately independent of the - /// wire `from` field: every packet here originates from the local connection and the phone does - /// not own `from` (MeshService rewrites it), so keying the gate on `from` lets a client bypass - /// it by setting `from != 0`. Static and free of connection state so it is unit-testable; the - /// caller supplies the connection's authorization and consumes `outAdmin` for the lockdown case. + /// Classify a phone->radio packet for the lockdown admin gate, ignoring the wire `from` (which a + /// client can forge) and deciding on the connection's authorization. Fills outAdmin for lockdown. static LocalAdminGate classifyLocalAdminPacket(const meshtastic_MeshPacket &p, bool adminAuthorized, meshtastic_AdminMessage &outAdmin); }; From 25534efa2ffee722b3b3e4048ab66db9d644038b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Fri, 17 Jul 2026 11:42:58 +0200 Subject: [PATCH 3/3] PhoneAPI: clarify lockdown wipe intent, reset payload size, test undecodable admin Note that the encoded-payload wipe is the security-critical one and the decoded-scratch wipe is defensive; document the block that scopes the decoded passphrase; reset payload.size after wiping the buffer. Add a test that an undecodable ADMIN_APP payload classifies as NotAdmin even when authorized. --- src/mesh/PhoneAPI.cpp | 7 +++++-- test/test_stream_api/test_main.cpp | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index b5bbaa88453..94020e6e3e3 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/src/mesh/PhoneAPI.cpp @@ -1736,19 +1736,22 @@ bool PhoneAPI::handleToRadioPacket(meshtastic_MeshPacket &p) // clients no longer reach AdminModule at all. // Gate on the connection, not the wire `from`, which a client can forge to a non-zero value to // bypass the check before MeshService normalizes it back to a local identity. + // Scope the decoded admin message: it holds the plaintext passphrase, so bound its lifetime. { meshtastic_AdminMessage admin = meshtastic_AdminMessage_init_zero; switch (classifyLocalAdminPacket(p, getAdminAuthorized(), admin)) { case LocalAdminGate::LockdownAuth: { handleLockdownAuthInline(admin.lockdown_auth); - // Wipe both copies of the passphrase: the decoded scratch and the still-encoded bytes in - // the packet buffer (handleLockdownAuthInline only clears the decoded copy). + // The encoded wipe is the security-critical one: nothing else clears the passphrase from + // the packet buffer. handleLockdownAuthInline already zeroes the decoded copy up to its + // size; re-zero the full scratch capacity here as defense in depth. volatile uint8_t *adminVol = const_cast(admin.lockdown_auth.passphrase.bytes); for (size_t i = 0; i < sizeof(admin.lockdown_auth.passphrase.bytes); i++) adminVol[i] = 0; volatile uint8_t *encodedVol = const_cast(p.decoded.payload.bytes); for (size_t i = 0; i < sizeof(p.decoded.payload.bytes); i++) encodedVol[i] = 0; + p.decoded.payload.size = 0; // keep the length consistent with the wiped buffer return true; } case LocalAdminGate::DropUnauthorized: diff --git a/test/test_stream_api/test_main.cpp b/test/test_stream_api/test_main.cpp index fe82b38afb5..5a657d44168 100644 --- a/test/test_stream_api/test_main.cpp +++ b/test/test_stream_api/test_main.cpp @@ -453,6 +453,28 @@ static void test_lockdown_admin_gate_ignores_wire_from(void) (int)PhoneAPI::classifyLocalAdminPacket(text, /*adminAuthorized=*/false, out)); } +// An ADMIN_APP packet whose payload is not a decodable AdminMessage must fall through to the +// normal reject path (NotAdmin), never be acted on as an admin command. The authorized control +// proves the decode-failure check runs before the auth branch, so it can't pass for the wrong reason. +static void test_lockdown_admin_gate_rejects_undecodable_admin(void) +{ + meshtastic_MeshPacket p = meshtastic_MeshPacket_init_zero; + p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + p.decoded.portnum = meshtastic_PortNum_ADMIN_APP; + // Length-delimited field (tag 0x0A) claiming 16 bytes with none following: pb_decode fails. + p.decoded.payload.bytes[0] = 0x0A; + p.decoded.payload.bytes[1] = 0x10; + p.decoded.payload.size = 2; + + meshtastic_AdminMessage out; + TEST_ASSERT_EQUAL_MESSAGE((int)PhoneAPI::LocalAdminGate::NotAdmin, + (int)PhoneAPI::classifyLocalAdminPacket(p, /*adminAuthorized=*/false, out), + "undecodable ADMIN_APP payload must fall through to the reject path"); + TEST_ASSERT_EQUAL_MESSAGE((int)PhoneAPI::LocalAdminGate::NotAdmin, + (int)PhoneAPI::classifyLocalAdminPacket(p, /*adminAuthorized=*/true, out), + "undecodable ADMIN_APP payload must not pass through even when authorized"); +} + /// Unity per-test setup; fixtures are local to each test. void setUp(void) {} /// Unity per-test teardown; fixtures clean themselves up. @@ -473,6 +495,7 @@ void setup() RUN_TEST(test_stream_api_finishes_pending_before_advancing_phone_api); RUN_TEST(test_stream_api_gates_logs_and_marks_them_best_effort); RUN_TEST(test_lockdown_admin_gate_ignores_wire_from); + RUN_TEST(test_lockdown_admin_gate_rejects_undecodable_admin); // usingProtobufs intentionally has no reset path, so this must run last. RUN_TEST(test_serial_console_suppresses_raw_output_in_protobuf_mode); exit(UNITY_END());