From 6c5beb7ff2227511ad1cae15aa7982dce2b26063 Mon Sep 17 00:00:00 2001 From: Reda Chouk Date: Thu, 13 Aug 2026 14:38:01 +0200 Subject: [PATCH] validate EC_PUB_X/EC_PUB_Y on import by routine them through the X9.63 point import --- src/wp_ecc_kmgmt.c | 113 +++++++++++++++++++--- test/test_ecc.c | 235 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 337 insertions(+), 11 deletions(-) diff --git a/src/wp_ecc_kmgmt.c b/src/wp_ecc_kmgmt.c index f691060d..ddc962d1 100644 --- a/src/wp_ecc_kmgmt.c +++ b/src/wp_ecc_kmgmt.c @@ -584,35 +584,126 @@ static int wp_ecc_set_params_enc_pub_key(wp_Ecc *ecc, const OSSL_PARAM params[], } /** - * Set the public key values into ECC key object. + * Set the public key ordinates into the ECC key object. + * + * X and Y are imported together as an X9.63 point, and the resulting point is + * checked against the curve, instead of being stored unvalidated. * * @param [in, out] ecc ECC key object. * @param [in] params Array of parameters and values. * @return 1 on success. * @return 0 on failure. */ -static int wp_ecc_set_params_pub(wp_Ecc *ecc, const OSSL_PARAM params[]) +static int wp_ecc_set_params_pub_xy(wp_Ecc* ecc, const OSSL_PARAM params[]) { int ok = 1; - int set = 0; + int setX = 0; + int setY = 0; + int init = 0; + int size = 0; + unsigned char* point = NULL; + mp_int x; + mp_int y; - WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_set_params_pub"); + WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_set_params_pub_xy"); - if (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_X, - ecc->key.pubkey.x, &set)) { + if (mp_init_multi(&x, &y, NULL, NULL, NULL, NULL) != MP_OKAY) { + ok = 0; + } + else { + init = 1; + } + if (ok && (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_X, &x, + &setX))) { + ok = 0; + } + if (ok && (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_Y, &y, + &setY))) { ok = 0; } - if (ok && (set == 1)) { - if (mp_iszero(ecc->key.pubkey.x)) { + /* One ordinate on its own is not a public key. */ + if (ok && (setX != setY)) { + ok = 0; + } + if (ok && setX) { + size = wc_ecc_get_curve_size_from_id(ecc->curveId); + if (size <= 0) { + ok = 0; + } + } + if (ok && setX) { + point = OPENSSL_malloc(1 + (2 * (size_t)size)); + if (point == NULL) { + ok = 0; + } + } + if (ok && setX) { + /* Uncompressed X9.63 point: 0x04 || X || Y. */ + point[0] = 0x04; + if (mp_to_unsigned_bin_len(&x, point + 1, size) != MP_OKAY) { + ok = 0; + } + } + if (ok && setX) { + if (mp_to_unsigned_bin_len(&y, point + 1 + size, size) != MP_OKAY) { + ok = 0; + } + } + if (ok && setX) { + int rc; + int origType; + + rc = wc_ecc_import_x963_ex(point, 1 + (2 * (word32)size), &ecc->key, + ecc->curveId); + if (rc != 0) { + WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, + "wc_ecc_import_x963_ex", rc); ok = 0; } if (ok) { + /* wc_ecc_import_x963_ex only checks the point against the curve + * when wolfSSL is built with WOLFSSL_VALIDATE_ECC_IMPORT, so check + * it here instead of relying on the build options. */ + origType = ecc->key.type; ecc->key.type = ECC_PUBLICKEY; - ecc->hasPub = 1; + rc = wc_ecc_check_key(&ecc->key); + ecc->key.type = origType; + if (rc != 0) { + WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, + "wc_ecc_check_key", rc); + ok = 0; + } + else { + ecc->hasPub = 1; + } } } - if (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_Y, - ecc->key.pubkey.y, NULL)) { + + OPENSSL_free(point); + if (init) { + mp_clear(&x); + mp_clear(&y); + } + + WOLFPROV_LEAVE(WP_LOG_COMP_ECC, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok); + return ok; +} + +/** + * Set the public key values into ECC key object. + * + * @param [in, out] ecc ECC key object. + * @param [in] params Array of parameters and values. + * @return 1 on success. + * @return 0 on failure. + */ +static int wp_ecc_set_params_pub(wp_Ecc *ecc, const OSSL_PARAM params[]) +{ + int ok = 1; + + WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_set_params_pub"); + + if (!wp_ecc_set_params_pub_xy(ecc, params)) { ok = 0; } if (wp_ecc_set_params_enc_pub_key(ecc, params, diff --git a/test/test_ecc.c b/test/test_ecc.c index 06337585..85c3b354 100644 --- a/test/test_ecc.c +++ b/test/test_ecc.c @@ -2873,6 +2873,230 @@ static int test_ec_import_group_no_nul(void) return err; } +/* + * Import an off-curve P-256 EC_PUB_X/EC_PUB_Y point, returning 1 when accepted + * (and setting *pkey), 0 when refused and -1 when the setup failed. + */ +static int test_ec_import_off_curve_xy(EVP_PKEY **pkey) +{ + int ret = -1; + int err = 0; + EVP_PKEY_CTX *ctx = NULL; + OSSL_PARAM *params = NULL; + OSSL_PARAM_BLD *bld = NULL; + BIGNUM *x = NULL; + BIGNUM *y = NULL; + unsigned char offCurveY[32]; + + memcpy(offCurveY, ecc_p256_pub + 1 + sizeof(offCurveY), sizeof(offCurveY)); + offCurveY[sizeof(offCurveY) - 1] ^= 0x01; + + err = (bld = OSSL_PARAM_BLD_new()) == NULL; + if (err == 0) { + err = OSSL_PARAM_BLD_push_utf8_string(bld, OSSL_PKEY_PARAM_GROUP_NAME, + ecc_p256_group_str, 0) != 1; + } + if (err == 0) { + err = (x = BN_bin2bn(ecc_p256_pub + 1, 32, NULL)) == NULL; + } + if (err == 0) { + err = (y = BN_bin2bn(offCurveY, sizeof(offCurveY), NULL)) == NULL; + } + if (err == 0) { + err = OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_EC_PUB_X, x) != 1; + } + if (err == 0) { + err = OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_EC_PUB_Y, y) != 1; + } + if (err == 0) { + err = (params = OSSL_PARAM_BLD_to_param(bld)) == NULL; + } + if (err == 0) { + err = (ctx = EVP_PKEY_CTX_new_from_name(wpLibCtx, "EC", NULL)) == NULL; + } + if (err == 0) { + err = EVP_PKEY_fromdata_init(ctx) != 1; + } + if (err == 0) { + ret = EVP_PKEY_fromdata(ctx, pkey, EVP_PKEY_PUBLIC_KEY, params) == 1; + } + + EVP_PKEY_CTX_free(ctx); + OSSL_PARAM_free(params); + OSSL_PARAM_BLD_free(bld); + BN_free(x); + BN_free(y); + + return ret; +} + +/* + * An EC_PUB_X/EC_PUB_Y point that is not on the curve must be rejected on + * import. + */ +static int test_ec_import_pub_xy_off_curve(void) +{ + int err = 0; + int rc; + EVP_PKEY *pkey = NULL; + + rc = test_ec_import_off_curve_xy(&pkey); + if (rc < 0) { + err = 1; + } + else if (rc == 1) { + PRINT_ERR_MSG("EC import accepted an off-curve EC_PUB_X/EC_PUB_Y point"); + err = 1; + } + + EVP_PKEY_free(pkey); + + return err; +} + +/* + * EC_PUB_X with no EC_PUB_Y is not a public key and must be rejected on + * import. + */ +static int test_ec_import_pub_x_only(void) +{ + int err = 0; + EVP_PKEY_CTX *ctx = NULL; + EVP_PKEY *pkey = NULL; + OSSL_PARAM *params = NULL; + OSSL_PARAM_BLD *bld = NULL; + BIGNUM *x = NULL; + + err = (bld = OSSL_PARAM_BLD_new()) == NULL; + if (err == 0) { + err = OSSL_PARAM_BLD_push_utf8_string(bld, OSSL_PKEY_PARAM_GROUP_NAME, + ecc_p256_group_str, 0) != 1; + } + if (err == 0) { + err = (x = BN_bin2bn(ecc_p256_pub + 1, 32, NULL)) == NULL; + } + if (err == 0) { + err = OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_EC_PUB_X, x) != 1; + } + if (err == 0) { + err = (params = OSSL_PARAM_BLD_to_param(bld)) == NULL; + } + if (err == 0) { + err = (ctx = EVP_PKEY_CTX_new_from_name(wpLibCtx, "EC", NULL)) == NULL; + } + if (err == 0) { + err = EVP_PKEY_fromdata_init(ctx) != 1; + } + if (err == 0 && + EVP_PKEY_fromdata(ctx, &pkey, EVP_PKEY_PUBLIC_KEY, params) == 1) { + PRINT_ERR_MSG("EC import accepted EC_PUB_X with no EC_PUB_Y"); + err = 1; + } + + EVP_PKEY_free(pkey); + EVP_PKEY_CTX_free(ctx); + OSSL_PARAM_free(params); + OSSL_PARAM_BLD_free(bld); + BN_free(x); + + return err; +} + +#ifdef WP_HAVE_ECDH +/* + * An off-curve peer key must never reach wc_ecc_shared_secret(), checked with + * validate_peer = 0 so that the provider and not OpenSSL is under test. + */ +static int test_ec_derive_off_curve_peer(void) +{ + int err = 0; + int rc; + EVP_PKEY_CTX *ctx = NULL; + EVP_PKEY_CTX *dctx = NULL; + EVP_PKEY *priv = NULL; + EVP_PKEY *peer = NULL; + OSSL_PARAM *params = NULL; + OSSL_PARAM_BLD *bld = NULL; + BIGNUM *d = NULL; + unsigned char secret[32]; + size_t secretLen = sizeof(secret); + + rc = test_ec_import_off_curve_xy(&peer); + if (rc < 0) { + err = 1; + } + else if (rc == 0) { + /* Import already rejects the point - nothing left to derive with. */ + PRINT_MSG("Off-curve point rejected on import, skipping derive"); + return 0; + } + + /* Our own private key: group plus private scalar only. */ + if (err == 0) { + err = (bld = OSSL_PARAM_BLD_new()) == NULL; + } + if (err == 0) { + err = OSSL_PARAM_BLD_push_utf8_string(bld, OSSL_PKEY_PARAM_GROUP_NAME, + ecc_p256_group_str, 0) != 1; + } + if (err == 0) { + err = (d = BN_bin2bn(ecc_p256_priv, sizeof(ecc_p256_priv), + NULL)) == NULL; + } + if (err == 0) { + err = OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_PRIV_KEY, d) != 1; + } + if (err == 0) { + err = (params = OSSL_PARAM_BLD_to_param(bld)) == NULL; + } + if (err == 0) { + err = (ctx = EVP_PKEY_CTX_new_from_name(wpLibCtx, "EC", NULL)) == NULL; + } + if (err == 0) { + err = EVP_PKEY_fromdata_init(ctx) != 1; + } + if (err == 0) { + err = EVP_PKEY_fromdata(ctx, &priv, EVP_PKEY_KEYPAIR, params) != 1; + } + + if (err == 0) { + err = (dctx = EVP_PKEY_CTX_new_from_pkey(wpLibCtx, priv, + NULL)) == NULL; + } + if (err == 0) { + err = EVP_PKEY_derive_init(dctx) != 1; + } + if (err == 0 && EVP_PKEY_derive_set_peer_ex(dctx, peer, 0) > 0 && + EVP_PKEY_derive(dctx, secret, &secretLen) > 0) { + size_t i; + int allZero = 1; + + for (i = 0; i < secretLen; i++) { + if (secret[i] != 0) { + allZero = 0; + break; + } + } + PRINT_ERR_MSG("ECDH derived a secret from an off-curve peer key"); + if (allZero) { + /* pubkey.z left at 0 makes the point behave as infinity. */ + PRINT_ERR_MSG("Derived secret is all zeros"); + } + err = 1; + } + + EVP_PKEY_CTX_free(dctx); + EVP_PKEY_CTX_free(ctx); + EVP_PKEY_free(priv); + EVP_PKEY_free(peer); + OSSL_PARAM_free(params); + OSSL_PARAM_BLD_free(bld); + BN_clear_free(d); + + return err; +} +#endif /* WP_HAVE_ECDH */ + /* * A truncated PEM (header only) must be rejected without an out-of-bounds read * in wp_pem2der_convert (base64Data past the buffer / base64Len underflow). @@ -2922,6 +3146,17 @@ int test_ec_import(void* data) if (err == 0) { err = test_ec_import_group_no_nul(); } + if (err == 0) { + err = test_ec_import_pub_xy_off_curve(); + } + if (err == 0) { + err = test_ec_import_pub_x_only(); + } +#ifdef WP_HAVE_ECDH + if (err == 0) { + err = test_ec_derive_off_curve_peer(); + } +#endif return err; }