From 90a6dec3f3a730c48a61142e416ccb32082ac0f2 Mon Sep 17 00:00:00 2001 From: Clive Blackledge Date: Thu, 20 Aug 2026 12:19:57 +0000 Subject: [PATCH] fix(NodeDB): require a full 32-byte key when demoting to the warm tier (#11431) * fix(NodeDB): require a full 32-byte key when demoting to the warm tier meshtastic_User.public_key is a wire `bytes` field with max_size 32, so any size in 0..32 decodes off the air, and nothing validates it on ingress: NodeInfoModule hands the decoded User straight to NodeDB::updateUser, whose PKI gates are all `== 32` and so fall through for a partial key, and TypeConversions::CopyUserToNodeInfoLite then stores it with the short size. demoteOldestHotNodesToWarm() admitted that partial key into the warm tier on a `size > 0` gate. WarmNodeEntry has no length field - it distinguishes "has a key" from "no key" purely by all-zero - so N real bytes plus 32-N zeros become indistinguishable from a genuine key. copyPublicKeyAuthoritative() then hands that fabricated key back with size = 32 and reports it AUTHORITATIVE, and re-admission writes size = 32 into the hot store. From then on updateUser's key pin permanently rejects the node's real NodeInfo, and DMs to it are encrypted to a key nobody holds. Require a full 32-byte key, so a partial one is absorbed as "no key" (nullptr) rather than as a truncated one. WarmNodeStore::place() already treats a null key as keyless and clears the slot's stale key when repurposing it. This aligns the site with its two siblings, which both already gate on `size == 32` (the purge path in cleanupMeshDB and the runtime eviction in getOrCreateMeshNode). The ingress gap - updateUser accepting a 1..31-byte key at all - is a separate, larger change and is left for its own review. Co-Authored-By: Claude Opus 5 * docs(NodeDB): shorten warm-demotion comment to two lines Repo guideline (AGENTS.md): keep code comments to one or two lines. Retains the non-obvious invariant - warm entries have no key length field - and drops the restated detail. Co-Authored-By: Claude Opus 5 * test(NodeDB): cover short-key demotion into the warm tier A warm record stores 32 raw key bytes with no length field, so a partial hot-store key is indistinguishable from a real one once demoted. The public_key.size == 32 gate in demoteOldestHotNodesToWarm() is what keeps a truncated key from being laundered into a full-looking warm key, but nothing exercised it. test_migration_dropsShortKeyOnDemotion overflows the hot store with one node carrying a 31-byte key and asserts it lands as a keyless placeholder while a genuine 32-byte key still survives. push() grows a keySize parameter to seed the partial key, and clearWarm() gives the test an empty warm tier, which it needs because the warm store outlives setUp() and a prior run's warm.dat. Verified to discriminate: with the size gate reverted to size > 0 the new test fails on "a 31-byte key must not be demoted as if it were a full key", and passes again once restored. Co-Authored-By: Claude Opus 5 * test(NodeDB): assert the keyless placeholder carries last_heard The test only proved a warm metadata row survived the demotion, not that the placeholder does the job the nullptr is there for, which is preserving last_heard when the key is dropped. Asserting the value needed the seeds fixing first. Warm entries pack role, protected category and the xeddsa flag into the low 7 bits of last_heard (WARM_TIME_MASK is 0xFFFFFF80), so warm time has 128 second granularity and the old seeds of 1, 2, 3 all quantised to 0. They are now multiples of 128, which keeps the demotion ordering identical and makes the values survive the round trip. Real last_heard is epoch seconds, so this is closer to production than the old counter was. Reads the entry through WarmNodeStore::take() rather than getOrCreateMeshNode(), which does not restore last_heard from the warm tier and would have been asserting a path that does not exist. Reported by CodeRabbit on #11431. --------- Co-authored-by: Claude Opus 5 Co-authored-by: Ben Meadors --- src/mesh/NodeDB.cpp | 6 ++-- test/test_nodedb_blocked/test_main.cpp | 45 ++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index 41d570f3e..040777857 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -2148,9 +2148,9 @@ void NodeDB::demoteOldestHotNodesToWarm() const meshtastic_NodeInfoLite &n = (*meshNodes)[i]; if (n.num == 0) continue; - // Keep the public key if we have one (40 B warm record); keyless nodes - // still get a placeholder so re-admission restores last_heard. - warmStore.absorb(n.num, n.last_heard, n.public_key.size > 0 ? n.public_key.bytes : nullptr, n.role, + // Warm entries carry no key length, so a partial key would be indistinguishable + // from a full one. nullptr keeps the keyless placeholder that restores last_heard. + warmStore.absorb(n.num, n.last_heard, n.public_key.size == 32 ? n.public_key.bytes : nullptr, n.role, warmProtectedCategory(n), nodeInfoLiteHasXeddsaSigned(&n)); // Demotion drops the node from the header table, so drop its satellites // too (the eviction chokepoint) - they'd otherwise orphan until the next diff --git a/test/test_nodedb_blocked/test_main.cpp b/test/test_nodedb_blocked/test_main.cpp index 96d392cd8..3825ed5ee 100644 --- a/test/test_nodedb_blocked/test_main.cpp +++ b/test/test_nodedb_blocked/test_main.cpp @@ -29,6 +29,7 @@ class NodeDBTestShim : public NodeDB // Read back the role + protected category the warm tier cached for a node. bool warmMeta(NodeNum n, uint8_t &role, uint8_t &prot) { return warmStore.lookupMeta(n, role, prot); } + bool warmTake(NodeNum n, WarmNodeEntry &out) { return warmStore.take(n, out); } void clearHot() { @@ -36,8 +37,13 @@ class NodeDBTestShim : public NodeDB numMeshNodes = 0; } + // The warm tier outlives setUp() (and a prior run's warm.dat), so a test that + // asserts on a warm row has to start from an empty one. + void clearWarm() { warmStore.clear(); } + + // keySize < 32 seeds a partial key, as a truncated/short NodeInfo would leave behind. void push(NodeNum num, uint32_t lastHeard, bool favorite, bool ignored, bool withUser, bool withKey, - meshtastic_Config_DeviceConfig_Role role = meshtastic_Config_DeviceConfig_Role_CLIENT) + meshtastic_Config_DeviceConfig_Role role = meshtastic_Config_DeviceConfig_Role_CLIENT, pb_size_t keySize = 32) { meshtastic_NodeInfoLite n = meshtastic_NodeInfoLite_init_zero; n.num = num; @@ -50,8 +56,8 @@ class NodeDBTestShim : public NodeDB if (withUser) nodeInfoLiteSetBit(&n, NODEINFO_BITFIELD_HAS_USER_MASK, true); if (withKey) { - n.public_key.size = 32; - memset(n.public_key.bytes, static_cast(num & 0xff), 32); + n.public_key.size = keySize; + memset(n.public_key.bytes, static_cast(num & 0xff), keySize); n.public_key.bytes[0] = 0x01; // ensure non-zero (all-zero == "no key") } meshNodes->push_back(n); @@ -161,6 +167,38 @@ static void test_migration_carriesSignerBitThroughWarm(void) TEST_ASSERT_FALSE_MESSAGE(nodeInfoLiteHasXeddsaSigned(plainBack), "re-admission must not invent the signer bit"); } +// A warm record stores 32 raw key bytes with no length, so a partial hot-store key would be +// indistinguishable from a real one once demoted. It must land as a keyless placeholder instead. +static void test_migration_dropsShortKeyOnDemotion(void) +{ + db->clearWarm(); + db->seedSelf(); + const NodeNum shortKeyNum = 2000 + 3; + const NodeNum fullKeyNum = 2000 + 4; + const int extra = MAX_NUM_NODES + 30; // overflow so the oldest non-protected are demoted + // Warm entries steal the low 7 bits of last_heard for role and protected-category metadata + // (WARM_TIME_MASK), so seed multiples of 128 to keep the values representable once demoted. + for (int i = 1; i <= extra; i++) + db->push(2000 + i, /*last_heard=*/(uint32_t)i * 128, /*favorite=*/false, /*ignored=*/false, /*withUser=*/true, + /*withKey=*/true, meshtastic_Config_DeviceConfig_Role_CLIENT, + /*keySize=*/(NodeNum)(2000 + i) == shortKeyNum ? 31 : 32); + + db->runDemote(); + + // Both left the hot store; only the full key is allowed through to the warm tier. + TEST_ASSERT_NULL(db->getMeshNode(shortKeyNum)); + TEST_ASSERT_NULL(db->getMeshNode(fullKeyNum)); + TEST_ASSERT_FALSE_MESSAGE(warmHasKey(shortKeyNum), "a 31-byte key must not be demoted as if it were a full key"); + TEST_ASSERT_TRUE_MESSAGE(warmHasKey(fullKeyNum), "a full 32-byte key still survives demotion"); + + // The short-key node is still held, just keyless, so re-admission restores its last_heard. + uint8_t role = 0xFF, prot = 0xFF; + TEST_ASSERT_TRUE_MESSAGE(db->warmMeta(shortKeyNum, role, prot), "keyless placeholder row must still be present"); + WarmNodeEntry placeholder = {}; + TEST_ASSERT_TRUE_MESSAGE(db->warmTake(shortKeyNum, placeholder), "placeholder must be readable from the warm tier"); + TEST_ASSERT_EQUAL_UINT32_MESSAGE(3u * 128, warmTimeOf(placeholder), "the keyless placeholder must carry last_heard"); +} + // Favourite handling: a favourite is never the eviction victim, even when it is // the oldest node in a full hot store. static void test_eviction_preservesFavorite(void) @@ -290,6 +328,7 @@ NDB_TEST_ENTRY void setup() RUN_TEST(test_migration_demotesOldestKeepsKeepersAndSelf); RUN_TEST(test_migration_carriesRoleAndProtectedIntoWarm); RUN_TEST(test_migration_carriesSignerBitThroughWarm); + RUN_TEST(test_migration_dropsShortKeyOnDemotion); RUN_TEST(test_eviction_preservesFavorite); RUN_TEST(test_eviction_prefersCurrentBootStampOverPost2038Epoch); RUN_TEST(test_ignored_survivesEvictionAndCleanup);