diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index 3a01aa357..89329abfe 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -480,8 +480,10 @@ NodeDB::NodeDB() #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) // Generate crypto keys if needed using consolidated function // Set my node num uint32 value to bytes from the public key (if we have one) - // Generate identity and crypto keys if needed; this will create a new identity if one does not exist - generateCryptoKeyPair(nullptr); + // Generate identity and crypto keys if needed; this will create a new identity if one does not exist. + // Skip on a degraded boot: the keypair isn't in RAM, so minting one would change our NodeNum. + if (!configDecodeFailed) + generateCryptoKeyPair(nullptr); #elif !(MESHTASTIC_EXCLUDE_PKI) // Calculate Curve25519 public and private keys if (config.security.private_key.size == 32 && config.security.public_key.size == 32) { @@ -625,7 +627,9 @@ NodeDB::NodeDB() saveWhat |= SEGMENT_DEVICESTATE; if (nodeDatabaseCRC != crc32Buffer(&nodeDatabase, sizeof(nodeDatabase))) saveWhat |= SEGMENT_NODEDATABASE; - if (configCRC != crc32Buffer(&config, sizeof(config))) + // Don't persist on a degraded boot: it would overwrite the unreadable-but-maybe-transient config file + // with no-key UNSET defaults. Runtime reconfiguration (admin set) still persists normally. + if (!configDecodeFailed && configCRC != crc32Buffer(&config, sizeof(config))) saveWhat |= SEGMENT_CONFIG; if (channelFileCRC != crc32Buffer(&channelFile, sizeof(channelFile))) saveWhat |= SEGMENT_CHANNELS; @@ -2120,6 +2124,7 @@ void NodeDB::loadFromDisk() #endif migrationSavePending = false; + configDecodeFailed = false; meshtastic_Config_SecurityConfig backupSecurity = meshtastic_Config_SecurityConfig_init_zero; @@ -2309,15 +2314,26 @@ void NodeDB::loadFromDisk() state = loadProto(configFileName, meshtastic_LocalConfig_size, sizeof(meshtastic_LocalConfig), &meshtastic_LocalConfig_msg, &config); - if (state != LoadFileResult::LOAD_SUCCESS) { - installDefaultConfig(); // Our in RAM copy might now be corrupt + if (state == LoadFileResult::DECODE_FAILED) { + // Config file present but undecodable this boot (corruption / torn write / transient decrypt fail). + // loadProto() already zeroed `config`, so the keypair is gone from RAM; minting a new one would change + // our NodeNum (== crc32(public_key)) and orphan us on the mesh. configDecodeFailed freezes identity and + // skips persisting (see ctor), so a transient failure self-heals on the next clean boot. A genuinely + // absent config returns OTHER_FAILURE, so this never fires on first boot. Boot degraded + radio-silent. + LOG_ERROR("Config decode failed - freezing identity, booting degraded (radio silent until restored)"); + configDecodeFailed = true; + installDefaultConfig(true); + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_UNSET; + config.lora.tx_enabled = false; + } else if (state != LoadFileResult::LOAD_SUCCESS) { + // No decodable config to work with: the file is absent (first boot) or could not be opened (OTHER_FAILURE + // / NO_FILESYSTEM). Unlike DECODE_FAILED there are no usable contents to protect, so install defaults. + installDefaultConfig(); + } else if (config.version < DEVICESTATE_MIN_VER) { + LOG_WARN("config %d is old, discard", config.version); + installDefaultConfig(true); } else { - if (config.version < DEVICESTATE_MIN_VER) { - LOG_WARN("config %d is old, discard", config.version); - installDefaultConfig(true); - } else { - LOG_INFO("Loaded saved config version %d", config.version); - } + LOG_INFO("Loaded saved config version %d", config.version); } // Coerce LoRa config fields derived from presets while bootstrapping. @@ -2334,7 +2350,9 @@ void NodeDB::loadFromDisk() // take effect even when NVS already has a valid config (e.g. region-locked // dev boards with no BLE/serial to set the region at runtime). #ifdef USERPREFS_CONFIG_LORA_REGION - config.lora.region = USERPREFS_CONFIG_LORA_REGION; + // Skip on a degraded boot to keep the radio silent (identity is already protected by the keygen gate). + if (!configDecodeFailed) + config.lora.region = USERPREFS_CONFIG_LORA_REGION; #endif #ifdef USERPREFS_LORACONFIG_USE_PRESET diff --git a/src/mesh/NodeDB.h b/src/mesh/NodeDB.h index b7f3d7f9e..4d1658dba 100644 --- a/src/mesh/NodeDB.h +++ b/src/mesh/NodeDB.h @@ -506,6 +506,10 @@ class NodeDB bool duplicateWarned = false; bool localPositionUpdatedSinceBoot = false; bool migrationSavePending = false; + /// Set when loadFromDisk() hit a present-but-undecodable config (DECODE_FAILED). The ctor uses it to + /// skip boot keygen and skip persisting defaults, so a transient read failure can't change our NodeNum + /// or overwrite the on-disk config. Cleared at the top of every loadFromDisk() run. + bool configDecodeFailed = false; uint32_t lastNodeDbSave = 0; // when we last saved our db to flash uint32_t lastBackupAttempt = 0; // when we last tried a backup automatically or manually uint32_t lastSort = 0; // When last sorted the nodeDB diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index e2a662db0..86477a9bc 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -1037,16 +1037,24 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) config.has_bluetooth = true; config.bluetooth = c.payload_variant.bluetooth; break; - case meshtastic_Config_security_tag: + case meshtastic_Config_security_tag: { LOG_INFO("Set config: Security"); - config.security = c.payload_variant.security; + meshtastic_Config_SecurityConfig incoming = c.payload_variant.security; + // Preserve our keypair when a SET omits the private key but we already hold one: regenerating would + // change our NodeNum (== crc32(public_key)) and orphan us on the mesh. A SET without the key is a + // partial/legacy client, not an identity reset (that goes through factory_reset). Done outside the + // PKI guard so non-PKI builds keep their key bytes too. + if (incoming.private_key.size != 32 && config.security.private_key.size == 32) { + LOG_WARN("Security set omitted private key; preserving existing identity keypair"); + incoming.private_key = config.security.private_key; + incoming.public_key = config.security.public_key; + } + config.security = incoming; #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN) && !(MESHTASTIC_EXCLUDE_PKI) - // Only regenerate keys if the private key is not 32 bytes + // First provisioning (no key) generates one; a private key supplied without its public key derives it. if (config.security.private_key.size != 32) { nodeDB->generateCryptoKeyPair(); - } - // If user provided a private key of correct size but no public key, generate the public key from private key - else if (config.security.private_key.size == 32 && config.security.public_key.size == 0) { + } else if (config.security.public_key.size == 0) { nodeDB->generateCryptoKeyPair(config.security.private_key.bytes); } #endif @@ -1063,6 +1071,7 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) requiresReboot = true; break; + } case meshtastic_Config_device_ui_tag: // NOOP! This is handled by handleStoreDeviceUIConfig break; diff --git a/src/modules/AdminModule.h b/src/modules/AdminModule.h index d072ab2fe..f123a137b 100644 --- a/src/modules/AdminModule.h +++ b/src/modules/AdminModule.h @@ -22,6 +22,8 @@ struct AdminModule_ObserverData { */ class AdminModule : public ProtobufModule, public Observable { + friend class AdminModuleTestShim; // native unit tests reach handleSetConfig + hasOpenEditTransaction + public: /** Constructor * name is for debugging output diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index d905cc8a0..0fef857fb 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/test/test_admin_radio/test_main.cpp @@ -933,6 +933,9 @@ class AdminModuleTestShim : public AdminModule { public: using AdminModule::handleSetConfig; + // Defer persistence so handleSetConfig() exercises the in-RAM config logic without saveToDisk() touching + // the (uninitialized) node database in this lightweight harness. + void deferSaves() { hasOpenEditTransaction = true; } }; static AdminModuleTestShim *testAdmin; @@ -1028,6 +1031,64 @@ static void test_handleSetConfig_fromOthers_invalidChannelNumFullyRejected() TEST_ASSERT_EQUAL_UINT32(0, config.lora.channel_num); } +// A security-config SET that omits the private key (partial/legacy client editing some other security field) +// must NOT regenerate our keypair: our NodeNum is crc32(public_key), so a new keypair would silently change +// our identity. The existing keypair has to be preserved. +static void test_handleSetConfig_security_preservesKeypairWhenPrivateOmitted() +{ + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0x11, 32); + config.security.public_key.size = 32; + memset(config.security.public_key.bytes, 0x22, 32); + + // Incoming SET carries no private/public key, just another security field. + meshtastic_Config c = meshtastic_Config_init_zero; + c.which_payload_variant = meshtastic_Config_security_tag; + c.payload_variant.security.serial_enabled = true; + + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + uint8_t expectedPriv[32]; + memset(expectedPriv, 0x11, 32); + uint8_t expectedPub[32]; + memset(expectedPub, 0x22, 32); + TEST_ASSERT_EQUAL_UINT(32, config.security.private_key.size); + TEST_ASSERT_EQUAL_MEMORY(expectedPriv, config.security.private_key.bytes, 32); + TEST_ASSERT_EQUAL_UINT(32, config.security.public_key.size); + TEST_ASSERT_EQUAL_MEMORY(expectedPub, config.security.public_key.bytes, 32); + // The non-key field still applies. + TEST_ASSERT_TRUE(config.security.serial_enabled); +} + +// A SET that DOES supply a full 32-byte keypair (legitimate key import) must apply it, not preserve the old one. +static void test_handleSetConfig_security_acceptsSuppliedKeypair() +{ + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0x11, 32); + config.security.public_key.size = 32; + memset(config.security.public_key.bytes, 0x22, 32); + + meshtastic_Config c = meshtastic_Config_init_zero; + c.which_payload_variant = meshtastic_Config_security_tag; + c.payload_variant.security.private_key.size = 32; + memset(c.payload_variant.security.private_key.bytes, 0x33, 32); + c.payload_variant.security.public_key.size = 32; + memset(c.payload_variant.security.public_key.bytes, 0x44, 32); + + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + uint8_t expectedPriv[32]; + memset(expectedPriv, 0x33, 32); + uint8_t expectedPub[32]; + memset(expectedPub, 0x44, 32); + TEST_ASSERT_EQUAL_MEMORY(expectedPriv, config.security.private_key.bytes, 32); + TEST_ASSERT_EQUAL_MEMORY(expectedPub, config.security.public_key.bytes, 32); +} + static void test_regionInfo_supportsPreset() { const RegionInfo *eu868 = getRegion(meshtastic_Config_LoRaConfig_RegionCode_EU_868); @@ -1197,6 +1258,8 @@ void setup() RUN_TEST(test_handleSetConfig_fromLocal_invalidPresetClamped); RUN_TEST(test_handleSetConfig_fromOthers_validPresetAccepted); RUN_TEST(test_handleSetConfig_fromOthers_invalidChannelNumFullyRejected); + RUN_TEST(test_handleSetConfig_security_preservesKeypairWhenPrivateOmitted); + RUN_TEST(test_handleSetConfig_security_acceptsSuppliedKeypair); RUN_TEST(test_regionInfo_supportsPreset); RUN_TEST(test_checkConfigRegion_quietCheckReportsReason); RUN_TEST(test_handleSetConfig_fromOthers_siblingLockedPresetSwapsRegion);