noise: ensure every requested source covers the whole buffer - #654
Open
Silexperience210 wants to merge 1 commit into
Open
noise: ensure every requested source covers the whole buffer#654Silexperience210 wants to merge 1 commit into
Silexperience210 wants to merge 1 commit into
Conversation
noise_get_random_bytes() could report success while leaving part of the output buffer untouched by the requested entropy sources: - MCU RNG looped buf_len/4 times, so with a buffer length that is not a multiple of 4 the trailing 1-3 bytes were never mixed. random_buffer() in the trezor-crypto glue takes an arbitrary length and routes to this path. - SE RNG looped buf_len/32 times, so any buffer shorter than 32 bytes received no SE entropy at all, and the function still returned true. - The buf_len < 4 early return left frequency_turbo() enabled. Both loops now advance by the number of bytes actually mixed, so every source covers the full buffer for any length >= 4, and the early return restores the clock. Seed generation is unaffected: new_seed_task uses Noise.ALL with a 32-byte buffer, where the avalanche source already fills the buffer and both the MCU and SE contributions were already complete. This is defensive hardening of the helper, not a fix for a reachable weakness.
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
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.
Summary
noise_get_random_bytes()could returntruewhile leaving part of the output buffer untouched by one of the requested entropy sources.buf_lenNOISE_MCU_RNG_SOURCENOISE_SE_RNG_SOURCEfalsebut leavesfrequency_turbo()enabledThe MCU loop ran
buf_len / 4times and the SE loopbuf_len / 32times, so a short or unaligned buffer silently fell through.random_buffer()in the trezor-crypto glue takes an arbitrarylenand routes to the MCU-only path, so the unaligned case is reachable by construction even though no current caller hits it.Scope
Seed generation is not affected, and this is not a fix for a reachable weakness.
new_seed_taskcallsNoise.ALLwith a 32-byte buffer: the avalanche source already fills the whole buffer first, and at 32 bytes both the MCU and SE contributions were already complete. Every current call site uses a length that is a multiple of 4 with eitherMCUorALL. This is defensive hardening of the helper so the failure mode cannot appear as call sites change.Change
Both loops now advance by the number of bytes actually mixed (
MIN(buf_len - pos, sizeof(sample))), so any requested source covers the full buffer for any length >= 4. The MCU path switches to the existingxor_mixin()helper for consistency with the SE path. The short-buffer early return now restores the clock before returning.One file, +21/-11, no behaviour change for any existing caller.
Testing
The patched function was extracted into a host harness with instrumented
xor_mixin()/rng_sample()/se_pick_nonce()stubs that record which bytes are actually written. Every buffer length from 4 to 64 is fully covered forMCU,SEandALL; the same harness reproduces the gaps on the unpatched version. Builds clean under-Wall -Wextra.I do not have Passport hardware, so this has not been run on-device — the change is confined to buffer arithmetic and touches no ADC, SE or clock configuration.
Not included here, but worth raising
bootloader/factory-test.conly runs at provisioning. If the analog front-end degrades in the field the ADC returns near-constant values and nothing detects it; the MCU and SE XOR still carry the seed, but the headline property is silently lost. Adding this needs a policy decision (warn vs. block seed generation), so it seemed wrong to bundle into this PR.adc.cconfigures ADC2 withOversampling.Ratio = 32xandRightBitShift = 5, i.e. hardware averaging, which attenuates exactly the high-frequency noise the avalanche circuit produces. This may well be deliberate and calibrated against the analog front-end — I have no hardware to measure the actual noise amplitude, so I am asking rather than proposing a change. Is the entropy budget characterised with oversampling enabled?Happy to split, reword or drop any part of this.