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

[polhenarejos]

Description

It adds support for EdDSA with Ed25519 curve (sign and verify).

Ed448 curve will be added in a separated PR.

Splitted from #5800.

Status

READY

Requires Backporting

NO

Migrations

NO

Additional comments

Any additional information that could be of interest

Todos

• Tests
• Documentation
• Changelog updated

[polhenarejos]

1 year today.

Any new on the progress?

[gilles-peskine-arm]

Hi Pol, thanks for your help, but unfortunately, given our current commitments, it seems unlikely that we'll have time to review in 2023. At the earliest maybe in the last quarter of 2023, but even that is highly optimistic.

[Emill]

Hi. I came across this PR and decided to review it. Note that I'm not associated with mbedtls in any way so the contents of the review are just my personal opinions.

API

First of all, please have a look at FIPS 186-5 (DSS) where both ECDSA and EdDSA are specified and you will note a very important difference: for ECDSA, inputs and outputs are either (pair of) integers or curve points (except the message hash, which is an octet string), while for EdDSA, all inputs and outputs are octet strings. This applies to private keys, public keys, message/message hashes and signatures. This affects both crypto implementation APIs as well as protocols and certificate standards. You will see that in e.g. TLS, that the ECDSA signatures are "represented as a DER-encode [X690] ECDSA-Sig-Value structure" while EdDSA signatures are simply represented in their standard byte string form, without modifications. In mbedtls, you can see that the ECDSA functions all take mpi integers as inputs/outputs, or the convenience wrapper functions that decode/encode the signature in DER format, while crypto libraries implementing EdDSA use byte arrays as parameters.

When reading the proposed API in this PR, I feel it's a bit too much of a copy-paste from the ECDSA API:

• The section about "Eddsa-Sig-Value ::= SEQUENCE { r INTEGER, s INTEGER }" and MBEDTLS_EDDSA_MAX_SIG_LEN can be removed. An EdDSA signature is always a byte array with a fixed length (64 bytes for EdDSA and 114 bytes for Ed448). MBEDTLS_ERR_ASN1_XXX should not be returned by the sign function.
• Remove references to RFC 4492 and SEC1. Those documents are for ECDSA and not for EdDSA.
• You have the note "If the bitlength of the message hash is larger than the bitlength of the group order, then the hash is truncated as defined in ..." at various places. While this is true for ECDSA, it's not how EdDSA works.
• At many places mpi objects are used when byte arrays should be used instead, since mpi objects should contain integers and nothing else. An encoded curve point (y coordinate serialized as little endian followed by a sign bit of x) should not be put in an mpi. For mbedtls_ecp_expand_edwards, the only parameter where an mpi makes sense is the first half of the hash output.
• For publically exposed API functions that take public keys as parameters, consider using byte arrays with the encoding of the curve point instead of mbedtls_ecp_point, to be aligned with the standard and all other implementations. Remember that ECDSA uses "curve points" for representing a public key while EdDSA uses a byte string to represent a public key. The fact that an EdDSA public key is in fact a compressed curve point is an implementation detail that the user of the API should not need to care about. Requiring an extra step in the form of an API call to first decode the public key into some mbedtls-specific structure seems unnecessary. It's good to have clean and simple APIs for security to avoid unintentional misuse/bugs.
• Is it really necessary to have two separate functions mbedtls_eddsa_verify/mbedtls_eddsa_read_signature? Instead, have only one function that operates on byte arrays, to actually follow the specification which says that inputs/outputs are byte arrays and not integers. Same goes for verify. I can however see the benefits of having the private key passed as a "context" instead, e.g. if the same API is used for an alternative hw implementation which has a secure element where the private key is instead referred to using some identifier.
• Is the function ecp_check_pubkey_ed really necessary? When the public key is decoded, it is at the same time validated. Removing this redundant function saves code space.
• The verify routine returns MBEDTLS_ERR_ECP_VERIFY_FAILED when the signature is invalid, but the API documentation says MBEDTLS_ERR_ECP_BAD_INPUT_DATA is returned when the signature is invalid.
• MBEDTLS_ERR_ECP_SIG_LEN_MISMATCH is documented as one of the possible return values for mbedtls_eddsa_read_signature but is actually never returned.

Other comments:

• The function name and brief for mbedtls_ecp_point_edwards "This function obtains a point on the Edwards curve." are pretty bad. What point does it obtain? "Public key derivation" would be a better hint. Both this function and the expand function should be in eddsa module and not the generic ecp module, since how the private key is fed through SHA-512 is very EdDSA-specific. In general, I think key-pair management functions, including e.g. mbedtls_ecp_gen_keypair_base for EdDSA should not be in ecp since that does not make much sense. It works for ECDH and ECDSA since they both use a plain random scalar as private key and the scalar multiplied by the base point as public key.
• The brief for mbedtls_eddsa_sign says the message must be pre-hashed. But this is not really true for PureEdDSA/EdDSActx. Either say that the hash function is the identity function H(M) = M or that the message shall not be pre-hashed, for PureEdDSA/EdDSActx.
• Would it be reasonable to return an error if ed_ctx_len is non-zero but PureEdDSA is used?
• What is the rationale having MBEDTLS_ECP_DOUBLE_ADD_EDXYZ_ALT for the point addition ecp_add_edxyz but not any ALT for the double operation ecp_double_edxyz? In any case, I assume that if there are hw accelerators for Ed25519, they will probably perform the whole (double-)point multiplication so there sholud be an ALT for that instead, and probably also for the unpack of the public key.

There is an earlier comment here that references An EdDSA signature implementation is unsafe if it allows a different public key to be used. Let me give some of my comments on that.

The issue arises when a signer signs the same message twice, using the same private key but incorrectly passes the wrong public key to the sign function at least one of the times, so that the public keys differ in the two signing operations. The two resulting signatures will share the same R but have different S. An attacker that sees these two signatures as well as the two different public keys used can immediately derive the private key (or actually the first half of the hash of the private key, which is enough to create forged signatures). The probabilities for this to happen in the real world sounds to me pretty low. Firstly, the protocol that uses Ed25519 sign must allow the same message to be signed twice. Secondly, the software must either be buggy (passing the incorrect public key), or the sign operation is a manual one where a user inputs the (incorrect) private/public key pair. For TLS, this will not happen for CertificateVerify, since the sender's random value will be different every time, which is part of the message to be hashed.

The original Ed25519 paper defines a private key to be 32 bytes and a public key to be 32 bytes, but does not really define any APIs for signing/verifying. It instead simply documents that their reference software assumes that the public key is already computed at the time a sign operation is to be performed. The API in the reference software uses a 64 bytes long byte array as private key, containing a concatenation of the private key and the public key. RFC 8032 defines a "procedure" for signing. Here, the only inputs to that procedure are the private key (32 bytes) and the message; the public key is derived from the private key as the first step of the sign procedure. FIPS 186-5 defines the signature generation process in a different way. Here, the inputs are the message and the "valid" private-public key pair, i.e. the public key is assumed to be pre-computed.

Now, that GitHub repository above shows a "blacklist"/"wall of shame" of libraries implementing the API in FIPS 186-5, where the public-private keypair is passed split up using two parameters. I think that is a bit too harsh. If anything, they should blame the standard FIPS 186-5 and not the libraries implementing this standard. Anyway, there are a few "solutions" to the "problem":

1. Recompute the public key every time a sign operation is performed instead of allowing the user to pass in a cached copy of the public key. This has the downside that it decreases the performance by 50% since two base mult operations are performed instead of one. The second downside is that it increases the chances of SPA/DPA attacks to succeed, since it now performs an extra scalar multiplication that additionally performs the exact same operation every time.

2. Make it harder to misuse the API, for example by combining the private-public keypair as a single 64 bytes long byte array, or by encapsulating it in a struct keypair, containing both the private key and the public key. This is the preferred solution in the original reference software. One could also say that technically this is how FIPS 186-5 defines the sign operation, since the keypair is defined as one input operand and not two separate ones.

3. Include the public key when computing the hash that produces r, e.g. r=H(h_b, ..., h_(2b-1), A, M). This would result in different signatures than the ones produced when strictly following the specification (so tests will have to be rewritten), but will still be valid when later verifying them. It eliminates the issue since now both R and S will be different for two different public keys, even though the private key and the message are the same. The performance cost is only the cost of hashing 32 extra bytes and this extra computation should not leak anything.

For mbedtls, I think the first option is out of the question since it is important to have efficient implementations in embedded systems.

Implementation

I can see in the previous discussion that there are of course different approaches for carrying out the scalar multiplications that all have tradeoffs between code size/RAM usage/time/reusability with e.g. X25519. I would like to join that discussion too.

Note that a big motivation/reason for using Ed25519 over ECDSA is the possibility to get better performance. So, an implementation that is slower than ECDSA, for the same security level, would not be well received. Mbedtls already has some fancy comb implementation for ECDSA which on a high level is pretty good. The proposed Montgomery ladder in this PR, using the generic Edwards curve formulae, not optimized for Twisted Edwards curves, and not optimized for a=-1, with a double-add strategy for every bit, taking in total 21M per bit for base point multiplication, is probably the slowest possible way to implement Ed25519. The "standard" Montgomery ladder, using the Montgomery curve, instead uses 9M per bit. As has been mentioned previously, it can even share code with X25519/X448. I would say it is worth the effort to use this approach instead, whatever modifications that are needed in the current Montgomery curve ladder to make this possible. And even better, use a comb method like how it's done for ECDSA, to actually be able to beat ECDSA in speed. If space is a concern, even a comb implementation that processes two bits at a time would be faster than the Montgomery curve ladder and would require only 2 pre-computed points (2^128+1)B and (2^128-1)B. In my opinion, 4 bits at a time using two tables is a pretty good balance between speed and space. This is assuming the sign or keygen operation is actually frequently used and not e.g. only the verify operation. Note that a fast Ed25519 base point multiplication can be reused for X25519 key generation, which can speed up TLS handshakes using X25519 as ECDHE algorithm.

Using "Shamir's trick" is a quite decent choice though for double scalarmult, instead of performing two generic multiplications and then adding the results. The signed wNAF approach in the ref10 implementation is however ~30% faster but needs to store 8 points in a table in RAM, so I think that algorithm is a good candidate as well. In any case, you should really consider the Twisted Edwards Curve formulae optimized for a=-1 to achieve 7M per point double operation and 8M per point add operation, instead of 8M and 13M as it is right now.

Now to bugs:

• mbedtls_sha512_* functions can return errors; this is not handled