Fix empty AKID/SKID handling

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.

Author: bernd-edlinger

apps/ca.c

@@ -1698,6 +1698,8 @@ static int do_body(X509 **xret, EVP_PKEY *pkey, X509 *x509,
             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) {

apps/include/apps.h

@@ -264,6 +264,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);

apps/lib/apps.c

@@ -2334,28 +2334,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;
@@ -2371,30 +2367,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 */
 

apps/req.c

@@ -854,6 +854,8 @@ int req_main(int argc, char **argv)
                     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 */

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,34 @@ 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 (!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;
     }

crypto/cmp/cmp_msg.c

@@ -327,6 +327,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))

crypto/x509/v3_akid.c

@@ -179,13 +179,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);

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)
                 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);

crypto/x509/x509_req.c

@@ -160,7 +160,8 @@ STACK_OF(X509_EXTENSION) *X509_REQ_get_extensions(OSSL_FUTURE_CONST X509_REQ *re
 
 /*
  * 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);
 

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;
     }