docs(security): RC28 open findings and handoff - #370
Open
BitHighlander wants to merge 9 commits into
Open
Conversation
Records the state of the rc28 release set after three audit rounds, so the next person does not rediscover any of it. RC28 is not merge-ready. #368 is closest; #366 cannot claim wallet-wide RNG assurance; #367 still publishes a manifest describing the unsigned draft rather than the signed binary; #369 has eight architectural blockers and is a draft, not a decided design. Captures what was fixed and verified this round (the storage_setPin_impl wallet lockout, the CRC tail that reached storage_wipe, the APT cutoff that shipped 3.5x looser than documented, the RCT reset at the APT window boundary, a PR that never compiled, and a constant-fold that kept CI red) so none of it gets re-litigated, and then everything still open with severity, the reason it matters, and a suggested order. Two things worth surfacing beyond the list. #367's hash-before-signing ordering is the likely origin of the wrong v7.14.1 hash already found pinned in Vault's table, so it is a live defect rather than a hygiene item. And "only ROM remains open" on the clear-sign roadmap is false: ROM measurement has to come after the substrate, authority model, updater invariant, certificate transcript, cross-variant preservation and policy integrity are decided, because each of them changes what would be measured. Also records the local build recipe, which took three rounds to work out -- the nanopb "rU" patch and `python` shim, PB_NO_PACKED_STRUCTS=1 for macOS ARM64, and the SecAESSTM32 worktree workaround -- plus the warning that scripts/format-source-files.sh under clang-format 22.1.1 rewrites 40+ untouched files, so the version must be pinned before anyone runs it repo-wide.
… ran The four code findings are closed; each keeps its original text so nobody re-derives it, with the resolution underneath. The correction worth reading: `firmware-unit` has never completed on a local macOS build. SIX suites hang at 100% CPU on the shared kkconfirm_preload driver -- Authenticator, Ethereum, Mayachain, Osmosis, Thorchain and Confirmation -- and all six were reproduced at the merge base 6ae3b96 with none of this work applied. So "the storage suite is 23/23" was always a filtered result, and any past claim of a clean full local run should be treated with suspicion. Excluding those six, #368 is 327/327 and #366 is 339/339, in about eight seconds. Section 4 keeps the one thing that can still undo #367: the checklist now tells key holders to regenerate the manifest from the signed binaries, and the first real release is the test of whether that instruction is followed.
…e was missing three traps The formatting note said the local clang-format disagrees with CI's and left it there. It does — 22.1.1 on PATH against CI's 20 — but brew llvm@20 is already installed at /opt/homebrew/opt/llvm@20/bin/clang-format, so the job was never unfixable locally, only run with the wrong binary. Records the exact CI check as a loop, and --lines=A:B for fixing one hunk instead of rewriting a file. Two real violations were found that way. Build recipe gains the QR-Code-generator submodule, a warning not to init trezor-firmware recursively (it drags in micropython and tinyusb), and the gtest filter without which the suite never finishes.
Both Stage-1 gates were red since 280f3b6 -- lint-format on an 82-column comment, static-analysis on a deliberate always-false comparison -- and every build and test job downstream was SKIPPED behind them. So the branch's green storage suite was a local claim only; CI had not compiled it. Records cppcheck as the second locally-runnable gate, including where the report actually goes: --error-exitcode=1 under bash -e kills the step before it cats cppcheck_report.txt, so the findings are only in the uploaded artifact unless you run it yourself.
…ings The previous revision said all four code findings were closed. Two were not, and the head hashes for #366 and #368 were a commit behind. Both reopened findings were the same mistake wearing different clothes -- a fix correct as far as it went, summarised as if it went further. #366 gated a list of call sites and called it "every key-material draw", missing the Orchard RedPallas nonce in a submodule nobody re-audits. #369 removed the certificate's three-way field inconsistency and called the result "canonical" while every offset after 0x02 was still a '*'. Neither was a coding error; both were scope claims outrunning the work, and both were found by reading the claim against the code rather than against the diff. That is now stated at the top, because it is the most useful thing in this document for whoever reads it next. Also recorded: the #367 self-test passed throughout the period its gate was inoperative, because it exercised the one input shape in which the bug is invisible. A green check on a gate is evidence about the test, not the gate. Heads, statuses, the #369 blocker table (8 downgraded to proposed, 5 widened to gate both phases) and the test counts are updated to match.
…able Round-2 review reopened both code PRs. #368 failed both ARM builds on _Alignas and then failed catalog validation on a test name renamed in 280f3b6; #366 failed every link on random_uniform and layout_warning_static, and its continuous RCT/APT state was never fed by the default path. So the previous claim that #369 was the sole reason RC28 is not merge-ready was wrong, and both are now listed as reopened-and-fixed rather than closed. The header now carries the pattern instead of a fourth round of individual corrections, because it has repeated three times in different shapes: work correct as far as it goes, reported as if it went further. The instance worth acting on is the last one -- "verified locally" meant an emulator build, and an emulator build proves nothing about shipping firmware. _Alignas compiles under clang and is rejected by the ARM toolchain; a missing link edge only appears under the archive ordering the ARM target uses. Neither is visible from cmake -DKK_EMULATOR=ON. There is a mechanical fix rather than a resolution to try harder, so it is now in the header AND the build recipe: a four-minute Docker cross-compile in CI's own image, both variants, no toolchain install. Every ARM claim in this document was reproduced that way before being written down, including the ROM delta that corrects the earlier "no ROM" claim for the bootloader. Also records that #367's structural gate is not blocked on obtaining signing keys -- pubkeys.h already has all five; it needs a host-side secp256k1 verifier.
The header explains why; §5 is where someone actually looks before running anything.
#366 made random32() abort on a failed RNG verdict, and the bootloader's stack canary draws through it. A device with a dead RNG would abort inside the bootloader: no verify, no boot, no reflash. Unrecoverable, in the component that exists to recover from everything else. Deliberately NOT fixed here. A firmware release does not update anyone's bootloader, so it cannot reach an RC28 user, and fixing it in an RC28 PR means bootloader changes reviewed under a firmware deadline that ship months later. All bootloader edits were reverted; tools/bootloader/main.c is byte-identical to develop. Records both prototyped fixes and the objdump check to confirm it, because 'nothing calls it' has already been wrong twice in this module.
…ranch died Coverage/RedPallas/degraded-RNG-recovery are one project, because each answer changes the others. All three were built during RC28 and descoped. The part worth keeping is why inverting the default failed, since it is not obvious and someone will try again: ECDSA blinding runs on the VERIFY path, so a checked draw aborts signatures_ok() before the display exists; making blinding raw does not help because generate_k_random() rejection-loops forever on a stuck source; and the raw patch is only safe while USE_RFC6979=1, since the same switch otherwise controls the real signing nonce. The prerequisite is a degraded-RNG recovery mode across firmware and bootloader crypto -- a device that still boots, still verifies, still signs with RFC6979 to move funds out, and still refuses to mint new key material. fix/blinding-draws-raw is deleted rather than left unmerged, so nobody repins to it on the strength of a commit message the loop condition disproves.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Docs only. State of the rc28 release set after three audit rounds, so none of it has to be rediscovered.
RC28 is not merge-ready.
Fixed and verified this round
storage_setPin_implhardcodedPIN_KDF_V19; it creates the wrap and runs on wallet creation, every PIN change and the V1 upgrade path. Under V17 the flag cannot round-trip, so the next boot failed every PIN on an intact wallet.flash_temp[2570]covered 2568 bytes, leaving the record's final byte unprotected on a path reachingstorage_wipe().Two things worth pulling out of the list
#367 is a live defect, not hygiene. Hashing before renaming and signing means the published full hash describes the unsigned draft. That is very likely the origin of the wrong
32155c11…v7.14.1 hash already found pinned in Vault's table."Only ROM remains open" on the roadmap is false. ROM measurement has to come after the substrate, authority model, updater invariant, certificate transcript, cross-variant preservation and policy integrity are decided — each changes what would be measured.
Also captured
The local build recipe, which took three rounds to work out (nanopb
"rU"patch +pythonshim,PB_NO_PACKED_STRUCTS=1on macOS ARM64, theSecAESSTM32worktree workaround), and a warning thatscripts/format-source-files.shunder clang-format 22.1.1 rewrites 40+ untouched files including vendoredpb_*.c— pin the version before anyone runs it repo-wide.Plus the mandatory release note: installing RC28 on a device that ran RC27 wipes it, which is the anti-rollback policy working as designed.