From 60d7db6fccbe5336a6d67df61cdfa174223e4f91 Mon Sep 17 00:00:00 2001 From: Michael Pham <61564344+Mikefly123@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:31:47 -0700 Subject: [PATCH] fix(TcSecurityDeframer): allow PROVISION_KEY on a genuinely blank key store A board whose key store file does not exist - a brand-new board, or one whose /keys partition was erased or reformatted - could not be provisioned. It came up keyless (correct) and then refused PROVISION_KEY with StoreUnreadable forever, which is total, unrecoverable command loss over every link. Root cause is a host/target divergence. The gate added in b214dd88c26 reads if (loadStatus != OP_OK && loadStatus != DOESNT_EXIST) -> StoreUnreadable but ZephyrFile::open (lib/fprime-zephyr Os/File.cpp) discards fs_open's errno and returns OTHER_ERROR for every failure, so Os::File::DOESNT_EXIST is unreachable on flight hardware. On the POSIX host the same missing file maps ENOENT -> DOESNT_EXIST and the gate passes, which is why host unit tests never saw it. CI hardware never saw it either: every bench board was seeded under 69dcec747d6 (which gated only on activeKeyCount(), reading the default-empty in-memory store), and the littlefs keystore partition survives reflashing, so the cold path is never exercised. The naive fix - map -ENOENT to DOESNT_EXIST in the Zephyr shim - would reintroduce the hole b214dd88c26 closed. Zephyr's fs_get_mnt_point returns -ENOENT when the mount point itself is absent, identically to a missing file, so ENOENT alone would let an attacker who can keep /keys from mounting provision their own key over a board that still holds a valid one (PROVISION_KEY is bypass-allowlisted, i.e. unauthenticated and reachable over RF). Instead, stop inferring anything from the read status and probe the filesystem: * fs_stat on the store file (via FileSystem::_getPathType, not getPathType(), which folds every error into NOT_EXIST) for a positive absence answer; * fs_statvfs on the mount point (via FileSystem::getFreeSpace) as independent proof that /keys is actually mounted. Provisioning is permitted only for a demonstrably-absent file on a demonstrably live mount, or a store that read back cleanly and holds no key. An unreadable store is never provisionable, so the security property is preserved. ADD_KEY and REMOVE_KEY use the same predicate, since their gates had the same defect. The decision itself lives in Components::KeyStore in Types.hpp as pure C++ over probe results rather than over an Os status, so the divergence has nowhere to hide and the host gtest suite can cover the whole domain. A plain component test could not have caught this bug, since on the host the buggy code passes. - Types.hpp: KeyStore::MountProbe / StoreProbe / storeStateIsKnown / storeIsProvisionable (pure, header-only, no Fw:: or Os:: dependency) - TcSecurityDeframer: probeKeyStore() + the three rewritten gates - test_TcSecurityDeframer_KeyStorePolicy.cpp: exhaustive mount x store x key-count matrix, plus a test that documents the host/target divergence - provision_key_test.py: spell out why NotEmpty (and only NotEmpty) is tolerated, and add an explicit skipped test marking the cold-provision coverage gap - sdd.md: document the probe and why the read status is not a usable signal Co-Authored-By: Claude Opus 4.8 --- .../TcSecurityDeframer/TcSecurityDeframer.cpp | 87 ++++++++++- .../TcSecurityDeframer/TcSecurityDeframer.hpp | 9 ++ .../Components/TcSecurityDeframer/Types.hpp | 50 ++++++ .../Components/TcSecurityDeframer/docs/sdd.md | 5 +- .../test/int/provision_key_test.py | 38 +++++ ...test_TcSecurityDeframer_KeyStorePolicy.cpp | 142 ++++++++++++++++++ docs-site/components/TcSecurityDeframer.md | 5 +- 7 files changed, 327 insertions(+), 9 deletions(-) create mode 100644 PROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_KeyStorePolicy.cpp diff --git a/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp b/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp index 6d993371..a75f6d64 100644 --- a/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp +++ b/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.cpp @@ -50,6 +50,34 @@ U32& keyStoreGeneration() { return generation; } +//! Asks the filesystem holding `filePath` whether it is actually mounted. +//! +//! fs_statvfs resolves the mount point and queries the underlying FS, so a success is direct +//! evidence that /keys is up. This is the signal that makes "stat says the file is missing" safe to +//! act on: on Zephyr, fs_stat returns -ENOENT for an unmounted mount point just as it does for a +//! missing file (subsys/fs/fs.c: fs_get_mnt_point), so absence alone proves nothing. +KeyStore::MountProbe probeMount(const char* filePath) { + char directory[64]; + const char* const lastSlash = std::strrchr(filePath, '/'); + if (lastSlash == nullptr || lastSlash == filePath) { + directory[0] = '/'; + directory[1] = '\0'; + } else { + const size_t length = static_cast(lastSlash - filePath); + if (length >= sizeof directory) { + return KeyStore::MountProbe::Unknown; + } + std::memcpy(directory, filePath, length); + directory[length] = '\0'; + } + + FwSizeType totalBytes = 0; + FwSizeType freeBytes = 0; + return (Os::FileSystem::getFreeSpace(directory, totalBytes, freeBytes) == Os::FileSystem::OP_OK) + ? KeyStore::MountProbe::Live + : KeyStore::MountProbe::Unknown; +} + } // namespace // ---------------------------------------------------------------------- @@ -230,9 +258,17 @@ void TcSecurityDeframer ::PROVISION_KEY_cmdHandler(FwOpcodeType opCode, // of a key we managed to read. PROVISION_KEY is bypass-allowlisted so it works on a keyless // board, so if an unreadable store (a truncated record reading back as BAD_SIZE, a littlefs // error, a mount not ready yet) counted as "keyless", anyone in radio range could induce a read - // failure and install their own key while a valid one still sits on flash. DOESNT_EXIST is the - // one failure that *is* proof of emptiness. - if (loadStatus != Os::File::OP_OK && loadStatus != Os::File::DOESNT_EXIST) { + // failure and install their own key while a valid one still sits on flash. + // + // The proof cannot come from loadStatus alone: on the Zephyr target an absent file reports + // OTHER_ERROR, not DOESNT_EXIST, so gating on the status made a factory-fresh (or /keys-erased) + // board refuse its own bootstrap forever - keyless and unprovisionable, i.e. total command + // loss. probeKeyStore() asks the filesystem instead. See Components::KeyStore in Types.hpp. + KeyStore::MountProbe mountProbe = KeyStore::MountProbe::Unknown; + KeyStore::StoreProbe storeProbe = KeyStore::StoreProbe::Unreadable; + this->probeKeyStore(loadStatus, mountProbe, storeProbe); + + if (!KeyStore::storeStateIsKnown(mountProbe, storeProbe)) { this->log_WARNING_HI_KeyProvisionFailed(KeyStoreProvisionStatus::StoreUnreadable); this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR); return; @@ -240,7 +276,7 @@ void TcSecurityDeframer ::PROVISION_KEY_cmdHandler(FwOpcodeType opCode, // PROVISION_KEY is trust-on-first-use bootstrap: only honored while the store is empty. // Once any key exists, rotation must go through ADD_KEY/REMOVE_KEY (which require auth). - if (this->activeKeyCount() != 0) { + if (!KeyStore::storeIsProvisionable(mountProbe, storeProbe, this->activeKeyCount())) { this->log_WARNING_HI_KeyProvisionFailed(KeyStoreProvisionStatus::NotEmpty); this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR); return; @@ -297,8 +333,13 @@ void TcSecurityDeframer ::ADD_KEY_cmdHandler(FwOpcodeType opCode, U32 cmdSeq, U1 // If the store could not be read back, its contents are unknown, and the read-modify-write // below would persist a guess: writing the last in-memory copy over whatever is actually on - // flash. Refuse rather than risk dropping a live key. - if (loadStatus != Os::File::OP_OK && loadStatus != Os::File::DOESNT_EXIST) { + // flash. Refuse rather than risk dropping a live key. As in PROVISION_KEY, "could not be read" + // has to be established by probing the filesystem, not by reading loadStatus. + KeyStore::MountProbe mountProbe = KeyStore::MountProbe::Unknown; + KeyStore::StoreProbe storeProbe = KeyStore::StoreProbe::Unreadable; + this->probeKeyStore(loadStatus, mountProbe, storeProbe); + + if (!KeyStore::storeStateIsKnown(mountProbe, storeProbe)) { this->log_WARNING_HI_KeyAddFailed(KeyStoreProvisionStatus::StoreUnreadable); this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR); return; @@ -375,7 +416,11 @@ void TcSecurityDeframer ::REMOVE_KEY_cmdHandler(FwOpcodeType opCode, U32 cmdSeq, const Os::File::Status loadStatus = this->loadKeyStore(); // See ADD_KEY: an unreadable store makes the read-modify-write a guess. - if (loadStatus != Os::File::OP_OK && loadStatus != Os::File::DOESNT_EXIST) { + KeyStore::MountProbe mountProbe = KeyStore::MountProbe::Unknown; + KeyStore::StoreProbe storeProbe = KeyStore::StoreProbe::Unreadable; + this->probeKeyStore(loadStatus, mountProbe, storeProbe); + + if (!KeyStore::storeStateIsKnown(mountProbe, storeProbe)) { this->log_WARNING_HI_KeyRemoveFailed(KeyStoreProvisionStatus::StoreUnreadable); this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR); return; @@ -544,6 +589,34 @@ Os::File::Status TcSecurityDeframer ::loadKeyStore() { return status; } +void TcSecurityDeframer ::probeKeyStore(const Os::File::Status loadStatus, + KeyStore::MountProbe& mount, + KeyStore::StoreProbe& store) const { + if (loadStatus == Os::File::OP_OK) { + // A full, successful read is self-evidently proof the filesystem served the file. + mount = KeyStore::MountProbe::Live; + store = KeyStore::StoreProbe::Present; + return; + } + + // The read failed. Do not infer why from its status: ZephyrFile::open collapses every fs_open + // errno into OTHER_ERROR, so on flight hardware a missing file and a corrupt filesystem are + // indistinguishable here. Ask the filesystem directly instead. + // + // Os::FileSystem::getPathType() is deliberately not used: it folds every error into NOT_EXIST, + // which would turn an I/O error into a false "absent" and re-open the very hole this gate + // exists to close. Go through the interface to keep the real status. + Os::FileSystem::PathType pathType = Os::FileSystem::PathType::NOT_EXIST; + const Os::FileSystem::Status statStatus = + Os::FileSystem::getSingleton()._getPathType(this->m_keyStoreFilePath.toChar(), pathType); + + // DOESNT_EXIST from stat is a positive answer ("this path is not there"), but on Zephyr it is + // also what an unmounted /keys reports, so it only counts once probeMount() corroborates it. + store = + (statStatus == Os::FileSystem::DOESNT_EXIST) ? KeyStore::StoreProbe::Absent : KeyStore::StoreProbe::Unreadable; + mount = probeMount(this->m_keyStoreFilePath.toChar()); +} + Os::File::Status TcSecurityDeframer ::writeKeyStore() { // Write to a temp file, flush, then rename over the target - the same pattern as // StartupManager::persist_boot_count, and for a more serious failure mode. Overwriting the diff --git a/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hpp b/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hpp index 40227f3b..0c09263d 100644 --- a/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hpp +++ b/PROVESFlightControllerReference/Components/TcSecurityDeframer/TcSecurityDeframer.hpp @@ -123,6 +123,15 @@ class TcSecurityDeframer final : public TcSecurityDeframerComponentBase { //! lock held. On a missing/unreadable file, m_keyStore is left with no valid slots (keyless state). Os::File::Status loadKeyStore(); + //! Determines what is actually known about the on-flash key store after a loadKeyStore() + //! attempt, by interrogating the filesystem rather than trusting the read status (which cannot + //! distinguish "missing" from "broken" on the Zephyr target). Feeds the pure + //! Components::KeyStore policy predicates. Must be called with the key store lock held. + void probeKeyStore(const Os::File::Status loadStatus, //!< Status returned by loadKeyStore() + KeyStore::MountProbe& mount, //!< Set to whether /keys is demonstrably mounted + KeyStore::StoreProbe& store //!< Set to what is known about the store file + ) const; + //! Writes m_keyStore to the file system. Must be called with the key store lock held. Os::File::Status writeKeyStore(); diff --git a/PROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hpp b/PROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hpp index b6084c4b..28c797da 100644 --- a/PROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hpp +++ b/PROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hpp @@ -25,6 +25,56 @@ struct ActiveSpiSlot { using ActiveSpiSlots = std::array; //!< The set of currently active SPIs +//! Key-store admission policy. +//! +//! The decision "may this board be provisioned / may its key store be rewritten" is expressed here +//! as pure C++ over probe results, deliberately away from any Os:: type, so it can be exercised +//! directly by the host gtest suite across the full status matrix. +//! +//! Why it cannot be written directly against Os::File::Status: on the Zephyr target a *genuinely +//! missing* file does not report Os::File::DOESNT_EXIST. ZephyrFile::open discards fs_open's errno +//! and returns OTHER_ERROR for every failure, so DOESNT_EXIST is unreachable on flight hardware +//! while it is the normal result on the POSIX host. Any gate written as +//! `status != OP_OK && status != DOESNT_EXIST -> refuse` therefore passes on the host and refuses +//! on the target, which is exactly how a brand-new board became unprovisionable. The read status +//! alone is not a usable signal; the caller must probe the filesystem for the answer. +namespace KeyStore { + +//! Whether the filesystem holding the key store is demonstrably mounted. +enum class MountProbe { + Live, //!< The mount point answered a statvfs, so the filesystem is up + Unknown, //!< The mount point could not be interrogated: not mounted, not ready, or erroring +}; + +//! What is demonstrably true about the key store file itself. +enum class StoreProbe { + Present, //!< The store was read back in full; its contents are known + Absent, //!< The store was positively confirmed not to exist (stat says so) + Unreadable, //!< The store may or may not exist, and its contents are unknown +}; + +//! True when the on-flash key store's contents are known with certainty. +//! +//! Note the asymmetry: `Present` needs no mount probe, because a successful full read is itself +//! proof the filesystem served the file. `Absent` does need one, because on Zephyr fs_stat reports +//! -ENOENT both for "file missing from a mounted filesystem" and for "no such mount point" +//! (subsys/fs/fs.c fs_get_mnt_point). Treating the latter as emptiness would let an attacker who +//! can keep /keys from mounting provision their own key over a board that still holds a valid one. +constexpr bool storeStateIsKnown(MountProbe mount, StoreProbe store) { + return (store == StoreProbe::Present) || (store == StoreProbe::Absent && mount == MountProbe::Live); +} + +//! True when trust-on-first-use provisioning (PROVISION_KEY) may proceed. +//! +//! PROVISION_KEY is bypass-allowlisted, i.e. reachable unauthenticated over RF, so it is only safe +//! while the store is *proven* empty. Proven means: we know what is on flash (storeStateIsKnown) +//! and it holds no active key. An unreadable store is never provisionable. +constexpr bool storeIsProvisionable(MountProbe mount, StoreProbe store, uint8_t activeKeyCount) { + return storeStateIsKnown(mount, store) && activeKeyCount == 0; +} + +} // namespace KeyStore + //! CCSDS 355.0-B-2 //! https://ccsds.org/Pubs/355x0b2.pdf namespace Ccsds355_0_B_2 { diff --git a/PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md b/PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md index 41cb4493..5343ec95 100644 --- a/PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md +++ b/PROVESFlightControllerReference/Components/TcSecurityDeframer/docs/sdd.md @@ -24,7 +24,9 @@ The store is shared across all `TcSecurityDeframer` instances (UART/LoRa/Sband): Three commands manage the store (see [Commands](#commands)): `PROVISION_KEY` (bootstrap, only while the store is empty), `ADD_KEY` (rotation, fails at 2 active keys), and `REMOVE_KEY` (rotation, fails at 1 remaining key). All three re-read the store from flash first, then re-import it into PSA and update `ActiveKeyCount` telemetry on success. -All three also refuse to act (`StoreUnreadable`) when that re-read fails with anything other than "file does not exist", because the store's contents are then unknown. For `ADD_KEY`/`REMOVE_KEY` that prevents a read-modify-write from persisting a stale guess over the real store. For `PROVISION_KEY` it is a security property: the command is bypass-allowlisted so it works on a keyless board, so trust-on-first-use must be gated on *proof* that the store is empty. Treating an unreadable store as keyless would let anyone in radio range induce a read failure and install their own key while a valid one still sits on flash. +All three also refuse to act (`StoreUnreadable`) unless the store's contents are *known*, because otherwise they would be acting on a guess. + +"Known" cannot be decided from the read status. On the Zephyr target `ZephyrFile::open` discards `fs_open`'s errno and reports `OTHER_ERROR` for every failure, so `Os::File::DOESNT_EXIST` is unreachable on flight hardware even though it is the normal result for a missing file on the POSIX host. Gating on the status therefore made a factory-fresh (or `/keys`-erased) board refuse `PROVISION_KEY` forever: keyless *and* unprovisionable, i.e. total command loss. Instead `probeKeyStore()` interrogates the filesystem — `fs_stat` on the store file for a positive absence answer, plus `fs_statvfs` on the mount point to prove `/keys` is actually mounted — and feeds the pure predicates in `Components::KeyStore` (`Types.hpp`). Both signals are required: Zephyr returns `-ENOENT` for an unmounted mount point exactly as it does for a missing file, so absence alone proves nothing. For `ADD_KEY`/`REMOVE_KEY` that prevents a read-modify-write from persisting a stale guess over the real store. For `PROVISION_KEY` it is a security property: the command is bypass-allowlisted so it works on a keyless board, so trust-on-first-use must be gated on *proof* that the store is empty. Treating an unreadable store as keyless would let anyone in radio range induce a read failure and install their own key while a valid one still sits on flash. `REMOVE_KEY` zeroizes the revoked slot's key bytes before persisting, so a removed key is not recoverable from the on-flash record; the slot is restored if the durable write fails. @@ -201,6 +203,7 @@ TcSecurityDeframer helper functionality is covered by unit tests in PROVESFlight |---|---| | test_TcSecurityDeframer_Parser.cpp | Valid parse path plus parse failures for SPI, sequence number, and MAC size checks. | | test_TcSecurityDeframer_Validator.cpp | SPI validation against the active `ActiveSpiSlots` set (single slot, second slot, no valid slots), out-of-window and replayed sequence numbers, window boundary, and wraparound handling. | +| test_TcSecurityDeframer_KeyStorePolicy.cpp | The key store admission predicates (`Components::KeyStore::storeStateIsKnown` / `storeIsProvisionable`) over the exhaustive mount x store-probe x active-key-count matrix: a cold board on a live mount is provisionable; an unreadable store never is; an absent store on an unproven mount never is; and a regression test documenting why a gate written over `Os::File::Status` diverges between host and target. | | test_TcSecurityDeframer_Authenticator.cpp | `parseHexKey` (valid upper/lowercase, null, wrong length, non-hex characters), key import via hex and raw bytes, `destroyHmacKey`, successful MAC verification, and failed verification with corrupted MAC, corrupted data, or a destroyed key. | These cover only the pure-function layer (Parser/Validator/Authenticator; no F Prime or Zephyr dependency). The key store mutation rules enforced in the command handlers (`PROVISION_KEY`/`ADD_KEY`/`REMOVE_KEY` — provision-only-when-empty, add fails at 2, remove fails at 1) are F-Prime-component-dependent and are not covered here; see the commented-out `register_fprime_ut` block in `CMakeLists.txt` for a future on-target/component test pass. diff --git a/PROVESFlightControllerReference/test/int/provision_key_test.py b/PROVESFlightControllerReference/test/int/provision_key_test.py index 4aab80bd..0b42d87f 100644 --- a/PROVESFlightControllerReference/test/int/provision_key_test.py +++ b/PROVESFlightControllerReference/test/int/provision_key_test.py @@ -78,7 +78,45 @@ def test_provision_key( if evt.template.get_full_name().endswith("KeyProvisionFailed"): status = evt.args[0].val + # NotEmpty is the ONLY tolerated failure, and only because CI boards keep the key store on + # a littlefs partition that survives reflashing, so every run after the first re-provisions + # a board that is already provisioned. + # + # StoreUnreadable in particular must never be tolerated here. On a board that already holds + # a key it means the store went unreadable (a real fault); on a keyless board it is the + # signature of the cold-provisioning lockout - the board cannot be commanded at all, and + # letting this pass is precisely how that shipped. See test_cold_provision_gap below. assert status == "NotEmpty", ( f"PROVISION_KEY failed with unexpected status {status!r}; " "board should either be keyless or already hold the CI key" ) + + +@pytest.mark.provision_key +def test_cold_provision_gap(): + """Placeholder for the cold-provision path, which is not yet reachable from CI. + + The bug this file's assertions now guard against (PROVISION_KEY refused with StoreUnreadable on + a board whose key store file does not exist) can only be exercised against a *genuinely blank* + key store. Nothing in the test fleet can produce that state: + + * every bench/CI board was provisioned under an earlier firmware revision, and the littlefs + keystore_partition survives reflashing, so the store is never absent; + * the existing ``fsFormat.FORMAT`` command formats the FatFS root ``/``, NOT the littlefs + ``/keys`` partition, so it cannot clear the store either. + + Automating this therefore needs new firmware capability - either a command that erases the + keystore partition (which must itself be authenticated, since it is a remote-bricking primitive + if it is not) or a CI step that flashes a blank keystore partition image over SWD. Rather than + fake a cold board with a mock that would re-hide the host/target divergence, the invariant is + covered exhaustively at the unit level in + ``test/unit-tests/test_TcSecurityDeframer_KeyStorePolicy.cpp``, and this test is left as an + explicit, visible gap. + + TODO(#472): implement once /keys can be erased or pre-flashed blank from CI. + """ + pytest.skip( + "Cold-provision path needs a blank /keys partition; no mechanism exists yet to produce " + "one from CI (fsFormat.FORMAT targets FatFS /, not littlefs /keys). Covered at unit level " + "by test_TcSecurityDeframer_KeyStorePolicy.cpp." + ) diff --git a/PROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_KeyStorePolicy.cpp b/PROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_KeyStorePolicy.cpp new file mode 100644 index 00000000..79c1c40c --- /dev/null +++ b/PROVESFlightControllerReference/test/unit-tests/test_TcSecurityDeframer_KeyStorePolicy.cpp @@ -0,0 +1,142 @@ +// ====================================================================== +// \title test_TcSecurityDeframer_KeyStorePolicy.cpp +// \brief Unit tests for the pure key-store admission policy (Components::KeyStore) +// +// These tests guard a bug class that a component-level host test structurally cannot catch: the +// original defect only manifested on the Zephyr target, because ZephyrFile::open collapses every +// fs_open errno into Os::File::OTHER_ERROR, while the POSIX host maps ENOENT to +// Os::File::DOESNT_EXIST. A gate written as "refuse unless OP_OK or DOESNT_EXIST" therefore passed +// every host test and bricked a factory-fresh board. +// +// The fix pushes the decision behind a pure predicate over *probe results* rather than over a read +// status, so the divergence has nowhere to hide: there is no "missing file" status input to get +// wrong, and the matrix below is exhaustive over the predicate's entire domain. +// ====================================================================== + +#include + +#include "PROVESFlightControllerReference/Components/TcSecurityDeframer/Types.hpp" + +using namespace Components; +using KeyStore::MountProbe; +using KeyStore::storeIsProvisionable; +using KeyStore::StoreProbe; +using KeyStore::storeStateIsKnown; + +namespace { + +constexpr MountProbe kMounts[] = {MountProbe::Live, MountProbe::Unknown}; +constexpr StoreProbe kStores[] = {StoreProbe::Present, StoreProbe::Absent, StoreProbe::Unreadable}; + +} // namespace + +// ---------------------------------------------------------------------- +// The bootstrap path: a keyless board must be able to accept its first key +// ---------------------------------------------------------------------- + +//! The C1 regression itself: a brand-new board (/keys freshly formatted, store file never written) +//! must be provisionable, or it is permanently keyless and unreachable by command. +TEST(KeyStorePolicy, ColdBoardOnLiveMountIsProvisionable) { + EXPECT_TRUE(storeIsProvisionable(MountProbe::Live, StoreProbe::Absent, 0)); +} + +//! A store file that exists and reads back cleanly but holds no valid slot is equally proven empty. +TEST(KeyStorePolicy, ReadableButEmptyStoreIsProvisionable) { + EXPECT_TRUE(storeIsProvisionable(MountProbe::Live, StoreProbe::Present, 0)); + // A successful read is its own proof the filesystem is up, so it does not need a mount probe. + EXPECT_TRUE(storeIsProvisionable(MountProbe::Unknown, StoreProbe::Present, 0)); +} + +// ---------------------------------------------------------------------- +// The security property: trust-on-first-use may never overwrite a live key +// ---------------------------------------------------------------------- + +//! PROVISION_KEY is bypass-allowlisted (unauthenticated, reachable over RF), so once any key is on +//! flash it must be refused; rotation has to go through the authenticated ADD_KEY/REMOVE_KEY. +TEST(KeyStorePolicy, ProvisionedBoardIsNotProvisionable) { + for (uint8_t count = 1; count <= kMaxActiveKeys; count++) { + EXPECT_FALSE(storeIsProvisionable(MountProbe::Live, StoreProbe::Present, count)) + << "count=" << static_cast(count); + } +} + +//! The core hardening property: if we cannot tell what is on flash, we must not write to it. An +//! attacker who can induce a read failure (glitching, a corrupt record, a wedged littlefs) must not +//! thereby be able to install their own key over a valid one. +TEST(KeyStorePolicy, UnreadableStoreIsNeverProvisionable) { + for (MountProbe mount : kMounts) { + for (uint8_t count = 0; count <= kMaxActiveKeys; count++) { + EXPECT_FALSE(storeIsProvisionable(mount, StoreProbe::Unreadable, count)); + EXPECT_FALSE(storeStateIsKnown(mount, StoreProbe::Unreadable)); + } + } +} + +//! The trap in the naive fix. On Zephyr, fs_open/fs_stat return -ENOENT both for "file missing from +//! a mounted filesystem" and for "that mount point does not exist" (subsys/fs/fs.c +//! fs_get_mnt_point). So mapping ENOENT straight to "absent, therefore empty, therefore +//! provisionable" would let anyone who can keep /keys from mounting take over a board that still +//! holds a valid key. Absence only counts when the mount independently answered. +TEST(KeyStorePolicy, AbsentFileWithoutLiveMountIsNeverProvisionable) { + EXPECT_FALSE(storeIsProvisionable(MountProbe::Unknown, StoreProbe::Absent, 0)); + EXPECT_FALSE(storeStateIsKnown(MountProbe::Unknown, StoreProbe::Absent)); +} + +// ---------------------------------------------------------------------- +// Exhaustive matrix +// ---------------------------------------------------------------------- + +//! Pin the entire domain, so any future widening of the predicate has to be a deliberate edit here. +TEST(KeyStorePolicy, ExhaustiveMatrix) { + for (MountProbe mount : kMounts) { + for (StoreProbe store : kStores) { + const bool expectKnown = + (store == StoreProbe::Present) || (store == StoreProbe::Absent && mount == MountProbe::Live); + EXPECT_EQ(expectKnown, storeStateIsKnown(mount, store)); + + for (uint8_t count = 0; count <= kMaxActiveKeys; count++) { + EXPECT_EQ(expectKnown && count == 0, storeIsProvisionable(mount, store, count)); + } + } + } +} + +// ---------------------------------------------------------------------- +// Host/target divergence documentation +// ---------------------------------------------------------------------- + +//! Mirrors of the Os::File::Status values a key-store read can produce, kept as plain ints so this +//! pure test needs no Os:: dependency. The point of this test is the *classification* step that +//! TcSecurityDeframer::probeKeyStore performs, and specifically that the classification is not +//! allowed to depend on the read status. +enum class ReadStatus { OpOk, DoesntExist, OtherError, BadSize }; + +//! What the old (buggy) gate concluded, purely from the read status. +bool legacyGateAllowed(ReadStatus status) { + return status == ReadStatus::OpOk || status == ReadStatus::DoesntExist; +} + +//! This is the whole bug in one assertion. On the POSIX host a missing file reads back as +//! DOESNT_EXIST and the legacy gate lets provisioning through; on the Zephyr target the identical +//! situation reads back as OTHER_ERROR and the legacy gate refuses. Same board state, opposite +//! answer, decided by which Os layer you happened to compile against. Any future gate that consumes +//! a raw read status reintroduces this. +TEST(KeyStorePolicy, LegacyStatusGateDivergesBetweenHostAndTarget) { + // The same physical condition - key store file genuinely absent: + const ReadStatus onHost = ReadStatus::DoesntExist; // POSIX open() sets ENOENT + const ReadStatus onTarget = ReadStatus::OtherError; // ZephyrFile::open discards fs_open's errno + + EXPECT_TRUE(legacyGateAllowed(onHost)); + EXPECT_FALSE(legacyGateAllowed(onTarget)); + EXPECT_NE(legacyGateAllowed(onHost), legacyGateAllowed(onTarget)) + << "A missing key store must not produce different decisions on host and target"; + + // The replacement predicate takes probe results, not a status, so both platforms feed it the + // same StoreProbe::Absent + MountProbe::Live and get the same answer. + EXPECT_TRUE(storeIsProvisionable(MountProbe::Live, StoreProbe::Absent, 0)); + + // And OTHER_ERROR that is *not* a missing file (a truncated record, a wedged FS) still lands on + // Unreadable, which is refused - the hardening property survives. + EXPECT_FALSE(storeIsProvisionable(MountProbe::Live, StoreProbe::Unreadable, 0)); + EXPECT_FALSE(storeIsProvisionable(MountProbe::Live, StoreProbe::Unreadable, 1)); +} diff --git a/docs-site/components/TcSecurityDeframer.md b/docs-site/components/TcSecurityDeframer.md index 41cb4493..5343ec95 100644 --- a/docs-site/components/TcSecurityDeframer.md +++ b/docs-site/components/TcSecurityDeframer.md @@ -24,7 +24,9 @@ The store is shared across all `TcSecurityDeframer` instances (UART/LoRa/Sband): Three commands manage the store (see [Commands](#commands)): `PROVISION_KEY` (bootstrap, only while the store is empty), `ADD_KEY` (rotation, fails at 2 active keys), and `REMOVE_KEY` (rotation, fails at 1 remaining key). All three re-read the store from flash first, then re-import it into PSA and update `ActiveKeyCount` telemetry on success. -All three also refuse to act (`StoreUnreadable`) when that re-read fails with anything other than "file does not exist", because the store's contents are then unknown. For `ADD_KEY`/`REMOVE_KEY` that prevents a read-modify-write from persisting a stale guess over the real store. For `PROVISION_KEY` it is a security property: the command is bypass-allowlisted so it works on a keyless board, so trust-on-first-use must be gated on *proof* that the store is empty. Treating an unreadable store as keyless would let anyone in radio range induce a read failure and install their own key while a valid one still sits on flash. +All three also refuse to act (`StoreUnreadable`) unless the store's contents are *known*, because otherwise they would be acting on a guess. + +"Known" cannot be decided from the read status. On the Zephyr target `ZephyrFile::open` discards `fs_open`'s errno and reports `OTHER_ERROR` for every failure, so `Os::File::DOESNT_EXIST` is unreachable on flight hardware even though it is the normal result for a missing file on the POSIX host. Gating on the status therefore made a factory-fresh (or `/keys`-erased) board refuse `PROVISION_KEY` forever: keyless *and* unprovisionable, i.e. total command loss. Instead `probeKeyStore()` interrogates the filesystem — `fs_stat` on the store file for a positive absence answer, plus `fs_statvfs` on the mount point to prove `/keys` is actually mounted — and feeds the pure predicates in `Components::KeyStore` (`Types.hpp`). Both signals are required: Zephyr returns `-ENOENT` for an unmounted mount point exactly as it does for a missing file, so absence alone proves nothing. For `ADD_KEY`/`REMOVE_KEY` that prevents a read-modify-write from persisting a stale guess over the real store. For `PROVISION_KEY` it is a security property: the command is bypass-allowlisted so it works on a keyless board, so trust-on-first-use must be gated on *proof* that the store is empty. Treating an unreadable store as keyless would let anyone in radio range induce a read failure and install their own key while a valid one still sits on flash. `REMOVE_KEY` zeroizes the revoked slot's key bytes before persisting, so a removed key is not recoverable from the on-flash record; the slot is restored if the durable write fails. @@ -201,6 +203,7 @@ TcSecurityDeframer helper functionality is covered by unit tests in PROVESFlight |---|---| | test_TcSecurityDeframer_Parser.cpp | Valid parse path plus parse failures for SPI, sequence number, and MAC size checks. | | test_TcSecurityDeframer_Validator.cpp | SPI validation against the active `ActiveSpiSlots` set (single slot, second slot, no valid slots), out-of-window and replayed sequence numbers, window boundary, and wraparound handling. | +| test_TcSecurityDeframer_KeyStorePolicy.cpp | The key store admission predicates (`Components::KeyStore::storeStateIsKnown` / `storeIsProvisionable`) over the exhaustive mount x store-probe x active-key-count matrix: a cold board on a live mount is provisionable; an unreadable store never is; an absent store on an unproven mount never is; and a regression test documenting why a gate written over `Os::File::Status` diverges between host and target. | | test_TcSecurityDeframer_Authenticator.cpp | `parseHexKey` (valid upper/lowercase, null, wrong length, non-hex characters), key import via hex and raw bytes, `destroyHmacKey`, successful MAC verification, and failed verification with corrupted MAC, corrupted data, or a destroyed key. | These cover only the pure-function layer (Parser/Validator/Authenticator; no F Prime or Zephyr dependency). The key store mutation rules enforced in the command handlers (`PROVISION_KEY`/`ADD_KEY`/`REMOVE_KEY` — provision-only-when-empty, add fails at 2, remove fails at 1) are F-Prime-component-dependent and are not covered here; see the commented-out `register_fprime_ut` block in `CMakeLists.txt` for a future on-target/component test pass.