Updates following review feedback

[openssl.git] / ssl / statem / statem_srvr.c

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 259be22106f155e67e99bdc644c65c0bfb9703bb..78f977fcc8516e36cf71f95f26345c6b4fd269fb 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1534,6 +1534,14 @@ static int tls_early_post_process_client_hello(SSL *s, int *al)
         goto err;
     }
 
+    /* TLSv1.3 specifies that a ClientHello must end on a record boundary */
+    if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) {
+        *al = SSL_AD_UNEXPECTED_MESSAGE;
+        SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO,
+               SSL_R_NOT_ON_RECORD_BOUNDARY);
+        goto err;
+    }
+
     if (SSL_IS_DTLS(s)) {
         /* Empty cookie was already handled above by returning early. */
         if (SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) {
@@ -1841,12 +1849,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *al)
      * Given s->session->ciphers and SSL_get_ciphers, we must pick a cipher
      */
 
-    if (!s->hit) {
-#ifdef OPENSSL_NO_COMP
-        s->session->compress_meth = 0;
-#else
-        s->session->compress_meth = (comp == NULL) ? 0 : comp->id;
-#endif
+    if (!s->hit || s->hello_retry_request) {
         sk_SSL_CIPHER_free(s->session->ciphers);
         s->session->ciphers = ciphers;
         if (ciphers == NULL) {
@@ -1855,6 +1858,14 @@ static int tls_early_post_process_client_hello(SSL *s, int *al)
             goto err;
         }
         ciphers = NULL;
+    }
+
+    if (!s->hit) {
+#ifdef OPENSSL_NO_COMP
+        s->session->compress_meth = 0;
+#else
+        s->session->compress_meth = (comp == NULL) ? 0 : comp->id;
+#endif
         if (!tls1_set_server_sigalgs(s)) {
             SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_CLIENTHELLO_TLSEXT);
             goto err;
@@ -1943,7 +1954,7 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
         wst = WORK_MORE_B;
     }
     if (wst == WORK_MORE_B) {
-        if (!s->hit) {
+        if (!s->hit || s->hello_retry_request) {
             /* Let cert callback update server certificates if required */
             if (s->cert->cert_cb) {
                 int rv = s->cert->cert_cb(s, s->cert->cert_cb_arg);
@@ -1967,18 +1978,30 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst)
                        SSL_R_NO_SHARED_CIPHER);
                 goto f_err;
             }
-            s->s3->tmp.new_cipher = cipher;
-            if (!tls_choose_sigalg(s, &al))
+            if (SSL_IS_TLS13(s) && s->s3->tmp.new_cipher != NULL
+                    && s->s3->tmp.new_cipher->id != cipher->id) {
+                /*
+                 * A previous HRR picked a different ciphersuite to the one we
+                 * just selected. Something must have changed.
+                 */
+                al = SSL_AD_ILLEGAL_PARAMETER;
+                SSLerr(SSL_F_TLS_POST_PROCESS_CLIENT_HELLO, SSL_R_BAD_CIPHER);
                 goto f_err;
-            /* check whether we should disable session resumption */
-            if (s->not_resumable_session_cb != NULL)
-                s->session->not_resumable =
-                    s->not_resumable_session_cb(s, ((cipher->algorithm_mkey
-                                                    & (SSL_kDHE | SSL_kECDHE))
-                                                   != 0));
-            if (s->session->not_resumable)
-                /* do not send a session ticket */
-                s->ext.ticket_expected = 0;
+            }
+            s->s3->tmp.new_cipher = cipher;
+            if (!s->hit) {
+                if (!tls_choose_sigalg(s, &al))
+                    goto f_err;
+                /* check whether we should disable session resumption */
+                if (s->not_resumable_session_cb != NULL)
+                    s->session->not_resumable =
+                        s->not_resumable_session_cb(s, ((cipher->algorithm_mkey
+                                                        & (SSL_kDHE | SSL_kECDHE))
+                                                       != 0));
+                if (s->session->not_resumable)
+                    /* do not send a session ticket */
+                    s->ext.ticket_expected = 0;
+            }
         } else {
             /* Session-id reuse */
             s->s3->tmp.new_cipher = s->session->cipher;
@@ -3654,17 +3677,18 @@ static int tls_construct_encrypted_extensions(SSL *s, WPACKET *pkt)
 static int tls_construct_hello_retry_request(SSL *s, WPACKET *pkt)
 {
     int al = SSL_AD_INTERNAL_ERROR;
+    size_t len = 0;
 
     /*
      * TODO(TLS1.3): Remove the DRAFT version before release
      * (should be s->version)
      */
     if (!WPACKET_put_bytes_u16(pkt, TLS1_3_VERSION_DRAFT)
+            || !s->method->put_cipher_by_char(s->s3->tmp.new_cipher, pkt, &len)
             || !tls_construct_extensions(s, pkt, EXT_TLS1_3_HELLO_RETRY_REQUEST,
                                          NULL, 0, &al)) {
         SSLerr(SSL_F_TLS_CONSTRUCT_HELLO_RETRY_REQUEST, ERR_R_INTERNAL_ERROR);
-        ssl3_send_alert(s, SSL3_AL_FATAL, al);
-        return 0;
+        goto err;
     }
 
     /* Ditch the session. We'll create a new one next time around */
@@ -3672,7 +3696,17 @@ static int tls_construct_hello_retry_request(SSL *s, WPACKET *pkt)
     s->session = NULL;
     s->hit = 0;
 
+    /*
+     * Re-initialise the Transcript Hash. We're going to prepopulate it with
+     * a synthetic message_hash in place of ClientHello1.
+     */
+    if (!create_synthetic_message_hash(s))
+        goto err;
+
     return 1;
+ err:
+    ssl3_send_alert(s, SSL3_AL_FATAL, al);
+    return 0;
 }
 
 MSG_PROCESS_RETURN tls_process_end_of_early_data(SSL *s, PACKET *pkt)