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.

Sign up for GitHub

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

Remove pkwrite dependency in pk using PSA for ECDSA #6389

Merged
merged 13 commits into from
Feb 28, 2023
Merged

Remove pkwrite dependency in pk using PSA for ECDSA #6389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b4a87b0
Don't use pk_write in ecdsa_verify_wrap with USE_PSA_CRYPTO
gilles-peskine-arm Oct 4, 2022
13caa94
Don't use pk_write in ecdsa_sign_wrap with USE_PSA_CRYPTO
gilles-peskine-arm Oct 4, 2022
bbccdd4
pk no longer needs pk_write for ECDSA with MBEDTLS_USE_PSA_CRYPTO
gilles-peskine-arm Oct 4, 2022
8a6022e
Clean up header inclusions in pk_wrap.c
gilles-peskine-arm Oct 4, 2022
be9e2a1
The pk_psa_sign test function needs pk_write
gilles-peskine-arm Oct 6, 2022
5bc5224
pk_wrap: fix for DETERMINISTIC_ECDSA case in ecdsa_sign_wrap()
valeriosetti Jan 30, 2023
1337a4f
pk_wrap: use specific lengths for EC's private key and key-pair
valeriosetti Jan 30, 2023
d0b83e1
build_info: fix PK's requirements for RSA_C
valeriosetti Jan 31, 2023
b761b15
fix code style
valeriosetti Jan 31, 2023
5c032b5
pk_wrap: fix comment in ecdsa_verify_wrap
valeriosetti Feb 2, 2023
a1e3e3a
test: pk: keep PK_WRITE_C only in RSA tests
valeriosetti Feb 2, 2023
683a432
fix code style
valeriosetti Feb 8, 2023
80d0798
pk_wrap: use new macros for ECDSA capabilities
valeriosetti Feb 8, 2023
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
13 changes: 1 addition & 12 deletions include/mbedtls/build_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,24 +82,13 @@

/* The PK wrappers need pk_write functions to format RSA key objects
* when they are dispatching to the PSA API. This happens under USE_PSA_CRYPTO,
* and also even without USE_PSA_CRYPTO for mbedtls_pk_sign_ext().
* PSA crypto also needs pk_write to export RSA keys (otherwise the build
* goes through but psa_export_key() and psa_export_public_key() fail on
* RSA keys), and pk_parse to work with RSA keys in almost any way.
*/
* and also even without USE_PSA_CRYPTO for mbedtls_pk_sign_ext(). */
#if defined(MBEDTLS_PSA_CRYPTO_C) && defined(MBEDTLS_RSA_C)
#define MBEDTLS_PK_C
#define MBEDTLS_PK_WRITE_C
#define MBEDTLS_PK_PARSE_C
#endif

/* Under MBEDTLS_USE_PSA_CRYPTO, the pk module needs pk_write functions
* to pass ECC keys to PSA. */
#if defined(MBEDTLS_PK_C) && \
defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_ECP_C)
#define MBEDTLS_PK_WRITE_C
#endif

#if !defined(MBEDTLS_SSL_PROTO_TLS1_2)
#undef MBEDTLS_KEY_EXCHANGE_RSA_ENABLED
#undef MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED
Expand Down
3 changes: 3 additions & 0 deletions include/mbedtls/psa_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,9 @@ static inline int mbedtls_psa_get_ecc_oid_from_id(
#define MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH \
PSA_KEY_EXPORT_ECC_PUBLIC_KEY_MAX_SIZE(PSA_VENDOR_ECC_MAX_CURVE_BITS)

#define MBEDTLS_PSA_MAX_EC_KEY_PAIR_LENGTH \
PSA_KEY_EXPORT_ECC_KEY_PAIR_MAX_SIZE(PSA_VENDOR_ECC_MAX_CURVE_BITS)

/* Expose whatever RNG the PSA subsystem uses to applications using the
* mbedtls_xxx API. The declarations and definitions here need to be
* consistent with the implementation in library/psa_crypto_random_impl.h.
Expand Down
135 changes: 37 additions & 98 deletions library/pk_wrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,46 +19,43 @@

#include "common.h"

#include "mbedtls/platform_util.h"

#if defined(MBEDTLS_PK_C)
#include "pk_wrap.h"
#include "mbedtls/error.h"

/* Even if RSA not activated, for the sake of RSA-alt */
#include "mbedtls/rsa.h"

#include <string.h>

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

#if defined(MBEDTLS_RSA_C) || defined(MBEDTLS_ECP_C)
#include "pkwrite.h"
#endif

#if defined(MBEDTLS_ECDSA_C)
#include "mbedtls/ecdsa.h"
#endif

#if defined(MBEDTLS_USE_PSA_CRYPTO)
#include "mbedtls/asn1write.h"
#endif

#if defined(MBEDTLS_PK_RSA_ALT_SUPPORT)
#include "mbedtls/platform_util.h"
#if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PSA_CRYPTO_C)
#include "pkwrite.h"
#endif

#if defined(MBEDTLS_USE_PSA_CRYPTO)
#include "psa/crypto.h"
#include "mbedtls/psa_util.h"
#include "mbedtls/asn1.h"
#include "hash_info.h"

#if defined(MBEDTLS_PK_CAN_ECDSA_SOME)
#include "mbedtls/asn1write.h"
#include "mbedtls/asn1.h"
#endif
#endif /* MBEDTLS_USE_PSA_CRYPTO */

#include "mbedtls/platform.h"

#include <limits.h>
#include <stdint.h>
#include <string.h>

#if defined(MBEDTLS_PSA_CRYPTO_C)
int mbedtls_pk_error_from_psa(psa_status_t status)
Expand Down Expand Up @@ -691,11 +688,14 @@ static int ecdsa_verify_wrap(void *ctx_arg, mbedtls_md_type_t md_alg,
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
mbedtls_svc_key_id_t key_id = MBEDTLS_SVC_KEY_ID_INIT;
psa_status_t status;
mbedtls_pk_context key;
int key_len;
unsigned char buf[MBEDTLS_PK_ECP_PUB_DER_MAX_BYTES];
size_t key_len;
/* This buffer will initially contain the public key and then the signature
* but at different points in time. For all curves except secp224k1, which
* is not currently supported in PSA, the public key is one byte longer
* (header byte + 2 numbers, while the signature is only 2 numbers),
* so use that as the buffer size. */
unsigned char buf[MBEDTLS_PSA_MAX_EC_PUBKEY_LENGTH];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is buffer on stack and holds public key or signature, but just to confirm. Shouldn't this buffer be cleared in cleanup stage?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, excellent question! As the name implies, public keys are not supposed to be sensitive data so they don't need to be wiped out. I'd say signatures are not sensitive either in general, but I may be missing some corner cases, so I'll ask on slack.

unsigned char *p;
mbedtls_pk_info_t pk_info = mbedtls_eckey_info;
psa_algorithm_t psa_sig_md = PSA_ALG_ECDSA_ANY;
size_t curve_bits;
psa_ecc_family_t curve =
Expand All @@ -707,22 +707,19 @@ static int ecdsa_verify_wrap(void *ctx_arg, mbedtls_md_type_t md_alg,
return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
}

/* mbedtls_pk_write_pubkey() expects a full PK context;
* re-construct one to make it happy */
key.pk_info = &pk_info;
key.pk_ctx = ctx;
p = buf + sizeof(buf);
key_len = mbedtls_pk_write_pubkey(&p, buf, &key);
if (key_len <= 0) {
return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
}

psa_set_key_type(&attributes, PSA_KEY_TYPE_ECC_PUBLIC_KEY(curve));
psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_VERIFY_HASH);
psa_set_key_algorithm(&attributes, psa_sig_md);

ret = mbedtls_ecp_point_write_binary(&ctx->grp, &ctx->Q,
MBEDTLS_ECP_PF_UNCOMPRESSED,
&key_len, buf, sizeof(buf));
if (ret != 0) {
goto cleanup;
}

status = psa_import_key(&attributes,
buf + sizeof(buf) - key_len, key_len,
buf, key_len,
&key_id);
if (status != PSA_SUCCESS) {
ret = mbedtls_pk_error_from_psa(status);
Expand Down Expand Up @@ -870,54 +867,6 @@ static int pk_ecdsa_sig_asn1_from_psa(unsigned char *sig, size_t *sig_len,
return 0;
}

/* Locate an ECDSA privateKey in a RFC 5915, or SEC1 Appendix C.4 ASN.1 buffer
*
* [in/out] buf: ASN.1 buffer start as input - ECDSA privateKey start as output
* [in] end: ASN.1 buffer end
* [out] key_len: the ECDSA privateKey length in bytes
*/
static int find_ecdsa_private_key(unsigned char **buf, unsigned char *end,
size_t *key_len)
{
size_t len;
int ret;

/*
* RFC 5915, or SEC1 Appendix C.4
*
* ECPrivateKey ::= SEQUENCE {
* version INTEGER { ecPrivkeyVer1(1) } (ecPrivkeyVer1),
* privateKey OCTET STRING,
* parameters [0] ECParameters {{ NamedCurve }} OPTIONAL,
* publicKey [1] BIT STRING OPTIONAL
* }
*/

if ((ret = mbedtls_asn1_get_tag(buf, end, &len,
MBEDTLS_ASN1_CONSTRUCTED |
MBEDTLS_ASN1_SEQUENCE)) != 0) {
return ret;
}

/* version */
if ((ret = mbedtls_asn1_get_tag(buf, end, &len,
MBEDTLS_ASN1_INTEGER)) != 0) {
return ret;
}

*buf += len;

/* privateKey */
if ((ret = mbedtls_asn1_get_tag(buf, end, &len,
MBEDTLS_ASN1_OCTET_STRING)) != 0) {
return ret;
}

*key_len = len;

return 0;
}

static int ecdsa_sign_wrap(void *ctx_arg, mbedtls_md_type_t md_alg,
const unsigned char *hash, size_t hash_len,
unsigned char *sig, size_t sig_size, size_t *sig_len,
Expand All @@ -928,19 +877,18 @@ static int ecdsa_sign_wrap(void *ctx_arg, mbedtls_md_type_t md_alg,
psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
mbedtls_svc_key_id_t key_id = MBEDTLS_SVC_KEY_ID_INIT;
psa_status_t status;
mbedtls_pk_context key;
size_t key_len;
unsigned char buf[MBEDTLS_PK_ECP_PRV_DER_MAX_BYTES];
unsigned char *p;
psa_algorithm_t psa_hash = mbedtls_hash_info_psa_from_md(md_alg);
unsigned char buf[MBEDTLS_PSA_MAX_EC_KEY_PAIR_LENGTH];
#if defined(MBEDTLS_ECDSA_DETERMINISTIC)
psa_algorithm_t psa_sig_md = PSA_ALG_DETERMINISTIC_ECDSA(psa_hash);
psa_algorithm_t psa_sig_md =
PSA_ALG_DETERMINISTIC_ECDSA(mbedtls_hash_info_psa_from_md(md_alg));
#else
psa_algorithm_t psa_sig_md = PSA_ALG_ECDSA(psa_hash);
psa_algorithm_t psa_sig_md =
PSA_ALG_ECDSA(mbedtls_hash_info_psa_from_md(md_alg));
#endif
size_t curve_bits;
psa_ecc_family_t curve =
mbedtls_ecc_group_to_psa(ctx->grp.id, &curve_bits);
size_t key_len = PSA_BITS_TO_BYTES(curve_bits);

/* PSA has its own RNG */
((void) f_rng);
Expand All @@ -950,17 +898,10 @@ static int ecdsa_sign_wrap(void *ctx_arg, mbedtls_md_type_t md_alg,
return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
}

/* mbedtls_pk_write_key_der() expects a full PK context;
* re-construct one to make it happy */
key.pk_info = &mbedtls_eckey_info;
key.pk_ctx = ctx;
key_len = mbedtls_pk_write_key_der(&key, buf, sizeof(buf));
if (key_len <= 0) {
return MBEDTLS_ERR_PK_BAD_INPUT_DATA;
if (key_len > sizeof(buf)) {
return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
}

p = buf + sizeof(buf) - key_len;
ret = find_ecdsa_private_key(&p, buf + sizeof(buf), &key_len);
ret = mbedtls_mpi_write_binary(&ctx->d, buf, key_len);
if (ret != 0) {
goto cleanup;
}
Expand All @@ -970,7 +911,7 @@ static int ecdsa_sign_wrap(void *ctx_arg, mbedtls_md_type_t md_alg,
psa_set_key_algorithm(&attributes, psa_sig_md);

status = psa_import_key(&attributes,
p, key_len,
buf, key_len,
&key_id);
if (status != PSA_SUCCESS) {
ret = mbedtls_pk_error_from_psa(status);
Expand Down Expand Up @@ -1009,8 +950,7 @@ static int ecdsa_sign_wrap(void *ctx, mbedtls_md_type_t md_alg,
#endif /* MBEDTLS_USE_PSA_CRYPTO */
#endif /* MBEDTLS_PK_CAN_ECDSA_SIGN */

#if defined(MBEDTLS_ECDSA_C)
#if defined(MBEDTLS_ECP_RESTARTABLE)
#if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE)
/* Forward declarations */
static int ecdsa_verify_rs_wrap(void *ctx, mbedtls_md_type_t md_alg,
const unsigned char *hash, size_t hash_len,
Expand Down Expand Up @@ -1116,8 +1056,7 @@ static int eckey_sign_rs_wrap(void *ctx, mbedtls_md_type_t md_alg,
cleanup:
return ret;
}
#endif /* MBEDTLS_ECP_RESTARTABLE */
#endif /* MBEDTLS_ECDSA_C */
#endif /* MBEDTLS_ECDSA_C && MBEDTLS_ECP_RESTARTABLE */

static int eckey_check_pair(const void *pub, const void *prv,
int (*f_rng)(void *, unsigned char *, size_t),
Expand Down
2 changes: 1 addition & 1 deletion tests/suites/test_suite_pk.data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ depends_on:MBEDTLS_PK_CAN_ECDSA_SIGN:MBEDTLS_ECP_DP_BP512R1_ENABLED
pk_psa_sign:MBEDTLS_ECP_DP_BP512R1:PSA_KEY_TYPE_ECC_KEY_PAIR(PSA_ECC_FAMILY_BRAINPOOL_P_R1):512

PSA wrapped sign: RSA PKCS1 v1.5
depends_on:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_GENPRIME
depends_on:MBEDTLS_RSA_C:MBEDTLS_PKCS1_V15:MBEDTLS_GENPRIME:MBEDTLS_PK_WRITE_C
pk_psa_sign:1024:PSA_KEY_TYPE_RSA_KEY_PAIR:1024

PK Sign ext:RSA2048,PK_RSA,MD_SHA256
Expand Down
39 changes: 37 additions & 2 deletions tests/suites/test_suite_pk.function
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1242,12 +1242,22 @@ void pk_psa_sign(int parameter_arg,
}

/* Export underlying public key for re-importing in a legacy context. */
#if defined(MBEDTLS_PK_WRITE_C)
ret = mbedtls_pk_write_pubkey_der(&pk, pkey_legacy,
sizeof(pkey_legacy));
TEST_ASSERT(ret >= 0);
klen_legacy = (size_t) ret;
/* mbedtls_pk_write_pubkey_der() writes backwards in the data buffer. */
pkey_legacy_start = pkey_legacy + sizeof(pkey_legacy) - klen_legacy;
#else
ret = mbedtls_ecp_point_write_binary(&(mbedtls_pk_ec(pk)->grp),
&(mbedtls_pk_ec(pk)->Q),
MBEDTLS_ECP_PF_UNCOMPRESSED,
&klen_legacy, pkey_legacy,
sizeof(pkey_legacy));
TEST_EQUAL(ret, 0);
pkey_legacy_start = pkey_legacy;
#endif /* MBEDTLS_PK_WRITE_C */

/* Turn PK context into an opaque one. */
TEST_ASSERT(mbedtls_pk_wrap_as_opaque(&pk, &key_id, alg_psa,
Expand All @@ -1268,12 +1278,21 @@ void pk_psa_sign(int parameter_arg,
NULL, NULL) == 0);

/* Export underlying public key for re-importing in a psa context. */
#if defined(MBEDTLS_PK_WRITE_C)
ret = mbedtls_pk_write_pubkey_der(&pk, pkey_psa,
sizeof(pkey_psa));
TEST_ASSERT(ret >= 0);
klen_psa = (size_t) ret;
/* mbedtls_pk_write_pubkey_der() writes backwards in the data buffer. */
pkey_psa_start = pkey_psa + sizeof(pkey_psa) - klen_psa;
#else
psa_status_t status;

status = psa_export_public_key(key_id, pkey_psa, sizeof(pkey_psa),
&klen_psa);
TEST_EQUAL(status, PSA_SUCCESS);
pkey_psa_start = pkey_psa;
#endif /* MBEDTLS_PK_WRITE_C */

TEST_ASSERT(klen_psa == klen_legacy);
TEST_ASSERT(memcmp(pkey_psa_start, pkey_legacy_start, klen_psa) == 0);
Expand All @@ -1282,8 +1301,24 @@ void pk_psa_sign(int parameter_arg,
TEST_ASSERT(PSA_SUCCESS == psa_destroy_key(key_id));

mbedtls_pk_init(&pk);
TEST_ASSERT(mbedtls_pk_parse_public_key(&pk, pkey_legacy_start,
klen_legacy) == 0);

/* If we used "pk_write" previously, then we go for a "pk_parse" here;
* otherwise if we went for "ecp_point_write_binary" then we'll go
* for a "ecp_point_read_binary" here. This allows to drop dependencies
* on "PK_WRITE" and "PK_PARSE" if required */
#if defined(MBEDTLS_PK_WRITE_C) && defined(MBEDTLS_PK_PARSE_C)
TEST_EQUAL(mbedtls_pk_parse_public_key(&pk, pkey_legacy_start,
klen_legacy), 0);
#else
TEST_EQUAL(mbedtls_pk_setup(&pk,
mbedtls_pk_info_from_type(MBEDTLS_PK_ECKEY)), 0);
TEST_EQUAL(mbedtls_ecp_group_load(
&(mbedtls_pk_ec(pk)->grp),
(mbedtls_ecp_group_id) parameter_arg), 0);
TEST_EQUAL(mbedtls_ecp_point_read_binary(&(mbedtls_pk_ec(pk)->grp),
&(mbedtls_pk_ec(pk)->Q),
pkey_legacy_start, klen_legacy), 0);
#endif
TEST_ASSERT(mbedtls_pk_verify(&pk, MBEDTLS_MD_SHA256,
hash, sizeof(hash), sig, sig_len) == 0);

Expand Down