ECDH: Add the Everest X25519 implementation #2073

wintersteiger · 2018-10-09T14:14:52Z

Description

These commits add the formally verified X25519 implementation from Project Everest to ECDH.

Status

Waiting for #1958 to be merged

Additional comments

Passes the tests and performance benchmarks, but only Ubuntu and Windows have been tested thoroughly.
Builds on Extend ECDH interface #1958, which isn't merged yet (yanesca/mbedtls branch kex-into-pk-v5)

CC @fournet @ad-l

Patater · 2018-10-09T16:10:44Z

Hi @wintersteiger

Thanks much for your contribution. This is the first third-party code we are adding to Mbed TLS, so we appreciate your patience so far in determining how best to integrate.

I've looked at the code at a glance and can recommend a few changes straight away. We'll need to take the time to review in further detail, but in the mean time could you please, before additional reviewers spend too much time on this:

Follow our commit style guidelines at https://os.mbed.com/docs/latest/reference/workflow.html#guidelines-for-github-pull-requests ?
- Use imperative mood
- Keep subject lines to 50 characters or so
- Don't merge from upstream into your PR branch. This makes reviewing more difficult and ties the git history in knots. I'd recommend git cherry-picking your commits atop the latest Extend ECDH interface #1958 branch.
- Please justify why the change a commit makes is necessary. For example, explain the motivation behind Added filter for #ifdefs in tests/scripts/list-enum-consts.pl in the git commit message.

Thanks again!

wintersteiger · 2018-10-10T09:55:11Z

@Patater That all sounds good. Is it okay to rebase and force-push or do you want to keep the whole history?

Patater · 2018-10-10T13:39:24Z

@wintersteiger Please link to the original branch in a comment and force push to this PR. You can see an example of this at #2007 (comment)

Thanks!

wintersteiger · 2018-10-25T12:39:41Z

Rebased; previous branch at https://github.com/project-everest/mbedtls/tree/everest-into-ecdh-old1.

This is still based on the unmerged #1958, those commits have been reviewed before, so those commits don't need reviewing.

yanesca

I have had a look at it and left a couple of comments. I also have a couple of remarks and questions that are not related to individual lines and therefore I am writing them here:

@Patater mentioned that we would like to have the commit titles in imperative mood. For example instead of "Added minimized Everest Curve25519 code to 3rdparty/everest." we would like to have "Add minimized Everest Curve25519 code to 3rdparty/everest."
The commit message in "Added minimized Everest Curve25519 code to 3rdparty/everest." says that these files are automatically generated by the Everest toolchain. Are all of these files generated? Aren't some of these part of KreMLin? (for example "3rdparty/everest/include/everest/kremlin/c_endianness.h").
FStar_UInt128_extracted.c is not added to CMake and a comment says that we need to pass -DKRML_VERIFIED_UINT128 to use it. How does it work, to which program should I pass that flag to use it?
There is a separate implementation for VS2010, I remember talking about it, but don't remember why do we need it and how it is different than the generic one. Could you please refresh my memory?

3rdparty/everest/include/everest/everest.h

3rdparty/everest/library/everest.c

3rdparty/everest/library/x25519.c

3rdparty/everest/library/everest.c

programs/test/benchmark.c

library/ecdh.c

library/ecdsa.c

library/ecp.c

visualc/VS2010/aescrypt2.vcxproj

wintersteiger · 2018-12-06T19:09:06Z

Rebased, previous branch at https://github.com/project-everest/mbedtls/tree/everest-into-ecdh-old2.

yanesca · 2018-12-11T14:22:54Z

Hi @wintersteiger, thank you very much for your answers and rework, I'll have a look at them at once!

Meanwhile, could you please do another rebase on current development head? #1958 has been merged recently and would make the PR cleaner and the job of other reviewers easier.

Also could you please update all of your commit message subject lines to

use imperative mood
be limited to 50 characters
not have period at the end?

For example:

- ECDH: Improves ECDH full handshake benchmark.
+ ECDH: Improve ECDH full handshake benchmark

wintersteiger · 2018-12-12T13:05:28Z

@yanesca I forgot to answer your 2. and 3.: Visual Studio 2010 does not support uint128_t, so we add our own implementation in FStar_UInt128_extracted.c. This is a fully verified implementation of uint128 operations, but it's slow and needs to be enabled with the pre-processor macro -DKRML_VERIFIED_UINT128.

yanesca · 2018-12-12T14:16:34Z

@wintersteiger thank you for your answer! Just to be sure I understand it correctly: I need to add -DKRML_VERIFIED_UINT128 to the preprocessor flags if and only if I build the library in VS2010. This is the only change I need to make (after setting MBEDTLS_ECDH_VARIANT_EVEREST_ENABLED in config.h) and the library will build in VS2010. Is this correct?

wintersteiger · 2018-12-12T14:38:27Z

@yanesca No, you don't need to add anything. I just forgot to add that definition to the VS project template in scripts/data_files; it's there now.

gilles-peskine-arm

I made a first pass of review. I looked at a build with default options and at the changes to existing files.

include/mbedtls/config.h

include/mbedtls/ecdh.h

gilles-peskine-arm · 2018-12-13T18:00:21Z

3rdparty/everest/library/kremlib/fstar_uint128.c

+
+/* GCC + using native unsigned __int128 support */
+
+uint128_t load128_le(uint8_t *b) {


And check-names.sh complains about it, for good reason because it isn't namespaced. If it's static then it doesn't need to be namespaced.

3rdparty/everest/include/everest/Hacl_Curve25519.h

include/mbedtls/config.h

gilles-peskine-arm · 2018-12-13T18:50:50Z

3rdparty/everest/library/kremlib/fstar_uint128.c

+
+/* GCC + using native unsigned __int128 support */
+
+uint128_t load128_le(uint8_t *b) {


Also GCC and Clang complain about it with -Wmissing-prototypes and we support a clean build with -Wmissing-prototypes.

library/ecp.c

library/ecdsa.c

library/ecdh.c

gilles-peskine-arm · 2018-12-13T19:05:53Z

library/Makefile

@@ -86,6 +89,13 @@ OBJS_CRYPTO=	aes.o		aesni.o		arc4.o		\
 		threading.o	timing.o	version.o	\
 		version_features.o		xtea.o

+OBJS_CRYPTO+= \


When building without Everest support, I'd prefer to avoid building these objects at all.

wintersteiger · 2018-12-14T10:54:50Z

Rebased; old branch is at https://github.com/project-everest/mbedtls/tree/everest-into-ecdh-old3

I'm now going through @gilles-peskine-arm comments/requests.

Mbed-TLS#2124 may suffer from the same problem.

Test a native build and a 32-bit build. For variety, the native build is with CMake and clang, and the 32-bit build is with GNU make and gcc.

wintersteiger · 2019-04-15T12:55:43Z

All fixed!

The build failures were due to changes in benchmark.c relating to platform.h that I copied over from #2124. I removed some of the #defines so that it passes with both config.hs, but I can't guarantee that this works everywhere.

I still get some failures from ssl-opt.sh and compat.sh, but I get similar ones on the development branch. So, not sure those are related to the changes in this PR.

yanesca · 2019-04-15T13:10:53Z

I was referring to 92bd50e because those changes were necessary to pass the new tests.

gilles-peskine-arm · 2019-04-15T15:18:29Z

You're probably getting failures in ssl-opt.sh and compat.sh because you don't have all the necessary versions of OpenSSL and GnuTLS (some tests with deprecated algorithms require old versions, some tests with newer features require recent versions). They're passing on the CI, so that's fine.

The remaining failures are in jobs that use the crypto submodule. We're in the process of separating the crypto part of the library from the x509+tls parts. @Patater Assuming that Janos and I approve this PR soon, what's the merge strategy here?

gilles-peskine-arm

Approved!

There are minor style issues. It would be nice if you could fix them, but they're minor enough that I don't mind merging the code as is.

gilles-peskine-arm · 2019-04-16T09:49:19Z

library/ecdh.c

+{
+    /* At this time, all groups support ECDH. */
+    (void) gid;
+    return 1;


Style (here and elsewhere):

Suggested change

return 1;

return( 1 );

No problem, those are easy to fix. 3487b9c

gilles-peskine-arm · 2019-04-16T09:50:18Z

library/ecdsa.c

@@ -263,7 +263,7 @@ static int ecdsa_sign_restartable( mbedtls_ecp_group *grp,
    mbedtls_mpi *pk = &k, *pr = r;

    /* Fail cleanly on curves such as Curve25519 that can't be used for ECDSA */
-    if( grp->N.p == NULL )
+    if( !mbedtls_ecdsa_can_do( grp->id ) || grp->N.p == NULL )


Style (here and elsewhere):

Suggested change

if( !mbedtls_ecdsa_can_do( grp->id ) || grp->N.p == NULL )

if( ! mbedtls_ecdsa_can_do( grp->id ) || grp->N.p == NULL )

gilles-peskine-arm · 2019-04-16T11:34:39Z

CMakeLists.txt

@@ -168,6 +168,8 @@ else()
    set(LIB_INSTALL_DIR lib)
 endif()

+include_directories(include/)


@Patater Do you see any incompatibility between this line and getting the proper include path with the crypto submodule?

Yes, this shouldn't be necessary to add. Maybe this was mistakenly readded during a rebase.

See 4cb814e for why we don't want this line anymore. We used to have this line, but removed it for submodule support.

I need include/CMakeLists.txt to be included so I can see the INSTALL_MBEDTLS_HEADERS option in 3rdparty/*. It definitely doesn't work without that line, but I've rearranged it make this a bit cleaner. Not sure what your strategy going forward is for this, so feel free to request changes to that. See 16bdf8c.

gilles-peskine-arm · 2019-04-16T11:42:54Z

Apart from the build scripts, we've agreed on the design for a while. Since the build scripts work on the CI, we can take their design as fundamentally sound. So I'm removing the "needs design approval" label.

Patater · 2019-04-16T12:30:05Z

what's the merge strategy here?

The merge strategy is test this PR in CI (including a trial merge to Mbed Crypto), merge to Mbed TLS if all goes well. We'll then merge from TLS development to Mbed Crypto development, to get things up to date there as well. Any issues with build scripts not being synchronized we can address as they come up.

yanesca

Looks good to me.

wintersteiger · 2019-05-01T14:27:44Z

Just checking; is there anything missing from our side for this to progress to the next stage?

yanesca · 2019-05-01T15:43:35Z

No, it is all on us now. Thank you for checking!

Patater · 2019-08-30T14:37:14Z

Closing in favor of #2799

RonEld added enhancement CLA valid component-crypto Crypto primitives and low-level interfaces labels Oct 9, 2018

yanesca requested review from Patater and yanesca October 9, 2018 15:36

yanesca added needs-design-approval needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Oct 9, 2018

yanesca requested a review from gilles-peskine-arm October 9, 2018 15:51

Patater added the component-platform Portability layer and build scripts label Oct 9, 2018

wintersteiger force-pushed the everest-into-ecdh branch 3 times, most recently from d0917b6 to af516f8 Compare October 25, 2018 12:39

yanesca mentioned this pull request Nov 6, 2018

Extend ECDH interface #1958

Merged

yanesca reviewed Nov 20, 2018

View reviewed changes

LLE8 mentioned this pull request Nov 26, 2018

OpenWrt packages PurpleI2P/i2pd#345

Closed

wintersteiger force-pushed the everest-into-ecdh branch from 5a51f6e to 046e4c6 Compare December 6, 2018 19:08

gilles-peskine-arm requested changes Dec 13, 2018

View reviewed changes

wintersteiger force-pushed the everest-into-ecdh branch from 2de9896 to 1722d83 Compare December 14, 2018 10:54

Christoph M. Wintersteiger and others added 4 commits April 15, 2019 11:09

Fix macros in benchmark.c

a4fd9f2

Mbed-TLS#2124 may suffer from the same problem.

3rdparty: Added config checks for Everest

9e1fe28

3rdparty: Fix Everest's mbedtls_x25519_get_params

92bd50e

Add Everest components to all.sh

8390fc9

Test a native build and a 32-bit build. For variety, the native build is with CMake and clang, and the 32-bit build is with GNU make and gcc.

gilles-peskine-arm previously approved these changes Apr 16, 2019

View reviewed changes

gilles-peskine-arm removed the needs-design-approval label Apr 16, 2019

Fix code style

3487b9c

wintersteiger dismissed gilles-peskine-arm’s stale review via 3487b9c April 16, 2019 11:57

gilles-peskine-arm previously approved these changes Apr 16, 2019

View reviewed changes

yanesca previously approved these changes Apr 16, 2019

View reviewed changes

yanesca removed the needs-review Every commit must be reviewed by at least two team members, label Apr 16, 2019

3rdparty: Fix include directories

16bdf8c

wintersteiger dismissed stale reviews from yanesca and gilles-peskine-arm via 16bdf8c April 16, 2019 13:35

gilles-peskine-arm approved these changes Apr 16, 2019

View reviewed changes

yanesca approved these changes Apr 17, 2019

View reviewed changes

yanesca mentioned this pull request Jun 5, 2019

Everest integration ARMmbed/mbed-crypto#140

Merged

yanesca requested a review from gilles-peskine-arm August 20, 2019 11:49

yanesca mentioned this pull request Aug 20, 2019

Everest Mbed TLS integration #2799

Merged

yanesca removed the request for review from gilles-peskine-arm August 20, 2019 15:13

Patater closed this Aug 30, 2019

gilles-peskine-arm mentioned this pull request Nov 2, 2023

License change #8462

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECDH: Add the Everest X25519 implementation #2073

ECDH: Add the Everest X25519 implementation #2073

wintersteiger commented Oct 9, 2018

Patater commented Oct 9, 2018

wintersteiger commented Oct 10, 2018

Patater commented Oct 10, 2018

wintersteiger commented Oct 25, 2018

yanesca left a comment

wintersteiger commented Dec 6, 2018

yanesca commented Dec 11, 2018

wintersteiger commented Dec 12, 2018

yanesca commented Dec 12, 2018

wintersteiger commented Dec 12, 2018

gilles-peskine-arm left a comment

gilles-peskine-arm Dec 13, 2018

gilles-peskine-arm Dec 13, 2018

gilles-peskine-arm Dec 13, 2018

wintersteiger commented Dec 14, 2018

wintersteiger commented Apr 15, 2019

yanesca commented Apr 15, 2019

gilles-peskine-arm commented Apr 15, 2019

gilles-peskine-arm left a comment

gilles-peskine-arm Apr 16, 2019

wintersteiger Apr 16, 2019

gilles-peskine-arm Apr 16, 2019

wintersteiger Apr 16, 2019

gilles-peskine-arm Apr 16, 2019

Patater Apr 16, 2019 •

edited

Loading

wintersteiger Apr 16, 2019

gilles-peskine-arm commented Apr 16, 2019

Patater commented Apr 16, 2019

yanesca left a comment

wintersteiger commented May 1, 2019

yanesca commented May 1, 2019

Patater commented Aug 30, 2019


		/* GCC + using native unsigned __int128 support */

		uint128_t load128_le(uint8_t *b) {

	if( !mbedtls_ecdsa_can_do( grp->id ) \|\| grp->N.p == NULL )
	if( ! mbedtls_ecdsa_can_do( grp->id ) \|\| grp->N.p == NULL )

ECDH: Add the Everest X25519 implementation #2073

ECDH: Add the Everest X25519 implementation #2073

Conversation

wintersteiger commented Oct 9, 2018

Description

Status

Additional comments

Patater commented Oct 9, 2018

wintersteiger commented Oct 10, 2018

Patater commented Oct 10, 2018

wintersteiger commented Oct 25, 2018

yanesca left a comment

Choose a reason for hiding this comment

wintersteiger commented Dec 6, 2018

yanesca commented Dec 11, 2018

wintersteiger commented Dec 12, 2018

yanesca commented Dec 12, 2018

wintersteiger commented Dec 12, 2018

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wintersteiger commented Dec 14, 2018

wintersteiger commented Apr 15, 2019

yanesca commented Apr 15, 2019

gilles-peskine-arm commented Apr 15, 2019

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Apr 16, 2019

Patater commented Apr 16, 2019

yanesca left a comment

Choose a reason for hiding this comment

wintersteiger commented May 1, 2019

yanesca commented May 1, 2019

Patater commented Aug 30, 2019

Patater Apr 16, 2019 •

edited

Loading