Members of mbedtls_ecp_keypair should be public or have accessors

[minego]

Suggested enhancement

Members of mbedtls_ecp_keypair should be public or have accessors

Justification

Mbed TLS needs this because many mbedtls functions are clearly documented to indicate that they act on bare components of this structure. For example, mbedtls_ecp_check_pubkey and mbedtls_ecp_check_privkey take an mbedtls_ecp_group, and mbedtls_ecp_point and mbedtls_mpi as arguments and their documentation states:

"This function uses bare components rather than an ::mbedtls_ecp_keypair structure, to ease use with other structures, such as ::mbedtls_ecdh_context or ::mbedtls_ecdsa_context."

[gilles-peskine-arm]

I agree that something needs to change here but I'm not sure what the right solution is. Applications should be able to read the values of grp, Q and d in some fashion. The question is how.

Direct field access means documenting that only read access is allowed, and locks the representation of the underlying data. For grp and Q, which are themselves structures with private parts, that's not a big deal. For d it's a bigger deal because it means locking down the representation of the private key. One day we'd like to overhaul the bignum implementation in Mbed TLS, and a plausible way to do that would be to replace mbedtls_mpi fields by some different representation. Exposing a mbedtls_mpi * field means that if we do that, we have to maintain the old-representation field anyway, which wastes time and memory and can leak sensitive information through side channels.

An accessor function that returns, e.g.

static inline mbedtls_mpi *mbedtls_ecp_keypair_get_private_key(const mbedtls_ecp_keypair *ecp) {
    return ecp->d;
}

has the same defect of effectively locking down the representation, because the value returned by this accessor function is a pointer to memory owned by the keypair object. We can't later change to having this function make a copy, because the caller would now have to take ownership of the copy.

For mbedtls_dhm_context, the bignum fields are currently not exposed, but there is a function mbedtls_dhm_get_value which copies a field to a bignum object. Copying has the advantage of not opening the can of worms of pointers with hard-to-understand lifetimes: you own a mbedtls_mpi X, you call mbedtls_dhm_get_value(&dhm, PARAM_X, &X), and X remains valid even if the dhm object is freed. The downside is that this requires more memory and more CPU time in the common case where the caller only needs access to the value for a short time. I think this was acceptable for DHM where it's relatively uncommon to need access to the values, but less acceptable for ECP, and somewhere in between for RSA.

[minego]

So, with that approach what is the proper way to load an mbedtls_ecp_keypair? If the fields are considered read only then there would need to be the ability to set them as well, right? For example, how would you do something like this: mbedtls_ecp_keypair keypair; mbedtls_ecp_keypair_init(&keypair); mbedtls_ecp_group_load(&keypair.grp, MBEDTLS_EC_DP_SEC256R1); mbedtls_ecp_point_read_binary(&keypair.group, &keypair.grp, &keypair.Q, MyECPointValue, MyECPointLen); ret = mbedtls_ecp_check_pubkey&keypair.grp, &keypair.Q); if (ret != 0) { printf("Failed: %d\n", ret); } else { printf("Success\n"); } mbedtls_ecp_keypair_free(&keypair);

On Wed, Aug 4, 2021, at 1:33 PM, Gilles Peskine wrote: I agree that something needs to change here but I'm not sure what the right solution is. Applications should be able to read the values of `grp`, `Q` and `d` in some fashion. The question is how. Direct field access means documenting that only read access is allowed, and locks the representation of the underlying data. For `grp` and `Q`, which are themselves structures with private parts, that's not a big deal. For `d` it's a bigger deal because it means locking down the representation of the private key. One day we'd like to overhaul the bignum implementation in Mbed TLS, and a plausible way to do that would be to replace `mbedtls_mpi` fields by some different representation. Exposing a `mbedtls_mpi *` field means that if we do that, we have to maintain the old-representation field anyway, which wastes time and memory and can leak sensitive information through side channels. An accessor function that returns, e.g. `static inline mbedtls_mpi *mbedtls_ecp_keypair_get_private_key(const mbedtls_ecp_keypair *ecp) { return ecp->d; }

`

…

has the same defect of effectively locking down the representation, because the value returned by this accessor function is a pointer to memory owned by the keypair object. We can't later change to having this function make a copy, because the caller would now have to take ownership of the copy. For `mbedtls_dhm_context`, the bignum fields are currently not exposed, but there is a function `mbedtls_dhm_get_value` which copies a field to a bignum object. Copying has the advantage of not opening the can of worms of pointers with hard-to-understand lifetimes: you own a `mbedtls_mpi X`, you call `mbedtls_dhm_get_value(&dhm, PARAM_X, &X)`, and `X` remains valid even if the `dhm` object is freed. The downside is that this requires more memory and more CPU time in the common case where the caller only needs access to the value for a short time. I think this was acceptable for DHM where it's relatively uncommon to need access to the values, but less acceptable for ECP, and somewhere in between for RSA. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4838 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADSRCFXPCCOSMHDO4QEWZLT3GIXLANCNFSM5BPM3UQQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.

[gilles-peskine-arm]

The same reasoning extends to the coordinates of a point. Requested by openthread.

[gilles-peskine-arm]

Other requests for the fields of mbedtls_ecp_keypair: for ECDSA_ALT (#5005), for l8w8jwt (#5081).

[ndilieto]

What about implementing a similar function to mbedtls_rsa_export? It could be something like

int mbedtls_ecp_export(const mbedtls_ecp_keypair *kp, mbedtls_ecp_group *grp, mbedtls_mpi *d, mbedtls_ecp_point *Q)