Improve ossl_cmp_msg_check_received() and rename to ossl_cmp_msg_check_update()

Bugfix: allow using extraCerts contained in msg already while checking signature
Improve function name, simplify its return value, and update its documentation

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/11998)

8 files changed, 98 insertions, 102 deletions

diff --git a/crypto/cmp/cmp_client.c b/crypto/cmp/cmp_client.c
index d309f84a78..f38d8651f4 100644
--- a/crypto/cmp/cmp_client.c
+++ b/crypto/cmp/cmp_client.c
@@ -189,8 +189,8 @@ static int send_receive_check(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *req,
      */
     ossl_cmp_log1(INFO, ctx, "received %s", ossl_cmp_bodytype_to_string(bt));
 
-    if ((bt = ossl_cmp_msg_check_received(ctx, *rep, unprotected_exception,
-                                          expected_type)) < 0)
+    if (!ossl_cmp_msg_check_update(ctx, *rep, unprotected_exception,
+                                   expected_type))
         return 0;
 
     if (bt == expected_type
diff --git a/crypto/cmp/cmp_local.h b/crypto/cmp/cmp_local.h
index 0c0ca3466a..13981ce597 100644
--- a/crypto/cmp/cmp_local.h
+++ b/crypto/cmp/cmp_local.h
@@ -908,8 +908,8 @@ int ossl_cmp_msg_protect(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg);
 typedef int (*ossl_cmp_allow_unprotected_cb_t)(const OSSL_CMP_CTX *ctx,
                                                const OSSL_CMP_MSG *msg,
                                                int invalid_protection, int arg);
-int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
-                                ossl_cmp_allow_unprotected_cb_t cb, int cb_arg);
+int ossl_cmp_msg_check_update(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
+                              ossl_cmp_allow_unprotected_cb_t cb, int cb_arg);
 int ossl_cmp_verify_popo(const OSSL_CMP_MSG *msg, int accept_RAVerified);
 
 /* from cmp_client.c */
diff --git a/crypto/cmp/cmp_server.c b/crypto/cmp/cmp_server.c
index b805dc8bcb..5cb313a32c 100644
--- a/crypto/cmp/cmp_server.c
+++ b/crypto/cmp/cmp_server.c
@@ -508,8 +508,8 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx,
         }
     }
 
-    if (ossl_cmp_msg_check_received(ctx, req, unprotected_exception,
-                                    srv_ctx->acceptUnprotected) < 0)
+    if (!ossl_cmp_msg_check_update(ctx, req, unprotected_exception,
+                                   srv_ctx->acceptUnprotected))
         goto err;
 
     switch (req_type) {
diff --git a/crypto/cmp/cmp_vfy.c b/crypto/cmp/cmp_vfy.c
index d32db60c54..6a25ce0f78 100644
--- a/crypto/cmp/cmp_vfy.c
+++ b/crypto/cmp/cmp_vfy.c
@@ -702,19 +702,29 @@ int OSSL_CMP_validate_msg(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg)
  * learns the senderNonce from the received message,
  * learns the transaction ID if it is not yet in ctx.
  *
- * returns body type (which is >= 0) of the message on success, -1 on error
+ * returns 1 on success, 0 on error
  */
-int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
-                                ossl_cmp_allow_unprotected_cb_t cb, int cb_arg)
+int ossl_cmp_msg_check_update(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
+                              ossl_cmp_allow_unprotected_cb_t cb, int cb_arg)
 {
-    int rcvd_type;
-
     if (!ossl_assert(ctx != NULL && msg != NULL))
-        return -1;
+        return 0;
 
     if (sk_X509_num(msg->extraCerts) > 10)
         ossl_cmp_warn(ctx,
                       "received CMP message contains more than 10 extraCerts");
+    /*
+     * Store any provided extraCerts in ctx for use in OSSL_CMP_validate_msg()
+     * and for future use, such that they are available to ctx->certConf_cb and
+     * the peer does not need to send them again in the same transaction.
+     * Note that it does not help validating the message before storing the
+     * extraCerts because they do not belong to the protected msg part anyway.
+     * For efficiency, the extraCerts are prepended so they get used first.
+     */
+    if (!ossl_cmp_sk_X509_add1_certs(ctx->untrusted_certs, msg->extraCerts,
+                                     0 /* this allows self-issued certs */,
+                                     1 /* no_dups */, 1 /* prepend */))
+        return 0;
 
     /* validate message protection */
     if (msg->header->protectionAlg != 0) {
@@ -723,7 +733,7 @@ int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
                 && (cb == NULL || (*cb)(ctx, msg, 1, cb_arg) <= 0)) {
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
             CMPerr(0, CMP_R_ERROR_VALIDATING_PROTECTION);
-            return -1;
+            return 0;
 #endif
         }
     } else {
@@ -731,7 +741,7 @@ int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
         if (cb == NULL || (*cb)(ctx, msg, 0, cb_arg) <= 0) {
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
             CMPerr(0, CMP_R_MISSING_PROTECTION);
-            return -1;
+            return 0;
 #endif
         }
     }
@@ -740,14 +750,14 @@ int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
     if (ossl_cmp_hdr_get_pvno(OSSL_CMP_MSG_get0_header(msg)) != OSSL_CMP_PVNO) {
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
         CMPerr(0, CMP_R_UNEXPECTED_PVNO);
-        return -1;
+        return 0;
 #endif
     }
 
-    if ((rcvd_type = ossl_cmp_msg_get_bodytype(msg)) < 0) {
+    if (ossl_cmp_msg_get_bodytype(msg) < 0) {
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
         CMPerr(0, CMP_R_PKIBODY_ERROR);
-        return -1;
+        return 0;
 #endif
     }
 
@@ -758,7 +768,7 @@ int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
                                          msg->header->transactionID) != 0)) {
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
         CMPerr(0, CMP_R_TRANSACTIONID_UNMATCHED);
-        return -1;
+        return 0;
 #endif
     }
 
@@ -769,7 +779,7 @@ int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
                                          msg->header->recipNonce) != 0)) {
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
         CMPerr(0, CMP_R_RECIPNONCE_UNMATCHED);
-        return -1;
+        return 0;
 #endif
     }
 
@@ -779,25 +789,14 @@ int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
      * --> Store for setting in next message
      */
     if (!ossl_cmp_ctx_set1_recipNonce(ctx, msg->header->senderNonce))
-        return -1;
+        return 0;
 
     /* if not yet present, learn transactionID */
     if (ctx->transactionID == NULL
         && !OSSL_CMP_CTX_set1_transactionID(ctx, msg->header->transactionID))
-        return -1;
-
-    /*
-     * Store any provided extraCerts in ctx for future use,
-     * such that they are available to ctx->certConf_cb and
-     * the peer does not need to send them again in the same transaction.
-     * For efficiency, the extraCerts are prepended so they get used first.
-     */
-    if (!ossl_cmp_sk_X509_add1_certs(ctx->untrusted_certs, msg->extraCerts,
-                                     0 /* this allows self-issued certs */,
-                                     1 /* no_dups */, 1 /* prepend */))
-        return -1;
+        return 0;
 
-    return rcvd_type;
+    return 1;
 }
 
 int ossl_cmp_verify_popo(const OSSL_CMP_MSG *msg, int accept_RAVerified)
diff --git a/doc/internal/man3/ossl_cmp_msg_check_received.pod b/doc/internal/man3/ossl_cmp_msg_check_update.pod
index 0fd4140356..c058e90ffb 100644
--- a/doc/internal/man3/ossl_cmp_msg_check_received.pod
+++ b/doc/internal/man3/ossl_cmp_msg_check_update.pod
@@ -3,8 +3,8 @@
 =head1 NAME
 
 ossl_cmp_allow_unprotected_cb_t,
-ossl_cmp_msg_check_received
-- does all checks on a received CMP message that can be done generically
+ossl_cmp_msg_check_update
+- generic checks on a received CMP message, updating the context
 
 =head1 SYNOPSIS
 
@@ -14,26 +14,29 @@ ossl_cmp_msg_check_received
                                                 const OSSL_CMP_MSG *msg,
                                                 int invalid_protection, int arg);
 
- int ossl_cmp_msg_check_received(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
-                                 ossl_cmp_allow_unprotected_cb_t cb, int cb_arg);
+ int ossl_cmp_msg_check_update(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
+                               ossl_cmp_allow_unprotected_cb_t cb, int cb_arg);
 
 =head1 DESCRIPTION
 
-ossl_cmp_msg_check_received() checks the given message B<msg>,
-which may be a server response or a request by some client.
+ossl_cmp_msg_check_update() does all generic checks on the given message B<msg>,
+which may be a server response or a request by some client,
+and updates the B<ctx> accordingly.
 
-It is ensured for the B<msg> that
+The B<msg> is checked for the following:
 
 =over 4
 
-=item it has a valid body type,
-
 =item its protection is present and valid (or a callback function B<cb>
 is present and indicates that a missing or invalid protection is acceptable),
 
-=item its recipNonce matches any previous senderNonce stored in B<ctx>, and
+=item its CMP protocol version is acceptable, namely B<OSSL_CMP_PVNO>,
+
+=item its body type is valid,
+
+=item its transaction ID matches any transaction ID given in B<ctx>, and
 
-=item its transaction ID matches any previous transaction ID stored in B<ctx>.
+=item its recipNonce matches any senderNonce given in B<ctx>.
 
 =back
 
@@ -43,28 +46,24 @@ case an invalid protection is present the B<invalid_protection> parameter is 1.
 The callback is passed also the arguments B<ctx>, B<msg>, and <cb_arg>
 (which typically contains the expected message type).
 The callback should return 1 on acceptance, 0 on rejection, or -1 on error.
-It should not put and error on the error stack since this could be misleading.
+It should not put an error on the error stack since this could be misleading.
 
-If all checks pass then ossl_cmp_msg_check_received()
-
-=over 4
-
-=item learns the senderNonce from the received message,
-
-=item learns the transaction ID if it is not yet in B<ctx>, and
-
-=item adds any extraCerts contained in the <msg> to the list of untrusted
-certificates in B<ctx> for future use, such that
-they are available already to the certificate confirmation callback and the
+ossl_cmp_msg_check_update() adds all extraCerts contained in the <msg> to
+the list of untrusted certificates in B<ctx> such that they are already usable
+for OSSL_CMP_validate_msg(), which is called internally, and for future use.
+Thus they are available also to the certificate confirmation callback, and the
 peer does not need to send them again (at least not in the same transaction).
+Note that it does not help validating the message before storing the extraCerts
+because they are not part of the protected portion of the message anyway.
 For efficiency, the extraCerts are prepended to the list so they get used first.
 
-=back
+If all checks pass then ossl_cmp_msg_check_update()
+records in B<ctx> the senderNonce of the received message as the new recipNonce
+and learns the transaction ID if none is currently present in B<ctx>.
 
 =head1 RETURN VALUES
 
-ossl_cmp_msg_check_received() returns the message body type (which is >= 0)
-on success, -1 on error.
+ossl_cmp_msg_check_update() returns 1 on success, -1 on error.
 
 =head1 SEE ALSO
 
diff --git a/fuzz/cmp.c b/fuzz/cmp.c
index 6883a286ff..100350ebfe 100644
--- a/fuzz/cmp.c
+++ b/fuzz/cmp.c
@@ -94,7 +94,7 @@ static void cmp_client_process_response(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg)
                                   OSSL_CMP_ITAV_free);
         break;
     default:
-        (void)ossl_cmp_msg_check_received(ctx, msg, allow_unprotected, 0);
+        (void)ossl_cmp_msg_check_update(ctx, msg, allow_unprotected, 0);
         break;
     }
  err:
diff --git a/test/cmp_client_test.c b/test/cmp_client_test.c
index ab2b930277..2f7fbf7b1e 100644
--- a/test/cmp_client_test.c
+++ b/test/cmp_client_test.c
@@ -252,7 +252,8 @@ static int execute_try_certreq_poll_test(CMP_SES_TEST_FIXTURE *fixture)
         && check_after == CHECK_AFTER
         && TEST_ptr_eq(OSSL_CMP_CTX_get0_newCert(ctx), NULL)
         && TEST_int_eq(fixture->expected, OSSL_CMP_try_certreq(ctx, TYPE, NULL))
-        && TEST_int_eq(0, X509_cmp(OSSL_CMP_CTX_get0_newCert(ctx), client_cert));
+        && TEST_int_eq(0,
+                       X509_cmp(OSSL_CMP_CTX_get0_newCert(ctx), client_cert));
 }
 
 static int test_try_certreq_poll(void)
diff --git a/test/cmp_vfy_test.c b/test/cmp_vfy_test.c
index c74dd2faec..22588aef1a 100644
--- a/test/cmp_vfy_test.c
+++ b/test/cmp_vfy_test.c
@@ -387,19 +387,19 @@ static int test_validate_cert_path_expired(void)
     return result;
 }
 
-static int execute_MSG_check_received_test(CMP_VFY_TEST_FIXTURE *fixture)
+static int execute_msg_check_test(CMP_VFY_TEST_FIXTURE *fixture)
 {
     const OSSL_CMP_PKIHEADER *hdr = OSSL_CMP_MSG_get0_header(fixture->msg);
     const ASN1_OCTET_STRING *tid = OSSL_CMP_HDR_get0_transactionID(hdr);
 
     if (!TEST_int_eq(fixture->expected,
-                     ossl_cmp_msg_check_received(fixture->cmp_ctx,
-                                                 fixture->msg,
-                                                 fixture->allow_unprotected_cb,
-                                                 fixture->additional_arg)))
+                     ossl_cmp_msg_check_update(fixture->cmp_ctx,
+                                               fixture->msg,
+                                               fixture->allow_unprotected_cb,
+                                               fixture->additional_arg)))
         return 0;
 
-    if (fixture->expected < 0) /* error expected aready during above check */
+    if (fixture->expected == 0) /* error expected aready during above check */
         return 1;
     return
         TEST_int_eq(0,
@@ -416,10 +416,10 @@ static int allow_unprotected(const OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg,
     return allow;
 }
 
-static void setup_check_received(CMP_VFY_TEST_FIXTURE **fixture, int expected,
-                                 ossl_cmp_allow_unprotected_cb_t cb, int arg,
-                                 const unsigned char *trid_data,
-                                 const unsigned char *nonce_data)
+static void setup_check_update(CMP_VFY_TEST_FIXTURE **fixture, int expected,
+                               ossl_cmp_allow_unprotected_cb_t cb, int arg,
+                               const unsigned char *trid_data,
+                               const unsigned char *nonce_data)
 {
     OSSL_CMP_CTX *ctx = (*fixture)->cmp_ctx;
     int nonce_len = OSSL_CMP_SENDERNONCE_LENGTH;
@@ -448,33 +448,32 @@ static void setup_check_received(CMP_VFY_TEST_FIXTURE **fixture, int expected,
 }
 
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-static int test_MSG_check_received_no_protection_no_cb(void)
+static int test_msg_check_no_protection_no_cb(void)
 {
     SETUP_TEST_FIXTURE(CMP_VFY_TEST_FIXTURE, set_up);
-    setup_check_received(&fixture, -1, NULL, 0, NULL, NULL);
-    EXECUTE_TEST(execute_MSG_check_received_test, tear_down);
+    setup_check_update(&fixture, 0, NULL, 0, NULL, NULL);
+    EXECUTE_TEST(execute_msg_check_test, tear_down);
     return result;
 }
 
-static int test_MSG_check_received_no_protection_restrictive_cb(void)
+static int test_msg_check_no_protection_restrictive_cb(void)
 {
     SETUP_TEST_FIXTURE(CMP_VFY_TEST_FIXTURE, set_up);
-    setup_check_received(&fixture, -1, allow_unprotected, 0, NULL, NULL);
-    EXECUTE_TEST(execute_MSG_check_received_test, tear_down);
+    setup_check_update(&fixture, 0, allow_unprotected, 0, NULL, NULL);
+    EXECUTE_TEST(execute_msg_check_test, tear_down);
     return result;
 }
 #endif
 
-static int test_MSG_check_received_no_protection_permissive_cb(void)
+static int test_msg_check_no_protection_permissive_cb(void)
 {
     SETUP_TEST_FIXTURE(CMP_VFY_TEST_FIXTURE, set_up);
-    setup_check_received(&fixture, OSSL_CMP_PKIBODY_IP, allow_unprotected, 1,
-                         NULL, NULL);
-    EXECUTE_TEST(execute_MSG_check_received_test, tear_down);
+    setup_check_update(&fixture, 1, allow_unprotected, 1, NULL, NULL);
+    EXECUTE_TEST(execute_msg_check_test, tear_down);
     return result;
 }
 
-static int test_MSG_check_received_check_transaction_id(void)
+static int test_msg_check_transaction_id(void)
 {
     /* Transaction id belonging to CMP_IR_rmprotection.der */
     const unsigned char trans_id[OSSL_CMP_TRANSACTIONID_LENGTH] = {
@@ -483,23 +482,22 @@ static int test_MSG_check_received_check_transaction_id(void)
     };
 
     SETUP_TEST_FIXTURE(CMP_VFY_TEST_FIXTURE, set_up);
-    setup_check_received(&fixture, OSSL_CMP_PKIBODY_IP, allow_unprotected, 1,
-                         trans_id, NULL);
-    EXECUTE_TEST(execute_MSG_check_received_test, tear_down);
+    setup_check_update(&fixture, 1, allow_unprotected, 1, trans_id, NULL);
+    EXECUTE_TEST(execute_msg_check_test, tear_down);
     return result;
 }
 
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-static int test_MSG_check_received_check_transaction_id_bad(void)
+static int test_msg_check_transaction_id_bad(void)
 {
     SETUP_TEST_FIXTURE(CMP_VFY_TEST_FIXTURE, set_up);
-    setup_check_received(&fixture, -1, allow_unprotected, 1, rand_data, NULL);
-    EXECUTE_TEST(execute_MSG_check_received_test, tear_down);
+    setup_check_update(&fixture, 0, allow_unprotected, 1, rand_data, NULL);
+    EXECUTE_TEST(execute_msg_check_test, tear_down);
     return result;
 }
 #endif
 
-static int test_MSG_check_received_check_recipient_nonce(void)
+static int test_msg_check_recipient_nonce(void)
 {
     /* Recipient nonce belonging to CMP_IP_ir_rmprotection.der */
     const unsigned char rec_nonce[OSSL_CMP_SENDERNONCE_LENGTH] = {
@@ -508,18 +506,17 @@ static int test_MSG_check_received_check_recipient_nonce(void)
     };
 
     SETUP_TEST_FIXTURE(CMP_VFY_TEST_FIXTURE, set_up);
-    setup_check_received(&fixture, OSSL_CMP_PKIBODY_IP, allow_unprotected, 1,
-                         NULL, rec_nonce);
-    EXECUTE_TEST(execute_MSG_check_received_test, tear_down);
+    setup_check_update(&fixture, 1, allow_unprotected, 1, NULL, rec_nonce);
+    EXECUTE_TEST(execute_msg_check_test, tear_down);
     return result;
 }
 
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-static int test_MSG_check_received_check_recipient_nonce_bad(void)
+static int test_msg_check_recipient_nonce_bad(void)
 {
     SETUP_TEST_FIXTURE(CMP_VFY_TEST_FIXTURE, set_up);
-    setup_check_received(&fixture, -1, allow_unprotected, 1, NULL, rand_data);
-    EXECUTE_TEST(execute_MSG_check_received_test, tear_down);
+    setup_check_update(&fixture, 0, allow_unprotected, 1, NULL, rand_data);
+    EXECUTE_TEST(execute_msg_check_test, tear_down);
     return result;
 }
 #endif
@@ -629,17 +626,17 @@ int setup_tests(void)
     ADD_TEST(test_validate_cert_path_wrong_anchor);
 
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-    ADD_TEST(test_MSG_check_received_no_protection_no_cb);
-    ADD_TEST(test_MSG_check_received_no_protection_restrictive_cb);
+    ADD_TEST(test_msg_check_no_protection_no_cb);
+    ADD_TEST(test_msg_check_no_protection_restrictive_cb);
 #endif
-    ADD_TEST(test_MSG_check_received_no_protection_permissive_cb);
-    ADD_TEST(test_MSG_check_received_check_transaction_id);
+    ADD_TEST(test_msg_check_no_protection_permissive_cb);
+    ADD_TEST(test_msg_check_transaction_id);
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-    ADD_TEST(test_MSG_check_received_check_transaction_id_bad);
+    ADD_TEST(test_msg_check_transaction_id_bad);
 #endif
-    ADD_TEST(test_MSG_check_received_check_recipient_nonce);
+    ADD_TEST(test_msg_check_recipient_nonce);
 #ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
-    ADD_TEST(test_MSG_check_received_check_recipient_nonce_bad);
+    ADD_TEST(test_msg_check_recipient_nonce_bad);
 #endif
 
     return 1;