Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid parse/unparse public ECC keys in PK with USE_PSA when !ECP_C #7554

Merged
merged 12 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 54 additions & 4 deletions include/mbedtls/pk.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,35 @@ typedef struct mbedtls_pk_rsassa_pss_options {
#define MBEDTLS_PK_CAN_ECDH
#endif

/* Internal helper to define which fields in the pk_context structure below
* should be used for EC keys: legacy ecp_keypair or the raw (PSA friendly)
* format. It should be noticed that this only affect how data is stored, not
* which functions are used for various operations. The overall picture looks
* like this:
* - if ECP_C is defined then use legacy functions
* - if USE_PSA is defined and
* - if ECP_C then use ecp_keypair structure, convert data to a PSA friendly
* format and use PSA functions
* - if !ECP_C then use new raw data and PSA functions directly.
*
* The main reason for the "intermediate" (USE_PSA + ECP_C) above is that as long
* as ECP_C is defined mbedtls_pk_ec() gives the user a read/write access to the
* ecp_keypair structure inside the pk_context so he/she can modify it using
* ECP functions which are not under PK module's control.
*/
mpg marked this conversation as resolved.
Show resolved Hide resolved
#if defined(MBEDTLS_USE_PSA_CRYPTO) && !defined(MBEDTLS_ECP_C) && \
defined(MBEDTLS_ECP_LIGHT)
#define MBEDTLS_PK_USE_PSA_EC_DATA
#endif /* MBEDTLS_USE_PSA_CRYPTO && !MBEDTLS_ECP_C */

/**
* \brief Types for interfacing with the debug module
*/
typedef enum {
MBEDTLS_PK_DEBUG_NONE = 0,
MBEDTLS_PK_DEBUG_MPI,
MBEDTLS_PK_DEBUG_ECP,
MBEDTLS_PK_DEBUG_PSA_EC,
} mbedtls_pk_debug_type;

/**
Expand All @@ -232,19 +254,47 @@ typedef struct mbedtls_pk_debug_item {
*/
typedef struct mbedtls_pk_info_t mbedtls_pk_info_t;

#define MBEDTLS_PK_MAX_EC_PUBKEY_RAW_LEN \
PSA_KEY_EXPORT_ECC_PUBLIC_KEY_MAX_SIZE(PSA_VENDOR_ECC_MAX_CURVE_BITS)
/**
* \brief Public key container
*
* \note The priv_id is guarded by MBEDTLS_PSA_CRYPTO_C and not
* by MBEDTLS_USE_PSA_CRYPTO because it can be used also
* in mbedtls_pk_sign_ext for RSA keys.
*/
typedef struct mbedtls_pk_context {
const mbedtls_pk_info_t *MBEDTLS_PRIVATE(pk_info); /**< Public key information */
void *MBEDTLS_PRIVATE(pk_ctx); /**< Underlying public key context */
/* When MBEDTLS_PSA_CRYPTO_C is enabled then the following priv_id field is
* used to store the ID of the opaque key.
* This priv_id is guarded by MBEDTLS_PSA_CRYPTO_C and not by
* MBEDTLS_USE_PSA_CRYPTO because it can be used also in mbedtls_pk_sign_ext
* for RSA keys. */
#if defined(MBEDTLS_PSA_CRYPTO_C)
mbedtls_svc_key_id_t MBEDTLS_PRIVATE(priv_id); /**< Key ID for opaque keys */
#endif /* MBEDTLS_PSA_CRYPTO_C */
/* The following fields are meant for storing the public key in raw format
* which is handy for:
* - easily importing it into the PSA context
* - reducing the ECP module dependencies in the PK one.
*
* When MBEDTLS_PK_USE_PSA_EC_DATA is enabled:
* - the pk_ctx above is not used anymore for storing the public key
* inside the ecp_keypair structure (only the private part, but also this
* one is going to change in the future)
* - the following fields are used for all public key operations: signature
* verify, key pair check and key write.
* Of course, when MBEDTLS_PK_USE_PSA_EC_DATA is not enabled, the legacy
* ecp_keypair structure is used for storing the public key and perform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* ecp_keypair structure is used for storing the public key and perform
* ecp_keypair structure is used for storing the public key and performing

* all the operations.
*
* Note: This new public key storing solution only works for EC keys, not
* other ones. The latters still use pk_ctx to store their own
* context.
*/
#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
uint8_t MBEDTLS_PRIVATE(pub_raw)[MBEDTLS_PK_MAX_EC_PUBKEY_RAW_LEN]; /**< Raw public key */
size_t MBEDTLS_PRIVATE(pub_raw_len); /**< Valid bytes in "pub_raw" */
psa_ecc_family_t MBEDTLS_PRIVATE(ec_family); /**< EC family of pk */
size_t MBEDTLS_PRIVATE(ec_bits); /**< Curve's bits of pk */
#endif /* MBEDTLS_PK_USE_PSA_EC_DATA */
} mbedtls_pk_context;

#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE)
Expand Down
51 changes: 51 additions & 0 deletions library/debug.c
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,52 @@ void mbedtls_debug_print_ecp(const mbedtls_ssl_context *ssl, int level,
#endif /* MBEDTLS_ECP_LIGHT */

#if defined(MBEDTLS_BIGNUM_C)
#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
void mbedtls_debug_print_psa_ec(const mbedtls_ssl_context *ssl, int level,
const char *file, int line,
const char *text, const mbedtls_pk_context *pk)
{
char str[DEBUG_BUF_SIZE];
mbedtls_mpi mpi;
const uint8_t *mpi_start;
size_t mpi_len;
int ret;

if (NULL == ssl ||
NULL == ssl->conf ||
NULL == ssl->conf->f_dbg ||
level > debug_threshold) {
return;
}

/* For the description of pk->pk_raw content please refer to the description
* psa_export_public_key() function. */
mpi_len = (pk->pub_raw_len - 1)/2;

/* X coordinate */
mbedtls_mpi_init(&mpi);
mpi_start = pk->pub_raw + 1;
ret = mbedtls_mpi_read_binary(&mpi, mpi_start, mpi_len);
mpg marked this conversation as resolved.
Show resolved Hide resolved
if (ret != 0) {
return;
}
mbedtls_snprintf(str, sizeof(str), "%s(X)", text);
mbedtls_debug_print_mpi(ssl, level, file, line, str, &mpi);
mbedtls_mpi_free(&mpi);

/* Y coordinate */
mbedtls_mpi_init(&mpi);
mpi_start = mpi_start + mpi_len;
ret = mbedtls_mpi_read_binary(&mpi, mpi_start, mpi_len);
if (ret != 0) {
return;
}
mbedtls_snprintf(str, sizeof(str), "%s(Y)", text);
mbedtls_debug_print_mpi(ssl, level, file, line, str, &mpi);
mbedtls_mpi_free(&mpi);
}
#endif /* MBEDTLS_PK_USE_PSA_EC_DATA */

void mbedtls_debug_print_mpi(const mbedtls_ssl_context *ssl, int level,
const char *file, int line,
const char *text, const mbedtls_mpi *X)
Expand Down Expand Up @@ -277,6 +323,11 @@ static void debug_print_pk(const mbedtls_ssl_context *ssl, int level,
if (items[i].type == MBEDTLS_PK_DEBUG_ECP) {
mbedtls_debug_print_ecp(ssl, level, file, line, name, items[i].value);
} else
#endif
#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
if (items[i].type == MBEDTLS_PK_DEBUG_PSA_EC) {
mbedtls_debug_print_psa_ec(ssl, level, file, line, name, items[i].value);
} else
#endif
{ debug_send_line(ssl, level, file, line,
"should not happen\n"); }
Expand Down
39 changes: 39 additions & 0 deletions library/pk.c
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ void mbedtls_pk_init(mbedtls_pk_context *ctx)
#if defined(MBEDTLS_PSA_CRYPTO_C)
ctx->priv_id = MBEDTLS_SVC_KEY_ID_INIT;
#endif /* MBEDTLS_PSA_CRYPTO_C */
#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
memset(ctx->pub_raw, 0, sizeof(ctx->pub_raw));
ctx->pub_raw_len = 0;
ctx->ec_family = 0;
ctx->ec_bits = 0;
#endif /* MBEDTLS_PK_USE_PSA_EC_DATA */
}

/*
Expand Down Expand Up @@ -190,6 +196,39 @@ int mbedtls_pk_setup_opaque(mbedtls_pk_context *ctx,
}
#endif /* MBEDTLS_USE_PSA_CRYPTO */

#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
int mbedtls_pk_update_public_key_from_keypair(mbedtls_pk_context *pk,
mbedtls_ecp_keypair *ecp_keypair)
{
int ret = MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE;

if (pk == NULL) {
return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
}
/* The raw public key storing mechanism is only supported for EC keys so
* we fail silently for other ones. */
if ((pk->pk_info->type != MBEDTLS_PK_ECKEY) &&
(pk->pk_info->type != MBEDTLS_PK_ECKEY_DH) &&
(pk->pk_info->type != MBEDTLS_PK_ECDSA)) {
return 0;
}

ret = mbedtls_ecp_point_write_binary(&ecp_keypair->grp, &ecp_keypair->Q,
MBEDTLS_ECP_PF_UNCOMPRESSED,
&pk->pub_raw_len,
pk->pub_raw,
MBEDTLS_PK_MAX_EC_PUBKEY_RAW_LEN);
if (ret != 0) {
return ret;
}

pk->ec_family = mbedtls_ecc_group_to_psa(ecp_keypair->grp.id,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should check here if the conversion was successful and if not then return an error.
It is checked in ecdsa_verify_wrap function.

&pk->ec_bits);

return 0;
}
#endif /* MBEDTLS_PK_USE_PSA_EC_DATA */

#if defined(MBEDTLS_PK_RSA_ALT_SUPPORT)
/*
* Initialize an RSA-alt context
Expand Down
47 changes: 44 additions & 3 deletions library/pk_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,24 @@
#ifndef MBEDTLS_PK_INTERNAL_H
#define MBEDTLS_PK_INTERNAL_H

#include "mbedtls/pk.h"

#if defined(MBEDTLS_ECP_LIGHT)
#include "mbedtls/ecp.h"
#endif

#if defined(MBEDTLS_USE_PSA_CRYPTO)
#include "psa/crypto.h"
#endif

#if defined(MBEDTLS_ECP_LIGHT)
/**
* Public function mbedtls_pk_ec() can be used to get direct access to the
* wrapped ecp_keypair strucure pointed to the pk_ctx. However this is not
* ideal because it bypasses the PK module on the control of its internal's
* wrapped ecp_keypair structure pointed to the pk_ctx. However this is not
* ideal because it bypasses the PK module on the control of its internal
* structure (pk_context) fields.
* For backward compatibility we keep mbedtls_pk_ec() when ECP_C is defined, but
* we provide 2 very similar function when only ECP_LIGHT is enabled and not
* we provide 2 very similar functions when only ECP_LIGHT is enabled and not
* ECP_C.
* These variants embed the "ro" or "rw" keywords in their name to make the
* usage of the returned pointer explicit. Of course the returned value is
Expand Down Expand Up @@ -63,6 +69,41 @@ static inline mbedtls_ecp_keypair *mbedtls_pk_ec_rw(const mbedtls_pk_context pk)
return NULL;
}
}

/* Helpers for Montgomery curves */
mpg marked this conversation as resolved.
Show resolved Hide resolved
#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || defined(MBEDTLS_ECP_DP_CURVE448_ENABLED)
#define MBEDTLS_PK_HAVE_RFC8410_CURVES

static inline int mbedtls_pk_is_rfc8410_curve(mbedtls_ecp_group_id id)
{
#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED)
if (id == MBEDTLS_ECP_DP_CURVE25519) {
return 1;
}
#endif
#if defined(MBEDTLS_ECP_DP_CURVE448_ENABLED)
if (id == MBEDTLS_ECP_DP_CURVE448) {
return 1;
}
#endif
return 0;
}
#endif /* MBEDTLS_ECP_DP_CURVE25519_ENABLED || MBEDTLS_ECP_DP_CURVE448_ENABLED */
#endif /* MBEDTLS_ECP_LIGHT */

#if defined(MBEDTLS_PK_USE_PSA_EC_DATA)
/**
* \brief Copy the public key content in raw format from "ctx->pk_ctx"
* (which is an ecp_keypair) into the internal "ctx->pub_raw" buffer.
*
* \note This is a temporary function that can be removed as soon as the pk
* module is free from ECP_C
*
* \param pk It is the pk_context which is going to be updated. It acts both
* as input and output.
*/
int mbedtls_pk_update_public_key_from_keypair(mbedtls_pk_context *pk,
mbedtls_ecp_keypair *ecp_keypair);
#endif /* MBEDTLS_PK_USE_PSA_EC_DATA */

#endif /* MBEDTLS_PK_INTERNAL_H */
Loading