Add Edwards curves support to Mbed TLS #3245

aurel32 · 2020-04-26T21:52:04Z

This PR adds support for Edwards and Twisted Edwards curves to Mbed TLS, in order to eventually support the EdDSA algorithm with Ed25519 and Ed448 curves. It only contains the Edwards curve arithmetic and reading/writing points in binary format, as discussed in #2452. It is however enough to implement EdDSA public key generation, message signing and message verification in an external program linked to Mbed TLS. Despite the goal of adding the EdDSA algorithm, care has been taken to implement generic Edwards curve arithmetic and not use specific optimizations found in the RFC.

Tests for the Edwards arithmetic have been added using the test vector from the RFC. I guess additional tests for reading/writing points in binary format are needed.

Please refer to the individual commit descriptions which (I hope) describe things more in details. Some of them has a number of FIXME entries (some of them replicated in the code) that should be solved before this PR can be merged. They require a discussion followed by a decision about how to implement certain things, especially when they impact the API. I'll start the discussion for those FIXME on the mailing list in the next days.

Status

IN DEVELOPMENT

Requires Backporting

NO

Migrations

YES, there is an API change due to the need to export a point addition function. More API changes might be need following the discussion about how to solve a few remaining issues.

Additional comments

Note that I am NOT an expert in cryptography. I have tried to follow the best practice found elsewhere in Mbed TLS to avoid leaks via side-channels, but I haven't done any extensive analysis to prevent those (and I don't know how to do that).

Todos

Tests
Documentation
Changelog updated

aurel32 · 2020-04-26T22:35:35Z

I have just done a force push to fix the issue reported by Travis.

Largely inspired by the curve25519 definition. Signed-off-by: Aurelien Jarno <[email protected]>

Largely inspired by the curve448 definition. Signed-off-by: Aurelien Jarno <[email protected]>

Also tweak mbedtls_ecp_get_type to handle the edwards case. FIXME: there is probably a cleaner way of doing that. Signed-off-by: Aurelien Jarno <[email protected]>

The RFC8032 recommends to implement ed25519 in extended coordinates and ed448 in projective coordinates. The two coordinates systems are very close with T=x*y for extended coordinates. The corresponding point addition and point doubling formulas are actually very similar. Note that the formulas given in the RFC are the EFD one simplified with a = -1 for ed25519 and a = 1 for ed448. The Mbed TLS philosophy is to share as much code as possible, so projective coordinates are used for both Twisted Edwards and Edwards curves, using the formulas from EFD, which do include the a parameter of the quation. This also matches the mbedtls_ecp_point implementation which supports up to three coordinates. In order to verify EdDSA signatures, it is necessary to add two points. A new mbedtls_ecp_add function is therefore exported in the API. It is only implemented for Edwards curve and returns MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE for other curves. The ecp_check_pubkey function simply checks that the points is on the curve as there are no recommended checks in the RFC8032 nor in the FIPS 186-5 draft. FIXME: There is currently no check for mbedtls_ecp_check_privkey. It's not clear what check should be implemented. FIXME: mbedtls_ecp_set_zero and mbedtls_ecp_is_zero are wrong for Edwards curve, the neutral element being (0, 1). This is not possible to fix that without changing the API as the group parameter is currently not in argument. FIXME: ecp_normalize_edxyz uses a modular inversion. For elliptic curves over GF(p) modular inversions could be done using x^-1 = x^(p-2) (mod p). As it doesn't seem to bring any measureable speed improvement, I kept the modular inversion. FIXME: The mbedtls_ecp_keypair type and the related functions (mbedtls_ecp_gen_key, mbedtls_ecp_read_key, mbedtls_ecp_check_pub_priv) assume that the secret key is a scalar and that the public key is a curve point that is obtained by the scalar multiplication of the base point by the secret key. This is only partially true for the EdDSA algorithm, which employs point enconding and hashes to manipulate those. Signed-off-by: Aurelien Jarno <[email protected]>

Those tests are extracted from RFC8032 sec 7 and correspond to the unique SECRET KEY and PUBLIC KEY tuples in the Ed25519 and Ed448 test vectors. The eddsa2 Python library defined from Appendix A is used to expand the SECRET KEY entries to a scalar value and to decode the PUBLIC KEY entries to a point value. Signed-off-by: Aurelien Jarno <[email protected]>

Note that decoding a point for curves over prime field p with p = 1 (mod 8) is not supported. This is not the case of Ed25519 and Ed448. Supporting that would require a more complex algorithm to compute the square root, for instance the Tonelli-Shanks algorithm. Note that the square root formulas come from the RFC Erratas 5758 and 5759. FIXME: mbedtls_ecp_point_read_binary_ed() has to be defined after the mbedtls_mpi_xxx_mod functions as some computation are needed to read a point. Currently mbedtls_ecp_point_read_binary() is defined before those functions, so a forward declaration is needed. This should probably be fixed by reshuffling functions in ecp.c. Signed-off-by: Aurelien Jarno <[email protected]>

gilles-peskine-arm

Thank you for working on this! I've made a first review pass for the general structure and the interface changes. I haven't reviewed the calculations yet.

Beyond my individual comments, and the need to fill in the FIXME:

Please add references to the existing comment near the top of ecp.c.
Missing support for ECP_EDWARDS in mbedtls_ecp_gen_privkey, mbedtls_ecp_read_key.
More test cases are needed: at least one test per curve per function that has code specific to ECP_EDWARDS; test cases for mbedtls_ecp_add; bad-data test cases.

gilles-peskine-arm · 2020-05-25T17:39:40Z

include/mbedtls/config.h

@@ -769,6 +769,8 @@
 #define MBEDTLS_ECP_DP_BP512R1_ENABLED
 #define MBEDTLS_ECP_DP_CURVE25519_ENABLED
 #define MBEDTLS_ECP_DP_CURVE448_ENABLED
+#define MBEDTLS_ECP_DP_ED25519_ENABLED
+#define MBEDTLS_ECP_DP_ED448_ENABLED


Please add the new symbols to the requirements for MBEDTLS_ECP_C in check_config.h.

gilles-peskine-arm · 2020-05-25T17:43:13Z

library/ecp.c


 cleanup:
    return( ret );
 }

+#if defined(ECP_EDWARDS)
+/* FIXME: The function is defined later in this file as reading a binary


Please avoid forward declarations. Define the function here.

gilles-peskine-arm · 2020-05-25T17:50:14Z

library/ecp.c

@@ -523,6 +528,9 @@ mbedtls_ecp_curve_type mbedtls_ecp_get_type( const mbedtls_ecp_group *grp )

    if( grp->G.Y.p == NULL )
        return( MBEDTLS_ECP_TYPE_MONTGOMERY );
+    /* FIXME: there is probably a cleaner way of doing that. */


Checking the id actually seems sensible to me, and less fragile than the way Montgomery curves are currently identified.

Yes, as discussed on the mailing-list, I think you can remove this comment.

While at it, if you want, you could change the grp->G.Y.p == NULL above to a similar test on the group ID, and then remove the corresponding comment This is used as a marker to identify Montgomery curves! in ecp_use_curve25519 and ecp_use_curve448.

gilles-peskine-arm · 2020-05-25T17:53:01Z

include/mbedtls/ecp.h

@@ -831,6 +834,28 @@ int mbedtls_ecp_tls_write_group( const mbedtls_ecp_group *grp,
                                 size_t *olen,
                                 unsigned char *buf, size_t blen );

+/**
+ * \brief           This function performs a point addition: \p R = \p P + \p Q.


Please document that this function only works on Edwards curves.

See also #3245 (comment) for why I think we might be better off not adding this function at all.

gilles-peskine-arm · 2020-05-26T08:31:11Z

library/ecp.c

+            }
+
+            /* x *= 2^((p-1)/4) */
+            MBEDTLS_MPI_CHK( mbedtls_mpi_read_string( &t, 16, "2B8324804FC1DF0B2B4D00993DFBD7A72F431806AD2FE478C4EE1B274A0EA0B0" ) );


I know this is a preexisting problem in ecp_curves.c, but we shouldn't embed hexadecimal data in the executable. They consume twice as much code size as they should, and they're converted on each use. Most of the code in ecp_curves.c defines constants that are used as bignums directly: see ecp_mpi_load and the constants defined for NIST curves (secp192r1_p, etc.). Please use a similar mechanism here. Maybe define a constant in ecp_curves.c and declare it in ecp_internal.h.

@gilles-peskine-arm wouldn't be wise to start breaking things done i.e per family before adding even more? yet instead of having static function literally flying everywhere. This is creating a touchy situation with very difficult files to maintain and monitor; especially when bringing new folks on board.

gilles-peskine-arm · 2020-05-26T17:47:27Z

library/ecp.c

@@ -791,11 +799,44 @@ int mbedtls_ecp_point_write_binary( const mbedtls_ecp_group *grp,
        }
    }
 #endif
+#if defined(ECP_EDWARDS)
+    if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_EDWARDS )


Preexisting, but it may be time to change it: this should be switch( mbedtls_ecp_get_type( grp ) ). I think it would make the code more readable, and also marginally more efficient (I don't think a compiler can see that the value of mbedtls_ecp_get_type( grp ) doesn't change).

Applies to other functions as well.

gilles-peskine-arm · 2020-05-26T17:49:51Z

library/ecp.c

+    if( mbedtls_ecp_get_type( grp ) == MBEDTLS_ECP_TYPE_EDWARDS )
+    {
+        /* Only the compressed format is defined for Edwards curves. */
+        ECP_VALIDATE_RET( format == MBEDTLS_ECP_PF_COMPRESSED );


Note that VALIDATE macros such as ECP_VALIDATE_RET expand to nothing by default. They're for extra protection against cases that can't happen unless something already went wrong before, not for ordinary checks of invalid arguments. So this should be

if( format != MBEDTLS_ECP_PF_COMPRESSED ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA );

On a related note does the Montgomery case need this check as well?

gilles-peskine-arm · 2020-05-26T17:50:40Z

library/ecp.c

+            return( MBEDTLS_ERR_ECP_BUFFER_TOO_SMALL );
+
+        if( mbedtls_mpi_cmp_int( &P->Z, 1 ) != 0 )
+            return MBEDTLS_ERR_MPI_BAD_INPUT_DATA;


Style: please put parentheses after all return statements for consistency.

gilles-peskine-arm · 2020-05-26T18:06:59Z

include/mbedtls/config.h

@@ -769,6 +769,8 @@
 #define MBEDTLS_ECP_DP_BP512R1_ENABLED
 #define MBEDTLS_ECP_DP_CURVE25519_ENABLED
 #define MBEDTLS_ECP_DP_CURVE448_ENABLED
+#define MBEDTLS_ECP_DP_ED25519_ENABLED
+#define MBEDTLS_ECP_DP_ED448_ENABLED


As long as only arithmetic is implemented but no cryptographic algorithm, should the Edwards curves be enabled by default?

➕ If someone wants to use them now, it's nicer to have them enabled by default.

➖ This increases the code size in the default build for something that most users can't use.

➕ If the curves are not enabled by default, tests/scripts/curves.pl needs to be modified to take this into account.

➖ This locks the API extensions — not a big deal since the API change (new enum entries, one new function) is very small and not controversial.

I'm inclined to have them enabled by default.

We don't really care about code size in the default build, which is currently defined as "everything unless there's a good reason not to enable it" - I think it makes more sense to have Edwards curves even with partial support than NIST P-192 (which nobody uses and provides less than 128-bit of security), and we do have P-192 in the default build.

Regarding API extensions, as you say they're fairly small and straightforward.

gilles-peskine-arm · 2020-05-26T18:16:57Z

By the way, it's a known bug that the library doesn't build if you only enable Montgomery curves. I'd expect that the same bug affects Edwards curves, this won't be a blocker until we fix that bug.

gilles-peskine-arm · 2022-05-02T16:51:55Z

Continued in #5800

Signed-off-by: Pol Henarejos <[email protected]>

The bit length of m is leaked through through timing in ecp_mul_mxz(). Initially found by Manuel Pégourié-Gonnard on ecp_mul_edxyz(), which has been inspired from ecp_mul_mxz(), during initial review of the EdDSA PR. See: Mbed-TLS#3245 (comment) Fix that by using grp->pbits instead, which anyway is very close to the length of m, which means there is no significant performance impact. Signed-off-by: Aurelien Jarno <[email protected]>

See Mbed-TLS#3245 (comment) See Mbed-TLS#3245 (comment) See Mbed-TLS#3245 (comment) Signed-off-by: Aurelien Jarno <[email protected]>

Mbed-TLS#3245 (comment) Signed-off-by: Aurelien Jarno <[email protected]>

Add mbedtls_ecp_set_zero_ext() and mbedtls_ecp_is_zero_ext() functions which takes the ECP group as first argument, so that the functions can handle all curve types, and especially Edwards curves. Deprecated the old mbedtls_ecp_set_zero() and mbedtls_ecp_is_zero() functions. Suggested during the initial EdDSA PR review: Mbed-TLS#3245 (comment) Signed-off-by: Aurelien Jarno <[email protected]>

The bit length of m is leaked through through timing in ecp_mul_mxz(). Initially found by Manuel Pégourié-Gonnard on ecp_mul_edxyz(), which has been inspired from ecp_mul_mxz(), during initial review of the EdDSA PR. See: Mbed-TLS#3245 (comment) Fix that by using grp->nbits + 1 instead, which anyway is very close to the length of m, which means there is no significant performance impact. Signed-off-by: Aurelien Jarno <[email protected]>

Add mbedtls_ecp_set_zero_ext() and mbedtls_ecp_is_zero_ext() functions which takes the ECP group as first argument, so that the functions can handle all curve types, and especially Edwards curves. Deprecated the old mbedtls_ecp_set_zero() and mbedtls_ecp_is_zero() functions. Suggested during the initial EdDSA PR review: Mbed-TLS#3245 (comment) Signed-off-by: Aurelien Jarno <[email protected]>

The bit length of m is leaked through through timing in ecp_mul_mxz(). Initially found by Manuel Pégourié-Gonnard on ecp_mul_edxyz(), which has been inspired from ecp_mul_mxz(), during initial review of the EdDSA PR. See: Mbed-TLS#3245 (comment) Fix that by using grp->nbits + 1 instead, which anyway is very close to the length of m, which means there is no significant performance impact. Signed-off-by: Aurelien Jarno <[email protected]>

aurel32 mentioned this pull request Apr 26, 2020

Adding Ed25519/EdDSA #2452

Open

aurel32 force-pushed the edwards branch from 0e0056a to 025475c Compare April 26, 2020 22:35

aurel32 force-pushed the edwards branch from 025475c to c6b4dc0 Compare April 27, 2020 07:07

mpg added component-crypto Crypto primitives and low-level interfaces enhancement needs-design-approval needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, labels Apr 27, 2020

aurel32 added 6 commits April 27, 2020 14:48

Add edwards25519 curve as defined in RFC7748

a29a565

Largely inspired by the curve25519 definition. Signed-off-by: Aurelien Jarno <[email protected]>

Add edwards448 curve as defined in RFC7748

d949b06

Largely inspired by the curve448 definition. Signed-off-by: Aurelien Jarno <[email protected]>

Add MBEDTLS_ECP_TYPE_EDWARDS to mbedtls_ecp_curve_type

2785a30

Also tweak mbedtls_ecp_get_type to handle the edwards case. FIXME: there is probably a cleaner way of doing that. Signed-off-by: Aurelien Jarno <[email protected]>

aurel32 force-pushed the edwards branch from c6b4dc0 to 7f7dc33 Compare April 27, 2020 12:49

danh-arm assigned mpg Apr 29, 2020

danh-arm added this to the May 2020 Sprint milestone May 14, 2020

danh-arm modified the milestones: May 2020 Sprint, June 2020 Sprint May 26, 2020

gilles-peskine-arm reviewed May 26, 2020

View reviewed changes

danh-arm modified the milestones: June 2020 Sprint, September 2020 Sprint Jul 1, 2020

polhenarejos mentioned this pull request May 2, 2022

EdDSA support (Ed25519 and Ed448 curves) with Pure, Context and Prehash types + SHA3 (SHA3, SHAKE, CSHAKE and KMAC) #5800

Closed

7 tasks

gilles-peskine-arm closed this May 2, 2022

polhenarejos added a commit to polhenarejos/mbedtls that referenced this pull request May 8, 2022

Adding curve ed25519 (code from aurel32 in PR Mbed-TLS#3245).

9c232a9

Signed-off-by: Pol Henarejos <[email protected]>

aurel32 mentioned this pull request May 14, 2022

Add support for EdDSA with ed25519 curve (pure ed25519, ed25519ctx and ed25519ph) #5819

Open

3 tasks

aurel32 mentioned this pull request May 15, 2022

Fix a timing leak in ecp_mul_mxz() #5841

Merged

4 tasks

aurel32 added a commit to aurel32/mbedtls that referenced this pull request May 15, 2022

Fix a few issues from the original review

801b90e

See Mbed-TLS#3245 (comment) See Mbed-TLS#3245 (comment) See Mbed-TLS#3245 (comment) Signed-off-by: Aurelien Jarno <[email protected]>

aurel32 added a commit to aurel32/mbedtls that referenced this pull request May 15, 2022

Use more compact encoding of Edwards curve constants

2f9015d

Mbed-TLS#3245 (comment) Signed-off-by: Aurelien Jarno <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Edwards curves support to Mbed TLS #3245

Add Edwards curves support to Mbed TLS #3245

aurel32 commented Apr 26, 2020 •

edited

Loading

aurel32 commented Apr 26, 2020

gilles-peskine-arm left a comment

gilles-peskine-arm May 25, 2020

gilles-peskine-arm May 25, 2020

gilles-peskine-arm May 25, 2020

mpg Sep 18, 2020

gilles-peskine-arm May 25, 2020

mpg Sep 18, 2020

gilles-peskine-arm May 26, 2020

mu578 Jul 3, 2020

gilles-peskine-arm May 26, 2020

gilles-peskine-arm May 26, 2020

gilles-peskine-arm May 26, 2020

gilles-peskine-arm May 26, 2020

mpg Sep 18, 2020

gilles-peskine-arm commented May 26, 2020

gilles-peskine-arm commented May 2, 2022

Add Edwards curves support to Mbed TLS #3245

Add Edwards curves support to Mbed TLS #3245

Conversation

aurel32 commented Apr 26, 2020 • edited Loading

Status

Requires Backporting

Migrations

Additional comments

Todos

aurel32 commented Apr 26, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented May 26, 2020

gilles-peskine-arm commented May 2, 2022

aurel32 commented Apr 26, 2020 •

edited

Loading