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.

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

Move constant-time functions into a separate module #4866

Conversation

gabor-mezei-arm
Copy link
Contributor

@gabor-mezei-arm gabor-mezei-arm commented Aug 11, 2021

Description

Goals (to be done in the order described here, so as to facilitate review):

  • Create a new library source file and add it to the build.
  • For the sake of Mbed OS, the name of the new file must be sufficiently distinctive: Mbed OS's build system does not support two .c files with the same name in different subprojects.
  • Do not expose any of the new functions publicly for now. Declare them only in a header file inside library.
  • Rename existing functions to have a suitable name (mbedtls_<modulename>_xxx). The names should be unique except for functions that have exactly the same prototype and interface.
  • Move existing constant-time code to the new file.
  • Merge functions that have the same (or nearly the same) semantics but different interfaces.
  • Unify the interfaces. For example: decide whether to output 0/1 or 0/~1, to mask uint8_t or size_t.

Resolves #3649

Requires Backporting

Todos

  • Documentation
  • Changelog updated
  • Backported

Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
@gabor-mezei-arm gabor-mezei-arm added enhancement component-crypto Crypto primitives and low-level interfaces labels Aug 11, 2021
@gabor-mezei-arm gabor-mezei-arm self-assigned this Aug 11, 2021
@gabor-mezei-arm gabor-mezei-arm added the needs-ci Needs to pass CI tests label Aug 11, 2021
@gabor-mezei-arm gabor-mezei-arm force-pushed the 3649_move_constant_time_functions_into_separate_module branch from 9d4c409 to ada562b Compare August 11, 2021 15:33
@gabor-mezei-arm gabor-mezei-arm force-pushed the 3649_move_constant_time_functions_into_separate_module branch 4 times, most recently from 5c29d58 to a283f18 Compare August 25, 2021 17:40
@gabor-mezei-arm gabor-mezei-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Aug 25, 2021
@gabor-mezei-arm gabor-mezei-arm force-pushed the 3649_move_constant_time_functions_into_separate_module branch from 3f8e951 to 0d26967 Compare August 26, 2021 08:18
@gabor-mezei-arm gabor-mezei-arm removed the needs-ci Needs to pass CI tests label Aug 30, 2021
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm 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 this PR commit by commit, but since I think f0c2880 needs major rework, I've only made an architectural review for the work done in the subsequent commits. Other than the issue with rsa_private and the recommendation to delay moving base64 functions until after #4814 is fixed, I agree with the general idea of the changes made, I just haven't fully verified that they're correct.

library/base64.c Outdated Show resolved Hide resolved
library/base64.c Outdated
@@ -65,99 +66,6 @@ static const unsigned char base64_dec_map[128] =

#define BASE64_SIZE_T_MAX ( (size_t) -1 ) /* SIZE_T_MAX is not standard */

Copy link
Contributor

Choose a reason for hiding this comment

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

There is another ongoing work on the Base64 constant-time code (#4835, which I started but @tom-daubney-arm has taken over). To facilitate concurrent work on two moderately complex pull requests, I propose to leave the base64 module alone in this pull request. This way, #4835 and the 3.0 version of it can make progress. Once both this PR and the base64 PR are merged, make another simple PR to move the new base64 functions to the new module.

library/rsa.c Outdated Show resolved Hide resolved
ChangeLog.d/constant_time_module.txt Outdated Show resolved Hide resolved
ChangeLog.d/constant_time_module.txt Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Sep 8, 2021
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
Signed-off-by: gabor-mezei-arm <gabor.mezei@arm.com>
@gabor-mezei-arm gabor-mezei-arm removed the needs-review Every commit must be reviewed by at least two team members, label Nov 4, 2021
@gilles-peskine-arm
Copy link
Contributor

pr-merge passed on 77390dc. Travis and pr-head failed only on known issues. So this PR is good to merge once the backport is ready.

@gabor-mezei-arm gabor-mezei-arm added the approved Design and code approved - may be waiting for CI or backports label Nov 22, 2021
@gilles-peskine-arm
Copy link
Contributor

The latest run of pr-merge fails with seemingly relevant errors. Can you please investigate?

@gilles-peskine-arm gilles-peskine-arm added needs-ci Needs to pass CI tests and removed needs-backports Backports are missing or are pending review and approval. approved Design and code approved - may be waiting for CI or backports labels Nov 22, 2021
@davidhorstmann-arm
Copy link
Contributor

davidhorstmann-arm commented Nov 22, 2021

Hmm. The failure seems to be related to a usage of mbedtls_ssl_safer_memcmp() introduced in aa5f5c1 (#4952), after this PR was created (in library/ssl_tls13_generic.c, line 905).
Rebasing or merging then changing the use of mbedtls_ssl_safer_memcmp() to use mbedtls_ct_memcmp() should fix it I believe, although a merge/rebase may not be desirable.

@mpg
Copy link
Contributor

mpg commented Nov 23, 2021

Ok, so that's a conflict (just not the kind that git can notice) so we should handle it like any other conflict and either rebase this branch on current development, or merge current development into this branch, depending on what's most convenient considering this PR's history. Labeling "needs: work" then.

@mpg mpg added the needs-work label Nov 23, 2021
@gilles-peskine-arm
Copy link
Contributor

Given that this PR involves moving code and renaming functions, a rebase would be very painful. So please do a merge, then separate commit(s) to resolve the bugs.

Signed-off-by: Gabor Mezei <gabor.mezei@arm.com>
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@mpg mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 24, 2021
@gabor-mezei-arm gabor-mezei-arm removed the needs-ci Needs to pass CI tests label Nov 24, 2021
@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Nov 24, 2021
@gilles-peskine-arm gilles-peskine-arm merged commit e2d707f into Mbed-TLS:development Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move constant-time functions into a separate module
6 participants