From a0745e2be6635ffdf286ba5bc3bd867c8d4152a9 Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Fri, 28 Aug 2020 12:11:31 +0200 Subject: Clean up CMP chain building for CMP signer, TLS client, and newly enrolled certs * Use strenghtened cert chain building, verifying chain using optional trust store while making sure that no certificate status (e.g., CRL) checks are done * Use OSSL_CMP_certConf_cb() by default and move its doc to OSSL_CMP_CTX_new.pod * Simplify certificate and cert store loading in apps/cmp.c Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/12741) --- crypto/cmp/cmp_client.c | 46 +++++++++++++++++++++++++++++++++++----------- crypto/cmp/cmp_ctx.c | 13 +++++-------- crypto/cmp/cmp_local.h | 1 + crypto/cmp/cmp_protect.c | 47 ++++++++++++++++++++++++++--------------------- 4 files changed, 67 insertions(+), 40 deletions(-) (limited to 'crypto') diff --git a/crypto/cmp/cmp_client.c b/crypto/cmp/cmp_client.c index d5a4f3c..4f8a9e2 100644 --- a/crypto/cmp/cmp_client.c +++ b/crypto/cmp/cmp_client.c @@ -22,6 +22,7 @@ #include "openssl/cmp_util.h" DEFINE_STACK_OF(ASN1_UTF8STRING) +DEFINE_STACK_OF(X509) DEFINE_STACK_OF(X509_CRL) DEFINE_STACK_OF(OSSL_CMP_CERTRESPONSE) DEFINE_STACK_OF(OSSL_CMP_PKISI) @@ -487,14 +488,35 @@ int OSSL_CMP_certConf_cb(OSSL_CMP_CTX *ctx, X509 *cert, int fail_info, const char **text) { X509_STORE *out_trusted = OSSL_CMP_CTX_get_certConf_cb_arg(ctx); + STACK_OF(X509) *chain = NULL; (void)text; /* make (artificial) use of var to prevent compiler warning */ if (fail_info != 0) /* accept any error flagged by CMP core library */ return fail_info; - if (out_trusted != NULL - && !OSSL_CMP_validate_cert_path(ctx, out_trusted, cert)) - fail_info = 1 << OSSL_CMP_PKIFAILUREINFO_incorrectData; + ossl_cmp_debug(ctx, "trying to build chain for newly enrolled cert"); + chain = ossl_cmp_build_cert_chain(ctx->libctx, ctx->propq, + out_trusted /* may be NULL */, + ctx->untrusted, cert); + if (sk_X509_num(chain) > 0) + X509_free(sk_X509_shift(chain)); /* remove leaf (EE) cert */ + if (out_trusted != NULL) { + if (chain == NULL) { + ossl_cmp_err(ctx, "failed building chain for newly enrolled cert"); + fail_info = 1 << OSSL_CMP_PKIFAILUREINFO_incorrectData; + } else { + ossl_cmp_debug(ctx, + "succeeded building proper chain for newly enrolled cert"); + } + } else if (chain == NULL) { + ossl_cmp_warn(ctx, "could not build approximate chain for newly enrolled cert, resorting to received extraCerts"); + chain = OSSL_CMP_CTX_get1_extraCertsIn(ctx); + } else { + ossl_cmp_debug(ctx, + "success building approximate chain for newly enrolled cert"); + } + (void)ossl_cmp_ctx_set1_newChain(ctx, chain); + sk_X509_pop_free(chain, X509_free); return fail_info; } @@ -515,10 +537,14 @@ static int cert_response(OSSL_CMP_CTX *ctx, int sleep, int rid, const char *txt = NULL; OSSL_CMP_CERTREPMESSAGE *crepmsg; OSSL_CMP_CERTRESPONSE *crep; + OSSL_CMP_certConf_cb_t cb; X509 *cert; char *subj = NULL; int ret = 1; + if (!ossl_assert(ctx != NULL)) + return 0; + retry: crepmsg = (*resp)->body->value.ip; /* same for cp and kup */ if (sk_OSSL_CMP_CERTRESPONSE_num(crepmsg->response) > 1) { @@ -584,21 +610,19 @@ static int cert_response(OSSL_CMP_CTX *ctx, int sleep, int rid, * OSSL_CMP_PKISTATUS_rejection, fail_info, txt) * not throwing CMP_R_CERTIFICATE_NOT_ACCEPTED with txt * not returning 0 - * since we better leave this for any ctx->certConf_cb to decide + * since we better leave this for the certConf_cb to decide */ } /* - * Execute the certification checking callback function possibly set in ctx, + * Execute the certification checking callback function, * which can determine whether to accept a newly enrolled certificate. * It may overrule the pre-decision reflected in 'fail_info' and '*txt'. */ - if (ctx->certConf_cb - && (fail_info = ctx->certConf_cb(ctx, ctx->newCert, - fail_info, &txt)) != 0) { - if (txt == NULL) - txt = "CMP client application did not accept it"; - } + cb = ctx->certConf_cb != NULL ? ctx->certConf_cb : OSSL_CMP_certConf_cb; + if ((fail_info = cb(ctx, ctx->newCert, fail_info, &txt)) != 0 + && txt == NULL) + txt = "CMP client did not accept it"; if (fail_info != 0) /* immediately log error before any certConf exchange */ ossl_cmp_log1(ERROR, ctx, "rejecting newly enrolled cert with subject: %s", subj); diff --git a/crypto/cmp/cmp_ctx.c b/crypto/cmp/cmp_ctx.c index 5b61108..6bbd351 100644 --- a/crypto/cmp/cmp_ctx.c +++ b/crypto/cmp/cmp_ctx.c @@ -189,6 +189,7 @@ void OSSL_CMP_CTX_free(OSSL_CMP_CTX *ctx) sk_X509_pop_free(ctx->untrusted, X509_free); X509_free(ctx->cert); + sk_X509_pop_free(ctx->chain, X509_free); EVP_PKEY_free(ctx->pkey); ASN1_OCTET_STRING_free(ctx->referenceValue); if (ctx->secretValue != NULL) @@ -489,11 +490,7 @@ int ossl_cmp_ctx_set1_newChain(OSSL_CMP_CTX *ctx, STACK_OF(X509) *newChain) return (ctx->newChain = X509_chain_up_ref(newChain)) != NULL; } -/* - * Returns the stack of certificates received in a response message. - * The stack is duplicated so the caller must handle freeing it! - * Returns pointer to created stack on success, NULL on error - */ +/* Returns the stack of extraCerts received in CertRepMessage, NULL on error */ STACK_OF(X509) *OSSL_CMP_CTX_get1_extraCertsIn(const OSSL_CMP_CTX *ctx) { if (ctx == NULL) { @@ -523,7 +520,7 @@ int ossl_cmp_ctx_set1_extraCertsIn(OSSL_CMP_CTX *ctx, } /* - * Duplicate and set the given stack as the new stack of X509 + * Copies any given stack as the new stack of X509 * certificates to send out in the extraCerts field. */ int OSSL_CMP_CTX_set1_extraCertsOut(OSSL_CMP_CTX *ctx, @@ -596,7 +593,7 @@ STACK_OF(X509) *OSSL_CMP_CTX_get1_caPubs(const OSSL_CMP_CTX *ctx) } /* - * Duplicate and copy the given stack of certificates to the given + * Copies any given stack of certificates to the given * OSSL_CMP_CTX structure so that they may be retrieved later. */ int ossl_cmp_ctx_set1_caPubs(OSSL_CMP_CTX *ctx, STACK_OF(X509) *caPubs) @@ -766,7 +763,7 @@ int OSSL_CMP_CTX_build_cert_chain(OSSL_CMP_CTX *ctx, X509_STORE *own_trusted, return 0; } ossl_cmp_debug(ctx, "success building chain for own CMP signer cert"); - sk_X509_pop_free(chain, X509_free); /* TODO(3.0) replace this by 'ctx->chain = chain;' when ctx->chain is available */ + ctx->chain = chain; return 1; } diff --git a/crypto/cmp/cmp_local.h b/crypto/cmp/cmp_local.h index d5ac7a5..434f9e0 100644 --- a/crypto/cmp/cmp_local.h +++ b/crypto/cmp/cmp_local.h @@ -71,6 +71,7 @@ struct ossl_cmp_ctx_st { /* client authentication */ int unprotectedSend; /* send unprotected PKI messages */ X509 *cert; /* protection cert used to identify and sign for MSG_SIG_ALG */ + STACK_OF(X509) *chain; /* (cached) chain of protection cert including it */ EVP_PKEY *pkey; /* the key pair corresponding to cert */ ASN1_OCTET_STRING *referenceValue; /* optional user name for MSG_MAC_ALG */ ASN1_OCTET_STRING *secretValue; /* password/shared secret for MSG_MAC_ALG */ diff --git a/crypto/cmp/cmp_protect.c b/crypto/cmp/cmp_protect.c index b65de09..6313cc9 100644 --- a/crypto/cmp/cmp_protect.c +++ b/crypto/cmp/cmp_protect.c @@ -139,32 +139,37 @@ int ossl_cmp_msg_add_extraCerts(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg) && (msg->extraCerts = sk_X509_new_null()) == NULL) return 0; - if (ctx->cert != NULL && ctx->pkey != NULL) { - /* make sure that our own cert is included in the first position */ - if (!X509_add_cert(msg->extraCerts, ctx->cert, - X509_ADD_FLAG_UP_REF | X509_ADD_FLAG_NO_DUP - | X509_ADD_FLAG_PREPEND)) - return 0; - /* if we have untrusted certs, try to add intermediate certs */ - if (ctx->untrusted != NULL) { - STACK_OF(X509) *chain; - int res; + /* Add first ctx->cert and its chain if using signature-based protection */ + if (!ctx->unprotectedSend && ctx->secretValue == NULL) { + int flags_prepend = X509_ADD_FLAG_UP_REF | X509_ADD_FLAG_NO_DUP + | X509_ADD_FLAG_PREPEND | X509_ADD_FLAG_NO_SS; + /* if not yet done try to build chain using available untrusted certs */ + if (ctx->chain == NULL) { ossl_cmp_debug(ctx, "trying to build chain for own CMP signer cert"); - chain = ossl_cmp_build_cert_chain(ctx->libctx, ctx->propq, NULL, - ctx->untrusted, ctx->cert); - res = X509_add_certs(msg->extraCerts, chain, - X509_ADD_FLAG_UP_REF | X509_ADD_FLAG_NO_DUP - | X509_ADD_FLAG_NO_SS); - sk_X509_pop_free(chain, X509_free); - if (res == 0) { - ossl_cmp_err(ctx, - "could not build chain for own CMP signer cert"); - return 0; + ctx->chain = + ossl_cmp_build_cert_chain(ctx->libctx, ctx->propq, NULL, + ctx->untrusted, ctx->cert); + if (ctx->chain != NULL) { + ossl_cmp_debug(ctx, + "success building chain for own CMP signer cert"); + } else { + /* dump errors to avoid confusion when printing further ones */ + OSSL_CMP_CTX_print_errors(ctx); + ossl_cmp_warn(ctx, + "could not build chain for own CMP signer cert"); } + } + if (ctx->chain != NULL) { + if (!X509_add_certs(msg->extraCerts, ctx->chain, flags_prepend)) + return 0; + } else { + /* make sure that at least our own signer cert is included first */ + if (!X509_add_cert(msg->extraCerts, ctx->cert, flags_prepend)) + return 0; ossl_cmp_debug(ctx, - "succeeded building chain for own CMP signer cert"); + "fallback: adding just own CMP signer cert"); } } -- cgit v1.1