From: beck Date: Sat, 25 Jun 2016 15:38:44 +0000 (+0000) Subject: Fix the ocsp code to actually check for errors when comparing time values X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=deb3dc466c68662f6b6879636710eb076b89f0fa;p=openbsd Fix the ocsp code to actually check for errors when comparing time values which was not being done due to a lack of checking of the return code for X509_cmp_time. Ensure that we only compare GERNERALIZEDTIME values because this is what is specified by RFC6960. Issue reported, and fix provided by Kazuki Yamaguchi ok bcook@ --- diff --git a/lib/libcrypto/ocsp/ocsp_cl.c b/lib/libcrypto/ocsp/ocsp_cl.c index a4320d9278c..83615e5434f 100644 --- a/lib/libcrypto/ocsp/ocsp_cl.c +++ b/lib/libcrypto/ocsp/ocsp_cl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ocsp_cl.c,v 1.8 2014/10/18 17:20:40 jsing Exp $ */ +/* $OpenBSD: ocsp_cl.c,v 1.9 2016/06/25 15:38:44 beck Exp $ */ /* Written by Tom Titchener for the OpenSSL * project. */ @@ -71,6 +71,9 @@ #include #include +int asn1_time_parse(const char *, size_t, struct tm *, int); +int asn1_tm_cmp(struct tm *, struct tm *); + /* Utility functions related to sending OCSP requests and extracting * relevant information from the response. */ @@ -329,25 +332,43 @@ OCSP_check_validity(ASN1_GENERALIZEDTIME *thisupd, { int ret = 1; time_t t_now, t_tmp; + struct tm tm_this, tm_next, tm_tmp; time(&t_now); + + /* + * Times must explicitly be a GENERALIZEDTIME as per section + * 4.2.2.1 of RFC 6960 - It is invalid to accept other times + * (such as UTCTIME permitted/required by RFC 5280 for certificates) + */ + /* Check thisUpdate is valid and not more than nsec in the future */ - if (!ASN1_GENERALIZEDTIME_check(thisupd)) { + if (asn1_time_parse(thisupd->data, thisupd->length, &tm_this, + V_ASN1_GENERALIZEDTIME) != V_ASN1_GENERALIZEDTIME) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_ERROR_IN_THISUPDATE_FIELD); ret = 0; } else { t_tmp = t_now + nsec; - if (X509_cmp_time(thisupd, &t_tmp) > 0) { + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (asn1_tm_cmp(&tm_this, &tm_tmp) > 0) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_STATUS_NOT_YET_VALID); ret = 0; } - /* If maxsec specified check thisUpdate is not more than maxsec in the past */ + /* + * If maxsec specified check thisUpdate is not more than maxsec + * in the past + */ if (maxsec >= 0) { t_tmp = t_now - maxsec; - if (X509_cmp_time(thisupd, &t_tmp) < 0) { + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (asn1_tm_cmp(&tm_this, &tm_tmp) < 0) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_STATUS_TOO_OLD); ret = 0; @@ -359,13 +380,16 @@ OCSP_check_validity(ASN1_GENERALIZEDTIME *thisupd, return ret; /* Check nextUpdate is valid and not more than nsec in the past */ - if (!ASN1_GENERALIZEDTIME_check(nextupd)) { + if (asn1_time_parse(nextupd->data, nextupd->length, &tm_next, + V_ASN1_GENERALIZEDTIME) != V_ASN1_GENERALIZEDTIME) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_ERROR_IN_NEXTUPDATE_FIELD); ret = 0; } else { t_tmp = t_now - nsec; - if (X509_cmp_time(nextupd, &t_tmp) < 0) { + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (asn1_tm_cmp(&tm_next, &tm_tmp) < 0) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_STATUS_EXPIRED); ret = 0; diff --git a/lib/libssl/src/crypto/ocsp/ocsp_cl.c b/lib/libssl/src/crypto/ocsp/ocsp_cl.c index a4320d9278c..83615e5434f 100644 --- a/lib/libssl/src/crypto/ocsp/ocsp_cl.c +++ b/lib/libssl/src/crypto/ocsp/ocsp_cl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ocsp_cl.c,v 1.8 2014/10/18 17:20:40 jsing Exp $ */ +/* $OpenBSD: ocsp_cl.c,v 1.9 2016/06/25 15:38:44 beck Exp $ */ /* Written by Tom Titchener for the OpenSSL * project. */ @@ -71,6 +71,9 @@ #include #include +int asn1_time_parse(const char *, size_t, struct tm *, int); +int asn1_tm_cmp(struct tm *, struct tm *); + /* Utility functions related to sending OCSP requests and extracting * relevant information from the response. */ @@ -329,25 +332,43 @@ OCSP_check_validity(ASN1_GENERALIZEDTIME *thisupd, { int ret = 1; time_t t_now, t_tmp; + struct tm tm_this, tm_next, tm_tmp; time(&t_now); + + /* + * Times must explicitly be a GENERALIZEDTIME as per section + * 4.2.2.1 of RFC 6960 - It is invalid to accept other times + * (such as UTCTIME permitted/required by RFC 5280 for certificates) + */ + /* Check thisUpdate is valid and not more than nsec in the future */ - if (!ASN1_GENERALIZEDTIME_check(thisupd)) { + if (asn1_time_parse(thisupd->data, thisupd->length, &tm_this, + V_ASN1_GENERALIZEDTIME) != V_ASN1_GENERALIZEDTIME) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_ERROR_IN_THISUPDATE_FIELD); ret = 0; } else { t_tmp = t_now + nsec; - if (X509_cmp_time(thisupd, &t_tmp) > 0) { + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (asn1_tm_cmp(&tm_this, &tm_tmp) > 0) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_STATUS_NOT_YET_VALID); ret = 0; } - /* If maxsec specified check thisUpdate is not more than maxsec in the past */ + /* + * If maxsec specified check thisUpdate is not more than maxsec + * in the past + */ if (maxsec >= 0) { t_tmp = t_now - maxsec; - if (X509_cmp_time(thisupd, &t_tmp) < 0) { + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (asn1_tm_cmp(&tm_this, &tm_tmp) < 0) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_STATUS_TOO_OLD); ret = 0; @@ -359,13 +380,16 @@ OCSP_check_validity(ASN1_GENERALIZEDTIME *thisupd, return ret; /* Check nextUpdate is valid and not more than nsec in the past */ - if (!ASN1_GENERALIZEDTIME_check(nextupd)) { + if (asn1_time_parse(nextupd->data, nextupd->length, &tm_next, + V_ASN1_GENERALIZEDTIME) != V_ASN1_GENERALIZEDTIME) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_ERROR_IN_NEXTUPDATE_FIELD); ret = 0; } else { t_tmp = t_now - nsec; - if (X509_cmp_time(nextupd, &t_tmp) < 0) { + if (gmtime_r(&t_tmp, &tm_tmp) == NULL) + return 0; + if (asn1_tm_cmp(&tm_next, &tm_tmp) < 0) { OCSPerr(OCSP_F_OCSP_CHECK_VALIDITY, OCSP_R_STATUS_EXPIRED); ret = 0;