From 9fc3ab7f7e9eedbe058f71d86dcf1975632024dc Mon Sep 17 00:00:00 2001 From: liquidraver <504870+liquidraver@users.noreply.github.com> Date: Thu, 20 Aug 2026 21:09:21 +0200 Subject: [PATCH] PR70 fix --- releasenotes/RELEASE_NOTES_1.17.2-zephcore.md | 47 ++++++++++++++- .../adapters/datastore/ZephyrDataStore.cpp | 30 +++++++++- zephcore/adapters/datastore/ZephyrDataStore.h | 3 + zephcore/app/RepeaterDataStore.cpp | 25 ++++++-- zephcore/include/mesh/Identity.h | 38 ++++++++++++ zephcore/src/Identity.cpp | 59 +++++++++++++++++++ 6 files changed, 192 insertions(+), 10 deletions(-) diff --git a/releasenotes/RELEASE_NOTES_1.17.2-zephcore.md b/releasenotes/RELEASE_NOTES_1.17.2-zephcore.md index 0db9725..a4f65cf 100644 --- a/releasenotes/RELEASE_NOTES_1.17.2-zephcore.md +++ b/releasenotes/RELEASE_NOTES_1.17.2-zephcore.md @@ -1,8 +1,7 @@ # ZephCore 1.17.2-zephcore -A **minor release.** Three small changes, two of which only affect specific hardware. If you are running -1.17.1 on anything other than a XIAO nRF52840 or an Ikoka Nano 30 dBm, there is little reason to flash -this. +A **minor release.** Four changes, two of which only affect specific hardware, plus a hardening pass on +how the node's identity is stored. > [!NOTE] > **Most people can skip this.** The one group that should not is **XIAO nRF52840 and Ikoka Nano 30 dBm** @@ -45,12 +44,54 @@ counter, so a node that could hear the mesh but never got a word in looked compl All roles, all boards. +## A node can no longer advertise a key it cannot sign for + +Your identity is a private key plus the public key derived from it. Those were stored side by side and +read back on trust, so if the two halves ever stopped matching — a stale file, a foreign write — the node +would advertise a public key it had no private key for and keep doing so through every reboot. Nothing +ever re-checked. In that state adverts verify nowhere, direct messages to you cannot be decrypted, and +the companion app is handed an exported key that contradicts what the node says about itself. + +One node in the field turned up like this after moving from Arduino MeshCore, which is how it was found. +The public half is redundant — it can always be recomputed from the private key — so it is now treated as +a checksum rather than as data: the node derives its public key on every boot and never reads the stored +copy. If the two disagree, that is now visible in the log instead of silent. + +Storage also moves to Arduino MeshCore's byte order (`pub || prv`), so all three roles finally agree on +one format and an identity file can be moved between the two projects. Every layout either project has +ever written is still readable, including the 64-byte prv-only files older repeaters wrote — nothing +needs migrating and no existing file is rewritten. + +If a file cannot be made sense of at all, the node tries each half as a private key and adopts one only +if exactly one is genuine. Failing that it keeps the bytes as `_main.id.bad` and generates a fresh +identity, so a broken key is never silently thrown away. + +Found and diagnosed by **Marcel Verdult** ([@marcelverdult](https://github.com/marcelverdult)) — +[#70](https://github.com/liquidraver/ZephCore/pull/70). His writeup is what made the byte-order +difference findable. + +All roles, all boards. + +> [!WARNING] +> **Downgrading below 1.17.2 after a factory reset or a fresh install.** Nodes carried over from 1.17.1 +> keep their existing identity file untouched and are unaffected. But an identity *generated* on 1.17.2 +> (or re-imported into it) uses the new byte order, and 1.17.1 and earlier will misread it — the node +> comes up with a broken identity. Nothing is lost: older firmware misreads the file but does not rewrite +> it, so flashing 1.17.2 again restores the identity exactly as it was. + --- ## Known limitations - **The two radio changes are not on-air validated** — both are build-verified and reasoned from the datasheet, not measured on a live mesh. +- **The identity repair and regenerate paths are not exercised on hardware.** Every layout and failure + case is covered by host tests against the same crypto library the firmware uses, and all three roles + build clean, but no physical node has been put into a broken state and recovered. The normal path — a + healthy node loading a healthy identity — is unchanged apart from one extra key derivation at boot. +- **How that one node's key halves stopped matching is still unexplained.** The byte-order difference + reproduces the symptom exactly, but it should not have been reachable on that hardware, so something + else may have put it there. What this release fixes is that the state can no longer go unnoticed. - **The reported X1 difficulty logging in to repeaters is not fixed here** and remains under investigation. The listen-before-talk reporting above will make that failure visible in the log if it is the cause, which is one of several open hypotheses. diff --git a/zephcore/adapters/datastore/ZephyrDataStore.cpp b/zephcore/adapters/datastore/ZephyrDataStore.cpp index ba812a0..be45664 100644 --- a/zephcore/adapters/datastore/ZephyrDataStore.cpp +++ b/zephcore/adapters/datastore/ZephyrDataStore.cpp @@ -532,16 +532,40 @@ bool ZephyrDataStore::loadMainIdentity(mesh::LocalIdentity &identity) { uint8_t buf[PRV_KEY_SIZE + PUB_KEY_SIZE + 32]; size_t len = 0; - if (!openRead(MAIN_ID_FILE, buf, sizeof(buf), len) || len < PRV_KEY_SIZE + PUB_KEY_SIZE) { + if (!openRead(MAIN_ID_FILE, buf, sizeof(buf), len) || len < PRV_KEY_SIZE) { return false; } - return identity.readFrom(buf, len); + if (identity.readFromStorage(buf, len)) { + return true; + } + if (identity.recoverFromStorage(buf, len)) { + /* Deliberately not re-persisted: the file is the only record of what + * went wrong, and re-deriving costs a few ms per boot. The repeated + * warning is the point — this state should be visible, not papered + * over silently. */ + LOG_WRN("main identity pub/prv mismatch - advertising the pub its private key owns"); + return true; + } + + /* No layout coheres — the stored pub is not prv·B under any reading, so + * the pair is unusable: adverts would verify nowhere, inbound DMs would + * not decrypt, and CMD_EXPORT_PRIVATE_KEY would hand the app a key that + * contradicts SELF_INFO. Returning false makes the caller generate a + * fresh identity, so park the bytes first rather than overwriting them — + * the private key may still be extractable by hand. */ + LOG_ERR("main identity incoherent (%d bytes) - keeping bytes at %s, regenerating", + (int)len, MAIN_ID_BAD_FILE); + fs_unlink(MAIN_ID_BAD_FILE); + if (fs_rename(MAIN_ID_FILE, MAIN_ID_BAD_FILE) < 0) { + LOG_ERR("failed to park corrupt identity - regenerating over it"); + } + return false; } bool ZephyrDataStore::saveMainIdentity(const mesh::LocalIdentity &identity) { uint8_t buf[PRV_KEY_SIZE + PUB_KEY_SIZE + 32]; - size_t n = identity.writeTo(buf, sizeof(buf)); + size_t n = identity.writeToStorage(buf, sizeof(buf)); if (n == 0) { return false; } diff --git a/zephcore/adapters/datastore/ZephyrDataStore.h b/zephcore/adapters/datastore/ZephyrDataStore.h index 32b9e20..0863e40 100644 --- a/zephcore/adapters/datastore/ZephyrDataStore.h +++ b/zephcore/adapters/datastore/ZephyrDataStore.h @@ -75,6 +75,9 @@ private: static constexpr const char *MNT_POINT = "/lfs"; static constexpr const char *PREFS_FILE = "/lfs/new_prefs"; static constexpr const char *MAIN_ID_FILE = "/lfs/_main.id"; + /* Where an identity that parses under no known layout is parked before + * a fresh one is generated over the top — see loadMainIdentity(). */ + static constexpr const char *MAIN_ID_BAD_FILE = "/lfs/_main.id.bad"; static constexpr const char *SHUTDOWN_FILE = "/lfs/shutdn"; /* External QSPI flash (optional) - contacts, channels, blobs */ diff --git a/zephcore/app/RepeaterDataStore.cpp b/zephcore/app/RepeaterDataStore.cpp index d2e9359..d79f1b1 100644 --- a/zephcore/app/RepeaterDataStore.cpp +++ b/zephcore/app/RepeaterDataStore.cpp @@ -69,13 +69,28 @@ bool RepeaterDataStore::loadIdentity(mesh::LocalIdentity& id) { LOG_DBG("loadIdentity: read %d bytes from %s", (int)n, path); if (n >= PRV_KEY_SIZE) { - if (id.readFrom(buf, n)) { + if (id.readFromStorage(buf, n)) { LOG_INF("Loaded identity from %s", path); return true; } - LOG_ERR("loadIdentity: readFrom failed"); + if (id.recoverFromStorage(buf, n)) { + /* Not re-persisted on purpose — see ZephyrDataStore::loadMainIdentity. */ + LOG_WRN("identity pub/prv mismatch - advertising the pub its private key owns"); + return true; + } + LOG_ERR("loadIdentity: no coherent key layout in %d bytes", (int)n); } + /* Unusable pair — the caller regenerates, so park the bytes instead of + * letting a fresh identity overwrite them (same as the companion). */ + char bad_path[56]; + if (snprintf(bad_path, sizeof(bad_path), "%s.bad", path) < (int)sizeof(bad_path)) { + fs_unlink(bad_path); + if (fs_rename(path, bad_path) == 0) { + LOG_ERR("Identity file corrupt - kept at %s", bad_path); + return false; + } + } LOG_ERR("Identity file corrupt"); return false; } @@ -101,8 +116,10 @@ bool RepeaterDataStore::saveIdentity(const mesh::LocalIdentity& id) { return false; } - uint8_t buf[PRV_KEY_SIZE]; - int len = id.writeTo(buf, sizeof(buf)); + /* pub || prv, same as the companion and Arduino MeshCore. Older builds + * wrote prv alone (64 bytes); readFromStorage() still accepts those. */ + uint8_t buf[PUB_KEY_SIZE + PRV_KEY_SIZE]; + int len = id.writeToStorage(buf, sizeof(buf)); ssize_t n = fs_write(&file, buf, len); ret = fs_sync(&file); fs_close(&file); diff --git a/zephcore/include/mesh/Identity.h b/zephcore/include/mesh/Identity.h index a67d0e4..753f63e 100644 --- a/zephcore/include/mesh/Identity.h +++ b/zephcore/include/mesh/Identity.h @@ -61,8 +61,46 @@ public: void calcSharedSecret(uint8_t *secret, const uint8_t *other_pub_key) const; static bool validatePrivateKey(const uint8_t prv[64]); + /* Arduino-compatible *buffer* format: prv || pub (or prv alone when the + * buffer is only PRV_KEY_SIZE). This is protocol-facing — it is what + * CMD_EXPORT/IMPORT_PRIVATE_KEY and the `prv.key` CLI put on the wire — + * so it must stay byte-identical to upstream. Do NOT use it for + * on-flash storage; use readFromStorage()/writeToStorage() instead. */ bool readFrom(const uint8_t *src, size_t len); size_t writeTo(uint8_t *dest, size_t max_len) const; + + /* On-flash format: pub || prv, matching Arduino MeshCore's IdentityStore + * (LocalIdentity::writeTo(Stream&)). Kept separate from the buffer + * format above because the two orders differ and conflating them is + * exactly how a node ends up advertising a key it cannot sign for. */ + size_t writeToStorage(uint8_t *dest, size_t max_len) const; + + /* Tolerant counterpart to writeToStorage(). Accepts every layout either + * project has ever written and tells them apart by deriving the pub from + * the candidate prv — the stored pub is a 256-bit checksum over the prv, + * so a wrong guess cannot match: + * + * len == 64 -> prv only (ZephCore repeater/room <= 1.17.x) + * len == 96 -> pub || prv (Arduino MeshCore, ZephCore >= 1.17.2) + * len == 96 -> prv || pub (ZephCore companion <= 1.17.x) + * + * pub_key is always the derived value, never the stored bytes, so a + * loaded identity is coherent by construction. Returns false when no + * layout coheres — the pair is unusable and the caller must not run + * with it. */ + bool readFromStorage(const uint8_t *src, size_t len); + + /* Last resort after readFromStorage() fails: one half of a 96-byte blob + * may still be an intact private key (the other having been clobbered by + * a stale/foreign write). Tests both candidate halves with + * validatePrivateKey() and adopts one only if exactly one qualifies — + * ambiguity means we cannot tell which half is which, and guessing wrong + * would cement a garbage identity. Recovers in RAM only: the file is + * left alone so the evidence survives and the warning repeats each boot. + * Note this changes the node's advertised pub_key to the one its private + * key actually owns; contacts must re-add it either way, since the pub it + * was advertising was never usable. */ + bool recoverFromStorage(const uint8_t *src, size_t len); }; } /* namespace mesh */ diff --git a/zephcore/src/Identity.cpp b/zephcore/src/Identity.cpp index b16ef60..e1aa495 100644 --- a/zephcore/src/Identity.cpp +++ b/zephcore/src/Identity.cpp @@ -199,6 +199,65 @@ size_t LocalIdentity::writeTo(uint8_t *dest, size_t max_len) const return PRV_KEY_SIZE + PUB_KEY_SIZE; } +size_t LocalIdentity::writeToStorage(uint8_t *dest, size_t max_len) const +{ + if (max_len < PUB_KEY_SIZE + PRV_KEY_SIZE) return 0; + memcpy(dest, pub_key, PUB_KEY_SIZE); + memcpy(dest + PUB_KEY_SIZE, prv_key, PRV_KEY_SIZE); + return PUB_KEY_SIZE + PRV_KEY_SIZE; +} + +bool LocalIdentity::readFromStorage(const uint8_t *src, size_t len) +{ + uint8_t derived[PUB_KEY_SIZE]; + + /* prv alone — nothing to disagree with, derive and go. */ + if (len == PRV_KEY_SIZE) { + memcpy(prv_key, src, PRV_KEY_SIZE); + crypto_eddsa_scalarbase(pub_key, prv_key); + return true; + } + if (len != PUB_KEY_SIZE + PRV_KEY_SIZE) return false; + + /* Canonical: pub || prv. */ + crypto_eddsa_scalarbase(derived, src + PUB_KEY_SIZE); + if (memcmp(derived, src, PUB_KEY_SIZE) == 0) { + memcpy(prv_key, src + PUB_KEY_SIZE, PRV_KEY_SIZE); + memcpy(pub_key, derived, PUB_KEY_SIZE); + return true; + } + + /* Legacy ZephCore companion: prv || pub. */ + crypto_eddsa_scalarbase(derived, src); + if (memcmp(derived, src + PRV_KEY_SIZE, PUB_KEY_SIZE) == 0) { + memcpy(prv_key, src, PRV_KEY_SIZE); + memcpy(pub_key, derived, PUB_KEY_SIZE); + return true; + } + + return false; +} + +bool LocalIdentity::recoverFromStorage(const uint8_t *src, size_t len) +{ + if (len != PUB_KEY_SIZE + PRV_KEY_SIZE) return false; + + /* Where the prv would sit under each layout. */ + const uint8_t *cand[2] = { src + PUB_KEY_SIZE, src }; /* pub||prv, prv||pub */ + int found = -1; + + for (int i = 0; i < 2; i++) { + if (!validatePrivateKey(cand[i])) continue; + if (found >= 0) return false; /* both plausible — refuse to guess */ + found = i; + } + if (found < 0) return false; + + memcpy(prv_key, cand[found], PRV_KEY_SIZE); + crypto_eddsa_scalarbase(pub_key, prv_key); + return true; +} + void LocalIdentity::sign(uint8_t *sig, const uint8_t *message, int msg_len) const { signExpanded(sig, prv_key, pub_key, message, (size_t)msg_len);