F-4287: check public point is on curve

include/wolfprovider/alg_funcs.h

@@ -227,6 +227,8 @@ ecc_key* wp_ecc_get_key(wp_Ecc* ecc);
 WC_RNG* wp_ecc_get_rng(wp_Ecc* ecc);
 int wp_ecc_get_size(wp_Ecc* ecc);
 int wp_ecc_check_usage(wp_Ecc* ecc);
+int wp_ecc_check_pub_point(wp_Ecc* ecc);
+int wp_ecc_check_pub_key(wp_Ecc* ecc);
 wolfSSL_Mutex* wp_ecc_get_mutex(wp_Ecc* ecc);
 
 /* Internal ECX types and functions. */

src/wp_ecc_kmgmt.c

@@ -243,6 +243,110 @@ int wp_ecc_check_usage(wp_Ecc* ecc)
     return ret;
 }
 
+/**
+ * Check a point is usable with a curve.
+ *
+ * Rejects the point at infinity and, where wolfSSL provides the check, any
+ * point that is not on the curve. A point off the curve lies on a group whose
+ * order may be small enough to recover the other party's private key from a
+ * key exchange.
+ *
+ * @param [in] point    Public key point.
+ * @param [in] curveId  wolfSSL identifier of curve point must be on.
+ * @return  1 on success.
+ * @return  0 on failure.
+ */
+static int wp_ecc_check_point(ecc_point* point, int curveId)
+{
+    int ok = 1;
+    int idx = ECC_CURVE_INVALID;
+#ifdef USE_ECC_B_PARAM
+    int rc;
+#endif
+
+    WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_check_point");
+
+    idx = wc_ecc_get_curve_idx(curveId);
+    if (idx == ECC_CURVE_INVALID) {
+        ok = 0;
+    }
+    if (ok && wc_ecc_point_is_at_infinity(point)) {
+        ok = 0;
+    }
+#ifdef USE_ECC_B_PARAM
+    /* wolfSSL only builds the on-curve check when the curve b parameter is
+     * available. */
+    if (ok) {
+        rc = wc_ecc_point_is_on_curve(point, idx);
+        if (rc != 0) {
+            WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG,
+                "wc_ecc_point_is_on_curve", rc);
+            ok = 0;
+        }
+    }
+#endif
+
+    WOLFPROV_LEAVE(WP_LOG_COMP_ECC, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
+    return ok;
+}
+
+/**
+ * Check the public key point is usable with the key's curve.
+ *
+ * @param [in] ecc  ECC key object.
+ * @return  1 on success.
+ * @return  0 on failure.
+ */
+int wp_ecc_check_pub_point(wp_Ecc* ecc)
+{
+    return (ecc != NULL) && wp_ecc_check_point(&ecc->key.pubkey, ecc->curveId);
+}
+
+/**
+ * Check the public key is valid for use with the key's curve.
+ *
+ * As well as the point being on the curve, the point must have the order of
+ * the curve. Any private key held alongside is ignored.
+ *
+ * @param [in] ecc  ECC key object.
+ * @return  1 on success.
+ * @return  0 on failure.
+ */
+int wp_ecc_check_pub_key(wp_Ecc* ecc)
+{
+    int ok = 1;
+    int origType;
+    int rc;
+
+    WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_check_pub_key");
+
+    if ((ecc == NULL) || (!ecc->hasPub)) {
+        ok = 0;
+    }
+    /* Fail closed if the key mutex can't be held for the check. */
+    if (ok && (wp_lock(wp_ecc_get_mutex(ecc)) != 1)) {
+        ok = 0;
+    }
+    if (ok) {
+        /* A private key may be present that does not match the public key,
+         * which is OK. Override the internal type to force a public key only
+         * check. */
+        origType = ecc->key.type;
+        ecc->key.type = ECC_PUBLICKEY;
+        rc = wc_ecc_check_key(&ecc->key);
+        ecc->key.type = origType;
+        wp_unlock(wp_ecc_get_mutex(ecc));
+        if (rc != 0) {
+            WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_ecc_check_key",
+                rc);
+            ok = 0;
+        }
+    }
+
+    WOLFPROV_LEAVE(WP_LOG_COMP_ECC, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
+    return ok;
+}
+
 /*
  * ECC key
  */
@@ -583,6 +687,58 @@ static int wp_ecc_set_params_enc_pub_key(wp_Ecc *ecc, const OSSL_PARAM params[],
     return ok;
 }
 
+/**
+ * Make a caller supplied point the public key of the ECC key object.
+ *
+ * The key is only modified once the point is known to be on the curve, so a
+ * rejected point leaves any existing public key as it was.
+ *
+ * @param [in, out] ecc    ECC key object.
+ * @param [in]      point  Public key point.
+ * @return  1 on success.
+ * @return  0 on failure.
+ */
+static int wp_ecc_set_pub_point(wp_Ecc *ecc, ecc_point* point)
+{
+    int ok = 1;
+    int rc;
+
+    WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_set_pub_point");
+
+    /* Curve must be known before the point can be checked against it. */
+    if (ecc->curveId == 0) {
+        ok = 0;
+    }
+    if (ok && (!wp_ecc_check_point(point, ecc->curveId))) {
+        ok = 0;
+    }
+    if (ok) {
+        rc = wc_ecc_set_curve(&ecc->key, 0, ecc->curveId);
+        if (rc != 0) {
+            WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG, "wc_ecc_set_curve",
+                rc);
+            ok = 0;
+        }
+    }
+    if (ok) {
+        if (wc_ecc_copy_point(point, &ecc->key.pubkey) == MP_OKAY) {
+            ecc->key.type = ECC_PUBLICKEY;
+            ecc->hasPub = 1;
+        }
+        else {
+            /* Key must not be left holding part of a point. */
+            mp_zero(ecc->key.pubkey.x);
+            mp_zero(ecc->key.pubkey.y);
+            mp_zero(ecc->key.pubkey.z);
+            ecc->hasPub = 0;
+            ok = 0;
+        }
+    }
+
+    WOLFPROV_LEAVE(WP_LOG_COMP_ECC, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
+    return ok;
+}
+
 /**
  * Set the public key values into ECC key object.
  *
@@ -594,33 +750,48 @@ static int wp_ecc_set_params_enc_pub_key(wp_Ecc *ecc, const OSSL_PARAM params[],
 static int wp_ecc_set_params_pub(wp_Ecc *ecc, const OSSL_PARAM params[])
 {
     int ok = 1;
-    int set = 0;
+    const OSSL_PARAM* px;
+    const OSSL_PARAM* py;
 
     WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_set_params_pub");
 
-    if (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_X,
-            ecc->key.pubkey.x, &set)) {
+    px = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_EC_PUB_X);
+    py = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_EC_PUB_Y);
+    /* One ordinate on its own does not describe a point. */
+    if ((px == NULL) != (py == NULL)) {
         ok = 0;
     }
-    if (ok && (set == 1)) {
-        if (mp_iszero(ecc->key.pubkey.x)) {
+    if (ok && (px != NULL)) {
+        /* Ordinates are read into a point of their own so that the key is
+         * left untouched when they don't describe a usable public key. */
+        ecc_point* point = wc_ecc_new_point();
+
+        if (point == NULL) {
             ok = 0;
         }
-        if (ok) {
-            ecc->key.type = ECC_PUBLICKEY;
-            ecc->hasPub = 1;
+        if (ok && (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_X,
+                point->x, NULL))) {
+            ok = 0;
         }
+        if (ok && (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_Y,
+                point->y, NULL))) {
+            ok = 0;
+        }
+        /* Ordinates are affine values - Z is always one. */
+        if (ok && (mp_set(point->z, 1) != MP_OKAY)) {
+            ok = 0;
+        }
+        if (ok && (!wp_ecc_set_pub_point(ecc, point))) {
+            ok = 0;
+        }
+        wc_ecc_del_point(point);
     }
-    if (!wp_params_get_mp(params, OSSL_PKEY_PARAM_EC_PUB_Y,
-            ecc->key.pubkey.y, NULL)) {
-        ok = 0;
-    }
-    if (wp_ecc_set_params_enc_pub_key(ecc, params,
-            OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY) != 1) {
+    if (ok && (wp_ecc_set_params_enc_pub_key(ecc, params,
+            OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY) != 1)) {
         ok = 0;
     }
-    if (wp_ecc_set_params_enc_pub_key(ecc, params,
-        OSSL_PKEY_PARAM_PUB_KEY) != 1) {
+    if (ok && (wp_ecc_set_params_enc_pub_key(ecc, params,
+            OSSL_PKEY_PARAM_PUB_KEY) != 1)) {
         ok = 0;
     }
 
@@ -987,37 +1158,6 @@ static int wp_ecc_match(wp_Ecc* ecc1, wp_Ecc* ecc2, int selection)
     return ok;
 }
 
-#if LIBWOLFSSL_VERSION_HEX >= 0x05000000
-/**
- * Quick validate the ECC public key.
- *
- * Check for infinity and point is on curve.
- *
- * @param [in] ecc        ECC key object.
- * @return  1 on success.
- * @return  0 on failure.
- */
-static int wp_ecc_validate_public_key_quick(const wp_Ecc* ecc)
-{
-    int ok = 1;
-
-    WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_validate_public_key_quick");
-
-    if (wc_ecc_point_is_at_infinity((ecc_point*)&ecc->key.pubkey)) {
-        ok = 0;
-    }
-#ifdef USE_ECC_B_PARAM
-    if (ok && (!wc_ecc_point_is_on_curve((ecc_point*)&ecc->key.pubkey,
-            ecc->curveId))) {
-        ok = 0;
-    }
-#endif
-
-    WOLFPROV_LEAVE(WP_LOG_COMP_ECC, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
-    return ok;
-}
-#endif
-
 /**
  * Validate the ECC key.
  *
@@ -1032,7 +1172,6 @@ static int wp_ecc_validate_public_key_quick(const wp_Ecc* ecc)
 static int wp_ecc_validate(const wp_Ecc* ecc, int selection, int checkType)
 {
     int ok = 1;
-    int origType;
     int rc;
 
     WOLFPROV_ENTER(WP_LOG_COMP_ECC, "wp_ecc_validate");
@@ -1046,37 +1185,13 @@ static int wp_ecc_validate(const wp_Ecc* ecc, int selection, int checkType)
         ok = 0;
     }
     if (ok && (selection & OSSL_KEYMGMT_SELECT_PUBLIC_KEY) != 0) {
-    #if LIBWOLFSSL_VERSION_HEX >= 0x05000000
-        /* TODO: Quick check for older versions? */
         if (checkType == OSSL_KEYMGMT_VALIDATE_QUICK_CHECK) {
-            if (!wp_ecc_validate_public_key_quick(ecc)) {
+            if (!wp_ecc_check_pub_point((wp_Ecc*)ecc)) {
                 ok = 0;
             }
         }
-        else
-    #else
-       (void)checkType;
-    #endif
-        {
-            /* Fail closed if the key mutex can't be held for the check. */
-            if (wp_lock(wp_ecc_get_mutex((wp_Ecc*)ecc)) != 1) {
-                ok = 0;
-            }
-            if (ok) {
-                /* We may have a private key inside that does not match the
-                 * public key that has been set, which is OK. Override the
-                 * internal type to force a public key only check */
-                origType = ecc->key.type;
-                ((wp_Ecc*)ecc)->key.type = ECC_PUBLICKEY;
-                rc = wc_ecc_check_key((ecc_key*)&ecc->key);
-                ((wp_Ecc*)ecc)->key.type = origType;
-                wp_unlock(wp_ecc_get_mutex((wp_Ecc*)ecc));
-                if (rc != 0) {
-                    WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_LEVEL_DEBUG,
-                        "wc_ecc_check_key", rc);
-                    ok = 0;
-                }
-            }
+        else if (!wp_ecc_check_pub_key((wp_Ecc*)ecc)) {
+            ok = 0;
         }
     }
     if (ok && ((selection & OSSL_KEYMGMT_SELECT_PRIVATE_KEY) != 0) &&

src/wp_ecdh_exch.c

@@ -281,12 +281,22 @@ static int wp_ecdh_derive_secret(wp_EcdhCtx* ctx, unsigned char* secret,
 
     WOLFPROV_ENTER(WP_LOG_COMP_ECDH, "wp_ecdh_derive_secret");
 
+    if (ctx->peer == NULL) {
+        ok = 0;
+    }
 #ifdef HAVE_ECC_CDH
-    if (ctx->cofactor) {
+    if (ok && ctx->cofactor) {
         wc_ecc_set_flags(wp_ecc_get_key(ctx->key), WC_ECC_FLAG_COFACTOR);
     }
 #endif
-    if ((ok = wp_ecc_check_usage(ctx->key))) {
+    if (ok && (!wp_ecc_check_usage(ctx->key))) {
+        ok = 0;
+    }
+    /* Peer's key may have been changed since it was set - check it again. */
+    if (ok && (!wp_ecc_check_pub_point(ctx->peer))) {
+        ok = 0;
+    }
+    if (ok) {
         /* Calculate secret. */
         PRIVATE_KEY_UNLOCK();
         rc = wc_ecc_shared_secret(wp_ecc_get_key(ctx->key),
@@ -325,13 +335,16 @@ static int wp_ecdh_derive(wp_EcdhCtx* ctx, unsigned char* secret,
     unsigned char* out;
     size_t outLen;
     unsigned char* tmp = NULL;
-    size_t maxLen = (size_t)wp_ecc_get_size(ctx->key);
+    size_t maxLen = 0;
 
     WOLFPROV_ENTER(WP_LOG_COMP_ECDH, "wp_ecdh_derive");
 
-    if (!wolfssl_prov_is_running()) {
+    if ((!wolfssl_prov_is_running()) || (ctx->key == NULL)) {
         ok = 0;
     }
+    if (ok) {
+        maxLen = (size_t)wp_ecc_get_size(ctx->key);
+    }
 
     /* No output buffer, return maximum size only. */
     if (ok && (secret == NULL)) {
@@ -405,6 +418,9 @@ static int wp_ecdh_set_peer(wp_EcdhCtx* ctx, wp_Ecc* peer)
     if (!wolfssl_prov_is_running()) {
         ok = 0;
     }
+    if (ok && (!wp_ecc_check_pub_key(peer))) {
+        ok = 0;
+    }
     if (ok && (ctx->peer != peer)) {
         /* Dispose of the old peer and up reference ECC key. */
         wp_ecc_free(ctx->peer);