CMP client: fix checking new cert enrolled with oldcert and without private key

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from https://github.com/openssl/openssl/pull/20832)

8 files changed, 74 insertions, 36 deletions

diff --git a/crypto/cmp/cmp_client.c b/crypto/cmp/cmp_client.c
index 74d544ab0e..46c3519810 100644
--- a/crypto/cmp/cmp_client.c
+++ b/crypto/cmp/cmp_client.c
@@ -411,12 +411,10 @@ static X509 *get1_cert_status(OSSL_CMP_CTX *ctx, int bodytype,
 {
     char buf[OSSL_CMP_PKISI_BUFLEN];
     X509 *crt = NULL;
-    EVP_PKEY *privkey;
 
     if (!ossl_assert(ctx != NULL && crep != NULL))
         return NULL;
 
-    privkey = OSSL_CMP_CTX_get0_newPkey(ctx, 1);
     switch (ossl_cmp_pkisi_get_status(crep->status)) {
     case OSSL_CMP_PKISTATUS_waiting:
         ossl_cmp_err(ctx,
@@ -454,7 +452,7 @@ static X509 *get1_cert_status(OSSL_CMP_CTX *ctx, int bodytype,
         ERR_raise(ERR_LIB_CMP, CMP_R_UNKNOWN_PKISTATUS);
         goto err;
     }
-    crt = ossl_cmp_certresponse_get1_cert(crep, ctx, privkey);
+    crt = ossl_cmp_certresponse_get1_cert(ctx, crep);
     if (crt == NULL) /* according to PKIStatus, we can expect a cert */
         ERR_raise(ERR_LIB_CMP, CMP_R_CERTIFICATE_NOT_FOUND);
 
@@ -560,7 +558,7 @@ static int cert_response(OSSL_CMP_CTX *ctx, int sleep, int rid,
                          OSSL_CMP_MSG **resp, int *checkAfter,
                          int req_type, int expected_type)
 {
-    EVP_PKEY *rkey = OSSL_CMP_CTX_get0_newPkey(ctx /* may be NULL */, 0);
+    EVP_PKEY *rkey = ossl_cmp_ctx_get0_newPubkey(ctx);
     int fail_info = 0; /* no failure */
     const char *txt = NULL;
     OSSL_CMP_CERTREPMESSAGE *crepmsg;
diff --git a/crypto/cmp/cmp_ctx.c b/crypto/cmp/cmp_ctx.c
index 7dd832481a..ed15f45489 100644
--- a/crypto/cmp/cmp_ctx.c
+++ b/crypto/cmp/cmp_ctx.c
@@ -755,6 +755,7 @@ int OSSL_CMP_CTX_set0_newPkey(OSSL_CMP_CTX *ctx, int priv, EVP_PKEY *pkey)
 }
 
 /* Get the private/public key to use for cert enrollment, or NULL on error */
+/* In case |priv| == 0, better use ossl_cmp_ctx_get0_newPubkey() below */
 EVP_PKEY *OSSL_CMP_CTX_get0_newPkey(const OSSL_CMP_CTX *ctx, int priv)
 {
     if (ctx == NULL) {
@@ -769,6 +770,21 @@ EVP_PKEY *OSSL_CMP_CTX_get0_newPkey(const OSSL_CMP_CTX *ctx, int priv)
     return ctx->pkey; /* may be NULL */
 }
 
+EVP_PKEY *ossl_cmp_ctx_get0_newPubkey(const OSSL_CMP_CTX *ctx)
+{
+    if (!ossl_assert(ctx != NULL))
+        return NULL;
+    if (ctx->newPkey != NULL)
+        return ctx->newPkey;
+    if (ctx->p10CSR != NULL)
+        return X509_REQ_get0_pubkey(ctx->p10CSR);
+    if (ctx->oldCert != NULL)
+        return X509_get0_pubkey(ctx->oldCert);
+    if (ctx->cert != NULL)
+        return X509_get0_pubkey(ctx->cert);
+    return ctx->pkey;
+}
+
 #define DEFINE_set1_ASN1_OCTET_STRING(PREFIX, FIELD) \
 int PREFIX##_set1_##FIELD(OSSL_CMP_CTX *ctx, const ASN1_OCTET_STRING *id) \
 { \
diff --git a/crypto/cmp/cmp_local.h b/crypto/cmp/cmp_local.h
index 7f69ad025f..507496e149 100644
--- a/crypto/cmp/cmp_local.h
+++ b/crypto/cmp/cmp_local.h
@@ -792,6 +792,7 @@ int ossl_cmp_ctx_set1_extraCertsIn(OSSL_CMP_CTX *ctx,
                                    STACK_OF(X509) *extraCertsIn);
 int ossl_cmp_ctx_set1_recipNonce(OSSL_CMP_CTX *ctx,
                                  const ASN1_OCTET_STRING *nonce);
+EVP_PKEY *ossl_cmp_ctx_get0_newPubkey(const OSSL_CMP_CTX *ctx);
 
 /* from cmp_status.c */
 int ossl_cmp_pkisi_get_status(const OSSL_CMP_PKISI *si);
@@ -905,8 +906,8 @@ ossl_cmp_pollrepcontent_get0_pollrep(const OSSL_CMP_POLLREPCONTENT *prc,
 OSSL_CMP_CERTRESPONSE *
 ossl_cmp_certrepmessage_get0_certresponse(const OSSL_CMP_CERTREPMESSAGE *crm,
                                           int rid);
-X509 *ossl_cmp_certresponse_get1_cert(const OSSL_CMP_CERTRESPONSE *crep,
-                                      const OSSL_CMP_CTX *ctx, EVP_PKEY *pkey);
+X509 *ossl_cmp_certresponse_get1_cert(const OSSL_CMP_CTX *ctx,
+                                      const OSSL_CMP_CERTRESPONSE *crep);
 OSSL_CMP_MSG *ossl_cmp_msg_load(const char *file);
 
 /* from cmp_protect.c */
diff --git a/crypto/cmp/cmp_msg.c b/crypto/cmp/cmp_msg.c
index fa4815c2e1..689a31a767 100644
--- a/crypto/cmp/cmp_msg.c
+++ b/crypto/cmp/cmp_msg.c
@@ -273,7 +273,7 @@ OSSL_CRMF_MSG *OSSL_CMP_CTX_setup_CRM(OSSL_CMP_CTX *ctx, int for_KUR, int rid)
     OSSL_CRMF_MSG *crm = NULL;
     X509 *refcert = ctx->oldCert != NULL ? ctx->oldCert : ctx->cert;
     /* refcert defaults to current client cert */
-    EVP_PKEY *rkey = OSSL_CMP_CTX_get0_newPkey(ctx, 0);
+    EVP_PKEY *rkey = ossl_cmp_ctx_get0_newPubkey(ctx);
     STACK_OF(GENERAL_NAME) *default_sans = NULL;
     const X509_NAME *ref_subj =
         refcert != NULL ? X509_get_subject_name(refcert) : NULL;
@@ -285,12 +285,6 @@ OSSL_CRMF_MSG *OSSL_CMP_CTX_setup_CRM(OSSL_CMP_CTX *ctx, int for_KUR, int rid)
     /* RFC5280: subjectAltName MUST be critical if subject is null */
     X509_EXTENSIONS *exts = NULL;
 
-    if (rkey == NULL && ctx->p10CSR != NULL)
-        rkey = X509_REQ_get0_pubkey(ctx->p10CSR);
-    if (rkey == NULL && refcert != NULL)
-        rkey = X509_get0_pubkey(refcert);
-    if (rkey == NULL)
-        rkey = ctx->pkey; /* default is independent of ctx->oldCert */
     if (rkey == NULL) {
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
         ERR_raise(ERR_LIB_CMP, CMP_R_MISSING_PUBLIC_KEY);
@@ -410,13 +404,7 @@ OSSL_CMP_MSG *ossl_cmp_certreq_new(OSSL_CMP_CTX *ctx, int type,
     if (type != OSSL_CMP_PKIBODY_P10CR) {
         EVP_PKEY *privkey = OSSL_CMP_CTX_get0_newPkey(ctx, 1);
 
-        /*
-         * privkey is NULL in case ctx->newPkey does not include a private key.
-         * We then may try to use ctx->pkey as fallback/default, but only
-         * if ctx-> newPkey does not include a (non-matching) public key:
-         */
-        if (privkey == NULL && OSSL_CMP_CTX_get0_newPkey(ctx, 0) == NULL)
-            privkey = ctx->pkey; /* default is independent of ctx->oldCert */
+        /* privkey is ctx->newPkey (if private, else NULL) or ctx->pkey */
         if (ctx->popoMethod >= OSSL_CRMF_POPO_SIGNATURE && privkey == NULL) {
             ERR_raise(ERR_LIB_CMP, CMP_R_MISSING_PRIVATE_KEY_FOR_POPO);
             goto err;
@@ -1047,14 +1035,15 @@ ossl_cmp_certrepmessage_get0_certresponse(const OSSL_CMP_CERTREPMESSAGE *crm,
 
 /*-
  * Retrieve the newly enrolled certificate from the given certResponse crep.
- * In case of indirect POPO uses the libctx and propq from ctx and private key.
+ * Uses libctx and propq from ctx, in case of indirect POPO also private key.
  * Returns a pointer to a copy of the found certificate, or NULL if not found.
  */
-X509 *ossl_cmp_certresponse_get1_cert(const OSSL_CMP_CERTRESPONSE *crep,
-                                      const OSSL_CMP_CTX *ctx, EVP_PKEY *pkey)
+X509 *ossl_cmp_certresponse_get1_cert(const OSSL_CMP_CTX *ctx,
+                                      const OSSL_CMP_CERTRESPONSE *crep)
 {
     OSSL_CMP_CERTORENCCERT *coec;
     X509 *crt = NULL;
+    EVP_PKEY *pkey;
 
     if (!ossl_assert(crep != NULL && ctx != NULL))
         return NULL;
@@ -1067,6 +1056,8 @@ X509 *ossl_cmp_certresponse_get1_cert(const OSSL_CMP_CERTRESPONSE *crep,
             break;
         case OSSL_CMP_CERTORENCCERT_ENCRYPTEDCERT:
             /* cert encrypted for indirect PoP; RFC 4210, 5.2.8.2 */
+            pkey = OSSL_CMP_CTX_get0_newPkey(ctx, 1);
+            /* pkey is ctx->newPkey (if private, else NULL) or ctx->pkey */
             if (pkey == NULL) {
                 ERR_raise(ERR_LIB_CMP, CMP_R_MISSING_PRIVATE_KEY);
                 return NULL;
diff --git a/crypto/cmp/cmp_vfy.c b/crypto/cmp/cmp_vfy.c
index 1552d94763..d6521d8700 100644
--- a/crypto/cmp/cmp_vfy.c
+++ b/crypto/cmp/cmp_vfy.c
@@ -324,11 +324,10 @@ static int check_cert_path_3gpp(const OSSL_CMP_CTX *ctx,
          * verify that the newly enrolled certificate (which assumed rid ==
          * OSSL_CMP_CERTREQID) can also be validated with the same trusted store
          */
-        EVP_PKEY *pkey = OSSL_CMP_CTX_get0_newPkey(ctx, 1);
         OSSL_CMP_CERTRESPONSE *crep =
             ossl_cmp_certrepmessage_get0_certresponse(msg->body->value.ip,
                                                       OSSL_CMP_CERTREQID);
-        X509 *newcrt = ossl_cmp_certresponse_get1_cert(crep, ctx, pkey);
+        X509 *newcrt = ossl_cmp_certresponse_get1_cert(ctx, crep);
 
         /*
          * maybe better use get_cert_status() from cmp_client.c, which catches
diff --git a/doc/internal/man3/ossl_cmp_pkisi_get_status.pod b/doc/internal/man3/ossl_cmp_pkisi_get_status.pod
index 21f6f90b39..135be39ed6 100644
--- a/doc/internal/man3/ossl_cmp_pkisi_get_status.pod
+++ b/doc/internal/man3/ossl_cmp_pkisi_get_status.pod
@@ -43,8 +43,8 @@ ossl_cmp_pkisi_check_pkifailureinfo
 # define OSSL_CMP_PKIFAILUREINFO_duplicateCertReq    26
 # define OSSL_CMP_PKIFAILUREINFO_MAX                 26
 
-  X509 *ossl_cmp_certresponse_get1_cert(const OSSL_CMP_CERTRESPONSE *crep,
-                                        const OSSL_CMP_CTX *ctx, EVP_PKEY *pkey);
+  X509 *ossl_cmp_certresponse_get1_cert(const OSSL_CMP_CTX *ctx,
+                                        const OSSL_CMP_CERTRESPONSE *crep);
   int ossl_cmp_pkisi_get_status(const OSSL_CMP_PKISI *si);
   const char *ossl_cmp_PKIStatus_to_string(int status);
   OSSL_CMP_PKIFREETEXT *ossl_cmp_pkisi_get0_statusString(const OSSL_CMP_PKISI *si);
@@ -55,7 +55,7 @@ ossl_cmp_pkisi_check_pkifailureinfo
 
 ossl_cmp_certresponse_get1_cert() returns a pointer to a copy of the newly
 enrolled certificate from the given certResponse I<crep>, or NULL on error.
-In case of indirect POPO uses data from the I<ctx> and the private key I<pkey>.
+Uses data from I<ctx>, which in case of indirect POPO includes the private key.
 
 ossl_cmp_pkisi_get_status() returns the PKIStatus of I<si>, or -1 on error.
 
diff --git a/test/cmp_client_test.c b/test/cmp_client_test.c
index 7b33490d8e..5782a91868 100644
--- a/test/cmp_client_test.c
+++ b/test/cmp_client_test.c
@@ -77,6 +77,7 @@ static CMP_SES_TEST_FIXTURE *set_up(const char *const test_case_name)
             || !OSSL_CMP_CTX_set_option(ctx, OSSL_CMP_OPT_UNPROTECTED_ERRORS, 1)
             || !OSSL_CMP_CTX_set1_oldCert(ctx, client_cert)
             || !OSSL_CMP_CTX_set1_pkey(ctx, client_key)
+            /* client_key is by default used also for newPkey */
             || !OSSL_CMP_CTX_set1_srvCert(ctx, server_cert)
             || !OSSL_CMP_CTX_set1_referenceValue(ctx, ref, sizeof(ref)))
         goto err;
@@ -252,26 +253,57 @@ static int test_exec_CR_ses_implicit_confirm(void)
         && test_exec_CR_ses(1, 1 /* granted */, 0);
 }
 
-static int test_exec_KUR_ses(int transfer_error)
+static int test_exec_KUR_ses(int transfer_error, int pubkey, int raverified)
 {
     SETUP_TEST_FIXTURE(CMP_SES_TEST_FIXTURE, set_up);
     fixture->req_type = OSSL_CMP_KUR;
+    /* ctx->oldCert has already been set */
+
     if (transfer_error)
         OSSL_CMP_CTX_set_transfer_cb_arg(fixture->cmp_ctx, NULL);
-    fixture->expected = transfer_error ? OSSL_CMP_PKISTATUS_trans
-        : OSSL_CMP_PKISTATUS_accepted;
+    if (pubkey) {
+        EVP_PKEY *key = raverified /* wrong key */ ? server_key : client_key;
+
+        EVP_PKEY_up_ref(key);
+        OSSL_CMP_CTX_set0_newPkey(fixture->cmp_ctx, 0 /* not priv */, key);
+        OSSL_CMP_SRV_CTX_set_accept_raverified(fixture->srv_ctx, 1);
+    }
+    if (pubkey || raverified)
+        OSSL_CMP_CTX_set_option(fixture->cmp_ctx, OSSL_CMP_OPT_POPO_METHOD,
+                                OSSL_CRMF_POPO_RAVERIFIED);
+    fixture->expected = transfer_error ? OSSL_CMP_PKISTATUS_trans :
+        raverified ? OSSL_CMP_PKISTATUS_rejection : OSSL_CMP_PKISTATUS_accepted;
     EXECUTE_TEST(execute_exec_certrequest_ses_test, tear_down);
     return result;
 }
 
 static int test_exec_KUR_ses_ok(void)
 {
-    return test_exec_KUR_ses(0);
+    return test_exec_KUR_ses(0, 0, 0);
 }
 
 static int test_exec_KUR_ses_transfer_error(void)
 {
-    return test_exec_KUR_ses(1);
+    return test_exec_KUR_ses(1, 0, 0);
+}
+
+static int test_exec_KUR_ses_wrong_popo(void)
+{
+#ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION /* cf ossl_cmp_verify_popo() */
+    return test_exec_KUR_ses(0, 0, 1);
+#else
+    return 1;
+#endif
+}
+
+static int test_exec_KUR_ses_pub(void)
+{
+    return test_exec_KUR_ses(0, 1, 0);
+}
+
+static int test_exec_KUR_ses_wrong_pub(void)
+{
+    return test_exec_KUR_ses(0, 1, 1);
 }
 
 static int test_certConf_cb(OSSL_CMP_CTX *ctx, X509 *cert, int fail_info,
@@ -497,6 +529,9 @@ int setup_tests(void)
     ADD_TEST(test_exec_IR_ses_poll_total_timeout);
     ADD_TEST(test_exec_KUR_ses_ok);
     ADD_TEST(test_exec_KUR_ses_transfer_error);
+    ADD_TEST(test_exec_KUR_ses_wrong_popo);
+    ADD_TEST(test_exec_KUR_ses_pub);
+    ADD_TEST(test_exec_KUR_ses_wrong_pub);
     ADD_TEST(test_exec_P10CR_ses_ok);
     ADD_TEST(test_exec_P10CR_ses_reject);
     ADD_TEST(test_try_certreq_poll);
diff --git a/test/cmp_msg_test.c b/test/cmp_msg_test.c
index 1f288f3b8d..4438b53cb3 100644
--- a/test/cmp_msg_test.c
+++ b/test/cmp_msg_test.c
@@ -382,7 +382,6 @@ static int execute_certrep_create(CMP_MSG_TEST_FIXTURE *fixture)
     OSSL_CMP_CTX *ctx = fixture->cmp_ctx;
     OSSL_CMP_CERTREPMESSAGE *crepmsg = OSSL_CMP_CERTREPMESSAGE_new();
     OSSL_CMP_CERTRESPONSE *read_cresp, *cresp = OSSL_CMP_CERTRESPONSE_new();
-    EVP_PKEY *privkey;
     X509 *certfromresp = NULL;
     int res = 0;
 
@@ -404,8 +403,7 @@ static int execute_certrep_create(CMP_MSG_TEST_FIXTURE *fixture)
         goto err;
     if (!TEST_ptr_null(ossl_cmp_certrepmessage_get0_certresponse(crepmsg, 88)))
         goto err;
-    privkey = OSSL_CMP_CTX_get0_newPkey(ctx, 1); /* may be NULL */
-    certfromresp = ossl_cmp_certresponse_get1_cert(read_cresp, ctx, privkey);
+    certfromresp = ossl_cmp_certresponse_get1_cert(ctx, read_cresp);
     if (certfromresp == NULL || !TEST_int_eq(X509_cmp(cert, certfromresp), 0))
         goto err;