From 57a61c785349641a8f531fe87e2183e5af6e2505 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20G=C3=B6ttgens?= Date: Thu, 16 Jul 2026 21:24:26 +0200 Subject: [PATCH] AdminModule: don't return the device private key to remote config reads A SECURITY_CONFIG get_config response copied config.security verbatim, so a remote request was answered with the device identity private_key and sent over the air. Only the local owner needs it, for backup. Zero private_key in the SECURITY_CONFIG response when the request is remote (from != 0); the local BLE/USB/TCP path (from == 0) still receives it. public_key and admin_key are public and stay as they were. --- src/modules/AdminModule.cpp | 9 +++ test/support/AdminModuleTestShim.h | 4 ++ test/test_admin_session_repro/test_main.cpp | 63 ++++++++++++++++++++- 3 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 278fe6179af..32f06f3cb86 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -1408,6 +1408,15 @@ void AdminModule::handleGetConfig(const meshtastic_MeshPacket &req, const uint32 LOG_INFO("Get config: Security"); res.get_config_response.which_payload_variant = meshtastic_Config_security_tag; res.get_config_response.payload_variant.security = config.security; + // The device identity private key is backup material for the local owner only. A local + // admin client sets from == 0 (BLE/USB/TCP); never return the key to a remote requester, + // even an authorized one, since it would travel over the air. public_key/admin_key are + // public and stay put. + if (req.from != 0) { + auto &sec = res.get_config_response.payload_variant.security; + memset(sec.private_key.bytes, 0, sizeof(sec.private_key.bytes)); + sec.private_key.size = 0; + } break; case meshtastic_AdminMessage_ConfigType_SESSIONKEY_CONFIG: LOG_INFO("Get config: Sessionkey"); diff --git a/test/support/AdminModuleTestShim.h b/test/support/AdminModuleTestShim.h index d3670b09a29..6d9c3f1a8ec 100644 --- a/test/support/AdminModuleTestShim.h +++ b/test/support/AdminModuleTestShim.h @@ -8,11 +8,15 @@ class AdminModuleTestShim : public AdminModule { public: using AdminModule::checkPassKey; // session-key gate seam (see test_admin_session_repro) + using AdminModule::handleGetConfig; using AdminModule::handleReceivedProtobuf; using AdminModule::handleSetConfig; using AdminModule::handleSetModuleConfig; using AdminModule::setPassKey; + // Peek at the reply a handler queued, before drainReply() releases it. + meshtastic_MeshPacket *reply() { return myReply; } + // With an "open edit transaction" saveChanges() is a pure no-op: no reloadConfig/saveToDisk/reboot. void deferSaves() { hasOpenEditTransaction = true; } diff --git a/test/test_admin_session_repro/test_main.cpp b/test/test_admin_session_repro/test_main.cpp index f1bb1711e6b..6502ab3c4b4 100644 --- a/test/test_admin_session_repro/test_main.cpp +++ b/test/test_admin_session_repro/test_main.cpp @@ -1,5 +1,6 @@ // Deterministic reproduction of the admin session-key behavior discussed on PR #10669 -// (ndoo's "Admin message without session_key!" report). +// (ndoo's "Admin message without session_key!" report), plus the remote-vs-local redaction of +// secret material in admin GET responses. // // Drives the REAL incoming-admin path (AdminModule::handleReceivedProtobuf) with a remote // (from != 0) PKC-authorized set_owner, exercising the exact checkPassKey/setPassKey gate. @@ -13,6 +14,7 @@ #include "mesh/Channels.h" #include "mesh/NodeDB.h" +#include "mesh/mesh-pb-constants.h" #include "modules/AdminModule.h" #include "support/AdminModuleTestShim.h" #include "support/MockMeshService.h" @@ -135,6 +137,63 @@ void test_session_gate_accepts_key_from_a_get_response(void) TEST_ASSERT_FALSE(admin->checkPassKey(&bad)); } +// Decode the SecurityConfig out of the get_config response a handler queued in myReply. +static bool decodeSecurityFromReply(meshtastic_MeshPacket *reply, meshtastic_Config_SecurityConfig &out) +{ + meshtastic_AdminMessage am = meshtastic_AdminMessage_init_zero; + if (!reply || reply->which_payload_variant != meshtastic_MeshPacket_decoded_tag) + return false; + if (!pb_decode_from_bytes(reply->decoded.payload.bytes, reply->decoded.payload.size, &meshtastic_AdminMessage_msg, &am)) + return false; + if (am.which_payload_variant != meshtastic_AdminMessage_get_config_response_tag || + am.get_config_response.which_payload_variant != meshtastic_Config_security_tag) + return false; + out = am.get_config_response.payload_variant.security; + return true; +} + +static meshtastic_MeshPacket makeGetConfigRequest(NodeNum from) +{ + meshtastic_MeshPacket req = meshtastic_MeshPacket_init_zero; + req.from = from; + req.which_payload_variant = meshtastic_MeshPacket_decoded_tag; + req.decoded.want_response = true; + return req; +} + +// The device identity private key must never leave over the air: a SECURITY_CONFIG response to a +// remote request (from != 0, even an authorized admin) carries an empty private_key. +void test_remote_security_config_omits_private_key(void) +{ + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0xAB, 32); + + meshtastic_MeshPacket req = makeGetConfigRequest(ADMIN_NODE); + admin->handleGetConfig(req, meshtastic_AdminMessage_ConfigType_SECURITY_CONFIG); + + meshtastic_Config_SecurityConfig sec; + TEST_ASSERT_TRUE(decodeSecurityFromReply(admin->reply(), sec)); + TEST_ASSERT_EQUAL_MESSAGE(0, sec.private_key.size, "remote security config must not carry the private key"); + admin->drainReply(); +} + +// Control: the local backup path (from == 0, BLE/USB/TCP) still receives the private key, so the +// redaction above is remote-specific rather than a blanket wipe. +void test_local_security_config_keeps_private_key(void) +{ + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0xAB, 32); + + meshtastic_MeshPacket req = makeGetConfigRequest(0); + admin->handleGetConfig(req, meshtastic_AdminMessage_ConfigType_SECURITY_CONFIG); + + meshtastic_Config_SecurityConfig sec; + TEST_ASSERT_TRUE(decodeSecurityFromReply(admin->reply(), sec)); + TEST_ASSERT_EQUAL_MESSAGE(32, sec.private_key.size, "local backup must still receive the private key"); + TEST_ASSERT_EACH_EQUAL_HEX8(0xAB, sec.private_key.bytes, 32); + admin->drainReply(); +} + #endif // !(MESHTASTIC_EXCLUDE_PKI) void setup() @@ -146,6 +205,8 @@ void setup() RUN_TEST(test_remote_setter_without_session_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); + RUN_TEST(test_local_security_config_keeps_private_key); #endif exit(UNITY_END()); }