dhm: Fix bias in private key generation #4628
Conversation
ronald-cron-arm
force-pushed
the
dhm-key-generation-bias
branch
from
acc268e to
d35a7bb
Compare
ronald-cron-arm
added
component-crypto
|
@gilles-peskine-arm @mpg I believe the commit in this PR addresses in a minimalist way #4612. Do we want to do more like fixing the equivalent bias in blinding values in dhm.c ? in ecp.c ? |
Ideally yes, if it can be done without too much trouble. |
Improve the validation of the output from mbedtls_dhm_make_params: * Test that the output in the byte buffer matches the value in the context structure. * Test that the calculated values are in the desired range. Cherry-picked dc0b6e4. Changed mbedtls_test_rnd_pseudo_rand to rnd_pseudo_rand. Removed test step code. Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: Ronald Cron <[email protected]>
Repeat a few tests that use random data. This way the code is exercised with a few different random values. Signed-off-by: Gilles Peskine <[email protected]>
mbedtls_dhm_make_params() with x_size != size of P is not likely to be useful, but it's supported, so test it. Cherry-picked 33ec863 Changed mbedtls_test_rnd_pseudo_info type to rnd_pseudo_info Signed-off-by: Gilles Peskine <[email protected]> Signed-off-by: Ronald Cron <[email protected]>
Unify the common parts of mbedtls_dhm_make_params and mbedtls_dhm_make_public. No intended behavior change, except that the exact error code may change in some corner cases which are too exotic for the existing unit tests. Removed usage of MBEDTLS_ERROR_ADD which does not exist in 2.16. Signed-off-by: Gilles Peskine <[email protected]>
P-1 is as bad as 1 as a blinding value. Don't accept it. The chance that P-1 would be randomly generated is infinitesimal, so this is not a practical issue, but it makes the code cleaner. It was inconsistent to accept P-1 as a blinding value but not as a private key. Signed-off-by: Gilles Peskine <[email protected]>
dhm_make_common includes a piece of code that is identical to dhm_random_below except for returning a different error code in one case. Call dhm_random_below instead of repeating the code. Signed-off-by: Gilles Peskine <[email protected]>
No need to build a bignum for the value 2. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Ronald Cron <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
ronald-cron-arm
force-pushed
the
dhm-key-generation-bias
branch
from
d35a7bb to
424d13c
Compare
|
I believe I have a decent proposal now for the backport of #4276 in 2.16 regarding DHM (ECP not handled yet). I've back-ported all DHM related commits but "DHM: use mbedtls_mpi_random for blinding and key generation" that I have replaced by "dhm: Fix bias in private key generation and blinding". When the backport of a commit was not clean I've added some lines in the commit message starting with "Cherry-picked ..." and signed-off the commit myself as well. Please have a look. |
There was a problem hiding this comment.
- I am satisfied that this is a complete backport (with minimal necessary adaptations) of the part of Backport 2.x: Fix non-uniform random generation in a range #4276 that concerns Diffie-Hellman, except for the change to use the new function
mbedtls_mpi_randomindhm_random_belowand the changelog entry. - I am satisfied that the commit "dhm: Fix bias in private key generation and blinding" is correct and does the same job as the part of Backport 2.x: Fix non-uniform random generation in a range #4276 concerning
dhm.cthat was not backported. - I am satisfied that the changes are not too disruptive for a LTS branch. I have slight concerns about "DHM refactoring: unify mbedtls_dhm_make_{params,public}" changing error codes in corner cases, but I can't identify one in a reasonable amount of time, so I think they would be too exotic to matter even by LTS standards.
So all that's missing for my approval is a changelog entry.
gilles-peskine-arm
added
needs: changelog
needs-review
Signed-off-by: Ronald Cron <[email protected]>
Fix Mbed-TLS#4245. Signed-off-by: Gilles Peskine <[email protected]>
|
Thanks for the feedback @gilles-peskine-arm. For ECP, I propose to just fix the blinding values generation in ecp_randomize_jac/mxz(). I've added a commit for that. Otherwise I've added the change log entry as in #4276. |
ronald-cron-arm
removed
the
needs-reviewer
There was a problem hiding this comment.
Looks good to me. I've verified that most of the commits are from the original PR (thanks for including details about the changes in the commit message!), and that all relevant commits have been backported. I've then review the two new commits independently, and I'm happy with the result.
mpg
added
approved
Description
Fix #4612
Status
READY