Merge pull request #6389 from gilles-peskine-arm/ecdsa-use-psa-without-pkwrite

Remove pkwrite dependency in pk using PSA for ECDSA

include/mbedtls/build_info.h

@@ -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().
+ * 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.
- */
 #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

include/mbedtls/psa_util.h

@@ -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.

library/pk_wrap.c

@@ -19,6 +19,8 @@
 
 #include "common.h"
 
+#include "mbedtls/platform_util.h"
+
 #if defined(MBEDTLS_PK_C)
 #include "pk_wrap.h"
 #include "mbedtls/error.h"
@@ -26,39 +28,34 @@
 /* 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)
+#if defined(MBEDTLS_RSA_C) && defined(MBEDTLS_PSA_CRYPTO_C)
-#include "mbedtls/asn1write.h"
+#include "pkwrite.h"
-#endif
-
-#if defined(MBEDTLS_PK_RSA_ALT_SUPPORT)
-#include "mbedtls/platform_util.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)
@@ -685,11 +682,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;
+    size_t key_len;
-    int key_len;
+    /* This buffer will initially contain the public key and then the signature
-    unsigned char buf[MBEDTLS_PK_ECP_PUB_DER_MAX_BYTES];
+     * 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];
     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 =
@@ -701,22 +701,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);
@@ -864,54 +861,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,
@@ -922,19 +871,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;
+    unsigned char buf[MBEDTLS_PSA_MAX_EC_KEY_PAIR_LENGTH];
-    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);
 #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);
@@ -944,17 +892,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;
+    if (key_len > sizeof(buf)) {
-     * re-construct one to make it happy */
+        return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
-    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;
     }
-
+    ret = mbedtls_mpi_write_binary(&ctx->d, buf, key_len);
-    p = buf + sizeof(buf) - key_len;
-    ret = find_ecdsa_private_key(&p, buf + sizeof(buf), &key_len);
     if (ret != 0) {
         goto cleanup;
     }
@@ -964,7 +905,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);
@@ -1003,8 +944,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_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE)
-#if 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,
@@ -1110,8 +1050,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 && MBEDTLS_ECP_RESTARTABLE */
-#endif /* MBEDTLS_ECDSA_C */
 
 static int eckey_check_pair(const void *pub, const void *prv,
                             int (*f_rng)(void *, unsigned char *, size_t),

tests/suites/test_suite_pk.data

@@ -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

tests/suites/test_suite_pk.function

@@ -1238,12 +1238,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,
@@ -1264,12 +1274,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);
@@ -1278,8 +1297,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);