Fix error when server does not send CertificateStatus message

If a server sends the status_request extension then it may choose
to send the CertificateStatus message. However this is optional.
We were treating it as mandatory and the connection was failing.

Thanks to BoringSSL for reporting this issue.

RT#4120

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 46cb4b02efb791688a58d62d2eb5d60f80d12cb0..d170ad121dccbbee4097c8bccd757b85e3b054ef 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -303,12 +303,13 @@ int ossl_statem_client_read_transition(SSL *s, int mt)
         break;
 
     case TLS_ST_CR_CERT:
         break;
 
     case TLS_ST_CR_CERT:

-        if (s->tlsext_status_expected) {
-            if (mt == SSL3_MT_CERTIFICATE_STATUS) {
-                st->hand_state = TLS_ST_CR_CERT_STATUS;
-                return 1;
-            }
-            return 0;
+        /*
+         * The CertificateStatus message is optional even if
+         * |tlsext_status_expected| is set
+         */
+        if (s->tlsext_status_expected && mt == SSL3_MT_CERTIFICATE_STATUS) {
+            st->hand_state = TLS_ST_CR_CERT_STATUS;
+            return 1;

         }
         /* Fall through */
 
         }
         /* Fall through */
 


@@ -2155,7 +2156,6 @@ MSG_PROCESS_RETURN tls_process_cert_status(SSL *s, PACKET *pkt)
         SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, SSL_R_LENGTH_MISMATCH);
         goto f_err;
     }
         SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, SSL_R_LENGTH_MISMATCH);
         goto f_err;
     }

-    OPENSSL_free(s->tlsext_ocsp_resp);

     s->tlsext_ocsp_resp = OPENSSL_malloc(resplen);
     if (s->tlsext_ocsp_resp == NULL) {
         al = SSL_AD_INTERNAL_ERROR;
     s->tlsext_ocsp_resp = OPENSSL_malloc(resplen);
     if (s->tlsext_ocsp_resp == NULL) {
         al = SSL_AD_INTERNAL_ERROR;


@@ -2168,20 +2168,6 @@ MSG_PROCESS_RETURN tls_process_cert_status(SSL *s, PACKET *pkt)
         goto f_err;
     }
     s->tlsext_ocsp_resplen = resplen;
         goto f_err;
     }
     s->tlsext_ocsp_resplen = resplen;

-    if (s->ctx->tlsext_status_cb) {
-        int ret;
-        ret = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
-        if (ret == 0) {
-            al = SSL_AD_BAD_CERTIFICATE_STATUS_RESPONSE;
-            SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, SSL_R_INVALID_STATUS_RESPONSE);
-            goto f_err;
-        }
-        if (ret < 0) {
-            al = SSL_AD_INTERNAL_ERROR;
-            SSLerr(SSL_F_TLS_PROCESS_CERT_STATUS, ERR_R_MALLOC_FAILURE);
-            goto f_err;
-        }
-    }

     return MSG_PROCESS_CONTINUE_READING;
  f_err:
     ssl3_send_alert(s, SSL3_AL_FATAL, al);
     return MSG_PROCESS_CONTINUE_READING;
  f_err:
     ssl3_send_alert(s, SSL3_AL_FATAL, al);


@@ -2220,6 +2206,28 @@ MSG_PROCESS_RETURN tls_process_server_done(SSL *s, PACKET *pkt)
         return MSG_PROCESS_ERROR;
     }
 
         return MSG_PROCESS_ERROR;
     }
 

+    /*
+     * Call the ocsp status callback if needed. The |tlsext_ocsp_resp| and
+     * |tlsext_ocsp_resplen| values will be set if we actually received a status
+     * message, or NULL and -1 otherwise
+     */
+    if (s->tlsext_status_expected && s->ctx->tlsext_status_cb != NULL) {
+        int ret;
+        ret = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
+        if (ret == 0) {
+            ssl3_send_alert(s, SSL3_AL_FATAL,
+                            SSL_AD_BAD_CERTIFICATE_STATUS_RESPONSE);
+            SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE,
+                   SSL_R_INVALID_STATUS_RESPONSE);
+            return MSG_PROCESS_ERROR;
+        }
+        if (ret < 0) {
+            ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
+            SSLerr(SSL_F_TLS_PROCESS_SERVER_DONE, ERR_R_MALLOC_FAILURE);
+            return MSG_PROCESS_ERROR;
+        }
+    }
+

 #ifndef OPENSSL_NO_SCTP
     /* Only applies to renegotiation */
     if (SSL_IS_DTLS(s) && BIO_dgram_is_sctp(SSL_get_wbio(s))
 #ifndef OPENSSL_NO_SCTP
     /* Only applies to renegotiation */
     if (SSL_IS_DTLS(s) && BIO_dgram_is_sctp(SSL_get_wbio(s))

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index d9cfe27295c1472128ed266a15ba455869fd120d..e7d9f64bf981908c16964712d4522822819a7856 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2847,6 +2847,9 @@ int ssl_check_serverhello_tlsext(SSL *s)
                                                        s->
                                                        initial_ctx->tlsext_servername_arg);
 
                                                        s->
                                                        initial_ctx->tlsext_servername_arg);
 

+    OPENSSL_free(s->tlsext_ocsp_resp);
+    s->tlsext_ocsp_resp = NULL;
+    s->tlsext_ocsp_resplen = -1;

     /*
      * If we've requested certificate status and we wont get one tell the
      * callback
     /*
      * If we've requested certificate status and we wont get one tell the
      * callback


@@ -2855,12 +2858,9 @@ int ssl_check_serverhello_tlsext(SSL *s)
         && s->ctx && s->ctx->tlsext_status_cb) {
         int r;
         /*
         && s->ctx && s->ctx->tlsext_status_cb) {
         int r;
         /*

-         * Set resp to NULL, resplen to -1 so callback knows there is no
-         * response.
+         * Call callback with resp == NULL and resplen == -1 so callback
+         * knows there is no response

          */
          */

-        OPENSSL_free(s->tlsext_ocsp_resp);
-        s->tlsext_ocsp_resp = NULL;
-        s->tlsext_ocsp_resplen = -1;

         r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
         if (r == 0) {
             al = SSL_AD_BAD_CERTIFICATE_STATUS_RESPONSE;
         r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
         if (r == 0) {
             al = SSL_AD_BAD_CERTIFICATE_STATUS_RESPONSE;