sys/random/fortuna: bug fixes and improvements#22388
Open
basilfx wants to merge 17 commits into
Open
Conversation
basilfx
requested review from
MichelRottleuthner,
aabadie and
leandrolanzieri
as code owners
github-actions
Bot
added
Area: doc
basilfx
added
Type: bug
basilfx
force-pushed
the
feature/fortuna_prng_improvements
branch
from
eb750dc to
bab7906
Compare
Contributor
|
The static test has some warnings, including missing copyright headers 🤔 |
Member
Author
|
The missing copyright headers should be solved with #22390. I asked the original author if it was OK to relicense it, and he gave permission. This should simplify it a bit. |
crasbe
added
State: waiting for other PR
Section 9.5.5 of [1] decides which entropy pools are used for a new generator key. The implementation used a bitwise-or of a shifted `i`, which will always be true. This effectively drains all the entropy. The entropy pools are a hardening measure: the attacker needs to control all of the entropy sources. Due to this bug, an attacker does not have to controll all entropy sources. The source of this bug comes from misinterpreting [1], where the `|` is defined as a divisor test, not a bitwise or (see last paragraph of section 9.5.5). [1] https://www.schneier.com/wp-content/uploads/2015/12/fortuna.pd
The Fortuna CSPRNG must not output more than FORTUNA_RESEED_LIMIT bytes (1 MiB), without reseeding (section 9.4.4. of [1]). The `_read()` wrapper takes care of this. When `bytes` is `FORTUNA_RESEED_LIMIT + 1`, the loop would exit after only a single iteration, but the goal of this loop is to allow for larger reads. This would then return less random data than requested. With the fix, the loop runs once more for the remaining chunk. This has not been an issue, because the interface of sys/random never read more than 4 bytes. On platforms where size_t cannot hold 1 MiB, the default setting will limit it to SIZE_MAX. [1]: https://www.schneier.com/wp-content/uploads/2015/12/fortuna.pdf
The seed file (section 9.6.1 of [1]) is defined as 64 bytes. Using `uint32_t` made it four times too large. The correct size was already defined as `FORTUNA_SEED_SIZE`. This is consistent with `fortuna_write_seed` and `fortuna_update_seed`, which never use more than `FORTUNA_SEED_SIZE`. [1]: https://www.schneier.com/wp-content/uploads/2015/12/fortuna.pdf
On current platforms, this is not a bug, since writing to uint8_t is atomically already (in an ISR context). It does not hurt to make this consistent with other writes, avoiding any assumptions for other platforms or architectures.
These variabls are stack-heavy and make it less useful for most applications. Since they are used mutually exclusive, move them to the state object, so they can be allocated statically.
Prior implementation required the allocation of 32 * FORTUNA_POOLS bytes of memory on the stack. By splitting up the `fortuna_reseed` into three parts (basically `sha256_init`/`sha256_update`/`sha256_final`), it is possible to compute a new generator key in a streaming fashion. This works, because `sha256_update(P1) + ... + sha256_update(Pn)` is equivalent to `sha256_update(P1 || .. || Pn)`. It is a deliberate decision to keep three individual reseed functions, to match with the operations as specified in the original paper.
`crypto_secure_wipe` is built in such a way that it cannot be optimized away by the compiler. This is exactly what is desired.
crasbe
removed
the
State: waiting for other PR
basilfx
force-pushed
the
feature/fortuna_prng_improvements
branch
from
bab7906 to
1198319
Compare
basilfx
force-pushed
the
feature/fortuna_prng_improvements
branch
from
32f56f9 to
2e08531
Compare
crasbe
removed
the
State: needs rebase
Member
Author
|
Can I rebase this one? I think it is ready. |
Member
Author
|
Friendly reminder :-) |
Contributor
I didn't forget, but I'm just the Cleaning Lady and can't really assess if the changes themselves are good, sorry 😅 @mguetschow works a lot with crypto, maybe he can take a look at it. |
Member
Author
|
I would love to give it an intermediate, and can do provide some more testing results afterwards ;-) |
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
4 participants
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.
Contribution description
Back in 2018, I contributed the Fortuna Cryptographically Secure Pseudorandom Number Generator (CSPRNG), which is based on a design by Niels Ferguson and Bruce Schneier. Although I did add a test suite to statistically test PRNGs in general, a unit test for Fortuna was not included (nor is there a compilation test).
I used Claude Fable 5 to analyze my contribution, and it spotted a few issues that would defeat the CS in CSPRNG. This PR addresses these issues.
It also improves the memory consumption in its default state, because it currently does not work on platforms other than native. This is a known issue though. I was able to move about 128 bytes of AES128/SHA256 context to the state (as a scratchpad), and optimize the
fortuna_random_databy preventing allocation of32 * FORTUNA_POOLSbuffers. No wonder why most stacks would overflow.Unit tests have been added too.
Testing procedure
I added unit tests that can be executed on native using
make -C tests/sys/random_fortuna term. With the memory optimizations, it should work on most boards with ~12-16 KiB of memory.The tests in
tests/sys/rngshould work too, if compiled withUSEMODULE=prng_fortuna make -C tests/sys/rng term. Here is some proof running on an SLTB001a:Issues/PRs references
None
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: