diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index 96b7ae81bb..9867a848c6 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -4417,6 +4417,39 @@ bool NodeDB::checkLowEntropyPublicKey(const meshtastic_Config_SecurityConfig_pub } #endif +#if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) +// A freshly minted keypair must not itself land on the blacklist. Fail with no key rather than persist +// a known-weak identity: only a broken entropy source can land here, and retrying would not fix that. +bool NodeDB::generateBlacklistCheckedKeyPair() +{ + crypto->generateKeyPair(config.security.public_key.bytes, config.security.private_key.bytes); + if (!checkLowEntropyPublicKey(config.security.public_key)) + return true; + LOG_ERROR("PKI keygen produced a known low-entropy key; entropy source is broken"); + config.security.public_key.size = 0; + config.security.private_key.size = 0; + return false; +} + +// Derive the public key from the stored private key and vet it. The entry check cannot see a weak key +// when the stored public key is absent, and a failed derivation must not leave sizes claiming a pair. +bool NodeDB::derivePublicKeyFromPrivate() +{ + config.security.public_key.size = 32; + if (!crypto->regeneratePublicKey(config.security.public_key.bytes, config.security.private_key.bytes)) { + LOG_ERROR("Can't generate public key from private key"); + config.security.public_key.size = 0; + config.security.private_key.size = 0; + return false; + } + if (!checkLowEntropyPublicKey(config.security.public_key)) + return true; + keyIsLowEntropy = true; + LOG_WARN("Private key derives a known low-entropy public key; generating a new keypair"); + return generateBlacklistCheckedKeyPair(); +} +#endif + bool NodeDB::generateCryptoKeyPair(const uint8_t *privateKey) { #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) @@ -4442,29 +4475,24 @@ bool NodeDB::generateCryptoKeyPair(const uint8_t *privateKey) LOG_INFO("Using provided private key for PKI"); memcpy(config.security.private_key.bytes, privateKey, 32); config.security.private_key.size = 32; - config.security.public_key.size = 32; - // Generate public key from the provided private key - if (crypto->regeneratePublicKey(config.security.public_key.bytes, config.security.private_key.bytes)) { - keygenSuccess = true; - } else { - LOG_ERROR("Can't generate public key from private key"); + if (!derivePublicKeyFromPrivate()) return false; - } + keygenSuccess = true; } // Try to regenerate public key from existing private key if it's valid and not low entropy else if (config.security.private_key.size == 32 && !keyIsLowEntropy) { - config.security.public_key.size = 32; LOG_DEBUG("Regenerate PKI public key from private key"); - if (crypto->regeneratePublicKey(config.security.public_key.bytes, config.security.private_key.bytes)) { - keygenSuccess = true; - } + if (!derivePublicKeyFromPrivate()) + return false; + keygenSuccess = true; } else { // Generate a new key pair LOG_INFO("Generate new PKI keys"); config.security.public_key.size = 32; config.security.private_key.size = 32; - crypto->generateKeyPair(config.security.public_key.bytes, config.security.private_key.bytes); + if (!generateBlacklistCheckedKeyPair()) + return false; keygenSuccess = true; } diff --git a/src/mesh/NodeDB.h b/src/mesh/NodeDB.h index 0e669cca54..4ab9655c31 100644 --- a/src/mesh/NodeDB.h +++ b/src/mesh/NodeDB.h @@ -80,6 +80,11 @@ static const uint8_t LOW_ENTROPY_HASHES[][32] = { {0xcc, 0x11, 0xfb, 0x1a, 0xab, 0xa1, 0x31, 0x87, 0x6a, 0xc6, 0xde, 0x88, 0x87, 0xa9, 0xb9, 0x59, 0x37, 0x82, 0x8d, 0xb2, 0xcc, 0xd8, 0x97, 0x40, 0x9a, 0x5c, 0x8f, 0x40, 0x55, 0xcb, 0x4c, 0x3e}}; static const char LOW_ENTROPY_WARNING[] = "Compromised keys were detected and regenerated."; +// Shown when a user tries to restore/set a known pre-2.8 low-entropy key: explains why the saved +// key did not persist and that the node's identity (NodeNum == crc32(public_key)) changed with it. +static const char LOW_ENTROPY_RESTORE_WARNING[] = + "That key is a known pre-2.8 low-entropy key and can't be restored. A new secure key was " + "generated; your node number has changed."; #endif static const char LICENSED_IDENTITY_MIGRATION_WARNING[] = "Licensed signing generated a new identity key; this node identity changed."; @@ -587,6 +592,10 @@ class NodeDB #if !defined(MESHTASTIC_EXCLUDE_PKI) bool checkLowEntropyPublicKey(const meshtastic_Config_SecurityConfig_public_key_t &keyToTest); #endif +#if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN || MESHTASTIC_EXCLUDE_PKI) + bool generateBlacklistCheckedKeyPair(); + bool derivePublicKeyFromPrivate(); +#endif /// Consolidate crypto key generation logic used across multiple modules /// @param privateKey Optional 32-byte private key to use. If nullptr, generates new random keys. diff --git a/src/modules/AdminModule.cpp b/src/modules/AdminModule.cpp index abc71034e6..4f22ef6738 100644 --- a/src/modules/AdminModule.cpp +++ b/src/modules/AdminModule.cpp @@ -1207,10 +1207,20 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers) config.security = incoming; #if !(MESHTASTIC_EXCLUDE_PKI_KEYGEN) && !(MESHTASTIC_EXCLUDE_PKI) // First provisioning (no key) generates one; a private key supplied without its public key derives it. + // A supplied public key that is itself blacklisted is re-derived too, so a restore carrying a whole + // low-entropy pair cannot skip the check just by populating both fields. if (config.security.private_key.size != 32) { nodeDB->generateCryptoKeyPair(); - } else if (config.security.public_key.size == 0) { - nodeDB->generateCryptoKeyPair(config.security.private_key.bytes); + } else if (config.security.public_key.size == 0 || nodeDB->checkLowEntropyPublicKey(config.security.public_key)) { + // Warn at set time, not after the next reboot, and only when the key really was replaced: a + // blacklisted public key whose private key derives a clean one is re-derived, and that stuck. + uint8_t priorPrivateKey[32]; + memcpy(priorPrivateKey, config.security.private_key.bytes, 32); + const bool keygenSucceeded = nodeDB->generateCryptoKeyPair(priorPrivateKey); + const bool keyWasReplaced = memcmp(priorPrivateKey, config.security.private_key.bytes, 32) != 0; + if (keygenSucceeded && keyWasReplaced && nodeDB->keyIsLowEntropy) { + sendWarning(LOW_ENTROPY_RESTORE_WARNING); + } } #endif if (config.security.is_managed && !(config.security.admin_key[0].size == 32 || config.security.admin_key[1].size == 32 || diff --git a/test/test_admin_radio/test_main.cpp b/test/test_admin_radio/test_main.cpp index 8a332b7a92..a0cfb416f2 100644 --- a/test/test_admin_radio/test_main.cpp +++ b/test/test_admin_radio/test_main.cpp @@ -21,7 +21,8 @@ #include "TestUtil.h" #include "graphics/draw/MenuHandler.h" #include "mesh/Channels.h" -#include "mesh/Router.h" // router global: allocErrorResponse() allocates the reply through it +#include "mesh/CryptoEngine.h" // crypto global: the tests swap in a stub engine to drive key derivation +#include "mesh/Router.h" // router global: allocErrorResponse() allocates the reply through it #include "modules/AdminModule.h" #include "modules/NodeInfoModule.h" #include // crc32Buffer(), for the my_node_num == crc32(public_key) invariant @@ -1022,8 +1023,13 @@ static void replaceAdminRadioGlobals() nodeDB = replacementNodeDB; } +// Defined with the crypto stub below; tearDown must undo an install even when a failed assertion +// longjmped out of the test body before it could. +static void dropRestoreCryptoStub(); + static void restoreAdminRadioGlobals() { + dropRestoreCryptoStub(); nodeInfoModule = savedNodeInfoModule; nodeDB = savedNodeDB; router = savedRouter; @@ -1703,6 +1709,241 @@ static void test_handleSetConfig_security_clearsAdminKeysWhenKeypairUnchanged() TEST_ASSERT_EQUAL_UINT(0, config.security.admin_key[0].size); } +// No low-entropy private key is published, so stand in for the engine to derive a blacklisted public +// key on demand. hash() is left real: the blacklist lookup runs through it. +static const uint8_t COMPROMISED_PUBLIC_KEY[32] = {0xac, 0xaf, 0x8c, 0x1c, 0x3c, 0x1c, 0x37, 0xac, 0x4f, 0x03, 0xa1, + 0xe9, 0xfc, 0x37, 0x23, 0x29, 0xc8, 0xa3, 0x5d, 0x7f, 0x05, 0x26, + 0xeb, 0x00, 0xbd, 0x26, 0xb8, 0x2e, 0xb1, 0x94, 0x7d, 0x24}; + +class RestoreDerivingCryptoEngine : public CryptoEngine +{ + public: + bool regenerateSucceeds = true; + bool derivesLowEntropy = true; + bool regeneratePublicKey(uint8_t *pubKey, uint8_t *privKey) override + { + if (!regenerateSucceeds) + return false; + if (derivesLowEntropy) + memcpy(pubKey, COMPROMISED_PUBLIC_KEY, 32); + else + memset(pubKey, 0x7C, 32); + return true; + } + bool mintsLowEntropy = false; + void generateKeyPair(uint8_t *pubKey, uint8_t *privKey) override + { + if (mintsLowEntropy) + memcpy(pubKey, COMPROMISED_PUBLIC_KEY, 32); + else + memset(pubKey, 0x5E, 32); + memset(privKey, 0x5F, 32); + } +}; + +static CryptoEngine *savedCrypto; +static RestoreDerivingCryptoEngine *restoreCrypto; + +// Installed here and torn down in restoreAdminRadioGlobals(), not at the end of the test body: a failed +// TEST_ASSERT longjmps straight out, which would leave later tests running against a freed stub. +static RestoreDerivingCryptoEngine *installRestoreCrypto() +{ + savedCrypto = crypto; + restoreCrypto = new RestoreDerivingCryptoEngine(); + crypto = restoreCrypto; + return restoreCrypto; +} + +static void dropRestoreCryptoStub() +{ + if (!restoreCrypto) + return; + crypto = savedCrypto; + delete restoreCrypto; + restoreCrypto = nullptr; +} + +// Arms a bare private-key restore: region set so keygen runs, private key present, public key absent. +static meshtastic_Config makeBareKeyRestoreConfig() +{ + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + initRegion(); + + 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, 0x11, 32); + return c; +} + +static bool capturedWarningsContain(const char *needle) +{ + for (const std::string &w : capturedWarnings) + if (w.find(needle) != std::string::npos) + return true; + return false; +} + +// A restored private key deriving a blacklisted public key is rejected and rotated at set time, and +// the client is told why - not left to discover it after the next reboot. +static void test_handleSetConfig_security_lowEntropyRestoreWarnsAndRotates() +{ + installRestoreCrypto(); + + const meshtastic_Config c = makeBareKeyRestoreConfig(); + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_TRUE(nodeDB->keyIsLowEntropy); + TEST_ASSERT_EQUAL_UINT(32, config.security.public_key.size); + TEST_ASSERT_TRUE(memcmp(COMPROMISED_PUBLIC_KEY, config.security.public_key.bytes, 32) != 0); + TEST_ASSERT_FALSE(nodeDB->checkLowEntropyPublicKey(config.security.public_key)); + TEST_ASSERT_TRUE(capturedWarningsContain(LOW_ENTROPY_RESTORE_WARNING)); +} + +// A restore carrying a whole blacklisted pair must not skip validation just because it populated the +// public key too - that path reaches neither keygen branch, so the weak identity used to be kept. +static void test_handleSetConfig_security_lowEntropyFullKeypairRestoreIsRejected() +{ + installRestoreCrypto(); + + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + initRegion(); + + 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, 0x11, 32); + c.payload_variant.security.public_key.size = 32; + memcpy(c.payload_variant.security.public_key.bytes, COMPROMISED_PUBLIC_KEY, 32); + + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_EQUAL_UINT(32, config.security.public_key.size); + TEST_ASSERT_TRUE(memcmp(COMPROMISED_PUBLIC_KEY, config.security.public_key.bytes, 32) != 0); + TEST_ASSERT_FALSE(nodeDB->checkLowEntropyPublicKey(config.security.public_key)); + TEST_ASSERT_TRUE(capturedWarningsContain(LOW_ENTROPY_RESTORE_WARNING)); +} + +// A blacklisted public key whose private key derives a clean one is only re-derived - the user's key +// does stick, so the "a new secure key was generated" warning would be a lie here. +static void test_handleSetConfig_security_reDerivedCleanKeyDoesNotWarn() +{ + installRestoreCrypto()->derivesLowEntropy = false; + + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + initRegion(); + + 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, 0x11, 32); + c.payload_variant.security.public_key.size = 32; + memcpy(c.payload_variant.security.public_key.bytes, COMPROMISED_PUBLIC_KEY, 32); + + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + // The supplied private key survives, and the blacklisted public key is replaced by its derivation. + uint8_t expectedPriv[32]; + memset(expectedPriv, 0x11, 32); + TEST_ASSERT_EQUAL_MEMORY(expectedPriv, config.security.private_key.bytes, 32); + TEST_ASSERT_FALSE(nodeDB->checkLowEntropyPublicKey(config.security.public_key)); + TEST_ASSERT_TRUE(memcmp(COMPROMISED_PUBLIC_KEY, config.security.public_key.bytes, 32) != 0); + TEST_ASSERT_FALSE(capturedWarningsContain(LOW_ENTROPY_RESTORE_WARNING)); +} + +// A replacement that is itself blacklisted leaves no identity behind - persisting a known-weak key +// would defeat the rejection this whole path exists for. +static void test_handleSetConfig_security_blacklistedMintLeavesNoKey() +{ + installRestoreCrypto()->mintsLowEntropy = true; + + const meshtastic_Config c = makeBareKeyRestoreConfig(); + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_EQUAL_UINT(0, config.security.private_key.size); + TEST_ASSERT_EQUAL_UINT(0, config.security.public_key.size); + TEST_ASSERT_FALSE(capturedWarningsContain(LOW_ENTROPY_RESTORE_WARNING)); +} + +// factory_reset_config keeps the private key and clears the public one, so the entry check sees no key +// and the boot-time derive path used to adopt whatever it produced - including a known-weak key. +static void test_generateCryptoKeyPair_derivedFromStoredPrivateIsChecked() +{ + installRestoreCrypto(); + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + initRegion(); + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0x11, 32); + config.security.public_key.size = 0; // as installDefaultConfig(preserveKey = true) leaves it + + TEST_ASSERT_TRUE(nodeDB->generateCryptoKeyPair()); + + TEST_ASSERT_TRUE(nodeDB->keyIsLowEntropy); + TEST_ASSERT_TRUE(memcmp(COMPROMISED_PUBLIC_KEY, config.security.public_key.bytes, 32) != 0); + TEST_ASSERT_FALSE(nodeDB->checkLowEntropyPublicKey(config.security.public_key)); +} + +// Same clear-and-fail on the boot path: a stored private key that derives nothing must not leave both +// sizes at 32, claiming a pair the node never got. +static void test_generateCryptoKeyPair_failedDerivationFromStoredPrivateClearsKeySizes() +{ + installRestoreCrypto()->regenerateSucceeds = false; + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_US; + initRegion(); + config.security.private_key.size = 32; + memset(config.security.private_key.bytes, 0x11, 32); + config.security.public_key.size = 0; + + TEST_ASSERT_FALSE(nodeDB->generateCryptoKeyPair()); + + TEST_ASSERT_EQUAL_UINT(0, config.security.private_key.size); + TEST_ASSERT_EQUAL_UINT(0, config.security.public_key.size); +} + +// keyIsLowEntropy survives from a boot-time regeneration, and generateCryptoKeyPair returns early on +// an unset region without clearing it. The restore warning must stay gated on this keygen running. +static void test_handleSetConfig_security_staleLowEntropyFlagDoesNotWarn() +{ + config.security = meshtastic_Config_SecurityConfig_init_zero; + config.lora.region = meshtastic_Config_LoRaConfig_RegionCode_UNSET; + initRegion(); + nodeDB->keyIsLowEntropy = true; + + 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, 0x11, 32); + + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_FALSE(capturedWarningsContain(LOW_ENTROPY_RESTORE_WARNING)); +} + +// A private key that derives nothing usable must not leave sizes claiming a 32-byte pair behind: +// that state gets persisted, and every later keygen re-derives from the same dead key. +static void test_handleSetConfig_security_failedDerivationClearsKeySizes() +{ + installRestoreCrypto()->regenerateSucceeds = false; + + const meshtastic_Config c = makeBareKeyRestoreConfig(); + testAdmin->deferSaves(); + testAdmin->handleSetConfig(c, false); + + TEST_ASSERT_EQUAL_UINT(0, config.security.private_key.size); + TEST_ASSERT_EQUAL_UINT(0, config.security.public_key.size); + TEST_ASSERT_FALSE(capturedWarningsContain(LOW_ENTROPY_RESTORE_WARNING)); +} + static void test_regionInfo_supportsPreset() { const RegionInfo *eu868 = getRegion(meshtastic_Config_LoRaConfig_RegionCode_EU_868); @@ -2397,6 +2638,14 @@ void setup() RUN_TEST(test_handleSetConfig_security_acceptsSuppliedKeypair); RUN_TEST(test_handleSetConfig_security_rotationPreservesAdminKeys); RUN_TEST(test_handleSetConfig_security_clearsAdminKeysWhenKeypairUnchanged); + RUN_TEST(test_handleSetConfig_security_lowEntropyRestoreWarnsAndRotates); + RUN_TEST(test_handleSetConfig_security_lowEntropyFullKeypairRestoreIsRejected); + RUN_TEST(test_handleSetConfig_security_reDerivedCleanKeyDoesNotWarn); + RUN_TEST(test_handleSetConfig_security_blacklistedMintLeavesNoKey); + RUN_TEST(test_generateCryptoKeyPair_derivedFromStoredPrivateIsChecked); + RUN_TEST(test_generateCryptoKeyPair_failedDerivationFromStoredPrivateClearsKeySizes); + RUN_TEST(test_handleSetConfig_security_staleLowEntropyFlagDoesNotWarn); + RUN_TEST(test_handleSetConfig_security_failedDerivationClearsKeySizes); RUN_TEST(test_regionInfo_supportsPreset); RUN_TEST(test_checkConfigRegion_quietCheckReportsReason); RUN_TEST(test_checkConfigRegion_allowsProspectiveLicensedOwner);