From 389559bddb104e3c9b8cf337dffb2c9d3ec2f71c Mon Sep 17 00:00:00 2001 From: Clive Blackledge Date: Thu, 20 Aug 2026 12:23:02 +0000 Subject: [PATCH] fix(NodeDB): re-derive my_node_num when ensurePkiKeys() mints the identity keypair (#11426) * fix(pki): re-derive NodeNum when setting a region mints the identity key A node's mesh address is derived from its identity key: my_node_num == crc32Buffer(config.security.public_key.bytes, 32) NodeDB::createNewIdentity() is what establishes that, and NodeDB:: generateCryptoKeyPair() is the only thing that called it. CryptoEngine::ensurePkiKeys() generates or re-derives the keypair and writes security.public_key, security.private_key and user.public_key - but never re-derives my_node_num. Boot-time keygen is suppressed while the LoRa region is UNSET (generateCryptoKeyPair()'s regionBlocksKeygen guard), so on a fresh device my_node_num is still the MAC-derived value from pickNewNodeNum(). The user then sets the region - the stock onboarding flow - ensurePkiKeys() mints a key, and the invariant is broken. The node then signs its broadcasts (Router.cpp signs when !pki_encrypted && (owner.is_licensed || isBroadcast(p->to))). Every receiver runs verifyFirstContactNodeInfo, fails crc32Buffer(user.public_key) != p->from, and drops the NodeInfo. The node's identity beacons are invisible to the mesh. Nothing reboots to repair it: AdminModule sets requiresReboot = false for LoRa changes ("All LoRa radio changes apply live via configChanged observer") and MenuHandler ends at service->reloadConfig(changes). Four call sites reached ensurePkiKeys(): 1. AdminModule set_config LORA, region first set (phone app - the common path) 2. MenuHandler applyLoraRegion (on-device region picker) 3. InkHUD MenuApplet applyLoRaRegion (schedules a reboot, so it self-healed at next boot) 4. portduino wasm wasm_set_region The reference implementation was already in the tree: the *licensed* branch of call site 1, thirteen lines below the broken unlicensed one, calls nodeDB->generateCryptoKeyPair() (which reaches createNewIdentity()) and widens the persisted mask with SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE. Rather than repeat that at four call sites, the key-mint is routed through one chokepoint that owns both halves of the identity: NodeDB::ensurePkiIdentity() calls crypto->ensurePkiKeys() and then createNewIdentity(). It lives in NodeDB because createNewIdentity() operates on the devicestate/node-DB globals, which CryptoEngine deliberately does not touch - ensurePkiKeys() takes the security config and user by reference precisely so it stays free of that dependency, and it is unit-tested against a standalone CryptoEngine. ensurePkiIdentity() returns true only when my_node_num actually moved (createNewIdentity() early-returns when the key is unchanged, so a repeat region change does not disturb the self entry or force a needless flash write). Callers use that to widen their save mask; my_node_num lives in devicestate and the self row moves in the node DB, so both segments must be persisted or the fix would revert at the next boot. SEGMENT_CONFIG, which carries the key itself, is already unconditional on all four paths. The InkHUD reboot is left as-is. It is now redundant for this invariant, but it covers the rest of that menu's behaviour and a redundant reboot is not a bug. Adds test_handleSetConfig_persistsUnlicensedFirstRegionIdentity, the unlicensed twin of the existing licensed test, asserting both the segment mask and my_node_num == crc32(public_key). Co-Authored-By: Claude Opus 5 * style(NodeDB): trim identity-recovery comments and guard the WASM nodeDB deref Two review asks, no behaviour change on any built target. Copilot flagged the unguarded nodeDB deref in the WASM region setter; it is the only ensurePkiIdentity() call site that did not check the pointer first. The rest is comment length. AGENTS.md:83 caps code comments at two lines, and the identity-recovery comments across the four call sites plus the NodeDB.h doc block ran to four and six lines. The rationale they carried is in the commit messages and the PR body, which is where AGENTS.md says it belongs. The PR's own fix in AdminModule.cpp is deliberately untouched. * fix(NodeDB): keep the identity move authoritative when the self record cannot be created createNewIdentity() removes the old node entry and assigns myNodeInfo.my_node_num before it tries to create the row for the new number. If getOrCreateMeshNode() came back null it returned false, so the first-region callers left SEGMENT_DEVICESTATE and SEGMENT_NODEDATABASE out of the save mask. The number had already moved in RAM at that point, and the freshly minted key goes to flash under SEGMENT_CONFIG regardless. The next boot therefore reloads the old number alongside the new key, which is exactly the crc32(public_key) != my_node_num break this path exists to prevent, reached through the error branch instead of the happy one. Rolling the number back is not an option either, since the key has already been replaced by the time this runs. So the move is now reported as the fact it is and the missing self record is logged separately; getOrCreateMeshNode() will recreate that row on the next contact. Reachable when the self record is absent and the table is full of protected nodes. Reported by CodeRabbit on #11426. --------- Co-authored-by: Claude Opus 5 --- src/graphics/draw/MenuHandler.cpp | 5 ++-- .../InkHUD/Applets/System/Menu/MenuApplet.cpp | 5 ++-- src/mesh/NodeDB.cpp | 24 ++++++++++++++-- src/mesh/NodeDB.h | 4 +++ src/modules/AdminModule.cpp | 6 ++-- .../portduino/wasm/portduino_glue_wasm.cpp | 13 +++++---- test/test_admin_radio/test_main.cpp | 28 +++++++++++++++++++ 7 files changed, 70 insertions(+), 15 deletions(-) diff --git a/src/graphics/draw/MenuHandler.cpp b/src/graphics/draw/MenuHandler.cpp index 6471d70c6..446f29a21 100644 --- a/src/graphics/draw/MenuHandler.cpp +++ b/src/graphics/draw/MenuHandler.cpp @@ -245,8 +245,9 @@ static void applyLoraRegion(meshtastic_Config_LoRaConfig_RegionCode region, bool } auto changes = SEGMENT_CONFIG; #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) - if (crypto) { - crypto->ensurePkiKeys(config.security, owner); + // Minting the key moves our node num with it, and nothing reboots on this path to repair it later. + if (nodeDB->ensurePkiIdentity()) { + changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE; } #endif initRegion(); diff --git a/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp b/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp index 5e8a08e75..863c1e85d 100644 --- a/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp +++ b/src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cpp @@ -324,8 +324,9 @@ static void applyLoRaRegion(meshtastic_Config_LoRaConfig_RegionCode region) auto changes = SEGMENT_CONFIG; #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) - if (crypto) { - crypto->ensurePkiKeys(config.security, owner); + // Minting the key moves our node num with it, and the reboot below only re-derives after the save. + if (nodeDB->ensurePkiIdentity()) { + changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE; } #endif diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index 040777857..e2557e8d0 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -4446,14 +4446,32 @@ bool NodeDB::createNewIdentity() myNodeInfo.my_node_num = newNodeNum; + // The number has moved, so the caller must persist it whatever happens next. Returning false here + // would leave the new key saved against the old number, which is the break this exists to prevent. meshtastic_NodeInfoLite *info = getOrCreateMeshNode(getNodeNum()); - if (!info) - return false; - TypeConversions::CopyUserToNodeInfoLite(info, owner); + if (info) + TypeConversions::CopyUserToNodeInfoLite(info, owner); + else + LOG_ERROR("No room for our own node 0x%08x, identity moved without a self record", newNodeNum); return true; } +bool NodeDB::ensurePkiIdentity() +{ +#if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) + // A failed or declined keygen leaves the existing key, and so the existing node num, untouched. + if (!crypto || !crypto->ensurePkiKeys(config.security, owner)) + return false; + + // ensurePkiKeys() writes key material only, so my_node_num is still the stale MAC-derived value. + // createNewIdentity() early-returns when the key, and so the node num, did not actually change. + return createNewIdentity(); +#else + return false; +#endif +} + bool NodeDB::backupPreferences(meshtastic_AdminMessage_BackupLocation location) { bool success = false; diff --git a/src/mesh/NodeDB.h b/src/mesh/NodeDB.h index ca0acf171..0e669cca5 100644 --- a/src/mesh/NodeDB.h +++ b/src/mesh/NodeDB.h @@ -596,6 +596,10 @@ class NodeDB bool createNewIdentity(); + /// Mint the identity keypair outside the boot path and re-seat my_node_num == crc32(public_key). + /// @return true if my_node_num moved; the caller must then also persist SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE. + bool ensurePkiIdentity(); + bool backupPreferences(meshtastic_AdminMessage_BackupLocation location); bool restorePreferences(meshtastic_AdminMessage_BackupLocation location, int restoreWhat = SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_CHANNELS); diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index 80bb79903..55b029f03 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -1034,8 +1034,10 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) // If we're setting region for the first time, init the region and regenerate the keys if (isRegionUnset && validatedLora.region > meshtastic_Config_LoRaConfig_RegionCode_UNSET) { #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) - if (crypto && !owner.is_licensed) { - crypto->ensurePkiKeys(config.security, owner); + // Minting the key moves our node num with it (my_node_num == crc32(public_key)), so + // persist devicestate + the node DB too - exactly as the licensed branch below does. + if (!owner.is_licensed && nodeDB->ensurePkiIdentity()) { + changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE; } #endif // new region is valid and we're coming from an unset region, so enable tx diff --git a/src/platform/portduino/wasm/portduino_glue_wasm.cpp b/src/platform/portduino/wasm/portduino_glue_wasm.cpp index a4b4a31e9..65d632084 100644 --- a/src/platform/portduino/wasm/portduino_glue_wasm.cpp +++ b/src/platform/portduino/wasm/portduino_glue_wasm.cpp @@ -12,10 +12,9 @@ // - exec() short-circuits to "" (no popen/shell in the browser). // Downstream is unchanged: Ch341Hal -> libpinedio_webusb.c -> WebUSB. -#include "CryptoEngine.h" // crypto->ensurePkiKeys() #include "MeshRadio.h" // initRegion() #include "MeshService.h" // service->reloadConfig() -#include "NodeDB.h" // config, owner globals + SEGMENT_CONFIG +#include "NodeDB.h" // config globals, SEGMENT_*, nodeDB->ensurePkiIdentity() #include "PhoneAPI.h" // the transport-agnostic client API seam #include "PortduinoFS.h" // portduinoVFS #include "PortduinoGlue.h" // declares `portduino_config` + Ch341Hal @@ -260,11 +259,13 @@ extern "C" EMSCRIPTEN_KEEPALIVE int wasm_set_region(int region) if (!(RadioInterface::validateConfigRegion(validated) && RadioInterface::validateConfigLora(validated))) return -1; + int changes = SEGMENT_CONFIG; bool wasUnset = (config.lora.region == meshtastic_Config_LoRaConfig_RegionCode_UNSET); if (wasUnset && newRegion > meshtastic_Config_LoRaConfig_RegionCode_UNSET) { #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) - if (crypto) - crypto->ensurePkiKeys(config.security, owner); // first real region -> generate keys + // Minting the key moves our node num with it, so persist devicestate + the node DB too. + if (nodeDB && nodeDB->ensurePkiIdentity()) + changes |= SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE; #endif validated.tx_enabled = true; } @@ -274,8 +275,8 @@ extern "C" EMSCRIPTEN_KEEPALIVE int wasm_set_region(int region) config.lora = validated; initRegion(); // repoint myRegion at the new region table if (service) - service->reloadConfig(SEGMENT_CONFIG); // reconfigure radio (new freq) + persist - wasm_fs_sync(); // browser: flush config.proto to IndexedDB + service->reloadConfig(changes); // reconfigure radio (new freq) + persist + wasm_fs_sync(); // browser: flush config.proto to IndexedDB return 0; } diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index ebdad8827..004037c9a 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/test/test_admin_radio/test_main.cpp @@ -23,6 +23,7 @@ #include "mesh/Channels.h" #include "modules/AdminModule.h" #include "modules/NodeInfoModule.h" +#include // crc32Buffer(), for the my_node_num == crc32(public_key) invariant #include #include #include @@ -1153,6 +1154,32 @@ static void test_handleSetConfig_persistsLicensedFirstRegionIdentity() TEST_ASSERT_EQUAL(32, owner.public_key.size); } +// Unlicensed twin of the test above. Without the re-derivation the node signs broadcasts every receiver +// drops (verifyFirstContactNodeInfo: crc32(user.public_key) != from). +static void test_handleSetConfig_persistsUnlicensedFirstRegionIdentity() +{ + owner = meshtastic_User_init_zero; + owner.is_licensed = false; + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.lora = meshtastic_Config_LoRaConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_UNSET; + initRegion(); + + testAdmin->deferSaves(); + const meshtastic_Config c = + makeLoraSetConfig(meshtastic_Config_LoRaConfig_RegionCode_US, true, meshtastic_Config_LoRaConfig_ModemPreset_LONG_FAST); + testAdmin->handleSetConfig(c, false); + + const int expectedSegments = SEGMENT_CONFIG | SEGMENT_MODULECONFIG | SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE; + TEST_ASSERT_EQUAL_INT(expectedSegments, testAdmin->savedSegments()); + TEST_ASSERT_EQUAL(32, config.security.private_key.size); + TEST_ASSERT_EQUAL(32, config.security.public_key.size); + TEST_ASSERT_EQUAL(32, owner.public_key.size); + // The invariant: a node's mesh address is derived from its identity key. + TEST_ASSERT_EQUAL_UINT32(crc32Buffer(config.security.public_key.bytes, config.security.public_key.size), + nodeDB->getNodeNum()); +} + static void test_handleSetConfig_fromOthers_invalidPresetRejected() { // Set up a known-good baseline in the global config @@ -1975,6 +2002,7 @@ void setup() // getRegion() RUN_TEST(test_handleSetOwner_persistsLicensedChannelSanitation); RUN_TEST(test_handleSetConfig_persistsLicensedFirstRegionIdentity); + RUN_TEST(test_handleSetConfig_persistsUnlicensedFirstRegionIdentity); RUN_TEST(test_bootDefense_sanitizesStaleLicensedChannelsOnce); RUN_TEST(test_restorePreferences_sanitizesLicensedBackupBeforeReturn); RUN_TEST(test_getRegion_returnsCorrectRegion_US);