Skip to content

Validate EC_PUB_X/EC_PUB_Y on import by routine them through the X9.63 point import #467

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

Open
gasbytes wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Validate EC_PUB_X/EC_PUB_Y on import by routine them through the X9.63 point import #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
113 changes: 102 additions & 11 deletions src/wp_ecc_kmgmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,

@padelsbach padelsbach Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI tells me this call can overwrite/zero existing fields within the ecc key, which is fine for import, but potentially dangerous when called from wp_ecc_set_params. Can you investigate?

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

@padelsbach padelsbach Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to the logic in this comment, we should also apply the check below in wp_ecc_set_params_enc_pub_key. Maybe add a helper and call from both?

iiuc, WOLFSSL_VALIDATE_ECC_IMPORT is fairly new (or at least the checking is), so we want to keep this check for older wolfSSL versions.

* when wolfSSL is built with WOLFSSL_VALIDATE_ECC_IMPORT, so check
* it here instead of relying on the build options. */
origType = ecc->key.type;

@padelsbach padelsbach Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI says ecc->key.type is always ECC_PUBLICKEY so the save+restore is not needed. But zooming out, do we need to check the key we just imported?

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,
Expand Down
235 changes: 235 additions & 0 deletions test/test_ecc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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;
}
Expand Down
Loading