Fix two bugs in clienthello processing

- Always process ALPN (previously there was an early return in the
  certificate status handling)
- Don't send a duplicate alert. Previously, both
  ssl_check_clienthello_tlsext_late and its caller would send an
  alert. Consolidate alert sending code in the caller.

Reviewed-by: Rich Salz <rsalz@openssl.org>

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 1cc63aa81922a425cef9bcb5b3ae8d38117fcb0d..25cd312c953600bb3bfa48b7680f4807379691fd 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -2004,7 +2004,7 @@ __owur unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
 __owur int ssl_parse_clienthello_tlsext(SSL *s, PACKET *pkt);
 void ssl_set_default_md(SSL *s);
 __owur int tls1_set_server_sigalgs(SSL *s);
-__owur int ssl_check_clienthello_tlsext_late(SSL *s);
+__owur int ssl_check_clienthello_tlsext_late(SSL *s, int *al);
 __owur int ssl_parse_serverhello_tlsext(SSL *s, PACKET *pkt);
 __owur int ssl_prepare_clienthello_tlsext(SSL *s);
 __owur int ssl_prepare_serverhello_tlsext(SSL *s);

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index a45acbd30ca909c5abf4f08490dc2b750b6e5961..b5cfc4f2209dcb2dbe56ab062eccdb39210321d4 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1454,7 +1454,7 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
 
         /* Handles TLS extensions that we couldn't check earlier */
         if (s->version >= SSL3_VERSION) {
-            if (ssl_check_clienthello_tlsext_late(s) <= 0) {
+            if (!ssl_check_clienthello_tlsext_late(s, &al)) {
                 SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO,
                        SSL_R_CLIENTHELLO_TLSEXT);
                 goto f_err;

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index e938d87de7d3bf3c6c3c71d2028b22d48f0724e3..2418c658628b59f111864e413f8aa6af0da19cd4 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1678,11 +1678,10 @@ static int tls1_alpn_handle_client_hello(SSL *s, PACKET *pkt, int *al)
 
 /*
  * Process the ALPN extension in a ClientHello.
- * ret: a pointer to the TLSEXT return value: SSL_TLSEXT_ERR_*
  * al: a pointer to the alert value to send in the event of a failure.
- * returns 1 on success, 0
+ * returns 1 on success, 0 on error.
  */
-static int tls1_alpn_handle_client_hello_late(SSL *s, int *ret, int *al)
+static int tls1_alpn_handle_client_hello_late(SSL *s, int *al)
 {
     const unsigned char *selected = NULL;
     unsigned char selected_len = 0;
@@ -1698,7 +1697,6 @@ static int tls1_alpn_handle_client_hello_late(SSL *s, int *ret, int *al)
             s->s3->alpn_selected = OPENSSL_memdup(selected, selected_len);
             if (s->s3->alpn_selected == NULL) {
                 *al = SSL_AD_INTERNAL_ERROR;
-                *ret = SSL_TLSEXT_ERR_ALERT_FATAL;
                 return 0;
             }
             s->s3->alpn_selected_len = selected_len;
@@ -1708,7 +1706,6 @@ static int tls1_alpn_handle_client_hello_late(SSL *s, int *ret, int *al)
 #endif
         } else {
             *al = SSL_AD_NO_APPLICATION_PROTOCOL;
-            *ret = SSL_TLSEXT_ERR_ALERT_FATAL;
             return 0;
         }
     }
@@ -2661,10 +2658,13 @@ int tls1_set_server_sigalgs(SSL *s)
     return 0;
 }
 
-int ssl_check_clienthello_tlsext_late(SSL *s)
+/*
+ * Upon success, returns 1.
+ * Upon failure, returns 0 and sets |al| to the appropriate fatal alert.
+ */
+int ssl_check_clienthello_tlsext_late(SSL *s, int *al)
 {
-    int ret = SSL_TLSEXT_ERR_OK;
-    int al = SSL_AD_INTERNAL_ERROR;
+    s->tlsext_status_expected = 0;
 
     /*
      * If status request then ask callback what to do. Note: this must be
@@ -2673,58 +2673,41 @@ int ssl_check_clienthello_tlsext_late(SSL *s)
      * influence which certificate is sent
      */
     if ((s->tlsext_status_type != -1) && s->ctx && s->ctx->tlsext_status_cb) {
-        int r;
+        int ret;
         CERT_PKEY *certpkey;
         certpkey = ssl_get_server_send_pkey(s);
         /* If no certificate can't return certificate status */
-        if (certpkey == NULL) {
-            s->tlsext_status_expected = 0;
-            return 1;
-        }
-        /*
-         * Set current certificate to one we will use so SSL_get_certificate
-         * et al can pick it up.
-         */
-        s->cert->key = certpkey;
-        r = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
-        switch (r) {
-            /* We don't want to send a status request response */
-        case SSL_TLSEXT_ERR_NOACK:
-            s->tlsext_status_expected = 0;
-            break;
-            /* status request response should be sent */
-        case SSL_TLSEXT_ERR_OK:
-            if (s->tlsext_ocsp_resp)
-                s->tlsext_status_expected = 1;
-            else
+        if (certpkey != NULL) {
+            /*
+             * Set current certificate to one we will use so SSL_get_certificate
+             * et al can pick it up.
+             */
+            s->cert->key = certpkey;
+            ret = s->ctx->tlsext_status_cb(s, s->ctx->tlsext_status_arg);
+            switch (ret) {
+                /* We don't want to send a status request response */
+            case SSL_TLSEXT_ERR_NOACK:
                 s->tlsext_status_expected = 0;
-            break;
-            /* something bad happened */
-        case SSL_TLSEXT_ERR_ALERT_FATAL:
-            ret = SSL_TLSEXT_ERR_ALERT_FATAL;
-            al = SSL_AD_INTERNAL_ERROR;
-            goto err;
+                break;
+                /* status request response should be sent */
+            case SSL_TLSEXT_ERR_OK:
+                if (s->tlsext_ocsp_resp)
+                    s->tlsext_status_expected = 1;
+                break;
+                /* something bad happened */
+            case SSL_TLSEXT_ERR_ALERT_FATAL:
+            default:
+                *al = SSL_AD_INTERNAL_ERROR;
+                return 0;
+            }
         }
-    } else
-        s->tlsext_status_expected = 0;
-
-    if (!tls1_alpn_handle_client_hello_late(s, &ret, &al)) {
-        goto err;
     }
 
- err:
-    switch (ret) {
-    case SSL_TLSEXT_ERR_ALERT_FATAL:
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
-        return -1;
-
-    case SSL_TLSEXT_ERR_ALERT_WARNING:
-        ssl3_send_alert(s, SSL3_AL_WARNING, al);
-        return 1;
-
-    default:
-        return 1;
+    if (!tls1_alpn_handle_client_hello_late(s, al)) {
+        return 0;
     }
+
+    return 1;
 }
 
 int ssl_check_serverhello_tlsext(SSL *s)