From cfca56dfaee0518c2cd99a9c5cda29ad557380e1 Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Wed, 26 Feb 2020 21:41:47 +0100 Subject: [PATCH] Fix handling of CMP msg senderKID and improve doc of related CTX functions Reviewed-by: Matt Caswell Reviewed-by: David von Oheimb (Merged from https://github.com/openssl/openssl/pull/11142) --- crypto/cmp/cmp_hdr.c | 36 ++++++++++++++----------- crypto/cmp/cmp_local.h | 1 + crypto/cmp/cmp_protect.c | 15 ++++++++++- doc/internal/man3/ossl_cmp_hdr_init.pod | 7 +++++ doc/man3/OSSL_CMP_CTX_new.pod | 35 +++++++++++------------- test/cmp_hdr_test.c | 13 ++------- test/cmp_protect_test.c | 35 +++++++++++++++++++++++- 7 files changed, 93 insertions(+), 49 deletions(-) diff --git a/crypto/cmp/cmp_hdr.c b/crypto/cmp/cmp_hdr.c index b5a9e9ba9e..9d6d6ceb2c 100644 --- a/crypto/cmp/cmp_hdr.c +++ b/crypto/cmp/cmp_hdr.c @@ -63,31 +63,42 @@ ASN1_OCTET_STRING *OSSL_CMP_HDR_get0_recipNonce(const OSSL_CMP_PKIHEADER *hdr) return hdr->recipNonce; } +int ossl_cmp_general_name_is_NULL_DN(GENERAL_NAME *name) +{ + X509_NAME *null = X509_NAME_new(); + int res = name == NULL || null == NULL + || (name->type == GEN_DIRNAME + && X509_NAME_cmp(name->d.directoryName, null) == 0); + + X509_NAME_free(null); + return res; +} + /* assign to *tgt a copy of src (which may be NULL to indicate an empty DN) */ static int set1_general_name(GENERAL_NAME **tgt, const X509_NAME *src) { - GENERAL_NAME *gen; + GENERAL_NAME *name; if (!ossl_assert(tgt != NULL)) return 0; - if ((gen = GENERAL_NAME_new()) == NULL) + if ((name = GENERAL_NAME_new()) == NULL) goto err; - gen->type = GEN_DIRNAME; + name->type = GEN_DIRNAME; if (src == NULL) { /* NULL-DN */ - if ((gen->d.directoryName = X509_NAME_new()) == NULL) + if ((name->d.directoryName = X509_NAME_new()) == NULL) goto err; - } else if (!X509_NAME_set(&gen->d.directoryName, src)) { + } else if (!X509_NAME_set(&name->d.directoryName, src)) { goto err; } GENERAL_NAME_free(*tgt); - *tgt = gen; + *tgt = name; return 1; err: - GENERAL_NAME_free(gen); + GENERAL_NAME_free(name); return 0; } @@ -265,19 +276,12 @@ int ossl_cmp_hdr_init(OSSL_CMP_CTX *ctx, OSSL_CMP_PKIHEADER *hdr) if (!ossl_cmp_hdr_set_pvno(hdr, OSSL_CMP_PVNO)) return 0; - sender = ctx->clCert != NULL ? - X509_get_subject_name(ctx->clCert) : ctx->subjectName; /* * The sender name is copied from the subject of the client cert, if any, * or else from the subject name provided for certification requests. - * As required by RFC 4210 section 5.1.1., if the sender name is not known - * to the client it set to NULL-DN. In this case for identification at least - * the senderKID must be set, which we take from any referenceValue given. */ - if (sender == NULL && ctx->referenceValue == NULL) { - CMPerr(0, CMP_R_MISSING_SENDER_IDENTIFICATION); - return 0; - } + sender = ctx->clCert != NULL ? + X509_get_subject_name(ctx->clCert) : ctx->subjectName; if (!ossl_cmp_hdr_set1_sender(hdr, sender)) return 0; diff --git a/crypto/cmp/cmp_local.h b/crypto/cmp/cmp_local.h index ebc42d8c52..353c7ce995 100644 --- a/crypto/cmp/cmp_local.h +++ b/crypto/cmp/cmp_local.h @@ -798,6 +798,7 @@ int ossl_cmp_pkisi_check_pkifailureinfo(const OSSL_CMP_PKISI *si, int index); int ossl_cmp_hdr_set_pvno(OSSL_CMP_PKIHEADER *hdr, int pvno); int ossl_cmp_hdr_get_pvno(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); int ossl_cmp_hdr_set1_recipient(OSSL_CMP_PKIHEADER *hdr, const X509_NAME *nm); int ossl_cmp_hdr_update_messageTime(OSSL_CMP_PKIHEADER *hdr); diff --git a/crypto/cmp/cmp_protect.c b/crypto/cmp/cmp_protect.c index c1b4b8584d..ce20ef203e 100644 --- a/crypto/cmp/cmp_protect.c +++ b/crypto/cmp/cmp_protect.c @@ -286,6 +286,8 @@ int ossl_cmp_msg_protect(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg) * to section 5.1.1 */ subjKeyIDStr = X509_get0_subject_key_id(ctx->clCert); + if (subjKeyIDStr == NULL) + subjKeyIDStr = ctx->referenceValue; /* fallback */ if (subjKeyIDStr != NULL && !ossl_cmp_hdr_set1_senderKID(msg->header, subjKeyIDStr)) goto err; @@ -306,7 +308,18 @@ int ossl_cmp_msg_protect(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg) } } - return 1; + /* + * As required by RFC 4210 section 5.1.1., if the sender name is not known + * to the client it set to NULL-DN. In this case for identification at least + * the senderKID must be set, where we took the referenceValue as fallback. + */ + + if (ossl_cmp_general_name_is_NULL_DN(msg->header->sender) + && msg->header->senderKID == NULL) + CMPerr(0, CMP_R_MISSING_SENDER_IDENTIFICATION); + else + return 1; + err: CMPerr(0, CMP_R_ERROR_PROTECTING_MESSAGE); return 0; diff --git a/doc/internal/man3/ossl_cmp_hdr_init.pod b/doc/internal/man3/ossl_cmp_hdr_init.pod index 31468a567b..5df7486cb0 100644 --- a/doc/internal/man3/ossl_cmp_hdr_init.pod +++ b/doc/internal/man3/ossl_cmp_hdr_init.pod @@ -5,6 +5,7 @@ ossl_cmp_hdr_set_pvno, ossl_cmp_hdr_get_pvno, ossl_cmp_hdr_get0_sendernonce, +ossl_cmp_general_name_is_NULL_DN, ossl_cmp_hdr_set1_sender, ossl_cmp_hdr_set1_recipient, ossl_cmp_hdr_update_messagetime, @@ -26,6 +27,7 @@ ossl_cmp_hdr_init int ossl_cmp_hdr_get_pvno(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); int ossl_cmp_hdr_set1_recipient(OSSL_CMP_PKIHEADER *hdr, const X509_NAME *nm); @@ -52,6 +54,9 @@ ossl_cmp_hdr_get_pvno() returns the pvno of the given B or -1 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 +is the NULL-DN. + ossl_cmp_hdr_set1_sender() sets the sender field in the given PKIHeader to the given X509 Name value, without consuming the pointer. @@ -107,6 +112,8 @@ 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_general_name_is_NULL_DN() returns 1 given a NULL-DN, else 0. + All other functions return 1 on success, 0 on error. See the individual functions above. diff --git a/doc/man3/OSSL_CMP_CTX_new.pod b/doc/man3/OSSL_CMP_CTX_new.pod index 626f7d65af..032ef817c0 100644 --- a/doc/man3/OSSL_CMP_CTX_new.pod +++ b/doc/man3/OSSL_CMP_CTX_new.pod @@ -362,7 +362,6 @@ The transfer callback may make use of a custom defined argument stored in the ctx by means of OSSL_CMP_CTX_set_transfer_cb_arg(), which may be retrieved again through OSSL_CMP_CTX_get_transfer_cb_arg(). - OSSL_CMP_CTX_set_transfer_cb_arg() sets an argument, respectively a pointer to a structure containing arguments, optionally to be used by the transfer callback. B is not consumed, and it must therefore explicitly be freed when not @@ -372,9 +371,9 @@ OSSL_CMP_CTX_get_transfer_cb_arg() gets the argument, respectively the pointer to a structure containing arguments, previously set by OSSL_CMP_CTX_set_transfer_cb_arg() or NULL if unset. -OSSL_CMP_CTX_set1_srvCert() pins the server certificate to be directly trusted -(even if it is expired) for verifying response messages. -The cert pointer is not consumed. It may be NULL to clear the entry. +OSSL_CMP_CTX_set1_srvCert() pins the given server certificate B +directly trusts it (even if it is expired) for verifying response messages. +The B argument may be NULL to clear the entry. OSSL_CMP_CTX_set1_expected_sender() sets the Distinguished Name (DN) expected to be given in the sender response for messages protected with MSG_SIG_ALG. This @@ -384,8 +383,7 @@ identify the server certificate. This can be used to ensure that only a particular entity is accepted to act as CMP server, and attackers are not able to use arbitrary certificates of a trusted PKI hierarchy to fraudulently pose as server. -This defaults to the subject DN of the certificate set via -OSSL_CMP_CTX_set1_srvCert(), if any. +This defaults to the subject of the B, if any. OSSL_CMP_CTX_set0_trustedStore() sets the X509_STORE type certificate store containing trusted (root) CA certificates. The certificate store may also hold @@ -441,15 +439,15 @@ the B value is taken as the fallback value for the senderKID. OSSL_CMP_CTX_set1_recipient() sets the recipient name that will be used in the PKIHeader of a request message, i.e. the X509 name of the (CA) server. -Setting is overruled by subject of srvCert if set. -If neither srvCert nor recipient are set, the recipient of the PKI message is +Setting is overruled by subject of B if set. +If neither B nor recipient are set, the recipient of the PKI message is determined in the following order: issuer, issuer of old cert (oldCert), -issuer of client cert (clCert), else NULL-DN. +issuer of client cert (B), else NULL-DN. When a response is received, its sender must match the recipient of the request. OSSL_CMP_CTX_push0_geninfo_ITAV() adds B to the stack in the B to be added to the GeneralInfo field of the CMP PKIMessage header of a request -message sent with this context. Consumes the pointer to B. +message sent with this context. OSSL_CMP_CTX_set1_extraCertsOut() sets the stack of extraCerts that will be sent to remote. @@ -470,24 +468,22 @@ will be set in the CertTemplate, i.e., the X509 name of the CA server. OSSL_CMP_CTX_set1_subjectName() sets the subject DN that will be used in the CertTemplate structure when requesting a new cert. For Key Update Requests -(KUR), it defaults to the subject DN of the reference certificate, +(KUR), it defaults to the subject DN of the B, see B. This default is used for Initialization Requests (IR) and Certification Requests (CR) only if no SANs are set. - -If clCert is not set (e.g. in case of IR with MSG_MAC_ALG), the subject DN -is also used as sender of the PKI message. +The B is also used as the "sender" field for outgoing CMP messages +if no B has been set (e.g., in case requests are protected using PBM). OSSL_CMP_CTX_push1_subjectAltName() adds the given X509 name to the list of alternate names on the certificate template request. This cannot be used if any Subject Alternative Name extension is set via OSSL_CMP_CTX_set0_reqExtensions(). By default, unless OSSL_CMP_OPT_SUBJECTALTNAME_NODEFAULT has been set, -the Subject Alternative Names are copied from the reference certificate, -see OSSL_CMP_CTX_set1_oldCert(). - +the Subject Alternative Names are copied from the B, +see B. If set and the subject DN is not set with OSSL_CMP_CTX_set1_subjectName(), then the certificate template of an IR and CR will not be filled with the default -subject DN from the reference certificate (see OSSL_CMP_CTX_set1_oldCert(). +subject DN from the B. If a subject DN is desired it needs to be set explicitly with OSSL_CMP_CTX_set1_subjectName(). @@ -503,7 +499,7 @@ to the X509_EXTENSIONS of the requested certificate template. OSSL_CMP_CTX_set1_oldCert() sets the old certificate to be updated in Key Update Requests (KUR) or to be revoked in Revocation Requests (RR). It must be given for RR, else it defaults to B. -The reference certificate determined in this way, if any, is also used for +The B determined in this way, if any, is also used for deriving default subject DN and Subject Alternative Names for IR, CR, and KUR. Its issuer, if any, is used as default recipient in the CMP message header. @@ -511,7 +507,6 @@ OSSL_CMP_CTX_set1_p10CSR() sets the PKCS#10 CSR to be used in P10CR. OSSL_CMP_CTX_push0_genm_ITAV() adds B to the stack in the B which will be the body of a General Message sent with this context. -Consumes the pointer to B. OSSL_CMP_CTX_set_certConf_cb() sets the callback used for evaluating the newly enrolled certificate before the library sends, depending on its result, diff --git a/test/cmp_hdr_test.c b/test/cmp_hdr_test.c index 25d0dad9f6..feba118c44 100644 --- a/test/cmp_hdr_test.c +++ b/test/cmp_hdr_test.c @@ -397,7 +397,7 @@ static int execute_HDR_init_test(CMP_HDR_TEST_FIXTURE *fixture) return 1; } -static int test_HDR_init(void) +static int test_HDR_init_with_ref(void) { SETUP_TEST_FIXTURE(CMP_HDR_TEST_FIXTURE, set_up); unsigned char ref[CMP_TEST_REFVALUE_LENGTH]; @@ -431,14 +431,6 @@ static int test_HDR_init_with_subject(void) return result; } -static int test_HDR_init_no_ref_no_subject(void) -{ - SETUP_TEST_FIXTURE(CMP_HDR_TEST_FIXTURE, set_up); - fixture->expected = 0; - EXECUTE_TEST(execute_HDR_init_test, tear_down); - return result; -} - void cleanup_tests(void) { @@ -464,9 +456,8 @@ int setup_tests(void) /* also tests public function OSSL_CMP_HDR_get0_transactionID(): */ /* also tests public function OSSL_CMP_HDR_get0_recipNonce(): */ /* also tests internal function ossl_cmp_hdr_get_pvno(): */ - ADD_TEST(test_HDR_init); + ADD_TEST(test_HDR_init_with_ref); ADD_TEST(test_HDR_init_with_subject); - ADD_TEST(test_HDR_init_no_ref_no_subject); /* * TODO make sure that total number of tests (here currently 24) is shown, * also for other cmp_*text.c. Currently the test drivers always show 1. diff --git a/test/cmp_protect_test.c b/test/cmp_protect_test.c index 5d5df89abd..a506ec33ea 100644 --- a/test/cmp_protect_test.c +++ b/test/cmp_protect_test.c @@ -278,6 +278,38 @@ static int test_MSG_protect_no_key_no_secret(void) return result; } +static int test_MSG_protect_pbmac_no_sender(int with_ref) +{ + static unsigned char secret[] = { 47, 11, 8, 15 }; + static unsigned char ref[] = { 0xca, 0xfe, 0xba, 0xbe }; + + SETUP_TEST_FIXTURE(CMP_PROTECT_TEST_FIXTURE, set_up); + fixture->expected = with_ref; + if (!TEST_ptr(fixture->msg = OSSL_CMP_MSG_dup(ir_unprotected)) + || !SET_OPT_UNPROTECTED_SEND(fixture->cmp_ctx, 0) + || !ossl_cmp_hdr_set1_sender(fixture->msg->header, NULL) + || !OSSL_CMP_CTX_set1_secretValue(fixture->cmp_ctx, + secret, sizeof(secret)) + || (!OSSL_CMP_CTX_set1_referenceValue(fixture->cmp_ctx, + with_ref ? ref : NULL, + sizeof(ref)))) { + tear_down(fixture); + fixture = NULL; + } + EXECUTE_TEST(execute_MSG_protect_test, tear_down); + return result; +} + +static int test_MSG_protect_pbmac_no_sender_with_ref(void) +{ + return test_MSG_protect_pbmac_no_sender(1); +} + +static int test_MSG_protect_pbmac_no_sender_no_ref(void) +{ + return test_MSG_protect_pbmac_no_sender(0); +} + static int execute_MSG_add_extraCerts_test(CMP_PROTECT_TEST_FIXTURE *fixture) { return TEST_true(ossl_cmp_msg_add_extraCerts(fixture->cmp_ctx, @@ -511,7 +543,8 @@ int setup_tests(void) ADD_TEST(test_MSG_protect_certificate_based_without_cert); ADD_TEST(test_MSG_protect_unprotected_request); ADD_TEST(test_MSG_protect_no_key_no_secret); - + ADD_TEST(test_MSG_protect_pbmac_no_sender_with_ref); + ADD_TEST(test_MSG_protect_pbmac_no_sender_no_ref); ADD_TEST(test_MSG_add_extraCerts); #ifndef OPENSSL_NO_EC -- 2.25.1