Fix non-uniform random generation in a range #4611
Merged
Conversation
This file contains 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
Prepare to isolate the Montgomery and short Weierstrass implementations of mbedtls_ecp_gen_privkey into their own function. Signed-off-by: Gilles Peskine <[email protected]>
Put the Montgomery and short Weierstrass implementations of mbedtls_ecp_gen_privkey into their own function which can be tested independently, but will not be part of the public ABI/API. Signed-off-by: Gilles Peskine <[email protected]>
For Montgomery keys, n_bits is actually the position of the highest bit and not the number of bits, which would be 1 more (fence vs posts). Rename the variable accordingly to lessen the confusion. No semantic change. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
If a fallback is not explicitly configured in the mbedtls_test_rnd_buf_info structure, fail after the buffer is exhausted. There is no intended behavior change in this commit: all existing uses of mbedtls_test_rnd_buffer_rand() have been updated to set mbedtls_test_rnd_std_rand as the fallback. Signed-off-by: Gilles Peskine <[email protected]>
Test the exact output from known RNG input. This is overly constraining, but ensures that the code has good properties. Signed-off-by: Gilles Peskine <[email protected]>
The library rejected an RNG input of all-bits-zero, which led to the
key 2^{254} (for Curve25519) having a 31/32 chance of being generated
compared to other keys. This had no practical impact because the
probability of non-compliance was 2^{-256}, but needlessly
complicated the code.
The exception was added in 98e28a7 to
avoid the case where b - 1 wraps because b is 0. Instead, change the
comparison code to avoid calculating b - 1.
Signed-off-by: Gilles Peskine <[email protected]>
Don't calculate the bit-size of the initially generated random number. This is not necessary to reach the desired distribution of private keys, and creates a (tiny) side channel opportunity. This changes the way the result is derived from the random number, but does not affect the resulting distribution. Signed-off-by: Gilles Peskine <[email protected]>
mbedtls_ecp_gen_privkey_mx generates a random number with a certain top bit set. Depending on the size, it would either generate a number with that top bit being random, then forcibly set the top bit to 1 (when high_bit is not a multiple of 8); or generate a number with that top bit being 0, then set the top bit to 1 (when high_bit is a multiple of 8). Change it to always generate the top bit randomly first. This doesn't make any difference in practice: the probability distribution is the same either way, and no supported or plausible curve has a size of the form 8n+1 anyway. But it slightly simplifies reasoning about the behavior of this function. Signed-off-by: Gilles Peskine <[email protected]>
Add unit tests for private key generation on short Weierstrass curves. These tests validate that the result is within the desired range. Additionally, they validate that after performing many iterations, the range is covered to an acceptable extent: for tiny ranges, all values must be reached; for larger ranges, all value bits must reach both 0 and 1. Signed-off-by: Gilles Peskine <[email protected]>
Rename mbedtls_ecp_gen_privkey_sw to mbedtls_mpi_random since it has no particular connection to elliptic curves beyond the fact that its operation is defined by the deterministic ECDSA specification. This is a generic function that generates a random MPI between 1 inclusive and N exclusive. Slightly generalize the function to accept a different lower bound, which adds a negligible amount of complexity. Signed-off-by: Gilles Peskine <[email protected]>
Since mbedtls_mpi_random() is not specific to ECC code, move it from the ECP module to the bignum module. This increases the code size in builds without short Weierstrass curves (including builds without ECC at all) that do not optimize out unused functions. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Instead of generating blinding values in a not-quite-uniform way (Mbed-TLS#4245) with copy-pasted code, use mbedtls_mpi_random(). Signed-off-by: Gilles Peskine <[email protected]>
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. Signed-off-by: Gilles Peskine <[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. Signed-off-by: Gilles Peskine <[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. 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]>
Instead of generating blinding values and keys in a not-quite-uniform way (Mbed-TLS#4245) with copy-pasted code, use mbedtls_mpi_random(). Signed-off-by: Gilles Peskine <[email protected]>
Fix Mbed-TLS#4245. Signed-off-by: Gilles Peskine <[email protected]>
mbedtls_mpi_random() uses mbedtls_mpi_cmp_mpi_ct(), which requires its two arguments to have the same storage size. This was not the case when the upper bound passed to mbedtls_mpi_random() had leading zero limbs. Fix this by forcing the result MPI to the desired size. Since this is not what mbedtls_mpi_fill_random() does, don't call it from mbedtls_mpi_random(), but instead call a new auxiliary function. Add tests to cover this and other conditions with varying sizes for the two arguments. Signed-off-by: Gilles Peskine <[email protected]>
Add unit tests for mbedtls_mpi_fill_random() and mbedtls_mpi_random() when the resulting MPI object previously had a nonzero value. I wrote those to catch a bug that I introduced during the development of mbedtls_mpi_random() (but does not appear in a committed version). Signed-off-by: Gilles Peskine <[email protected]>
Slightly simplify is_significantly_above_a_power_of_2() to make it easier to understand: * Remove the explicit negative answer for x <= 4. The only functional difference this makes is that is_significantly_above_a_power_of_2(3) is now true. * Shift the most significant bit of x to position 8 rather than 15. This makes the final comparison easier to explain. Signed-off-by: Gilles Peskine <[email protected]>
This comment is no longer in the specific context of generating a random point on an elliptic curve. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
The idiom "resize an mpi to a given size" appeared 4 times. Unify it in a single function. Guarantee that the value is set to 0, which is required by some of the callers and not a significant expense where not required. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm
added
bug
needs-review
mpg
approved these changes
Jun 4, 2021
|
I think as straightforward port, this only requires a single approval. |
mpg
added
single-reviewer
mpg
added
approved
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.
3.0 version of #4276. This is a straightforward rebase.