Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.23k stars 2.56k forks source link

Curve25519 and mbedtls_ecp_curve_list() #464

Closed azadkuh closed 5 years ago

azadkuh commented 8 years ago

even by enabling MBEDTLS_ECP_DP_CURVE25519_ENABLED the mbedtls_ecp_curve_list() never reports MBEDTLS_ECP_DP_CURVE25519

in ecp.c :

static const mbedtls_ecp_curve_info ecp_supported_curves[] =
{
...
// no entry for MBEDTLS_ECP_DP_CURVE25519
};

is there any technical reason for that?

ciarmcom commented 8 years ago

ARM Internal Ref: IOTSSL-712

simonbutcher commented 8 years ago

This is an odd bug, and thanks for pointing it out.

Simply, I believe the reason Curve25519 was left out is because it's not supported as a possible curve in TLS1.2 (although it is in TLS1.3). That's not good if you want to use just the crypto library, and not the TLS library, so to me it's a bug in the interface and we'll have a look at it.

Again, thanks for the support, and sorry for such a slow response on a bug!

azadkuh commented 8 years ago

thank you for the detailed response.

it would be great if mbedtls expands its unit testing to cover more type checks (i found it through writing type checking unit test for mbedcrypto)

NWilson commented 8 years ago

I wouldn't rush into adding X25519 to the list. Actually, to me it makes sense not to include X25519 in the output of mbedtls_ecp_curve_list(). The docstrings in ecp.h mention in places that "Montgomery curves are excluded for now" (eg MBEDTLS_ECP_DP_MAX), so the current behaviour is intentional.

The problem is that the members of the mbedtls_ecp_group structure actually have different meaning depending on the form of the elliptic curve equation. Returning X25519 along with the Weierstrass curves would certainly break backwards compatibility, because apps using the mbedtls_ecp_group structure could risk misinterpreting the Montgomery curve data. I'd argue it would be positively unhelpful to list X25519 alongside the other curves, since it can't be used in just the same way as the Weierstrass curves.

I'd suggest adding a new function, mbedtls_ecp_curve_list_ext(int flags), with mbedtls_ecp_curve_list() defined as being identical to mbedtls_ecp_curve_list_ext(MBEDTLS_WEIERSTRASS). The Montgomery curves could be retrieved using mbedtls_ecp_curve_list_ext(MBEDTLS_MONTGOMERY).

Note that there is currently only one Montgomery curve; X25519 is the odd one out. However I have implemented support for X448 (Ed448 "Goldilocks"), which is waiting in a pull request. In that PR, in the test binaries I have an ad-hoc function for returning the list [X25519,X448], so a supported function mbedtls_ecp_curve_list_ext would be useful there.

azadkuh commented 8 years ago

so the ecp.h docs should be modified in first place to shed some light on the matter. at the moment the doc strings are ambiguous.

/**
 * Number of supported curves (plus one for NONE).
 *
 * --> (Montgomery curves excluded for now.) <--
 */
#define MBEDTLS_ECP_DP_MAX     12

says montgomery curves are excluded ... (is it right place to note that?!)

/**
 * \brief           ECP group structure
 *
 * --> We consider two types of curves equations: <--
 * ...
 * 2. --> Montgomery,       y^2 = x^3 + A x^2 + x   mod P   (Curve25519 + draft) <--
 ...
*/
typedef struct
{ ... }
mbedtls_ecp_group;

as an enduser, just by reading ecp.h I can not figure out the valuable points @NWilson said.

simonbutcher commented 8 years ago

Thanks @azadkuh and @NWilson for the feedback.

The elliptic curve modules should have a meaningful API independent of the TLS, and I think our mistake here was to think Curve25519 would be adopted into TLS1.2 more rapidly. I don't know if you're aware, but @mpg was working on the RFC when he was working on this code, and since then we've been overtaken by standards work on TLS1.3.

I'm not sure right now what my preferred fix is, but we'll take on the feedback, look at the options and see what's best to avoid breaking the API.

tjmao commented 6 years ago

RFC 8422 has been published, standardising the use of Curve25519 / Curve448 ECDH key exchange in TLS 1.2 and earlier.

As X25519 / X448 uses a custom point format, in order to support them in TLS handshakes while minimising API breaks, I would suggest rewriting mbedtls_ecp_point_read_binary() to bypass the check for PointConversionForm (0x04 == uncompressed) if grp is X25519 / X448, reading buf into &pt->X if grp is X25519 / X448, thus treating these points differently from NIST curve points (per RFC 8422 Section 5.4).