-
Notifications
You must be signed in to change notification settings - Fork 610
Rework ChaCha_RNG #5121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Rework ChaCha_RNG #5121
Conversation
CC @reneme |
@reneme Thanks for your review, I probably addressed everything mentioned. I left your suggestions open for you to compare and close, as I did not apply them 1:1. My code also uses the IV/Nonce of ChaCha20 now to extend the key material. I therefore centrally check the length assertion in the suggested truncate function (now a split into two sub-key chunks). I needed some additional variables typically named |
Just FYI speed comparison on 4 Byte
8 Byte
16 Byte
32 Byte
64 Byte
512 Byte
1024 Byte
All taken from |
Ready from my side, will squash, when review is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thillux I didn't realize I am authorized to push into your fork 😨 I meant to push into my own branch, but a sole git push -f
was a bit too vigorous. Please make sure I didn't accidentally crush something. Sorry about that.
Changes itself look good to me and I think the improvements make sense. Thanks!
To the best of my understanding, the ChaCha RNG isn't standardized in any way, so we should have the liberty to improve the implementation as suggested. However, depending on how pedantically we want to follow semantic versioning, these changes might be seen as a violation. E.g. if someone based their unit tests on a hard-coded seed for this RNG their tests would break (much like Botan's KATs had to be updated).
As far as I can see, "breaking" changes are:
- "HMAC(SHA-256)" replaced by "HMAC(SHA-512)"
- derivation and usage of an IV for the internal ChaCha updates
@randombit What's your take on that? Do we want to guarantee stable DRNG outputs across minor versions?
Your push did not break things for me :). I'll squash together now. |
89f02e4
to
abe423d
Compare
@reneme Squashed everything into two commits (change DRNG + tests). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, apart from the semver concerns and a typo.
Oh, and please note and update the related documentation in rng.rst
The current ChaCha_RNG implementation did not provide backtracking resistance on internal state compromise and could insert more entropy into the internal ChaCha20 state by additionally using the IV. Change this by: - Using HMAC(SHA-512) instead of HMAC(SHA-256) for key derivation on (re-)seed. - Add optional rekeying after each output to add backtracking resistance. - Also generate IV from input entropy. This makes it possible, to have up to 320 bit of entropy in the effective internal state (ChaCha key + IV). A next commit will adapt KATs to this new structure. Co-authored-by: René Meusel <[email protected]> Signed-off-by: Markus Theil <[email protected]>
This adapts the KATs for new key derivation with HMAC(SHA-512) and optional fast key erasure. Signed-off-by: Markus Theil <[email protected]>
Signed-off-by: Markus Theil <[email protected]>
abe423d
to
f7eca75
Compare
I updated the documentation. |
While there, I saw, that ESDM_RNG is not documented there. Can I do it in this PR or open another one? |
Please open a separate one. |
I am a bit hesitant to change the output bytes here since (in a Hyrum's Law sense) the
So probably fine to do. I would request that the doc update note that a) the output changed in 3.10 b) the output might change again at any future time, and if RNG stability is required to use [I still do want to review this PR fully before merging] |
Signed-off-by: Markus Theil <[email protected]>
@randombit I added some more notes on output stability as requested. |
This is currently WIP. Will squash when ready.
When reading the source code of the ChaCha_RNG I saw some room for improvement:
@randombit If interested, can you provide me your go program, which created the KAT for the RNG?
I'll adapted KAT test results via copy paste from CLI output and can also generate them externally if needed.