Browse Source

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)
master
Dr. David von Oheimb 2 years ago
parent
commit
430efff1b9
8 changed files with 98 additions and 102 deletions
  1. +2
    -2
      crypto/cmp/cmp_client.c
  2. +2
    -2
      crypto/cmp/cmp_local.h
  3. +2
    -2
      crypto/cmp/cmp_server.c
  4. +26
    -27
      crypto/cmp/cmp_vfy.c
  5. +25
    -26
      doc/internal/man3/ossl_cmp_msg_check_update.pod
  6. +1
    -1
      fuzz/cmp.c
  7. +2
    -1
      test/cmp_client_test.c
  8. +38
    -41
      test/cmp_vfy_test.c

+ 2
- 2
crypto/cmp/cmp_client.c View File

@ -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


+ 2
- 2
crypto/cmp/cmp_local.h View File

@ -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 */


+ 2
- 2
crypto/cmp/cmp_server.c View File

@ -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) {


+ 26
- 27
crypto/cmp/cmp_vfy.c View File

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


doc/internal/man3/ossl_cmp_msg_check_received.pod → doc/internal/man3/ossl_cmp_msg_check_update.pod View File


+ 1
- 1
fuzz/cmp.c View File

@ -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:


+ 2
- 1
test/cmp_client_test.c View File

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


+ 38
- 41
test/cmp_vfy_test.c View File

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


Loading…
Cancel
Save