From 430efff1b9baa36099b5443c924f96b854e00300 Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Thu, 28 May 2020 17:19:36 +0200 Subject: 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 (Merged from https://github.com/openssl/openssl/pull/11998) --- crypto/cmp/cmp_client.c | 4 +- crypto/cmp/cmp_local.h | 4 +- crypto/cmp/cmp_server.c | 4 +- crypto/cmp/cmp_vfy.c | 53 +++++++------- doc/internal/man3/ossl_cmp_msg_check_received.pod | 86 ----------------------- doc/internal/man3/ossl_cmp_msg_check_update.pod | 85 ++++++++++++++++++++++ fuzz/cmp.c | 2 +- test/cmp_client_test.c | 3 +- test/cmp_vfy_test.c | 79 ++++++++++----------- 9 files changed, 158 insertions(+), 162 deletions(-) delete mode 100644 doc/internal/man3/ossl_cmp_msg_check_received.pod create mode 100644 doc/internal/man3/ossl_cmp_msg_check_update.pod 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_received.pod deleted file mode 100644 index 0fd4140356..0000000000 --- a/doc/internal/man3/ossl_cmp_msg_check_received.pod +++ /dev/null @@ -1,86 +0,0 @@ -=pod - -=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 - -=head1 SYNOPSIS - - #include "cmp_local.h" - - 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); - -=head1 DESCRIPTION - -ossl_cmp_msg_check_received() checks the given message B, -which may be a server response or a request by some client. - -It is ensured for the B that - -=over 4 - -=item it has a valid body type, - -=item its protection is present and valid (or a callback function B -is present and indicates that a missing or invalid protection is acceptable), - -=item its recipNonce matches any previous senderNonce stored in B, and - -=item its transaction ID matches any previous transaction ID stored in B. - -=back - -In case no protection is present and B is not NULL then this callback -function is called with its B parameter being 0, while in -case an invalid protection is present the B parameter is 1. -The callback is passed also the arguments B, B, and -(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. - -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, and - -=item adds any extraCerts contained in the to the list of untrusted -certificates in B for future use, such that -they are available already to the certificate confirmation callback and the -peer does not need to send them again (at least not in the same transaction). -For efficiency, the extraCerts are prepended to the list so they get used first. - -=back - -=head1 RETURN VALUES - -ossl_cmp_msg_check_received() returns the message body type (which is >= 0) -on success, -1 on error. - -=head1 SEE ALSO - -L - -=head1 HISTORY - -The OpenSSL CMP support was added in OpenSSL 3.0. - -=head1 COPYRIGHT - -Copyright 2007-2020 The OpenSSL Project Authors. All Rights Reserved. - -Licensed under the Apache License 2.0 (the "License"). You may not use -this file except in compliance with the License. You can obtain a copy -in the file LICENSE in the source distribution or at -L. - -=cut diff --git a/doc/internal/man3/ossl_cmp_msg_check_update.pod b/doc/internal/man3/ossl_cmp_msg_check_update.pod new file mode 100644 index 0000000000..c058e90ffb --- /dev/null +++ b/doc/internal/man3/ossl_cmp_msg_check_update.pod @@ -0,0 +1,85 @@ +=pod + +=head1 NAME + +ossl_cmp_allow_unprotected_cb_t, +ossl_cmp_msg_check_update +- generic checks on a received CMP message, updating the context + +=head1 SYNOPSIS + + #include "cmp_local.h" + + 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_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_update() does all generic checks on the given message B, +which may be a server response or a request by some client, +and updates the B accordingly. + +The B is checked for the following: + +=over 4 + +=item its protection is present and valid (or a callback function B +is present and indicates that a missing or invalid protection is acceptable), + +=item its CMP protocol version is acceptable, namely B, + +=item its body type is valid, + +=item its transaction ID matches any transaction ID given in B, and + +=item its recipNonce matches any senderNonce given in B. + +=back + +In case no protection is present and B is not NULL then this callback +function is called with its B parameter being 0, while in +case an invalid protection is present the B parameter is 1. +The callback is passed also the arguments B, B, and +(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 an error on the error stack since this could be misleading. + +ossl_cmp_msg_check_update() adds all extraCerts contained in the to +the list of untrusted certificates in B 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. + +If all checks pass then ossl_cmp_msg_check_update() +records in B the senderNonce of the received message as the new recipNonce +and learns the transaction ID if none is currently present in B. + +=head1 RETURN VALUES + +ossl_cmp_msg_check_update() returns 1 on success, -1 on error. + +=head1 SEE ALSO + +L + +=head1 HISTORY + +The OpenSSL CMP support was added in OpenSSL 3.0. + +=head1 COPYRIGHT + +Copyright 2007-2020 The OpenSSL Project Authors. All Rights Reserved. + +Licensed under the Apache License 2.0 (the "License"). You may not use +this file except in compliance with the License. You can obtain a copy +in the file LICENSE in the source distribution or at +L. + +=cut 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; -- cgit v1.2.3