review

apps/cmp.c

@@ -268,9 +268,9 @@ typedef enum OPTION_choice {
     OPT_SRV_REF, OPT_SRV_SECRET,
     OPT_SRV_CERT, OPT_SRV_KEY, OPT_SRV_KEYPASS,
     OPT_SRV_TRUSTED, OPT_SRV_UNTRUSTED,
-    OPT_REF_CERT, OPT_RSP_CERT, OPT_RSP_CRL, OPT_RSP_EXTRACERTS,
-    OPT_RSP_CAPUBS, OPT_RSP_NEWWITHNEW, OPT_RSP_NEWWITHOLD,
-    OPT_RSP_OLDWITHNEW, OPT_POLL_COUNT, OPT_CHECK_AFTER,
+    OPT_REF_CERT, OPT_RSP_CERT, OPT_RSP_CRL, OPT_RSP_EXTRACERTS, OPT_RSP_CAPUBS,
+    OPT_RSP_NEWWITHNEW, OPT_RSP_NEWWITHOLD, OPT_RSP_OLDWITHNEW,
+    OPT_POLL_COUNT, OPT_CHECK_AFTER,
     OPT_GRANT_IMPLICITCONF,
     OPT_PKISTATUS, OPT_FAILURE,
     OPT_FAILUREBITS, OPT_STATUSSTRING,
@@ -1166,7 +1166,7 @@ static OSSL_CMP_SRV_CTX *setup_srv_ctx(ENGINE *engine)
                         (add_X509_fn_t)ossl_cmp_mock_srv_set1_certOut))
             goto err;
     }
-    if (!setup_crl(srv_ctx, opt_rsp_crl, "CRL the mock server returns",
+    if (!setup_crl(srv_ctx, opt_rsp_crl, "CRL to be returned by the mock server",
                    (add_X509_CRL_fn_t)ossl_cmp_mock_srv_set1_crlOut))
         goto err;
     if (!setup_certs(opt_rsp_extracerts,
@@ -2219,14 +2219,14 @@ static int write_cert(BIO *bio, X509 *cert)
 
 static int write_crl(BIO *bio, X509_CRL *crl)
 {
-    if ((opt_certform == FORMAT_PEM && PEM_write_bio_X509_CRL(bio, crl))
-            || (opt_certform == FORMAT_ASN1 && i2d_X509_CRL_bio(bio, crl)))
-        return 1;
-    if (opt_certform != FORMAT_PEM && opt_certform != FORMAT_ASN1)
-        BIO_printf(bio_err,
-                   "error: unsupported type '%s' for writing CRLs\n",
+    if (opt_certform != FORMAT_PEM && opt_certform != FORMAT_ASN1) {
+        BIO_printf(bio_err, "error: unsupported type '%s' for writing CRLs\n",
                    opt_certform_s);
-    return 0;
+        return 0;
+    }
+
+    return opt_certform == FORMAT_PEM ? PEM_write_bio_X509_CRL(bio, crl)
+                                      : i2d_X509_CRL_bio(bio, crl);
 }
 
 /*
@@ -2345,7 +2345,7 @@ static int save_crl_or_delete(X509_CRL *crl, const char *file, const char *desc)
     if (crl == NULL) {
         char desc_crl[80];
 
-        BIO_snprintf(desc_crl, sizeof(desc_crl), "%s CRL", desc);
+        BIO_snprintf(desc_crl, sizeof(desc_crl), "%s", desc);
         return delete_file(file, desc_crl);
     } else {
         return save_free_crl(crl, file, desc);

apps/lib/cmp_mock_srv.c

@@ -408,7 +408,7 @@ static OSSL_CMP_PKISI *process_rr(OSSL_CMP_SRV_CTX *srv_ctx,
     return OSSL_CMP_PKISI_dup(ctx->statusOut);
 }
 
-/* return -1 for error */
+/* return -1 for error, 0 for no update available */
 static int check_client_crl(const STACK_OF(OSSL_CMP_CRLSTATUS) *crlStatusList,
                             const X509_CRL *crl)
 {
@@ -417,19 +417,17 @@ static int check_client_crl(const STACK_OF(OSSL_CMP_CRLSTATUS) *crlStatusList,
     GENERAL_NAMES *gen;
     ASN1_TIME *thisupd = NULL;
 
-    if (crlStatusList == NULL || crl == NULL)
-        return 0;
-    if (sk_OSSL_CMP_CRLSTATUS_num(crlStatusList) != 1)
+    if (sk_OSSL_CMP_CRLSTATUS_num(crlStatusList) != 1) {
+        ERR_raise(ERR_LIB_CMP, CMP_R_UNEXPECTED_CRLSTATUSLIST);
+        return -1;
+    }
+    if (crl == NULL)
         return 0;
 
     crlstatus = sk_OSSL_CMP_CRLSTATUS_value(crlStatusList, 0);
     if (!OSSL_CMP_CRLSTATUS_get0(crlstatus, &distpoint, &gen, &thisupd))
         return -1;
 
-    if (thisupd != NULL
-        && ASN1_TIME_compare(thisupd, X509_CRL_get0_lastUpdate(crl)) >= 0)
-        return 0;
-
     if (gen != NULL) {
         GENERAL_NAME *gn = sk_GENERAL_NAME_value(gen, 0);
 
@@ -443,7 +441,8 @@ static int check_client_crl(const STACK_OF(OSSL_CMP_CRLSTATUS) *crlStatusList,
         }
     }
 
-    return 1;
+    return thisupd == NULL
+        || ASN1_TIME_compare(thisupd, X509_CRL_get0_lastUpdate(crl)) < 0;
 }
 
 static OSSL_CMP_ITAV *process_genm_itav(mock_srv_ctx *ctx, int req_nid,

crypto/cmp/cmp_asn.c

@@ -427,7 +427,7 @@ static GENERAL_NAMES *gennames_new(const X509_NAME *nm)
 
     if ((names = sk_GENERAL_NAME_new_reserve(NULL, 1)) == NULL)
         return NULL;
-    if (!GENERAL_NAME_create(&name, nm)) {
+    if (!GENERAL_NAME_set1_X509_NAME(&name, nm)) {
         sk_GENERAL_NAME_free(names);
         return NULL;
     }
@@ -459,11 +459,11 @@ OSSL_CMP_CRLSTATUS *OSSL_CMP_CRLSTATUS_create(const X509_CRL *crl,
     int i, NID_akid = NID_authority_key_identifier;
 
     /*
-     * Note: X509{,_CRL}_get_ext_d2i(..., NID, &i, ...) return the 1st extension
-     * with the given NID that is available, if any. There might be more such.
+     * Note: X509{,_CRL}_get_ext_d2i(..., NID, ..., NULL) return the 1st extension
+     * with the given NID that is available, if any. If there are more, this is an error.
      */
     if (cert != NULL) {
-        crldps = X509_get_ext_d2i(cert, NID_crl_distribution_points, &i, NULL);
+        crldps = X509_get_ext_d2i(cert, NID_crl_distribution_points, NULL, NULL);
         /* if available, take the first suitable element */
         for (i = 0; i < sk_DIST_POINT_num(crldps); i++) {
             DIST_POINT *dp = sk_DIST_POINT_value(crldps, i);
@@ -484,21 +484,21 @@ OSSL_CMP_CRLSTATUS *OSSL_CMP_CRLSTATUS_create(const X509_CRL *crl,
             return NULL;
         }
         idp = X509_CRL_get_ext_d2i(crl,
-                                   NID_issuing_distribution_point, &i, NULL);
+                                   NID_issuing_distribution_point, NULL, NULL);
         if (idp != NULL && idp->distpoint != NULL)
             dpn = idp->distpoint;
     }
 
     if (dpn == NULL && CRLissuer == NULL) {
         if (cert != NULL) {
-            akid = X509_get_ext_d2i(cert, NID_akid, &i, NULL);
+            akid = X509_get_ext_d2i(cert, NID_akid, NULL, NULL);
             if (akid != NULL && gennames_allowed(akid->issuer, only_DN))
                 CRLissuer = akid->issuer;
             else
                 CRLissuer = issuers = gennames_new(X509_get_issuer_name(cert));
         }
         if (CRLissuer == NULL && crl != NULL) {
-            akid = X509_CRL_get_ext_d2i(crl, NID_akid, &i, NULL);
+            akid = X509_CRL_get_ext_d2i(crl, NID_akid, NULL, NULL);
             if (akid != NULL && gennames_allowed(akid->issuer, only_DN))
                 CRLissuer = akid->issuer;
             else
@@ -556,11 +556,12 @@ OSSL_CMP_ITAV *OSSL_CMP_ITAV_new_crls(const X509_CRL *crl)
     if ((itav = OSSL_CMP_ITAV_new()) == NULL)
         return NULL;
 
-    if (crl != NULL
-        && ((crls = sk_X509_CRL_new_reserve(NULL, 1)) == NULL
-            || (crl_copy = X509_CRL_dup(crl)) == NULL
-            || !sk_X509_CRL_push(crls, crl_copy)))
-        goto err;
+    if (crl != NULL) {
+        if ((crls = sk_X509_CRL_new_reserve(NULL, 1)) == NULL
+                || (crl_copy = X509_CRL_dup(crl)) == NULL)
+            goto err;
+        (void)sk_X509_CRL_push(crls, crl_copy); /* cannot fail */
+    }
 
     itav->infoType = OBJ_nid2obj(NID_id_it_crls);
     itav->infoValue.crls = crls;
@@ -586,7 +587,7 @@ int OSSL_CMP_ITAV_get0_crls(const OSSL_CMP_ITAV *itav, STACK_OF(X509_CRL) **out)
     return 1;
 }
 
-/* get ASN.1 encoded integer, return -1 on error */
+/* get ASN.1 encoded integer, return -2 on error; -1 is valid for certReqId */
 int ossl_cmp_asn1_get_int(const ASN1_INTEGER *a)
 {
     int64_t res;

crypto/cmp/cmp_err.c

@@ -150,6 +150,8 @@ static const ERR_STRING_DATA CMP_str_reasons[] = {
     {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_UNCLEAN_CTX), "unclean ctx"},
     {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_UNEXPECTED_CERTPROFILE),
     "unexpected certprofile"},
+    {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_UNEXPECTED_CRLSTATUSLIST),
+    "unexpected crlstatuslist"},
     {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_UNEXPECTED_PKIBODY), "unexpected pkibody"},
     {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_UNEXPECTED_PKISTATUS),
     "unexpected pkistatus"},

crypto/cmp/cmp_genm.c

@@ -356,20 +356,17 @@ int OSSL_CMP_get1_crlUpdate(OSSL_CMP_CTX *ctx, const X509 *crlcert,
     int res = 0;
 
     if (crl == NULL) {
-        ERR_raise_data(ERR_LIB_CMP, CMP_R_NULL_ARGUMENT,
-                       "No crl output parameter given");
+        ERR_raise(ERR_LIB_CMP, CMP_R_NULL_ARGUMENT);
         return 0;
     }
     *crl = NULL;
 
     if ((status = OSSL_CMP_CRLSTATUS_create(last_crl, crlcert, 1)) == NULL) {
-        ERR_raise_data(ERR_LIB_CMP, CMP_R_GENERATE_CRLSTATUS,
-                       "Cannot set up CRLStatus structure");
+        ERR_raise(ERR_LIB_CMP, CMP_R_GENERATE_CRLSTATUS);
         goto end;
     }
     if ((list = sk_OSSL_CMP_CRLSTATUS_new_reserve(NULL, 1)) == NULL) {
-        ERR_raise_data(ERR_LIB_CMP, CMP_R_GENERATE_CRLSTATUS,
-                       "Cannot set up CRLStatus list");
+        ERR_raise(ERR_LIB_CMP, CMP_R_GENERATE_CRLSTATUS);
         goto end;
     }
     (void)sk_OSSL_CMP_CRLSTATUS_push(list, status); /* cannot fail */

crypto/cmp/cmp_hdr.c

@@ -98,14 +98,14 @@ int ossl_cmp_hdr_set1_sender(OSSL_CMP_PKIHEADER *hdr, const X509_NAME *nm)
 {
     if (!ossl_assert(hdr != NULL))
         return 0;
-    return GENERAL_NAME_create(&hdr->sender, nm);
+    return GENERAL_NAME_set1_X509_NAME(&hdr->sender, nm);
 }
 
 int ossl_cmp_hdr_set1_recipient(OSSL_CMP_PKIHEADER *hdr, const X509_NAME *nm)
 {
     if (!ossl_assert(hdr != NULL))
         return 0;
-    return GENERAL_NAME_create(&hdr->recipient, nm);
+    return GENERAL_NAME_set1_X509_NAME(&hdr->recipient, nm);
 }
 
 int ossl_cmp_hdr_update_messageTime(OSSL_CMP_PKIHEADER *hdr)

crypto/err/openssl.txt

@@ -276,6 +276,7 @@ CMP_R_TRANSACTIONID_UNMATCHED:152:transactionid unmatched
 CMP_R_TRANSFER_ERROR:159:transfer error
 CMP_R_UNCLEAN_CTX:191:unclean ctx
 CMP_R_UNEXPECTED_CERTPROFILE:196:unexpected certprofile
+CMP_R_UNEXPECTED_CRLSTATUSLIST:201:unexpected crlstatuslist
 CMP_R_UNEXPECTED_PKIBODY:133:unexpected pkibody
 CMP_R_UNEXPECTED_PKISTATUS:185:unexpected pkistatus
 CMP_R_UNEXPECTED_POLLREQ:105:unexpected pollreq

crypto/x509/v3_genn.c

@@ -58,14 +58,17 @@ GENERAL_NAME *GENERAL_NAME_dup(const GENERAL_NAME *a)
                                     (char *)a);
 }
 
-int GENERAL_NAME_create(GENERAL_NAME **tgt, const X509_NAME *src)
+int GENERAL_NAME_set1_X509_NAME(GENERAL_NAME **tgt, const X509_NAME *src)
 {
     GENERAL_NAME *name;
 
-    if (!ossl_assert(tgt != NULL))
+    if (tgt == NULL){
+        ERR_raise(ERR_LIB_X509V3, X509V3_R_INVALID_NULL_ARGUMENT);
         return 0;
+    }
+
     if ((name = GENERAL_NAME_new()) == NULL)
-        goto err;
+        return 0;
     name->type = GEN_DIRNAME;
 
     if (src == NULL) { /* NULL-DN */

doc/man3/GENERAL_NAME.pod

@@ -3,7 +3,7 @@
 =head1 NAME
 
 GENERAL_NAME,
-GENERAL_NAME_create
+GENERAL_NAME_set1_X509_NAME
 - GENERAL_NAME method routines
 
 =head1 SYNOPSIS
@@ -12,24 +12,24 @@ GENERAL_NAME_create
 
  typedef struct GENERAL_NAME_st GENERAL_NAME;
 
- int GENERAL_NAME_create(GENERAL_NAME **tgt, const X509_NAME *src);
+ int GENERAL_NAME_set1_X509_NAME(GENERAL_NAME **tgt, const X509_NAME *src);
 
 =head1 DESCRIPTION
 
-GENERAL_NAME_create() creates a new GENERAL_NAME of type GEN_DIRNAME
+GENERAL_NAME_set1_X509_NAME() creates a new GENERAL_NAME of type GEN_DIRNAME
 and populates it based on provided X509_NAME I<src> which can be NULL.
-If I<tgt> must not be NULL. If successful, I<*tgt> will be set to point
+I<tgt> must not be NULL. If successful, I<*tgt> will be set to point
 to the newly created GENERAL_NAME.
 
 =head1 NOTES
 =head1 RETURN VALUES
 
-GENERAL_NAME_create() return 1 on success, 0 on error.
+GENERAL_NAME_set1_X509_NAME() return 1 on success, 0 on error.
 
 =head1 SEE ALSO
 =head1 HISTORY
 
-GENERAL_NAME_create() was added in OpenSSL 3.3.
+GENERAL_NAME_set1_X509_NAME() was added in OpenSSL 3.3.
 
 =head1 COPYRIGHT
 

doc/man3/OSSL_CMP_CTX_new.pod

@@ -614,7 +614,8 @@ to the X509_EXTENSIONS of the requested certificate template.
 
 OSSL_CMP_CTX_set1_oldCert() sets the old certificate to be updated in
 Key Update Requests (KUR) or to be revoked in Revocation Requests (RR)
-or to request CRL in General Message with infotype crlStatusList.
+or to be used for specifying a CRL issuer when requesting a CRL 
+in a General Message with infoType B<crlStatusList>.
 For RR, this is ignored if an issuer name and a serial number are provided using
 OSSL_CMP_CTX_set1_issuer() and OSSL_CMP_CTX_set1_serialNumber(), respectively.
 For IR/CR/KUR this sets the I<reference certificate>,

doc/man3/OSSL_CMP_ITAV_new_caCerts.pod

@@ -8,8 +8,6 @@ OSSL_CMP_ITAV_new_rootCaCert,
 OSSL_CMP_ITAV_get0_rootCaCert,
 OSSL_CMP_ITAV_new_rootCaKeyUpdate,
 OSSL_CMP_ITAV_get0_rootCaKeyUpdate,
-OSSL_CMP_ITAV_new0_certReqTemplate,
-OSSL_CMP_ITAV_get1_certReqTemplate,
 OSSL_CMP_CRLSTATUS_new1,
 OSSL_CMP_CRLSTATUS_create,
 OSSL_CMP_CRLSTATUS_get0,
@@ -25,6 +23,7 @@ OSSL_CMP_ITAV_get0_crls
 
  OSSL_CMP_ITAV *OSSL_CMP_ITAV_new_caCerts(const STACK_OF(X509) *caCerts);
  int OSSL_CMP_ITAV_get0_caCerts(const OSSL_CMP_ITAV *itav, STACK_OF(X509) **out);
+
  OSSL_CMP_ITAV *OSSL_CMP_ITAV_new_rootCaCert(const X509 *rootCaCert);
  int OSSL_CMP_ITAV_get0_rootCaCert(const OSSL_CMP_ITAV *itav, X509 **out);
  OSSL_CMP_ITAV *OSSL_CMP_ITAV_new_rootCaKeyUpdate(const X509 *newWithNew,
@@ -34,6 +33,7 @@ OSSL_CMP_ITAV_get0_crls
                                         X509 **newWithNew,
                                         X509 **newWithOld,
                                         X509 **oldWithNew);
+
  OSSL_CMP_CRLSTATUS *OSSL_CMP_CRLSTATUS_new1(const DIST_POINT_NAME *dpn,
                                              const GENERAL_NAMES *issuer,
                                              const ASN1_TIME *thisUpdate);
@@ -89,9 +89,10 @@ that contains either a copy of the distribution point name I<dpn>
 or a copy of the certificate issuer I<issuer>, while giving both is an error.
 If given, a copy of the CRL issuance time I<thisUpdate> is also included.
 
-OSSL_CMP_CRLSTATUS_create() is a high-level variant of OSSL_CMP_CRLSTATUS_new1()
-using data obtained from the I<crl> and/or I<cert> parameters.
-The thisUpdate field is filled with the thisUpdate field of I<crl> if present.
+OSSL_CMP_CRLSTATUS_create() is a high-level variant of OSSL_CMP_CRLSTATUS_new1().
+It fills the thisUpdate field with a copy of the thisUpdate field of I<crl> if present.
+It fills the CRLSource field with a copy of the first data item found using the I<crl>
+and/or I<cert> parameters as follows.
 The CRLSource field is filled with the first data item found in them as follows.
 Any available distribution point name is preferred over issuer names.
 Data from I<cert>, if present, is preferred over data from I<crl>.
@@ -132,8 +133,8 @@ The pointer may be NULL if no CRL status data is included.
 It is an error if the infoType of I<itav> is not B<crlStatusList>.
 
 OSSL_CMP_ITAV_new_crls() creates a new B<OSSL_CMP_ITAV> structure
-of type B<crls> and fills it with a copy of the provided CRL.
-The I<crl> argument may be NULL.
+of type B<crls> including an empty list of CRLs if the I<crl> argument is NULL
+or including a singleton list a with copy of the provided CRL otherwise.
 
 OSSL_CMP_ITAV_get0_crls() on success assigns to I<*out> an internal pointer to
 the list of CRLs contained in the infoValue field of I<itav>.

include/openssl/cmperr.h

@@ -102,6 +102,7 @@
 #  define CMP_R_TRANSFER_ERROR                             159
 #  define CMP_R_UNCLEAN_CTX                                191
 #  define CMP_R_UNEXPECTED_CERTPROFILE                     196
+#  define CMP_R_UNEXPECTED_CRLSTATUSLIST                   201
 #  define CMP_R_UNEXPECTED_PKIBODY                         133
 #  define CMP_R_UNEXPECTED_PKISTATUS                       185
 #  define CMP_R_UNEXPECTED_POLLREQ                         105

include/openssl/x509v3.h.in

@@ -178,7 +178,7 @@ typedef struct ACCESS_DESCRIPTION_st {
     GENERAL_NAME *location;
 } ACCESS_DESCRIPTION;
 
-int GENERAL_NAME_create(GENERAL_NAME **tgt, const X509_NAME *src);
+int GENERAL_NAME_set1_X509_NAME(GENERAL_NAME **tgt, const X509_NAME *src);
 
 {-
     generate_stack_macros("ACCESS_DESCRIPTION")