UPSTREAM PR #30940: Avoid duplicate export of EC key point format

crypto/ec/ec_ameth.c

@@ -282,7 +282,8 @@ static int do_EC_KEY_print(BIO *bp, const EC_KEY *x, int off, ec_print_t ktype)
     }
 
     if (ktype != EC_KEY_PRINT_PARAM && EC_KEY_get0_public_key(x) != NULL) {
-        publen = EC_KEY_key2buf(x, EC_KEY_get_conv_form(x), &pub, NULL);
+        publen = EC_KEY_key2buf(x,
+            EC_GROUP_get_point_conversion_form(group), &pub, NULL);
         if (publen == 0)
             goto err;
     }
@@ -500,7 +501,7 @@ static int ec_pkey_export_to(const EVP_PKEY *from, void *to_keydata,
 
     if (pub_point != NULL) {
         /* convert pub_point to a octet string according to the SECG standard */
-        point_conversion_form_t format = EC_KEY_get_conv_form(eckey);
+        point_conversion_form_t format = EC_GROUP_get_point_conversion_form(ecg);
 
         if ((pub_key_buflen = EC_POINT_point2buf(ecg, pub_point,
                  format,

crypto/ec/ec_asn1.c

@@ -1064,7 +1064,8 @@ int i2d_ECPrivateKey(const EC_KEY *a, unsigned char **out)
             goto err;
         }
 
-        publen = EC_KEY_key2buf(a, a->conv_form, &pub, NULL);
+        publen = EC_KEY_key2buf(a,
+            EC_GROUP_get_point_conversion_form(a->group), &pub, NULL);
 
         if (publen == 0 || publen > INT_MAX) {
             ERR_raise(ERR_LIB_EC, ERR_R_EC_LIB);
@@ -1156,6 +1157,7 @@ EC_KEY *o2i_ECPublicKey(EC_KEY **a, const unsigned char **in, long len)
 
 int i2o_ECPublicKey(const EC_KEY *a, unsigned char **out)
 {
+    point_conversion_form_t form;
     size_t buf_len = 0;
     int new_buffer = 0;
 
@@ -1164,8 +1166,16 @@ int i2o_ECPublicKey(const EC_KEY *a, unsigned char **out)
         return 0;
     }
 
-    buf_len = EC_POINT_point2oct(a->group, a->pub_key,
-        a->conv_form, NULL, 0, NULL);
+    /*
+     * The encoded form follows the group's asn1_form, which the deprecated
+     * EC_KEY_set_conv_form() and the EC_KEY_oct2key() decode path both keep
+     * in sync with the key's conv_form.  Reading the group avoids a stale
+     * key-side value when the group was updated through one of the EC_GROUP
+     * setters directly.
+     */
+    form = EC_GROUP_get_point_conversion_form(a->group);
+
+    buf_len = EC_POINT_point2oct(a->group, a->pub_key, form, NULL, 0, NULL);
 
     if (buf_len > INT_MAX) {
         ERR_raise(ERR_LIB_EC, ERR_R_PASSED_INVALID_ARGUMENT);
@@ -1180,7 +1190,7 @@ int i2o_ECPublicKey(const EC_KEY *a, unsigned char **out)
             return 0;
         new_buffer = 1;
     }
-    if (!EC_POINT_point2oct(a->group, a->pub_key, a->conv_form,
+    if (!EC_POINT_point2oct(a->group, a->pub_key, form,
             *out, buf_len, NULL)) {
         ERR_raise(ERR_LIB_EC, ERR_R_EC_LIB);
         if (new_buffer) {

crypto/ec/ec_key.c

@@ -959,9 +959,16 @@ int EC_KEY_oct2key(EC_KEY *key, const unsigned char *buf, size_t len,
      * the last significant bit) contains the point conversion form.
      * EC_POINT_oct2point() has already performed sanity checking of
      * the buffer so we know it is valid.
+     *
+     * Mirror the form to the group as well, so that subsequent reads
+     * via EC_GROUP_get_point_conversion_form() see the loaded form
+     * rather than the EC_GROUP_new() default.  The encoder and the
+     * provider OSSL_PARAM emitter both consult the group.
      */
-    if ((key->group->meth->flags & EC_FLAGS_CUSTOM_CURVE) == 0)
+    if ((key->group->meth->flags & EC_FLAGS_CUSTOM_CURVE) == 0) {
         key->conv_form = (point_conversion_form_t)(buf[0] & ~0x01);
+        EC_GROUP_set_point_conversion_form(key->group, key->conv_form);
+    }
     return 1;
 }
 

crypto/evp/p_lib.c

@@ -2408,7 +2408,12 @@ int EVP_PKEY_get_ec_point_conv_form(const EVP_PKEY *pkey)
         if (ec == NULL)
             return 0;
 
-        return EC_KEY_get_conv_form(ec);
+        /*
+         * Read the form from the group rather than the deprecated
+         * EC_KEY_get_conv_form().  EC_KEY_set_conv_form() and
+         * EC_KEY_oct2key() both keep the group's asn1_form in sync.
+         */
+        return EC_GROUP_get_point_conversion_form(EC_KEY_get0_group(ec));
 #else
         return 0;
 #endif

providers/implementations/encode_decode/encode_key2text.c

@@ -357,7 +357,8 @@ static int ec_to_text(BIO *out, const void *key, int selection)
             goto err;
         }
 
-        pub_len = EC_KEY_key2buf(ec, EC_KEY_get_conv_form(ec), &pub, NULL);
+        pub_len = EC_KEY_key2buf(ec,
+            EC_GROUP_get_point_conversion_form(group), &pub, NULL);
         if (pub_len == 0)
             goto err;
     }

providers/implementations/keymgmt/ec_kmgmt.c

@@ -156,7 +156,7 @@ static ossl_inline int key_to_params(const EC_KEY *eckey, OSSL_PARAM_BLD *tmpl,
 
         if (p != NULL || tmpl != NULL) {
             /* convert pub_point to a octet string according to the SECG standard */
-            point_conversion_form_t format = EC_KEY_get_conv_form(eckey);
+            point_conversion_form_t format = EC_GROUP_get_point_conversion_form(ecg);
 
             if ((pub_key_len = EC_POINT_point2buf(ecg, pub_point,
                      format,
@@ -250,19 +250,10 @@ static ossl_inline int otherparams_to_params(const EC_KEY *ec, OSSL_PARAM_BLD *t
 {
     int ecdh_cofactor_mode = 0, group_check = 0;
     const char *name = NULL;
-    point_conversion_form_t format;
 
     if (ec == NULL)
         return 0;
 
-    format = EC_KEY_get_conv_form(ec);
-    name = ossl_ec_pt_format_id2name((int)format);
-    if (name != NULL
-        && !ossl_param_build_set_utf8_string(tmpl, params,
-            OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT,
-            name))
-        return 0;
-
     group_check = EC_KEY_get_flags(ec) & EC_FLAG_CHECK_NAMED_GROUP_MASK;
     name = ossl_ec_check_group_type_id2name(group_check);
     if (name != NULL
@@ -512,19 +503,23 @@ static int ec_export(void *keydata, int selection, OSSL_CALLBACK *param_cb,
         && (selection & OSSL_KEYMGMT_SELECT_PUBLIC_KEY) == 0)
         return 0;
 
-    tmpl = OSSL_PARAM_BLD_new();
-    if (tmpl == NULL)
+    if ((bnctx = BN_CTX_new_ex(ossl_ec_key_get_libctx(ec))) == NULL)
         return 0;
+    BN_CTX_start(bnctx);
 
-    if ((selection & OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS) != 0) {
-        bnctx = BN_CTX_new_ex(ossl_ec_key_get_libctx(ec));
-        if (bnctx == NULL) {
-            ok = 0;
-            goto end;
-        }
-        BN_CTX_start(bnctx);
-        ok = ok && ossl_ec_group_todata(EC_KEY_get0_group(ec), tmpl, NULL, ossl_ec_key_get_libctx(ec), ossl_ec_key_get0_propq(ec), bnctx, &genbuf);
+    if ((tmpl = OSSL_PARAM_BLD_new()) == NULL) {
+        ok = 0;
+        goto end;
     }
+    /*
+     * OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT is added based on the group's
+     * asn1_form by the call below; otherparams_to_params() no longer adds the
+     * key's deprecated conv_form, so this is the only source on the export
+     * side.
+     */
+    ok = ossl_ec_group_todata(EC_KEY_get0_group(ec), tmpl, NULL,
+        ossl_ec_key_get_libctx(ec), ossl_ec_key_get0_propq(ec),
+        bnctx, &genbuf);
 
     if ((selection & OSSL_KEYMGMT_SELECT_KEYPAIR) != 0) {
         int include_private = selection & OSSL_KEYMGMT_SELECT_PRIVATE_KEY ? 1 : 0;
@@ -748,6 +743,12 @@ static int common_get_params(void *key, OSSL_PARAM params[], int sm2)
             goto err;
     }
 
+    /*
+     * OSSL_PKEY_PARAM_EC_POINT_CONVERSION_FORMAT is added base on the group's
+     * asn1_form by ossl_ec_group_todata() below; otherparams_to_params() no
+     * longer adds the key's deprecated conv_form, so this is the only source
+     * on the get-params side.
+     */
     ret = ec_get_ecm_params(ecg, params)
         && ossl_ec_group_todata(ecg, NULL, params, libctx, propq, bnctx,
             &genbuf)
@@ -1258,6 +1259,18 @@ static int ec_gen_assign_group(EC_KEY *ec, EC_GROUP *group)
     return EC_KEY_set_group(ec, group) > 0;
 }
 
+/*
+ * Mirror the group's point-conversion form to the key's conv_form
+ * field so the two stay aligned after keygen.  EC_KEY_new() leaves
+ * conv_form at the UNCOMPRESSED default; this sync ensures any caller
+ * that asks the key directly sees the same value the encoders do.
+ */
+static void ec_gen_sync_conv_form(EC_KEY *ec)
+{
+    EC_KEY_set_conv_form(ec,
+        EC_GROUP_get_point_conversion_form(EC_KEY_get0_group(ec)));
+}
+
 /*
  * The callback arguments (osslcb & cbarg) are not used by EC_KEY generation
  */
@@ -1300,6 +1313,8 @@ static void *ec_gen(void *genctx, OSSL_CALLBACK *osslcb, void *cbarg)
 
     /* We must always assign a group, no matter what */
     ret = ec_gen_assign_group(ec, gctx->gen_group);
+    if (ret)
+        ec_gen_sync_conv_form(ec);
 
     /* Whether you want it or not, you get a keypair, not just one half */
     if ((gctx->selection & OSSL_KEYMGMT_SELECT_KEYPAIR) != 0) {
@@ -1376,6 +1391,8 @@ static void *sm2_gen(void *genctx, OSSL_CALLBACK *osslcb, void *cbarg)
 
     /* We must always assign a group, no matter what */
     ret = ec_gen_assign_group(ec, gctx->gen_group);
+    if (ret)
+        ec_gen_sync_conv_form(ec);
 
     /* Whether you want it or not, you get a keypair, not just one half */
     if ((gctx->selection & OSSL_KEYMGMT_SELECT_KEYPAIR) != 0)

ssl/t1_lib.c

@@ -1911,6 +1911,8 @@ static int tls1_check_pkey_comp(SSL_CONNECTION *s, EVP_PKEY *pkey)
     unsigned char comp_id;
     size_t i;
     int point_conv;
+    const unsigned char *own_formats;
+    size_t own_formats_len;
 
     /* If not an EC key nothing to check */
     if (!EVP_PKEY_is_a(pkey, "EC"))
@@ -1944,6 +1946,21 @@ static int tls1_check_pkey_comp(SSL_CONNECTION *s, EVP_PKEY *pkey)
      */
     if (s->ext.peer_ecpointformats == NULL)
         return 1;
+    /*
+     * The cert's point form must also be in our OWN advertised list -- we
+     * mustn't commit to a cert in a form we told the peer we don't speak.
+     * Without this the server's cert-selection happily picks (and the
+     * client happily decodes-and-then-rejects) a leaf whose form is in the
+     * peer's list but not ours, leaving the peer to alert illegal_parameter
+     * on receipt instead of catching the mismatch before it's wire-visible.
+     */
+    tls1_get_formatlist(s, &own_formats, &own_formats_len);
+    for (i = 0; i < own_formats_len; i++) {
+        if (own_formats[i] == comp_id)
+            break;
+    }
+    if (i == own_formats_len)
+        return 0;
 
     for (i = 0; i < s->ext.peer_ecpointformats_len; i++) {
         if (s->ext.peer_ecpointformats[i] == comp_id)

test/certs/mkcert.sh

@@ -52,7 +52,17 @@ key() {
         case $alg in
         rsa) args=("${args[@]}" -pkeyopt rsa_keygen_bits:$bits );;
         ec)  args=("${args[@]}" -pkeyopt "ec_paramgen_curve:$bits")
-               args=("${args[@]}" -pkeyopt ec_param_enc:named_curve);;
+               args=("${args[@]}" -pkeyopt ec_param_enc:named_curve)
+               # OPENSSL_EC_POINT_FORMAT (uncompressed|compressed|hybrid) drives
+               # the per-key point conversion form recorded on the generated
+               # EC_KEY, surfaced in SubjectPublicKeyInfo and negotiated via the
+               # TLS 1.2 ec_point_formats extension.  Empty/unset preserves the
+               # provider default (uncompressed).
+               if [ -n "$OPENSSL_EC_POINT_FORMAT" ]; then
+                   args=("${args[@]}" \
+                       -pkeyopt "point-format:$OPENSSL_EC_POINT_FORMAT")
+               fi
+               ;;
         dsa)  args=(-paramfile "$bits");;
         ed25519)  ;;
         ed448)  ;;

test/certs/server-ec-compressed-cert.pem

@@ -0,0 +1,11 @@
+-----BEGIN CERTIFICATE-----
+MIIBozCCASigAwIBAgIBAjAKBggqhkjOPQQDAjAbMRkwFwYDVQQDDBBFQ0RTQSBQ
+LTM4NCByb290MCAXDTI2MDQyNTA1MDgzNFoYDzIxMjYwNDI2MDUwODM0WjAZMRcw
+FQYDVQQDDA5zZXJ2ZXIuZXhhbXBsZTA5MBMGByqGSM49AgEGCCqGSM49AwEHAyIA
+A4Mt9T6fKt3APp8/Frw65PDi2eMYdZK98nhBW9pA1Ccho30wezAdBgNVHQ4EFgQU
+6MzfJGpHsWRJ3Nuj1KOStlvabW0wHwYDVR0jBBgwFoAUJtCPHXtf3B5/QYB9Y8oc
+dYHWhWkwCQYDVR0TBAIwADATBgNVHSUEDDAKBggrBgEFBQcDATAZBgNVHREEEjAQ
+gg5zZXJ2ZXIuZXhhbXBsZTAKBggqhkjOPQQDAgNpADBmAjEApXyoA/nmW3woPvjq
+H/ea3zDcN7mcaUYMwaCsyvsNNlKjFv1xdSFjxRFKUTqepJ/UAjEA7hQZq6hSm85b
++2TawUUn6Yj04cYq/XTirif3VtVauLABTRJ40S57jEpkDh6sovSC
+-----END CERTIFICATE-----

test/certs/server-ec-compressed-key.pem

@@ -0,0 +1,5 @@
+-----BEGIN PRIVATE KEY-----
+MGcCAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcETTBLAgEBBCAb1VgxSUhJyh43soLb
+FMsebjWSp/Hma3kSyw6lT4txDaEkAyIAA4Mt9T6fKt3APp8/Frw65PDi2eMYdZK9
+8nhBW9pA1Cch
+-----END PRIVATE KEY-----

test/certs/setup.sh

@@ -211,6 +211,16 @@ OPENSSL_KEYBITS=768 \
 # EC cert with named curve signed by named curve ca
 ./mkcert.sh genee server.example ee-key-ec-named-named \
     ee-cert-ec-named-named ca-key-ec-named ca-cert-ec-named
+# EC EE cert directly issued by the P-384 EC root, with a P-256 named-curve
+# leaf key whose per-key conv_form is COMPRESSED.  The leading SPKI bit-string
+# byte is 0x02 or 0x03, exercising the OSSL_PKEY_PARAM_EC_POINT_CONVERSION_-
+# FORMAT keygen path end-to-end.  Anchors TLS 1.2 ec_point_formats negotiation
+# tests in 80-test_ssl_new (sslnew tests rely solely on PKIX, so the CN/SAN
+# value is functionally arbitrary; we keep "server.example" for consistency).
+OPENSSL_KEYALG=ec OPENSSL_KEYBITS=prime256v1 \
+OPENSSL_EC_POINT_FORMAT=compressed \
+./mkcert.sh genee server.example server-ec-compressed-key \
+    server-ec-compressed-cert p384-root-key p384-root
 # 1024-bit leaf key
 OPENSSL_KEYBITS=1024 \
 ./mkcert.sh genee server.example ee-key-1024 ee-cert-1024 ca-key ca-cert

test/recipes/80-test_ssl_new.t

@@ -42,7 +42,7 @@ if (defined $ENV{SSL_TESTS}) {
     @conf_srcs = glob(srctop_file("test", "ssl-tests", "*.cnf.in"));
     # We hard-code the number of tests to double-check that the globbing above
     # finds all files as expected.
-    plan tests => 31;
+    plan tests => 32;
 }
 map { s/;.*// } @conf_srcs if $^O eq "VMS";
 my @conf_files = map { basename($_, ".in") } @conf_srcs;
@@ -100,6 +100,7 @@ my %conf_dependent_tests = (
   "28-seclevel.cnf" => disabled("tls1_2") || $no_ecx,
   "30-extended-master-secret.cnf" => disabled("tls1_2"),
   "32-compressed-certificate.cnf" => disabled("comp") || disabled("tls1_3"),
+  "33-compressed-spki.cnf" => disabled("tls1_2") || disabled("tls1_3") || $no_ec,
 );
 
 # Add your test here if it should be skipped for some compile-time
@@ -135,6 +136,7 @@ my %skip = (
   "26-tls13_client_auth.cnf" => disabled("tls1_3") || ($no_ec && $no_dh),
   "29-dtls-sctp-label-bug.cnf" => disabled("sctp") || disabled("sock"),
   "32-compressed-certificate.cnf" => disabled("comp") || disabled("tls1_3"),
+  "33-compressed-spki.cnf" => disabled("tls1_2") || disabled("tls1_3") || $no_ec,
 );
 
 foreach my $conf (@conf_files) {