feat(rng): gate wallet creation on a seed-time RNG self-test - #366
Merged
Conversation
reset_init() now refuses to generate int_entropy unless the random number
generator is demonstrably present, enabled and not stuck. Failing closed
here is the point: every later step of wallet creation is downstream of
those 32 bytes, and this is the last moment before key material exists.
Two separate checks, because they cover two different failures:
rng_source_live() reads the STM32 RNG peripheral's own control and
status registers -- RNGEN set, no latched SEIS/CEIS,
and two distinct RNG_DR reads within a bounded wait.
The answer does not depend on any build-configuration
macro having the value its name suggests.
rng_health_analyze() SP 800-90B repetition-count and adaptive-proportion
tests over 1024 freshly drawn bytes. Pure function,
unit tested, no I/O.
Both cutoffs are derived in comments rather than copied from a table, so a
reviewer can recompute them: RCT C = 1 + ceil(30/8) = 5 for alpha = 2^-30,
and APT C = 16 from Poisson(511/256) tail 4.7e-10 < 2^-30. The same
derivation at alpha = 2^-20 reproduces NIST's published C = 13, which is
the cross-check that it is right. False-positive rate on a good source is
about 2.4e-7 per wallet creation.
SCOPE, stated plainly because it is easy to overclaim. Neither check
detects a healthy-looking generator seeded with very little state. That was
the July 2026 Coldcard failure -- a substituted software PRNG passed every
statistical test because it was a CSPRNG, just seeded with ~40 bits -- and
no output test finds it at any sample size. What covers that case is the
pair of #error build guards already in lib/rand/rng.c; rng_source_live() is
the runtime backstop for a configuration that gets past them, since a build
where the hardware path was never enabled reads RNGEN back clear.
unittests/firmware/rng_health.cpp pins that limitation as an expectation:
a 16-bit-seeded generator passes, and the test says so in a comment, so
nobody later "fixes" it into a claim the code cannot support.
Follow-ups deliberately not in this PR: latching a failure into storage so
a fault survives replug, and a proto field reporting the result to the host
(needs a device-protocol change and a repin).
Audit findings against d71a110, all confirmed locally. DID NOT COMPILE. FailureType_Failure_ProcessError does not exist; the enum stops at Failure_FirmwareError = 99. The earlier "builds clean" claim came from compiling the analyzer standalone against a stub harness -- reset.c was never built. Now FirmwareError, which is also the right code: nothing about the request is wrong, the device's own entropy source failed. APT CUTOFF WAS OFF BY ONE, and looser than documented. NIST's counter is initialised to 1 because it INCLUDES the window reference; this code counts only the samples that FOLLOW it. Initialising to 1 while using the following-matches cutoff failed a window at 15 following matches rather than 16. Exact tail for X ~ Binomial(511, 1/256): P(X >= 15) = 3.227e-9 P(X >= 16) = 3.891e-10 alpha = 2^-30 = 9.313e-10 so the shipped alpha was 3.227e-9 -- about 3.5x looser than claimed. The counter now counts following matches only. The old comment also cited "the same derivation at alpha = 2^-20 reproduces NIST's published C = 13" as a cross-check. That is wrong under the inclusive convention, which gives 14, and it was an assertion of a check that had not been performed. Removed; the exact tails are stated instead, with the convention spelled out so the two cutoffs cannot be conflated again. SEIS/CEIS is now re-read on every sampling iteration. Checking it once before the loop accepted samples drawn after a fault latched mid-loop. NO SAMPLE BUFFER. RCT and APT are now streaming O(1) state, and rng_health_check draws in 32-byte chunks. The previous 1 KiB automatic in a device with a 16 KiB reserve gate and a history of boot faults from large frames was not worth the convenience. rng_health_analyze remains as a thin wrapper over the same streaming path, so tests exercise the production code rather than a parallel implementation. Fixed while rewriting: the window-in-progress flag cannot stand in for "saw data", or a sample that is an exact multiple of the window (1024 bytes is exactly two) would report failure. Verified: kkfirmware and kkrand build, reset.c included this time; 7/7 unit tests pass with the APT boundary asserting cutoff-1 following matches passes and exactly cutoff fails. Still open from the audit and NOT addressed here: the gate protects only the generate-mnemonic path, while recovery, LoadDevice, PIN and wipe-code changes and storage init also draw from the same source to build storage encryption keys. That wants a centralised checked-RNG state rather than more call sites, and is a separate change.
Confirmed audit finding, reproduced before fixing.
One `started` flag initialised both tests, so at each 512-sample APT window
boundary the next byte also reset rct_prev and rct_run. A five-byte repeat
straddling bytes 510-514 -- exactly the stuck-source signature RCT exists to
catch -- was accepted. The existing cutoff test placed its run near offset 100
and could never see it.
RCT and APT now keep independent initialisation state: RCT is continuous across
the whole stream, only APT is windowed.
Two regressions added, both failing before the fix:
- RctSpansAptWindowBoundary: a cutoff-length run straddling byte 512.
- IrregularChunkingMatchesOneShot: the same stream fed in 1, 7, 32 and 511
byte chunks, since chunk sizes that do not divide the window are where
streaming state diverges from one-shot.
Also removes the constant-fold that kept CI red: rng_source_live() returned a
bare `true` under EMULATOR, making every caller's check an always-false branch
that static analysis correctly flagged. It now draws twice from the emulator's
own source and requires the values to differ -- cheap, but a real check rather
than an assertion, and the caller's branch stays meaningful in both builds. No
suppression added.
NOT fixed here, and still blocking any wallet-wide RNG claim: this gate covers
only the generate-mnemonic path. Recovery, LoadDevice, PIN and wipe-code
changes, upgrades and storage init all create storage keys without passing it.
That needs centralised checked-RNG state consumed by every security-key draw,
not more call-site checks.
Note for whoever chases the formatting job: scripts/format-source-files.sh with
clang-format 22.1.1 rewrites 40+ untouched files including vendored pb_*.c, so
the local version disagrees with CI's. Only the files changed here were
formatted.
The gate landed in reset_init() only, so it covered generate-mnemonic and
nothing else. Recovery and import, LoadDevice, PIN and wipe-code changes, U2F
registration, the storage-key rewrap on upgrade and the one-shot OTP
randomness block all created key material straight from random_buffer(). The
check was real; the claim "this device will not create key material on a
broken generator" was not, and RC28 must not make it until this lands.
Repeating rng_health_check() at each of those sites would redraw a 1 KiB
sample per call and, worse, leave the next new site to remember. Instead the
verdict is computed once and latched, and random_buffer_checked() is the only
way to draw key material:
- one place decides (rng_health_check), one place fails closed
(random_buffer_checked zeroes the buffer and returns false);
- every byte it hands out is folded into a CONTINUOUS SP 800-90B context,
which is what the RCT and APT are specified for -- constant space, so
covering every draw costs nothing over covering one sample;
- the latch is per boot and one-way. Retrying a failed source until it
passes is how a marginal generator talks its way in.
Converted, with the failure disposition each site can actually support:
storage key, PIN-KDF salt and wipe-code key halt (random_buffer_or_die, the
same disposition storage_secMigrate already takes when secrets fail to
decrypt); U2F key-handle derivation returns NULL and refuses to register;
reset_init keeps its host-visible FirmwareError; the OTP randomness block
writes NOTHING, leaving the block unlocked for a later healthy boot, because
it is locked forever once written and runs before kk_board_init() so there is
no display to warn on.
Left alone, deliberately: stack canaries, timer jitter, U2F channel ids,
memcmp_s decoys and the device UUID are not key material and must not be able
to halt the device. GetEntropy is also left alone -- it is the RNG audit
interface, and gating it would block the very measurement that finds a failing
source.
Tests pin the fail-closed property: a refused draw returns false AND leaves
the buffer zeroed, so a caller ignoring the return still gets nothing usable,
and random_buffer_or_die halts.
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
… 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.
Google style with PointerAlignment left: (uint8_t*), not (uint8_t *).
…named
Review found the previous approach unsound in principle, and it was: gating a
LIST of call sites can only ever cover the call sites someone remembered. It
missed the one that matters most.
redpallas.c:281, inside the pinned crypto submodule, draws the Orchard
RedPallas signing nonce with a bare random_buffer(), and the production Zcash
path reaches it. The signature is s = r + c*rsk, so two signatures sharing r
give up the spend authorization key by algebra. Also missed: ECDSA
operation/coordinate blinding via random32(), and SecAESSTM32 key/state
masking. None of these are ours to enumerate, and the next dependency bump
could add more.
So the default is inverted rather than the list extended:
random32() checked; HALTS if the verdict has failed
random_buffer() trezor-crypto's, built on random32() -> inherits it
random32_raw() the unchecked hardware draw, explicitly named
random_buffer_raw() likewise
Every cryptographic consumer inside deps/ is now covered without touching
deps/ at all, because they all reach random32() through the same symbol. Link
proof from the built binary: redpallas.o's only undefined RNG symbol is
_random_buffer; _random_buffer's body is `bl _random32`; rng.c's undefined
symbols include _rng_health_require.
The raw entries exist because some draws must never halt, and each one now
says why at the call site: the health gate itself (which would otherwise
recurse into its own verdict), GetEntropy (the RNG audit interface -- gating
it would block the measurement that finds a failing source), stack canaries in
both firmware and bootloader, timer jitter, U2F channel ids, constant-time
compare decoys, and drbg_init's seeding, which also runs pre-display in the
bootloader and which nothing in the tree consumes today.
The bootloader therefore gains no fatal RNG path, and no ROM: it calls only
raw entries, and the device build uses -ffunction-sections/--gc-sections.
random_buffer_or_die() is deleted -- plain random_buffer() now does exactly
that -- and storage.c's three call sites revert to plain random_buffer().
random_buffer_checked() stays for the paths with somewhere better to go than a
halt: reset_init's host-visible FirmwareError, U2F registration returning NULL,
and the one-shot OTP block, which must skip and let a later healthy boot claim
it rather than halt before the display exists.
Tests: random_buffer() and random32() both halt on a failed verdict; the raw
entries keep working on one, which is what the boot paths depend on.
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
…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.
Three defects in the inversion, all found by review because I never cross-compiled and reported CI as "running" instead of green. **Both ARM variants failed to link**, and so did board-unit and crypto-unit: `undefined reference to random_uniform`. rng.c called trezor-crypto's, which made kkrand depend on trezorcrypto -- and GNU ld resolves static archives left-to-right in one pass, so whether that linked came down to archive order. It had been resolving by accident; removing rng.c's random_buffer reference stopped dragging rand.o in early and the accident ended. Four lines of rejection sampling now live in rng.c, so kkrand depends on nothing. **crypto-unit failed on layout_warning_static**, for the mirror-image reason: rng_health.c reached UP into kkboard, which is listed BEFORE kkrand everywhere. rng_health_require() now halts with abort() and the module includes no board headers at all. Firmware paths that have somewhere to report -- reset_init, U2F registration, the OTP block -- still render a proper error by calling rng_health_check() first; abort() is the backstop for draws with no UI context, which is exactly the dependency draws it exists for. **memcmp_s pulled the fatal path into the bootloader by the back door.** It filled decoys with random_buffer_raw() and then shuffled them with the CHECKED permutation, and the bootloader verifies signatures through memcmp_s(). Decoy ORDER is a timing-equalisation detail exactly like decoy CONTENT, so it now uses random_permute_char_raw(). Recovery-cipher and PIN-matrix shuffles stay checked. **The default path did not feed the continuous test.** Only random_buffer_checked() folded bytes into rng_continuous, so ordinary draws -- RedPallas, ECDSA blinding, SecAESSTM32 -- enforced the boot verdict and nothing else, and a source going degenerate after the gate would not have been noticed by the RCT or APT that exist to notice exactly that. random32() now calls rng_health_observe() on every checked draw. Two tests cover it: a stuck run observed after the gate latches the verdict, and the next plain draw dies. MEASURED, not asserted, in CI's own image (kktech/firmware@sha256:7438e539): both ARM variants and the bootloader build; and my earlier "no ROM" claim for the bootloader was wrong. Bootloader text +784 bytes, firmware text +1408. The bootloader edge is trezor-crypto's generate_k_random, an ECDSA SIGNING nonce that gc-sections keeps but the bootloader never calls -- it verifies. So there is no reachable fatal path there, but there is real ROM, and against a 256 KiB partition holding ~103 KiB that is affordable rather than free.
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
…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.
Three defects, and one scope correction. **The triggering draw was still returned.** random32() observed the word and returned it regardless, so only the NEXT call aborted -- and random_buffer() is built from four-byte draws, so a run tripping on the final word handed back the whole degenerate buffer first. For a RedPallas nonce that is the disclosure this gate exists to prevent, delivered by the gate itself. rng_health_observe() now returns whether these very bytes tripped the test, and random32() aborts before returning them. The regression forces the raw source so the trip happens on a chosen draw; it fails if the abort is removed, which is the only way to prove the triggering word never escapes. **The firmware could not boot with a dead RNG.** signatures_ok() runs at keepkey_main.c:177, BEFORE kk_board_init(), and reaches the gate through ecdsa_verify_digest -> curve_to_jacobian -> generate_k_random. A device whose generator died would abort at boot with no display, and could not be used to move funds out either -- RFC6979 signing needs no entropy at all. Fixed at the right layer, in the crypto fork (7a4c3464): blinding draws raw. With USE_RFC6979 those callers are blinding only, never a nonce, so a weak draw degrades masking and discloses nothing. Verified from the built ELF: the firmware's only caller of checked random32() is now random_buffer(). **lib/rand was never in my clang-format sweep** -- CI's file list does not cover it and neither did I. Formatted. **Scope: every bootloader and blupdater edit is reverted.** tools/bootloader/main.c is byte-identical to develop again. A firmware release does not update anyone's bootloader, so a reachable gate in a bootloader we are not building-and-shipping cannot affect an RC28 user -- and carrying bootloader changes in an RC means they ship months later, reviewed under this deadline rather than their own. The bootloader's stack canary therefore still draws through checked random32(). **That is a hard gate on the next bootloader release, recorded in the RC28 handoff.**
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
#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.
7.15 ships the narrow gate this PR was originally scoped as. The inverted
default -- random32() checked, so every consumer including deps/ inherited it
-- is removed, along with the crypto-submodule patch it needed.
Why it was descoped rather than fixed again:
- generate_k_random() is blinding ONLY while USE_RFC6979=1. The macro that
made it draw raw would, under a different configuration, control the actual
ECDSA signing nonce. rng_health.c opens by warning against trusting a
build-configuration macro to have the value its name suggests -- that is
the Coldcard lesson -- and this reintroduced exactly that.
- The raw path did not deliver what it promised anyway. generate_k_random()
loops `while (bn_is_zero(k) || !bn_is_less(k, prime))`, so a source stuck
at zero never leaves it. A dead-RNG device would hang rather than abort:
less diagnosable, no more recoverable.
- Making a dead-RNG device genuinely bootable and spendable needs a defined
degraded-RNG recovery mode across firmware and bootloader crypto. That is a
project, not an RC patch.
RedPallas stays uncovered. That is develop's existing behaviour, not a
regression introduced by 7.15, and it is now stated in the header rather than
implied.
WHAT THIS SHIPS. Six draws opt in by name via random_buffer_checked(): the
device half of the seed, the storage encryption key, the wipe-code key, the
PIN-KDF salt on the V1 upgrade path, the U2F key-handle path, and the one-shot
OTP randomness block. The first four halt on failure; U2F returns NULL and the
OTP write is skipped, leaving the block unlocked for a later healthy boot.
Kept from the wider work: the corrected RCT/APT (independent state, exact
cutoffs), the latched per-boot verdict, and the triggering-byte protection --
random_buffer_checked() observes the bytes it just drew and wipes them if THEY
tripped the test, rather than returning the output the test just rejected.
Coverage is OPT-IN and the header says so, listing every covered site. Two
earlier revisions of this branch described it as wallet-wide; both were wrong.
deps/ and the bootloader are untouched -- 0 files each. Bootloader text is
102656 bytes, identical to develop.
Verified: both ARM variants build in CI's image; firmware-unit 341,
crypto-unit 18, board-unit 5, RngHealth 15; clang-format 20 and cppcheck
clean; trezor-crypto's ecdsa.c compiles standalone under CI's -Werror set.
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.
What
reset_init()refuses to generateint_entropyunless the RNG is demonstrably present, enabled and not stuck. Last checkpoint before any key material exists — everything downstream of those 32 bytes depends on them.Two checks, covering two different failures:
rng_source_live()RNG_CR_RNGENset, no latchedSEIS/CEIS, two distinctRNG_DRreads within a bounded waitrng_health_analyze()The bounded wait matters:
random32()spins forever by design, and a self-test has to be able to conclude "dead" rather than hang the device before the user has been told anything.Cutoffs are derived, not copied
Both are worked out in comments so a reviewer can recompute rather than trust a transcribed table:
C = 1 + ceil(-log2(α)/H) = 1 + ceil(30/8) = 5for α = 2^-30, H = 8.P(X≥15) ≈ 3.9e-9,P(X≥16) ≈ 4.7e-10, α = 2^-30 ≈ 9.3e-10, so C = 16.α sits at the strict end of NIST's 2^-20..2^-40 range on purpose — this gate blocks wallet creation, so a spurious block is a support ticket. False-positive rate on a good source is ~2.4e-7 per wallet creation.
Scope — stated plainly, because this is easy to overclaim
Neither check detects a healthy-looking generator seeded with very little state. That was the July 2026 Coldcard failure: a substituted software PRNG passed every statistical test because it was a CSPRNG, just seeded with ~40 bits. No output test finds that at any sample size — recovering a 40-bit pool by collision needs ~2^20 independent seedings.
What covers that case is the pair of
#errorbuild guards already inlib/rand/rng.c.rng_source_live()is the runtime backstop for a configuration that gets past them: on a build where the hardware path was never enabled,RNGENreads back clear and this refuses to produce a seed.unittests/firmware/rng_health.cpppins the limitation as an expectation — a 16-bit-seeded generator passes, with a comment saying so — so nobody later "fixes" it into a claim the code cannot support.#ifndef EMULATORis not an escape hatch hererng.calready asserts#if defined(EMULATOR) && defined(__arm__)is a compile error, and__arm__comes from the compiler's target definition rather than any board config. ARM firmware therefore always compiles the register path; there is no build of the shipping firmware where these checks are absent.Testing
7/7 in
unittests/firmware/rng_health.cpp— null/empty rejection, all-zeros, stuck 0xFF, exact RCT boundary (4 passes / 5 fails), exact APT boundary (15 passes / 16 fails), and the tiny-seed limitation.Run standalone (
clang++against the vendored gtest,-DEMULATOR) becausefirmware-unitdoes not complete locally — tests that start the emulator UDP listener hang waiting for a driver, which is pre-existing. The ARM register path inrng_source_live()is not exercised by that run and needs CI plus on-hardware confirmation at rc28.Not in this PR
Reviewer checklist
rng_source_live()cannot hang (bounded loop) and cannot false-negative on a legitimateRNG_DRvalue of 0awaiting_entropyis cleared before the gate)