Fix empty AKID/SKID handling

bernd-edlinger · bernd-edlinger · commit d9dc08b98025 · 2025-11-13T12:57:32.000+01:00
in all kinds of self-signed and non-self-signed X509, CSR, CRLs.

The default, when not defined by config for self-signed X509 is:
subjectKeyIdentifier=hash
authorityKeyIdentifier=none

For non-self-signed X509 it is:
subjectKeyIdentifier=hash
authorityKeyIdentifier=keyid,issuer

For CSR and CRLs the default is:
subjectKeyIdentifier=none
authorityKeyIdentifier=none

When defined by config the SKID=none or AKID=none must delete
any prior definition of the same OID.
diff --git a/apps/ca.c b/apps/ca.c
@@ -1691,13 +1691,14 @@ static int do_body(X509 **xret, EVP_PKEY *pkey, X509 *x509,
                    ret, NULL /* no need to give req, needed info is in ret */,
                    NULL, X509V3_CTX_REPLACE);
     /* prepare fallback for AKID, but only if issuer cert equals subject cert */
-    if (selfsign) {
+    if (selfsign && !cert_matches_key(ret, pkey)) {
         if (!X509V3_set_issuer_pkey(&ext_ctx, pkey))
             goto end;
-        if (!cert_matches_key(ret, pkey))
-            BIO_printf(bio_err,
-                       "Warning: Signature key and public key of cert do not match\n");
+        BIO_printf(bio_err,
+                   "Warning: Signature key and public key of cert do not match\n");
     }
+    if (!add_X509_default_keyids(ret, pkey, &ext_ctx))
+        goto end;
 
     /* Lets add the extensions, if there are any */
     if (ext_sect) {
diff --git a/apps/include/apps.h b/apps/include/apps.h
@@ -284,6 +284,7 @@ int init_gen_str(EVP_PKEY_CTX **pctx,
                  const char *algname, ENGINE *e, int do_param,
                  OSSL_LIB_CTX *libctx, const char *propq);
 int cert_matches_key(const X509 *cert, const EVP_PKEY *pkey);
+int add_X509_default_keyids(X509 *x, EVP_PKEY *pkey, X509V3_CTX *ext_ctx);
 int do_X509_sign(X509 *x, int force_v1, EVP_PKEY *pkey, const char *md,
                  STACK_OF(OPENSSL_STRING) *sigopts, X509V3_CTX *ext_ctx);
 int do_X509_verify(X509 *x, EVP_PKEY *pkey, STACK_OF(OPENSSL_STRING) *vfyopts);
diff --git a/apps/lib/apps.c b/apps/lib/apps.c
@@ -2300,28 +2300,24 @@ static int do_sign_init(EVP_MD_CTX *ctx, EVP_PKEY *pkey,
 }
 
 static int adapt_keyid_ext(X509 *cert, X509V3_CTX *ext_ctx,
-                           const char *name, const char *value, int add_default)
+                           const char *name, const char *value)
 {
     const STACK_OF(X509_EXTENSION) *exts = X509_get0_extensions(cert);
     X509_EXTENSION *new_ext = X509V3_EXT_nconf(NULL, ext_ctx, name, value);
-    int idx, rv = 0;
+    ASN1_OCTET_STRING *encoded;
+    int disabled, idx, rv = 0;
 
     if (new_ext == NULL)
         return rv;
 
     idx = X509v3_get_ext_by_OBJ(exts, X509_EXTENSION_get_object(new_ext), -1);
     if (idx >= 0) {
-        X509_EXTENSION *found_ext = X509v3_get_ext(exts, idx);
-        ASN1_OCTET_STRING *encoded = X509_EXTENSION_get_data(found_ext);
-        int disabled = ASN1_STRING_length(encoded) <= 2; /* indicating "none" */
-
-        if (disabled) {
-            X509_delete_ext(cert, idx);
-            X509_EXTENSION_free(found_ext);
-        } /* else keep existing key identifier, which might be outdated */
+        /* keep existing key identifier, which might be outdated */
         rv = 1;
     } else {
-        rv = !add_default || X509_add_ext(cert, new_ext, -1);
+        encoded = X509_EXTENSION_get_data(new_ext);
+        disabled = ASN1_STRING_length(encoded) <= 2; /* indicating "none" */
+        rv = disabled || X509_add_ext(cert, new_ext, -1);
     }
     X509_EXTENSION_free(new_ext);
     return rv;
@@ -2337,30 +2333,40 @@ int cert_matches_key(const X509 *cert, const EVP_PKEY *pkey)
     return match;
 }
 
-/* Ensure RFC 5280 compliance, adapt keyIDs as needed, and sign the cert info */
+/* Add default keyIDs as needed */
+int add_X509_default_keyids(X509 *cert, EVP_PKEY *pkey, X509V3_CTX *ext_ctx)
+{
+    int self_sign = 0;
+    int rv = 0;
+
+    /*
+     * Add default SKID before AKID such that AKID can make use of it
+     * in case the certificate is self-signed
+     */
+    if (X509_get_extension_flags(cert) & EXFLAG_SI)
+        self_sign = cert_matches_key(cert, pkey);
+    /* Prevent X509_V_ERR_MISSING_SUBJECT_KEY_IDENTIFIER */
+    if (!adapt_keyid_ext(cert, ext_ctx, "subjectKeyIdentifier", "hash"))
+        goto end;
+    /* Prevent X509_V_ERR_MISSING_AUTHORITY_KEY_IDENTIFIER */
+    if (!adapt_keyid_ext(cert, ext_ctx, "authorityKeyIdentifier",
+                         self_sign ? "none" : "keyid, issuer"))
+        goto end;
+    rv = 1;
+ end:
+    return rv;
+}
+
+/* Ensure RFC 5280 compliance, and sign the cert info */
 int do_X509_sign(X509 *cert, int force_v1, EVP_PKEY *pkey, const char *md,
                  STACK_OF(OPENSSL_STRING) *sigopts, X509V3_CTX *ext_ctx)
 {
     EVP_MD_CTX *mctx = EVP_MD_CTX_new();
-    int self_sign;
     int rv = 0;
 
     if (!force_v1) {
         if (!X509_set_version(cert, X509_VERSION_3))
             goto end;
-
-        /*
-         * Add default SKID before AKID such that AKID can make use of it
-         * in case the certificate is self-signed
-         */
-        /* Prevent X509_V_ERR_MISSING_SUBJECT_KEY_IDENTIFIER */
-        if (!adapt_keyid_ext(cert, ext_ctx, "subjectKeyIdentifier", "hash", 1))
-            goto end;
-        /* Prevent X509_V_ERR_MISSING_AUTHORITY_KEY_IDENTIFIER */
-        self_sign = cert_matches_key(cert, pkey);
-        if (!adapt_keyid_ext(cert, ext_ctx, "authorityKeyIdentifier",
-                             "keyid, issuer", !self_sign))
-            goto end;
     }
     /* May add further measures for ensuring RFC 5280 compliance, see #19805 */
 
diff --git a/apps/req.c b/apps/req.c
@@ -847,13 +847,14 @@ int req_main(int argc, char **argv)
             X509V3_set_ctx(&ext_ctx, CAcert != NULL ? CAcert : new_x509,
                            new_x509, NULL, NULL, X509V3_CTX_REPLACE);
             /* prepare fallback for AKID, but only if issuer cert == new_x509 */
-            if (CAcert == NULL) {
+            if (CAcert == NULL && !cert_matches_key(new_x509, issuer_key)) {
                 if (!X509V3_set_issuer_pkey(&ext_ctx, issuer_key))
                     goto end;
-                if (!cert_matches_key(new_x509, issuer_key))
-                    BIO_printf(bio_err,
-                               "Warning: Signature key and public key of cert do not match\n");
+                BIO_printf(bio_err,
+                           "Warning: Signature key and public key of cert do not match\n");
             }
+            if (!x509v1 && !add_X509_default_keyids(new_x509, issuer_key, &ext_ctx))
+                goto end;
             X509V3_set_nconf(&ext_ctx, req_conf);
 
             /* Add extensions */
diff --git a/apps/x509.c b/apps/x509.c
@@ -896,23 +896,6 @@ int x509_main(int argc, char **argv)
         }
     }
 
-    X509V3_set_ctx(&ext_ctx, issuer_cert, x, NULL, NULL, X509V3_CTX_REPLACE);
-    /* prepare fallback for AKID, but only if issuer cert equals subject cert */
-    if (CAfile == NULL) {
-        if (!X509V3_set_issuer_pkey(&ext_ctx, privkey))
-            goto err;
-    }
-    if (extconf != NULL && !x509toreq) {
-        X509V3_set_nconf(&ext_ctx, extconf);
-        if (!X509V3_EXT_add_nconf(extconf, &ext_ctx, extsect, x)) {
-            BIO_printf(bio_err,
-                       "Error adding extensions from section %s\n", extsect);
-            goto err;
-        }
-    }
-
-    /* At this point the contents of the certificate x have been finished. */
-
     pkey = X509_get0_pubkey(x);
     if ((print_pubkey != 0 || modulus != 0) && pkey == NULL) {
         BIO_printf(bio_err, "Error getting public key\n");
@@ -930,6 +913,7 @@ int x509_main(int argc, char **argv)
         }
         if ((rq = x509_to_req(x, ext_copy, ext_names)) == NULL)
             goto err;
+        X509V3_set_ctx(&ext_ctx, NULL, NULL, rq, NULL, X509V3_CTX_REPLACE);
         if (extconf != NULL) {
             X509V3_set_nconf(&ext_ctx, extconf);
             if (!X509V3_EXT_REQ_add_nconf(extconf, &ext_ctx, extsect, rq)) {
@@ -964,9 +948,36 @@ int x509_main(int argc, char **argv)
             goto err;
         }
 
+        X509V3_set_ctx(&ext_ctx, xca, x, NULL, NULL, X509V3_CTX_REPLACE);
+        if (!add_X509_default_keyids(x, CAkey, &ext_ctx))
+            goto err;
+        if (extconf != NULL) {
+            X509V3_set_nconf(&ext_ctx, extconf);
+            if (!X509V3_EXT_add_nconf(extconf, &ext_ctx, extsect, x)) {
+                BIO_printf(bio_err,
+                           "Error adding extensions from section %s\n", extsect);
+                goto err;
+            }
+        }
         if (!do_X509_sign(x, 0, CAkey, digest, sigopts, &ext_ctx))
             goto err;
     } else if (privkey != NULL) {
+        X509V3_set_ctx(&ext_ctx, x, x, NULL, NULL, X509V3_CTX_REPLACE);
+        /* prepare fallback for AKID, but only if issuer cert equals subject cert */
+        if (!cert_matches_key(x, privkey)) {
+            if (!X509V3_set_issuer_pkey(&ext_ctx, privkey))
+                goto err;
+        }
+        if (!add_X509_default_keyids(x, privkey, &ext_ctx))
+            goto err;
+        if (extconf != NULL) {
+            X509V3_set_nconf(&ext_ctx, extconf);
+            if (!X509V3_EXT_add_nconf(extconf, &ext_ctx, extsect, x)) {
+                BIO_printf(bio_err,
+                           "Error adding extensions from section %s\n", extsect);
+                goto err;
+            }
+        }
         if (!do_X509_sign(x, 0, privkey, digest, sigopts, &ext_ctx))
             goto err;
     }
diff --git a/crypto/cmp/cmp_msg.c b/crypto/cmp/cmp_msg.c
@@ -127,6 +127,7 @@ X509_PUBKEY *OSSL_CMP_MSG_get0_certreq_publickey(const OSSL_CMP_MSG *msg)
 static int add1_extension(X509_EXTENSIONS **pexts, int nid, int crit, void *ex)
 {
     X509_EXTENSION *ext;
+    int idx;
     int res;
 
     if (!ossl_assert(pexts != NULL)) /* pointer to var must not be NULL */
@@ -135,6 +136,9 @@ static int add1_extension(X509_EXTENSIONS **pexts, int nid, int crit, void *ex)
     if ((ext = X509V3_EXT_i2d(nid, crit, ex)) == NULL)
         return 0;
 
+    while ((idx = X509v3_get_ext_by_NID(*pexts, nid, -1)) >= 0)
+        X509_EXTENSION_free(X509v3_delete_ext(*pexts, idx));
+
     res = X509v3_add_ext(pexts, ext, 0) != NULL;
     X509_EXTENSION_free(ext);
     return res;
@@ -327,6 +331,8 @@ OSSL_CRMF_MSG *OSSL_CMP_CTX_setup_CRM(OSSL_CMP_CTX *ctx, int for_KUR, int rid)
     if (ctx->p10CSR != NULL
             && (exts = X509_REQ_get_extensions(ctx->p10CSR)) == NULL)
         goto err;
+    if (exts == NULL && (exts = sk_X509_EXTENSION_new_null()) == NULL)
+        goto err;
     if (!ctx->SubjectAltName_nodefault && !HAS_SAN(ctx) && refcert != NULL
         && (default_sans = X509V3_get_d2i(X509_get0_extensions(refcert),
                                           NID_subject_alt_name, NULL, NULL))
diff --git a/crypto/x509/v3_akid.c b/crypto/x509/v3_akid.c
@@ -171,13 +171,8 @@ static AUTHORITY_KEYID *v2i_AUTHORITY_KEYID(X509V3_EXT_METHOD *method,
             X509_PUBKEY_free(pubkey);
         } else {
             i = X509_get_ext_by_NID(issuer_cert, NID_subject_key_identifier, -1);
-            if (i >= 0 && (ext = X509_get_ext(issuer_cert, i)) != NULL) {
+            if (i >= 0 && (ext = X509_get_ext(issuer_cert, i)) != NULL)
                 ikeyid = X509V3_EXT_d2i(ext);
-                if (ASN1_STRING_length(ikeyid) == 0) /* indicating "none" */ {
-                    ASN1_OCTET_STRING_free(ikeyid);
-                    ikeyid = NULL;
-                }
-            }
         }
         if (keyid == 2 && ikeyid == NULL) {
             ERR_raise(ERR_LIB_X509V3, X509V3_R_UNABLE_TO_GET_ISSUER_KEYID);
diff --git a/crypto/x509/v3_conf.c b/crypto/x509/v3_conf.c
@@ -340,11 +340,18 @@ int X509V3_EXT_add_nconf_sk(CONF *conf, X509V3_CTX *ctx, const char *section,
                                         val->name, val->value)) == NULL)
             return 0;
         if (sk != NULL) {
-            if (ctx->flags == X509V3_CTX_REPLACE)
+            if ((ctx->flags & X509V3_CTX_REPLACE) != 0)
                 delete_ext(*sk, ext);
-            if (X509v3_add_ext(sk, ext, -1) == NULL) {
-                X509_EXTENSION_free(ext);
-                return 0;
+            if ((ctx->flags & X509V3_CTX_REPLACE) == 0
+                    || ctx->subject_req != NULL || (i != akid && i != skid)
+                    || ASN1_STRING_length(X509_EXTENSION_get_data(ext)) > 2) {
+                if (X509v3_add_ext(sk, ext, -1) == NULL) {
+                    X509_EXTENSION_free(ext);
+                    return 0;
+                }
+            } else if (*sk != NULL && sk_X509_EXTENSION_num(*sk) == 0) {
+                sk_X509_EXTENSION_free(*sk);
+                *sk = NULL;
             }
         }
         X509_EXTENSION_free(ext);
diff --git a/crypto/x509/x509_req.c b/crypto/x509/x509_req.c
@@ -160,7 +160,8 @@ STACK_OF(X509_EXTENSION) *X509_REQ_get_extensions(const X509_REQ *req)
 
 /*
  * Add a STACK_OF extensions to a certificate request: allow alternative OIDs
- * in case we want to create a non standard one.
+ * in case we want to create a non standard one. If there are already
+ * extensions with the same OID, replace them with the new ones.
  */
 int X509_REQ_add_extensions_nid(X509_REQ *req,
                                 const STACK_OF(X509_EXTENSION) *exts, int nid)
@@ -178,24 +179,32 @@ int X509_REQ_add_extensions_nid(X509_REQ *req,
     if (loc != -1) {
         if ((mod_exts = get_extensions_by_nid(req, nid)) == NULL)
             return 0;
-        if (X509v3_add_extensions(&mod_exts, exts) == NULL)
-            goto end;
+    } else {
+        if ((mod_exts = sk_X509_EXTENSION_new_null()) == NULL)
+            return 0;
     }
 
-    /* Generate encoding of extensions */
-    extlen = ASN1_item_i2d((const ASN1_VALUE *)
-                           (mod_exts == NULL ? exts : mod_exts),
-                           &ext, ASN1_ITEM_rptr(X509_EXTENSIONS));
-    if (extlen <= 0)
+    if (X509v3_add_extensions(&mod_exts, exts) == NULL)
         goto end;
-    if (mod_exts != NULL) {
+
+    if (loc != -1) {
         X509_ATTRIBUTE *att = X509at_delete_attr(req->req_info.attributes, loc);
 
         if (att == NULL)
             goto end;
         X509_ATTRIBUTE_free(att);
     }
 
+    if (sk_X509_EXTENSION_num(mod_exts) == 0) {
+        rv = 1;
+        goto end;
+    }
+
+    /* Generate encoding of extensions */
+    extlen = ASN1_item_i2d((const ASN1_VALUE *)mod_exts,
+                           &ext, ASN1_ITEM_rptr(X509_EXTENSIONS));
+    if (extlen <= 0)
+        goto end;
     rv = X509_REQ_add1_attr_by_NID(req, nid, V_ASN1_SEQUENCE, ext, extlen);
     OPENSSL_free(ext);
 
diff --git a/crypto/x509/x509_v3.c b/crypto/x509/x509_v3.c
@@ -156,6 +156,7 @@ STACK_OF(X509_EXTENSION) *X509v3_add_extensions(STACK_OF(X509_EXTENSION) **targe
     for (i = 0; i < sk_X509_EXTENSION_num(exts); i++) {
         X509_EXTENSION *ext = sk_X509_EXTENSION_value(exts, i);
         ASN1_OBJECT *obj = X509_EXTENSION_get_object(ext);
+        ASN1_OCTET_STRING *encoded = X509_EXTENSION_get_data(ext);
         int idx = X509v3_get_ext_by_OBJ(*target, obj, -1);
 
         /* Does extension exist in target? */
@@ -166,6 +167,15 @@ STACK_OF(X509_EXTENSION) *X509v3_add_extensions(STACK_OF(X509_EXTENSION) **targe
                 idx = X509v3_get_ext_by_OBJ(*target, obj, -1);
             } while (idx != -1);
         }
+        switch (OBJ_obj2nid(obj)) {
+        case NID_subject_key_identifier:
+        case NID_authority_key_identifier:
+            if (ASN1_STRING_length(encoded) <= 2) /* indicating "none" */
+                continue;
+            break;
+        default:
+            break;
+        }
         if (!X509v3_add_ext(target, ext, -1))
             return NULL;
     }
diff --git a/doc/man3/X509v3_get_ext_by_NID.pod b/doc/man3/X509v3_get_ext_by_NID.pod
@@ -90,8 +90,13 @@ The passed extension I<ex> is duplicated so it must be freed after use.
 X509v3_add_extensions() adds the list of extensions I<exts> to STACK I<*target>.
 The STACK I<*target> is returned unchanged if I<exts> is NULL or an empty list.
 Otherwise a new stack is allocated if I<*target> is NULL.
-An extension to be added
-that has the same OID as a pre-existing one replaces this earlier one.
+An extension to be added that has the same OID as a pre-existing one replaces
+this earlier one. However in case the added extensions contain just an
+empty AKID and/or SKID extension, that extension is not added.
+Only the pre-existing extensions of the same type are deleted.
+In that case it is possible that the returned stack is an empty array.
+It is recommended to call this functions with an empty array instead of
+NULL I<*target> and drop the empty array return afterwards.
 
 X509_get_ext_count(), X509_get_ext(), X509_get_ext_by_NID(),
 X509_get_ext_by_OBJ(), X509_get_ext_by_critical(), X509_delete_ext()
diff --git a/doc/man5/x509v3_config.pod b/doc/man5/x509v3_config.pod
@@ -210,11 +210,6 @@ Otherwise it may have the value B<keyid> or B<issuer>
 or both of them, separated by C<,>.
 Either or both can have the option B<always>,
 indicated by putting a colon C<:> between the value and this option.
-For self-signed certificates the AKID is suppressed unless B<always> is present.
-In order to find out, if the certificate is about to be self-signed, the
-private key as given by B<X509V3_set_issuer_pkey> is compared to the
-public key in the subject_certificate.
-When there is no private key available, the AKID will not be suppressed.
 
 By default the B<x509>, B<req>, and B<ca> apps behave as if B<none> was given
 for self-signed certificates and B<keyid>C<,> B<issuer> otherwise.
@@ -223,7 +218,8 @@ If B<keyid> is present, an attempt is made to
 copy the subject key identifier (SKID) from the issuer certificate except if
 the issuer certificate does not have the subject key identifier extension.
 The hash of the public key related to the signing key is taken as fallback
-if the issuer certificate is the same as the current certificate.
+if the issuer certificate is the same as the current certificate and the
+public key of the issuer certificate does not match the signing key.
 If B<always> is present but no value can be obtained, an error is returned.
 
 If B<issuer> is present, and in addition it has the option B<always> specified
diff --git a/test/recipes/25-test_req.t b/test/recipes/25-test_req.t
diff --git a/test/recipes/25-test_x509.t b/test/recipes/25-test_x509.t
