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@

2 files changed, 62 insertions, 14 deletions

diff --git a/src/lib/libcrypto/ocsp/ocsp_cl.c b/src/lib/libcrypto/ocsp/ocsp_cl.c
index a4320d9278..83615e5434 100644
--- a/src/lib/libcrypto/ocsp/ocsp_cl.c
+++ b/src/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/src/lib/libssl/src/crypto/ocsp/ocsp_cl.c b/src/lib/libssl/src/crypto/ocsp/ocsp_cl.c
index a4320d9278..83615e5434 100644
--- a/src/lib/libssl/src/crypto/ocsp/ocsp_cl.c
+++ b/src/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;