Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Backport 2.x: Fix non-uniform random generation in a range #4276

Merged
merged 48 commits into from
Jun 4, 2021
Merged

Backport 2.x: Fix non-uniform random generation in a range #4276

merged 48 commits into from
Jun 4, 2021

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Mar 31, 2021 •
edited
Loading

  1. Refactor mbedtls_ecp_gen_privkey and extract the part that generates a random number uniformly in a range [2, P-1].
  2. Fix Montgomery private key generation to not reject 0. This doesn't make a functional difference since the chance of generating 0 is negligible, but it removes an opportunity for a minor side channel that could disclose how many leading null bits the secret key has.
  3. Create a new function mbedtls_mpi_random to generate a random number uniformly in a range [m, P-1] with m small.
  4. Refactor some duplicated code in dhm.c.
  5. Use mbedtls_mpi_random in places where a random number was generated in a non-uniform way (key generation and blinding in both ECP and DHM). Fix [BUG] Bias in private key generation in Diffie-Hellman #4245.

Needs backports: yes, the non-uniform generation should be fixed in 2.16. However, the refactorings in this pull request are far too invasive for an LTS branch, so we need to figure out a more targeted fix in 2.16.

Given the complexity of this PR, I'll make the 3.0 forward-port once it has two approvals.

There is a conflict with #4297 because both introduce library/ecp_invasive.h. Since #4297 fixes newly-introduced bugs, it should go in first, then this PR should be rebased. The conflict is trivial so this PR can be reviewed even before the rebase.

@gilles-peskine-arm gilles-peskine-arm added bug mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. component-crypto Crypto primitives and low-level interfaces needs-reviewer This PR needs someone to pick it up for review labels Mar 31, 2021
This was referenced Mar 31, 2021
Closed
Open
@gilles-peskine-arm gilles-peskine-arm force-pushed the random-range-uniformity branch 2 times, most recently from 50fe56a to f8b66e3 Compare April 1, 2021 22:08
@gilles-peskine-arm gilles-peskine-arm force-pushed the random-range-uniformity branch from f8b66e3 to 79d0b53 Compare April 3, 2021 18:41
@gilles-peskine-arm gilles-peskine-arm added the needs-preceding-pr Requires another PR to be merged first label Apr 3, 2021
@mpg mpg self-requested a review April 8, 2021 09:58
@gilles-peskine-arm gilles-peskine-arm force-pushed the random-range-uniformity branch from 79d0b53 to 5201c52 Compare April 9, 2021 20:24
@gilles-peskine-arm
Copy link
Contributor Author

I have rebased this pull request due to add/add conflicts with #4297.

@mpg
Copy link
Contributor

mpg commented Apr 12, 2021

The prerequisite PR #4297 has now been merged, removing the label and changing the base branche twice in order to force-refresh github's cache.

@mpg mpg removed the needs-preceding-pr Requires another PR to be merged first label Apr 12, 2021
@mpg mpg changed the base branch from development to development_3.0 April 12, 2021 08:30
@mpg mpg changed the base branch from development_3.0 to development April 12, 2021 08:31
mpg
mpg requested changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good overall but I have a number of comments, mainly about readability / internal documentation, plus what looks like to typos in test data.

Thank you very much for cleanly isolating "moving code" in specific commits that do just that, it makes those commits so much easier to review with git show --color-moved --color-moved-ws=allow-indentation-change <commit> (tip for the other reviewer).

@mpg mpg added the needs-work label Apr 13, 2021
@gilles-peskine-arm gilles-peskine-arm requested a review from mpg April 13, 2021 20:30
@gilles-peskine-arm gilles-peskine-arm force-pushed the random-range-uniformity branch from 1893e5b to ee340d9 Compare June 2, 2021 20:28
@gilles-peskine-arm
New internal function mbedtls_mpi_resize_clear
3130ce2 
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]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the random-range-uniformity branch from ee340d9 to 3130ce2 Compare June 2, 2021 21:48
mpg
mpg previously approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the changes (rebases and new commits) since my previous approval, and this looks good to me.

@gilles-peskine-arm gilles-peskine-arm requested a review from mpg June 3, 2021 09:52
mpg
mpg approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ronald-cron-arm
ronald-cron-arm approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments. This looks good to me as well now.

This was referenced Jun 3, 2021
Merged
Closed
@mpg mpg removed the needs-review Every commit must be reviewed by at least two team members, label Jun 4, 2021
@mpg
Copy link
Contributor

mpg commented Jun 4, 2021

The need for a 2.16 backport is tracked in #4612, so as discussed this can be merged before without waiting for the 2.16 backport. (The 3.0 forward port is #4611)

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-backports Backports are missing or are pending review and approval. labels Jun 4, 2021
@mpg mpg merged commit 62da8ac into Mbed-TLS:development_2.x Jun 4, 2021
@ronald-cron-arm ronald-cron-arm mentioned this pull request Jun 10, 2021
Merged
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Jun 10, 2021
Closed
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jun 11, 2021
@gilles-peskine-arm
Changelog entry for the mbedtls_mpi_read_xxx changes
9ede892 
mbedtls_mpi_read_binary{,_le} (in Mbed-TLS#4276)
and mbedtls_mpi_read_string (in Mbed-TLS#4644)
changed their behavior on an empty input from constructing an MPI object with
one limb to not allocating a limb. In principle, this change should be
transparent to applications, however it caused a bug in the library and it does
affect the value when writing back out, so list the change in the changelog.

Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jun 11, 2021
@gilles-peskine-arm
Changelog entry for the mbedtls_mpi_read_xxx changes
34de0fb 
mbedtls_mpi_read_binary{,_le} (in Mbed-TLS#4276)
and mbedtls_mpi_read_string (in Mbed-TLS#4644)
changed their behavior on an empty input from constructing an MPI object with
one limb to not allocating a limb. In principle, this change should be
transparent to applications, however it caused a bug in the library and it does
affect the value when writing back out, so list the change in the changelog.

Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jun 11, 2021
@gilles-peskine-arm
Changelog entry for the mbedtls_mpi_read_xxx changes
0a2a2f9 
mbedtls_mpi_read_binary{,_le} (in Mbed-TLS#4276)
and mbedtls_mpi_read_string (in Mbed-TLS#4644)
changed their behavior on an empty input from constructing an MPI object with
one limb to not allocating a limb. In principle, this change should be
transparent to applications, however it caused a bug in the library and it does
affect the value when writing back out, so list the change in the changelog.

Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jun 22, 2021
@gilles-peskine-arm
Changelog entry for the mbedtls_mpi_read_xxx changes
c513934 
mbedtls_mpi_read_binary{,_le} (in Mbed-TLS#4276)
and mbedtls_mpi_read_string (in Mbed-TLS#4644)
changed their behavior on an empty input from constructing an MPI object with
one limb to not allocating a limb. In principle, this change should be
transparent to applications, however it caused a bug in the library and it does
affect the value when writing back out, so list the change in the changelog.

Signed-off-by: Gilles Peskine <[email protected]>
gilles-peskine-arm added a commit to gilles-peskine-arm/mbedtls that referenced this pull request Jun 22, 2021
@gilles-peskine-arm
Changelog entry for the mbedtls_mpi_read_xxx changes
34bfa15 
mbedtls_mpi_read_binary{,_le} (in Mbed-TLS#4276)
and mbedtls_mpi_read_string (in Mbed-TLS#4644)
changed their behavior on an empty input from constructing an MPI object with
one limb to not allocating a limb. In principle, this change should be
transparent to applications, however it caused a bug in the library and it does
affect the value when writing back out, so list the change in the changelog.

Signed-off-by: Gilles Peskine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Bias in private key generation in Diffie-Hellman
5 participants
@gilles-peskine-arm @mpg @ronald-cron-arm @yanesca @bensze01