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 <k@rhe.jp>
ok bcook@

diff --git a/lib/libcrypto/ocsp/ocsp_cl.c b/lib/libcrypto/ocsp/ocsp_cl.c
index a4320d9..83615e5 100644 (file)
--- 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 <Tom_Titchener@groove.net> for the OpenSSL
  * project. */
 
@@ -71,6 +71,9 @@
 #include <openssl/x509.h>
 #include <openssl/x509v3.h>
 
+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 a4320d9..83615e5 100644 (file)
--- 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 <Tom_Titchener@groove.net> for the OpenSSL
  * project. */
 
@@ -71,6 +71,9 @@
 #include <openssl/x509.h>
 #include <openssl/x509v3.h>
 
+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;