From 11baa470a21b514ab247071e80273ddc0a80c504 Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Wed, 17 Jun 2020 08:12:19 +0200 Subject: [PATCH] Fix CMP -days option range checking and test failing with enable-ubsan Reviewed-by: Nicola Tuveri (Merged from https://github.com/openssl/openssl/pull/12175) --- apps/cmp.c | 9 +++-- crypto/cmp/cmp_ctx.c | 12 +++--- crypto/cmp/cmp_err.c | 3 ++ crypto/cmp/cmp_msg.c | 15 ++++--- crypto/crmf/crmf_lib.c | 26 ++++-------- crypto/err/openssl.txt | 5 ++- ...ty.pod => OSSL_CRMF_MSG_set0_validity.pod} | 40 ++++++++++--------- include/openssl/cmperr.h | 3 ++ include/openssl/crmf.h | 3 +- include/openssl/crmferr.h | 2 +- .../81-test_cmp_cli_data/test_enrollment.csv | 2 +- util/libcrypto.num | 2 +- 12 files changed, 66 insertions(+), 56 deletions(-) rename doc/man3/{OSSL_CRMF_MSG_set_validity.pod => OSSL_CRMF_MSG_set0_validity.pod} (68%) diff --git a/apps/cmp.c b/apps/cmp.c index f80410fe9c..05fae77d38 100644 --- a/apps/cmp.c +++ b/apps/cmp.c @@ -1880,9 +1880,12 @@ static int setup_request_ctx(OSSL_CMP_CTX *ctx, ENGINE *e) } } - if (opt_days > 0) - (void)OSSL_CMP_CTX_set_option(ctx, OSSL_CMP_OPT_VALIDITY_DAYS, - opt_days); + if (opt_days > 0 + && !OSSL_CMP_CTX_set_option(ctx, OSSL_CMP_OPT_VALIDITY_DAYS, + opt_days)) { + CMP_err("could to set requested cert validity period"); + goto err; + } if (opt_policies != NULL && opt_policy_oids != NULL) { CMP_err("cannot have policies both via -policies and via -policy_oids"); diff --git a/crypto/cmp/cmp_ctx.c b/crypto/cmp/cmp_ctx.c index 9f70de5038..558414bb5c 100644 --- a/crypto/cmp/cmp_ctx.c +++ b/crypto/cmp/cmp_ctx.c @@ -916,14 +916,14 @@ int OSSL_CMP_CTX_set_option(OSSL_CMP_CTX *ctx, int opt, int val) break; } if (val < min_val) { - CMPerr(0, CMP_R_INVALID_ARGS); + CMPerr(0, CMP_R_VALUE_TOO_SMALL); return 0; } switch (opt) { case OSSL_CMP_OPT_LOG_VERBOSITY: if (val > OSSL_CMP_LOG_DEBUG) { - CMPerr(0, CMP_R_INVALID_ARGS); + CMPerr(0, CMP_R_VALUE_TOO_LARGE); return 0; } ctx->log_verbosity = val; @@ -957,7 +957,7 @@ int OSSL_CMP_CTX_set_option(OSSL_CMP_CTX *ctx, int opt, int val) break; case OSSL_CMP_OPT_POPO_METHOD: if (val > OSSL_CRMF_POPO_KEYAGREE) { - CMPerr(0, CMP_R_INVALID_ARGS); + CMPerr(0, CMP_R_VALUE_TOO_LARGE); return 0; } ctx->popoMethod = val; @@ -982,13 +982,13 @@ int OSSL_CMP_CTX_set_option(OSSL_CMP_CTX *ctx, int opt, int val) break; case OSSL_CMP_OPT_REVOCATION_REASON: if (val > OCSP_REVOKED_STATUS_AACOMPROMISE) { - CMPerr(0, CMP_R_INVALID_ARGS); + CMPerr(0, CMP_R_VALUE_TOO_LARGE); return 0; } ctx->revocationReason = val; break; default: - CMPerr(0, CMP_R_INVALID_ARGS); + CMPerr(0, CMP_R_INVALID_OPTION); return 0; } @@ -1044,7 +1044,7 @@ int OSSL_CMP_CTX_get_option(const OSSL_CMP_CTX *ctx, int opt) case OSSL_CMP_OPT_REVOCATION_REASON: return ctx->revocationReason; default: - CMPerr(0, CMP_R_INVALID_ARGS); + CMPerr(0, CMP_R_INVALID_OPTION); return -1; } } diff --git a/crypto/cmp/cmp_err.c b/crypto/cmp/cmp_err.c index 5f2f713b08..1ee1002233 100644 --- a/crypto/cmp/cmp_err.c +++ b/crypto/cmp/cmp_err.c @@ -85,6 +85,7 @@ static const ERR_STRING_DATA CMP_str_reasons[] = { {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_FAIL_INFO_OUT_OF_RANGE), "fail info out of range"}, {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_INVALID_ARGS), "invalid args"}, + {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_INVALID_OPTION), "invalid option"}, {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_MISSING_KEY_INPUT_FOR_CREATING_PROTECTION), "missing key input for creating protection"}, {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_MISSING_KEY_USAGE_DIGITALSIGNATURE), @@ -143,6 +144,8 @@ static const ERR_STRING_DATA CMP_str_reasons[] = { "unsupported key type"}, {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_UNSUPPORTED_PROTECTION_ALG_DHBASEDMAC), "unsupported protection alg dhbasedmac"}, + {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_VALUE_TOO_LARGE), "value too large"}, + {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_VALUE_TOO_SMALL), "value too small"}, {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_WRONG_ALGORITHM_OID), "wrong algorithm oid"}, {ERR_PACK(ERR_LIB_CMP, 0, CMP_R_WRONG_CERTID_IN_RP), "wrong certid in rp"}, diff --git a/crypto/cmp/cmp_msg.c b/crypto/cmp/cmp_msg.c index 9735a1c0b7..bbc3e9157e 100644 --- a/crypto/cmp/cmp_msg.c +++ b/crypto/cmp/cmp_msg.c @@ -253,12 +253,17 @@ static OSSL_CRMF_MSG *crm_new(OSSL_CMP_CTX *ctx, int bodytype, int rid) NULL /* serial */)) goto err; if (ctx->days != 0) { - time_t notBefore, notAfter; - - notBefore = time(NULL); - notAfter = notBefore + 60 * 60 * 24 * ctx->days; - if (!OSSL_CRMF_MSG_set_validity(crm, notBefore, notAfter)) + time_t now = time(NULL); + ASN1_TIME *notBefore = ASN1_TIME_adj(NULL, now, 0, 0); + ASN1_TIME *notAfter = ASN1_TIME_adj(NULL, now, ctx->days, 0); + + if (notBefore == NULL + || notAfter == NULL + || !OSSL_CRMF_MSG_set0_validity(crm, notBefore, notAfter)) { + ASN1_TIME_free(notBefore); + ASN1_TIME_free(notAfter); goto err; + } } /* extensions */ diff --git a/crypto/crmf/crmf_lib.c b/crypto/crmf/crmf_lib.c index c20a6da0f2..7530120ff3 100644 --- a/crypto/crmf/crmf_lib.c +++ b/crypto/crmf/crmf_lib.c @@ -244,35 +244,23 @@ OSSL_CRMF_CERTTEMPLATE *OSSL_CRMF_MSG_get0_tmpl(const OSSL_CRMF_MSG *crm) } -int OSSL_CRMF_MSG_set_validity(OSSL_CRMF_MSG *crm, time_t from, time_t to) +int OSSL_CRMF_MSG_set0_validity(OSSL_CRMF_MSG *crm, + ASN1_TIME *notBefore, ASN1_TIME *notAfter) { - OSSL_CRMF_OPTIONALVALIDITY *vld = NULL; - ASN1_TIME *from_asn = NULL; - ASN1_TIME *to_asn = NULL; + OSSL_CRMF_OPTIONALVALIDITY *vld; OSSL_CRMF_CERTTEMPLATE *tmpl = OSSL_CRMF_MSG_get0_tmpl(crm); if (tmpl == NULL) { /* also crm == NULL implies this */ - CRMFerr(CRMF_F_OSSL_CRMF_MSG_SET_VALIDITY, CRMF_R_NULL_ARGUMENT); + CRMFerr(CRMF_F_OSSL_CRMF_MSG_SET0_VALIDITY, CRMF_R_NULL_ARGUMENT); return 0; } - if (from != 0 && ((from_asn = ASN1_TIME_set(NULL, from)) == NULL)) - goto err; - if (to != 0 && ((to_asn = ASN1_TIME_set(NULL, to)) == NULL)) - goto err; if ((vld = OSSL_CRMF_OPTIONALVALIDITY_new()) == NULL) - goto err; - - vld->notBefore = from_asn; - vld->notAfter = to_asn; - + return 0; + vld->notBefore = notBefore; + vld->notAfter = notAfter; tmpl->validity = vld; - return 1; - err: - ASN1_TIME_free(from_asn); - ASN1_TIME_free(to_asn); - return 0; } diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index a30b808a25..1585688c83 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -378,7 +378,7 @@ CRMF_F_OSSL_CRMF_MSG_SET0_SINGLEPUBINFO:113:OSSL_CRMF_MSG_set0_SinglePubInfo CRMF_F_OSSL_CRMF_MSG_SET_CERTREQID:114:OSSL_CRMF_MSG_set_certReqId CRMF_F_OSSL_CRMF_MSG_SET_PKIPUBLICATIONINFO_ACTION:115:\ OSSL_CRMF_MSG_set_PKIPublicationInfo_action -CRMF_F_OSSL_CRMF_MSG_SET_VALIDITY:116:OSSL_CRMF_MSG_set_validity +CRMF_F_OSSL_CRMF_MSG_SET0_VALIDITY:116:OSSL_CRMF_MSG_set0_validity CRMF_F_OSSL_CRMF_PBMP_NEW:117:OSSL_CRMF_pbmp_new CRMF_F_OSSL_CRMF_PBM_NEW:118:OSSL_CRMF_pbm_new CRYPTO_F_CMAC_CTX_NEW:120:CMAC_CTX_new @@ -2119,6 +2119,7 @@ CMP_R_FAILED_EXTRACTING_PUBKEY:141:failed extracting pubkey CMP_R_FAILURE_OBTAINING_RANDOM:110:failure obtaining random CMP_R_FAIL_INFO_OUT_OF_RANGE:129:fail info out of range CMP_R_INVALID_ARGS:100:invalid args +CMP_R_INVALID_OPTION:174:invalid option CMP_R_MISSING_KEY_INPUT_FOR_CREATING_PROTECTION:130:\ missing key input for creating protection CMP_R_MISSING_KEY_USAGE_DIGITALSIGNATURE:142:missing key usage digitalsignature @@ -2157,6 +2158,8 @@ CMP_R_UNSUPPORTED_ALGORITHM:136:unsupported algorithm CMP_R_UNSUPPORTED_KEY_TYPE:137:unsupported key type CMP_R_UNSUPPORTED_PROTECTION_ALG_DHBASEDMAC:154:\ unsupported protection alg dhbasedmac +CMP_R_VALUE_TOO_LARGE:175:value too large +CMP_R_VALUE_TOO_SMALL:177:value too small CMP_R_WRONG_ALGORITHM_OID:138:wrong algorithm oid CMP_R_WRONG_CERTID_IN_RP:187:wrong certid in rp CMP_R_WRONG_PBM_VALUE:155:wrong pbm value diff --git a/doc/man3/OSSL_CRMF_MSG_set_validity.pod b/doc/man3/OSSL_CRMF_MSG_set0_validity.pod similarity index 68% rename from doc/man3/OSSL_CRMF_MSG_set_validity.pod rename to doc/man3/OSSL_CRMF_MSG_set0_validity.pod index 2544c65775..3f86bfaf07 100644 --- a/doc/man3/OSSL_CRMF_MSG_set_validity.pod +++ b/doc/man3/OSSL_CRMF_MSG_set0_validity.pod @@ -2,7 +2,7 @@ =head1 NAME -OSSL_CRMF_MSG_set_validity, +OSSL_CRMF_MSG_set0_validity, OSSL_CRMF_MSG_set_certReqId, OSSL_CRMF_CERTTEMPLATE_fill, OSSL_CRMF_MSG_set0_extensions, @@ -15,7 +15,8 @@ OSSL_CRMF_MSGS_verify_popo #include - int OSSL_CRMF_MSG_set_validity(OSSL_CRMF_MSG *crm, time_t from, time_t to); + int OSSL_CRMF_MSG_set0_validity(OSSL_CRMF_MSG *crm, + ASN1_TIME *notBefore, ASN1_TIME *notAfter); int OSSL_CRMF_MSG_set_certReqId(OSSL_CRMF_MSG *crm, int rid); @@ -37,29 +38,32 @@ OSSL_CRMF_MSGS_verify_popo =head1 DESCRIPTION -OSSL_CRMF_MSG_set_validity() sets B as notBefore and B as notAfter -as the validity in the certTemplate of B. +OSSL_CRMF_MSG_set0_validity() sets the I and I fields +as validity constraints in the certTemplate of I. +Any of the I and I parameters may be NULL, +which means no constraint for the respective field. +On success ownership of I and I is transferred to I. -OSSL_CRMF_MSG_set_certReqId() sets B as the certReqId of B. +OSSL_CRMF_MSG_set_certReqId() sets I as the certReqId of I. -OSSL_CRMF_CERTTEMPLATE_fill() sets those fields of the certTemplate B -for which non-NULL values are provided: B, B, B, -and/or B. -On success the reference counter of the B (if given) is incremented, -while the B, B, and B structures (if given) are copied. +OSSL_CRMF_CERTTEMPLATE_fill() sets those fields of the certTemplate I +for which non-NULL values are provided: I, I, I, +and/or I. +On success the reference counter of the I (if given) is incremented, +while the I, I, and I structures (if given) are copied. -OSSL_CRMF_MSG_set0_extensions() sets B as the extensions in the -certTemplate of B. Frees any pre-existing ones and consumes B. +OSSL_CRMF_MSG_set0_extensions() sets I as the extensions in the +certTemplate of I. Frees any pre-existing ones and consumes I. -OSSL_CRMF_MSG_push0_extension() pushes the X509 extension B to the -extensions in the certTemplate of B. Consumes B. +OSSL_CRMF_MSG_push0_extension() pushes the X509 extension I to the +extensions in the certTemplate of I. Consumes I. OSSL_CRMF_MSG_create_popo() creates and sets the Proof-of-Possession (POPO) -according to the method B in B. +according to the method I in I. In case the method is OSSL_CRMF_POPO_SIGNATURE the POPO is calculated -using the private B and the digest algorithm NID B. +using the private I and the digest algorithm NID I. -B can be one of the following: +I can be one of the following: =over 8 @@ -82,7 +86,7 @@ challenge-response exchange (challengeResp) not yet supported. =back OSSL_CRMF_MSGS_verify_popo verifies the Proof-of-Possession of the request with -the given B in the list of B. Optionally accepts RAVerified. +the given I in the list of I. Optionally accepts RAVerified. =head1 RETURN VALUES diff --git a/include/openssl/cmperr.h b/include/openssl/cmperr.h index d1ce2256fa..d220e55c5e 100644 --- a/include/openssl/cmperr.h +++ b/include/openssl/cmperr.h @@ -74,6 +74,7 @@ int ERR_load_CMP_strings(void); # define CMP_R_FAILURE_OBTAINING_RANDOM 110 # define CMP_R_FAIL_INFO_OUT_OF_RANGE 129 # define CMP_R_INVALID_ARGS 100 +# define CMP_R_INVALID_OPTION 174 # define CMP_R_MISSING_KEY_INPUT_FOR_CREATING_PROTECTION 130 # define CMP_R_MISSING_KEY_USAGE_DIGITALSIGNATURE 142 # define CMP_R_MISSING_PRIVATE_KEY 131 @@ -109,6 +110,8 @@ int ERR_load_CMP_strings(void); # define CMP_R_UNSUPPORTED_ALGORITHM 136 # define CMP_R_UNSUPPORTED_KEY_TYPE 137 # define CMP_R_UNSUPPORTED_PROTECTION_ALG_DHBASEDMAC 154 +# define CMP_R_VALUE_TOO_LARGE 175 +# define CMP_R_VALUE_TOO_SMALL 177 # define CMP_R_WRONG_ALGORITHM_OID 138 # define CMP_R_WRONG_CERTID_IN_RP 187 # define CMP_R_WRONG_PBM_VALUE 155 diff --git a/include/openssl/crmf.h b/include/openssl/crmf.h index d262a9b759..bf9c1c6159 100644 --- a/include/openssl/crmf.h +++ b/include/openssl/crmf.h @@ -105,7 +105,8 @@ int OSSL_CRMF_MSG_set1_regInfo_utf8Pairs(OSSL_CRMF_MSG *msg, int OSSL_CRMF_MSG_set1_regInfo_certReq(OSSL_CRMF_MSG *msg, const OSSL_CRMF_CERTREQUEST *cr); -int OSSL_CRMF_MSG_set_validity(OSSL_CRMF_MSG *crm, time_t from, time_t to); +int OSSL_CRMF_MSG_set0_validity(OSSL_CRMF_MSG *crm, + ASN1_TIME *notBefore, ASN1_TIME *notAfter); int OSSL_CRMF_MSG_set_certReqId(OSSL_CRMF_MSG *crm, int rid); int OSSL_CRMF_MSG_get_certReqId(const OSSL_CRMF_MSG *crm); int OSSL_CRMF_MSG_set0_extensions(OSSL_CRMF_MSG *crm, X509_EXTENSIONS *exts); diff --git a/include/openssl/crmferr.h b/include/openssl/crmferr.h index 22936c620e..17e5c85cc2 100644 --- a/include/openssl/crmferr.h +++ b/include/openssl/crmferr.h @@ -44,7 +44,7 @@ int ERR_load_CRMF_strings(void); # define CRMF_F_OSSL_CRMF_MSG_SET0_SINGLEPUBINFO 0 # define CRMF_F_OSSL_CRMF_MSG_SET_CERTREQID 0 # define CRMF_F_OSSL_CRMF_MSG_SET_PKIPUBLICATIONINFO_ACTION 0 -# define CRMF_F_OSSL_CRMF_MSG_SET_VALIDITY 0 +# define CRMF_F_OSSL_CRMF_MSG_SET0_VALIDITY 0 # define CRMF_F_OSSL_CRMF_PBMP_NEW 0 # define CRMF_F_OSSL_CRMF_PBM_NEW 0 # endif diff --git a/test/recipes/81-test_cmp_cli_data/test_enrollment.csv b/test/recipes/81-test_cmp_cli_data/test_enrollment.csv index 64fb3fdcd6..305302e180 100644 --- a/test/recipes/81-test_cmp_cli_data/test_enrollment.csv +++ b/test/recipes/81-test_cmp_cli_data/test_enrollment.csv @@ -27,7 +27,7 @@ expected,description, -section,val, -cmd,val, -newkey,val,val, -newkeypass,val, ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, 0,days 1, -section,, -cmd,ir, -newkey,new.key,, -newkeypass,pass:,,,BLANK,, -days,1,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,, -certout,test.cert.pem,, -out_trusted,root.crt,,BLANK,,BLANK,,, 0,days 0, -section,, -cmd,ir, -newkey,new.key,, -newkeypass,pass:,,,BLANK,, -days,0,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,, -certout,test.cert.pem,, -out_trusted,root.crt,,BLANK,,BLANK,,, -0,days 36500, -section,, -cmd,ir, -newkey,new.key,, -newkeypass,pass:,,,BLANK,, -days,36500,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,, -certout,test.cert.pem,, -out_trusted,root.crt,,BLANK,,BLANK,,, +0,days 365*100 beyond 2038, -section,, -cmd,ir, -newkey,new.key,, -newkeypass,pass:,,,BLANK,, -days,36500,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,, -certout,test.cert.pem,, -out_trusted,root.crt,,BLANK,,BLANK,,, 1,days missing arg, -section,, -cmd,ir, -newkey,new.key,, -newkeypass,pass:,,,BLANK,, -days,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,, -certout,test.cert.pem,, -out_trusted,root.crt,,BLANK,,BLANK,,, 1,days negative, -section,, -cmd,ir, -newkey,new.key,, -newkeypass,pass:,,,BLANK,, -days,-10,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,, -certout,test.cert.pem,, -out_trusted,root.crt,,BLANK,,BLANK,,, 1,days no not integer, -section,, -cmd,ir, -newkey,new.key,, -newkeypass,pass:,,,BLANK,, -days,1.5,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,,BLANK,, -certout,test.cert.pem,, -out_trusted,root.crt,,BLANK,,BLANK,,, diff --git a/util/libcrypto.num b/util/libcrypto.num index acaa17394e..683cddbc79 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -4546,7 +4546,7 @@ OSSL_CRMF_MSG_set1_regCtrl_oldCertID ? 3_0_0 EXIST::FUNCTION:CRMF OSSL_CRMF_CERTID_gen ? 3_0_0 EXIST::FUNCTION:CRMF OSSL_CRMF_MSG_set1_regInfo_utf8Pairs ? 3_0_0 EXIST::FUNCTION:CRMF OSSL_CRMF_MSG_set1_regInfo_certReq ? 3_0_0 EXIST::FUNCTION:CRMF -OSSL_CRMF_MSG_set_validity ? 3_0_0 EXIST::FUNCTION:CRMF +OSSL_CRMF_MSG_set0_validity ? 3_0_0 EXIST::FUNCTION:CRMF OSSL_CRMF_MSG_set_certReqId ? 3_0_0 EXIST::FUNCTION:CRMF OSSL_CRMF_MSG_get_certReqId ? 3_0_0 EXIST::FUNCTION:CRMF OSSL_CRMF_MSG_set0_extensions ? 3_0_0 EXIST::FUNCTION:CRMF -- 2.25.1