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@

1 files changed, 31 insertions, 7 deletions

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 <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;