From 12bbcee21bf45665f0940f29e57b74281a861c1c Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Fri, 29 May 2020 10:16:06 +0200 Subject: Make CMP server use same protection for response as for request Also adds ossl_cmp_hdr_get_protection_nid() simplifying cmp_vfy.c Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/11998) --- crypto/cmp/cmp_hdr.c | 8 ++++++++ crypto/cmp/cmp_local.h | 1 + crypto/cmp/cmp_protect.c | 6 ++---- crypto/cmp/cmp_server.c | 32 ++++++++++++++++++++++---------- crypto/cmp/cmp_vfy.c | 20 ++++---------------- doc/internal/man3/ossl_cmp_hdr_init.pod | 9 ++++++++- 6 files changed, 45 insertions(+), 31 deletions(-) diff --git a/crypto/cmp/cmp_hdr.c b/crypto/cmp/cmp_hdr.c index 38b3bce3f5..4b98a598cd 100644 --- a/crypto/cmp/cmp_hdr.c +++ b/crypto/cmp/cmp_hdr.c @@ -41,6 +41,14 @@ int ossl_cmp_hdr_get_pvno(const OSSL_CMP_PKIHEADER *hdr) return (int)pvno; } +int ossl_cmp_hdr_get_protection_nid(const OSSL_CMP_PKIHEADER *hdr) +{ + if (!ossl_assert(hdr != NULL) + || hdr->protectionAlg == NULL) + return NID_undef; + return OBJ_obj2nid(hdr->protectionAlg->algorithm); +} + ASN1_OCTET_STRING *OSSL_CMP_HDR_get0_transactionID(const OSSL_CMP_PKIHEADER *hdr) { diff --git a/crypto/cmp/cmp_local.h b/crypto/cmp/cmp_local.h index 13981ce597..0d874ae785 100644 --- a/crypto/cmp/cmp_local.h +++ b/crypto/cmp/cmp_local.h @@ -796,6 +796,7 @@ int ossl_cmp_pkisi_check_pkifailureinfo(const OSSL_CMP_PKISI *si, int index); /* from cmp_hdr.c */ int ossl_cmp_hdr_set_pvno(OSSL_CMP_PKIHEADER *hdr, int pvno); int ossl_cmp_hdr_get_pvno(const OSSL_CMP_PKIHEADER *hdr); +int ossl_cmp_hdr_get_protection_nid(const OSSL_CMP_PKIHEADER *hdr); ASN1_OCTET_STRING *ossl_cmp_hdr_get0_senderNonce(const OSSL_CMP_PKIHEADER *hdr); int ossl_cmp_general_name_is_NULL_DN(GENERAL_NAME *name); int ossl_cmp_hdr_set1_sender(OSSL_CMP_PKIHEADER *hdr, const X509_NAME *nm); diff --git a/crypto/cmp/cmp_protect.c b/crypto/cmp/cmp_protect.c index 5d70c174ee..880051d3dd 100644 --- a/crypto/cmp/cmp_protect.c +++ b/crypto/cmp/cmp_protect.c @@ -259,10 +259,8 @@ int ossl_cmp_msg_protect(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg) goto err; } - if (msg->header->protectionAlg == NULL) - if ((msg->header->protectionAlg = X509_ALGOR_new()) == NULL) - goto err; - + if ((msg->header->protectionAlg = X509_ALGOR_new()) == NULL) + goto err; if (!OBJ_find_sigid_by_algs(&algNID, ctx->digest, EVP_PKEY_id(ctx->pkey))) { CMPerr(0, CMP_R_UNSUPPORTED_KEY_TYPE); diff --git a/crypto/cmp/cmp_server.c b/crypto/cmp/cmp_server.c index c2f0e1a113..8570885eed 100644 --- a/crypto/cmp/cmp_server.c +++ b/crypto/cmp/cmp_server.c @@ -452,8 +452,10 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx, const OSSL_CMP_MSG *req) { OSSL_CMP_CTX *ctx; + ASN1_OCTET_STRING *backup_secret; OSSL_CMP_PKIHEADER *hdr; int req_type, rsp_type; + int res; OSSL_CMP_MSG *rsp = NULL; if (srv_ctx == NULL || srv_ctx->ctx == NULL @@ -463,7 +465,12 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx, return 0; } ctx = srv_ctx->ctx; + backup_secret = ctx->secretValue; + /* + * Some things need to be done already before validating the message in + * order to be able to send an error message as far as needed and possible. + */ if (hdr->sender->type != GEN_DIRNAME) { CMPerr(0, CMP_R_SENDER_GENERALNAME_TYPE_NOT_SUPPORTED); goto err; @@ -506,8 +513,12 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx, } } - if (!ossl_cmp_msg_check_update(ctx, req, unprotected_exception, - srv_ctx->acceptUnprotected)) + res = ossl_cmp_msg_check_update(ctx, req, unprotected_exception, + srv_ctx->acceptUnprotected); + if (ctx->secretValue != NULL && ctx->pkey != NULL + && ossl_cmp_hdr_get_protection_nid(hdr) != NID_id_PasswordBasedMAC) + ctx->secretValue = NULL; /* use MSG_SIG_ALG when protecting rsp */ + if (!res) goto err; switch (req_type) { @@ -573,15 +584,16 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx, } if ((si = OSSL_CMP_STATUSINFO_new(OSSL_CMP_PKISTATUS_rejection, - fail_info, NULL)) == NULL) - return 0; - if (err != 0 && (flags & ERR_TXT_STRING) != 0) - data = ERR_reason_error_string(err); - rsp = ossl_cmp_error_new(srv_ctx->ctx, si, - err != 0 ? ERR_GET_REASON(err) : -1, - data, srv_ctx->sendUnprotectedErrors); - OSSL_CMP_PKISI_free(si); + fail_info, NULL)) != NULL) { + if (err != 0 && (flags & ERR_TXT_STRING) != 0) + data = ERR_reason_error_string(err); + rsp = ossl_cmp_error_new(srv_ctx->ctx, si, + err != 0 ? ERR_GET_REASON(err) : -1, + data, srv_ctx->sendUnprotectedErrors); + OSSL_CMP_PKISI_free(si); + } } + ctx->secretValue = backup_secret; /* possibly close the transaction */ rsp_type = diff --git a/crypto/cmp/cmp_vfy.c b/crypto/cmp/cmp_vfy.c index 6a25ce0f78..9eccb71d1b 100644 --- a/crypto/cmp/cmp_vfy.c +++ b/crypto/cmp/cmp_vfy.c @@ -70,7 +70,7 @@ static int verify_signature(const OSSL_CMP_CTX *cmp_ctx, prot_part_der_len = (size_t) len; /* verify signature of protected part */ - if (!OBJ_find_sigid_algs(OBJ_obj2nid(msg->header->protectionAlg->algorithm), + if (!OBJ_find_sigid_algs(ossl_cmp_hdr_get_protection_nid(msg->header), &digest_nid, &pk_nid) || digest_nid == NID_undef || pk_nid == NID_undef || (digest = EVP_get_digestbynid(digest_nid)) == NULL) { @@ -574,9 +574,6 @@ static int check_msg_find_cert(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg) */ int OSSL_CMP_validate_msg(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg) { - X509_ALGOR *alg; - int nid = NID_undef, pk_nid = NID_undef; - const ASN1_OBJECT *algorOID = NULL; X509 *scrt; const X509_NAME *expected_sender; @@ -605,17 +602,13 @@ int OSSL_CMP_validate_msg(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg) return 0; /* Note: if recipient was NULL-DN it could be learned here if needed */ - if ((alg = msg->header->protectionAlg) == NULL /* unprotected message */ + if (msg->header->protectionAlg == NULL /* unprotected message */ || msg->protection == NULL || msg->protection->data == NULL) { CMPerr(0, CMP_R_MISSING_PROTECTION); return 0; } - /* determine the nid for the used protection algorithm */ - X509_ALGOR_get0(&algorOID, NULL, NULL, alg); - nid = OBJ_obj2nid(algorOID); - - switch (nid) { + switch (ossl_cmp_hdr_get_protection_nid(msg->header)) { /* 5.1.3.1. Shared Secret Information */ case NID_id_PasswordBasedMAC: if (ctx->secretValue == 0) { @@ -665,11 +658,6 @@ int OSSL_CMP_validate_msg(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg) * 5.1.3.3. Signature */ default: - if (!OBJ_find_sigid_algs(OBJ_obj2nid(alg->algorithm), NULL, &pk_nid) - || pk_nid == NID_undef) { - CMPerr(0, CMP_R_UNKNOWN_ALGORITHM_ID); - break; - } scrt = ctx->srvCert; if (scrt == NULL) { if (check_msg_find_cert(ctx, msg)) @@ -727,7 +715,7 @@ int ossl_cmp_msg_check_update(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg, return 0; /* validate message protection */ - if (msg->header->protectionAlg != 0) { + if (msg->header->protectionAlg != NULL) { /* detect explicitly permitted exceptions for invalid protection */ if (!OSSL_CMP_validate_msg(ctx, msg) && (cb == NULL || (*cb)(ctx, msg, 1, cb_arg) <= 0)) { diff --git a/doc/internal/man3/ossl_cmp_hdr_init.pod b/doc/internal/man3/ossl_cmp_hdr_init.pod index cf7f596551..0c6405054f 100644 --- a/doc/internal/man3/ossl_cmp_hdr_init.pod +++ b/doc/internal/man3/ossl_cmp_hdr_init.pod @@ -4,6 +4,7 @@ ossl_cmp_hdr_set_pvno, ossl_cmp_hdr_get_pvno, +ossl_cmp_hdr_get_protection_nid, ossl_cmp_hdr_get0_sendernonce, ossl_cmp_general_name_is_NULL_DN, ossl_cmp_hdr_set1_sender, @@ -25,6 +26,7 @@ ossl_cmp_hdr_init int ossl_cmp_hdr_set_pvno(OSSL_CMP_PKIHEADER *hdr, int pvno); int ossl_cmp_hdr_get_pvno(const OSSL_CMP_PKIHEADER *hdr); + int ossl_cmp_hdr_get_protection_nid(const OSSL_CMP_PKIHEADER *hdr); ASN1_OCTET_STRING *ossl_cmp_hdr_get0_sendernonce(const OSSL_CMP_PKIHEADER *hdr); int ossl_cmp_general_name_is_NULL_DN(GENERAL_NAME *name); @@ -52,6 +54,9 @@ ossl_cmp_hdr_set_pvno() sets hdr->pvno to the given B. ossl_cmp_hdr_get_pvno() returns the pvno of the given B or -1 on error. +ossl_cmp_hdr_get_protection_nid returns the NID of the protection algorithm +in B or NID_undef on error. + ossl_cmp_hdr_get0_sendernonce() returns the sender nonce of the given PKIHeader. ossl_cmp_general_name_is_NULL_DN() determines if the given GENERAL_NAME @@ -110,7 +115,9 @@ CMP is defined in RFC 4210 (and CRMF in RFC 4211). ossl_cmp_hdr_get_pvno() returns the pvno of the given B or -1 on error. -ossl_cmp_hdr_get0_sendernonce() returns the respective nonce. +ossl_cmp_hdr_get_protection_nid returns the respective NID, NID_undef on error. + +ossl_cmp_hdr_get0_sendernonce() returns the respective nonce, or NULL. ossl_cmp_general_name_is_NULL_DN() returns 1 given a NULL-DN, else 0. -- cgit v1.2.3