PhoneAPI: gate local admin on the connection, not the wire from (#11033)

* 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.

* PhoneAPI: wipe the encoded lockdown passphrase, shorten comments

---------

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

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,33 @@ 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`, 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;
-        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<volatile uint8_t *>(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);
+            // 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<volatile uint8_t *>(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<volatile uint8_t *>(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:
+            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
 

src/mesh/PhoneAPI.h

@@ -319,4 +319,18 @@ 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, 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);
 };

test/test_stream_api/test_main.cpp

@@ -408,6 +408,73 @@ 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));
+}
+
+// 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.
@@ -427,6 +494,8 @@ 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);
+    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());