Client side sanity check of ALPN after server has accepted early_data

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/3926)

3 files changed, 61 insertions, 19 deletions

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 9d32366e12..b8ab5c880e 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -844,7 +844,7 @@ static int final_server_name(SSL *s, unsigned int context, int sent,
 
     case SSL_TLSEXT_ERR_NOACK:
         s->servername_done = 0;
-        if (s->session->ext.hostname != NULL)
+        if (s->server && s->session->ext.hostname != NULL)
             s->ext.early_data_ok = 0;
         return 1;
 
@@ -945,6 +945,9 @@ static int init_alpn(SSL *s, unsigned int context)
 
 static int final_alpn(SSL *s, unsigned int context, int sent, int *al)
 {
+    if (!s->server && !sent && s->session->ext.alpn_selected != NULL)
+            s->ext.early_data_ok = 0;
+
     if (!s->server || !SSL_IS_TLS13(s))
         return 1;
 
@@ -1387,8 +1390,24 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart,
 
 static int final_early_data(SSL *s, unsigned int context, int sent, int *al)
 {
-    if (!s->server || !sent)
+    if (!sent)
+        return 1;
+
+    if (!s->server) {
+        if (context == SSL_EXT_TLS1_3_ENCRYPTED_EXTENSIONS
+                && sent
+                && !s->ext.early_data_ok) {
+            /*
+             * If we get here then the server accepted our early_data but we
+             * later realised that it shouldn't have done (e.g. inconsistent
+             * ALPN)
+             */
+            *al = SSL_AD_ILLEGAL_PARAMETER;
+            return 0;
+        }
+
         return 1;
+    }
 
     if (s->max_early_data == 0
             || !s->hit
diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index cb7b211f7c..bcbcbac873 100644
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -771,6 +771,7 @@ EXT_RETURN tls_construct_ctos_early_data(SSL *s, WPACKET *pkt,
      * extension, we set it to accepted.
      */
     s->ext.early_data = SSL_EARLY_DATA_REJECTED;
+    s->ext.early_data_ok = 1;
 
     return EXT_RETURN_SENT;
 }
@@ -1400,15 +1401,22 @@ int tls_parse_stoc_alpn(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
     }
     s->s3->alpn_selected_len = len;
 
-    /* We also put a copy in the session */
-    OPENSSL_free(s->session->ext.alpn_selected);
-    s->session->ext.alpn_selected = OPENSSL_memdup(s->s3->alpn_selected,
-                                                   s->s3->alpn_selected_len);
-    s->session->ext.alpn_selected_len = s->s3->alpn_selected_len;
-
-    if (s->session->ext.alpn_selected == NULL) {
-        *al = SSL_AD_INTERNAL_ERROR;
-        return 0;
+    if (s->session->ext.alpn_selected != NULL
+            && (s->session->ext.alpn_selected_len != len
+                || memcmp(s->session->ext.alpn_selected, s->s3->alpn_selected,
+                          len) != 0)) {
+        /* ALPN not consistent with the old session so cannot use early_data */
+        s->ext.early_data_ok = 0;
+    }
+    if (!s->hit) {
+        /* If a new session then update it with the selected ALPN */
+        s->session->ext.alpn_selected =
+            OPENSSL_memdup(s->s3->alpn_selected, s->s3->alpn_selected_len);
+        if (s->session->ext.alpn_selected == NULL) {
+            *al = SSL_AD_INTERNAL_ERROR;
+            return 0;
+        }
+        s->session->ext.alpn_selected_len = s->s3->alpn_selected_len;
     }
 
     return 1;
@@ -1637,12 +1645,13 @@ int tls_parse_stoc_early_data(SSL *s, PACKET *pkt, unsigned int context,
         return 0;
     }
 
-    if (s->ext.early_data != SSL_EARLY_DATA_REJECTED
+    if (!s->ext.early_data_ok
             || !s->hit
             || s->session->ext.tick_identity != 0) {
         /*
          * If we get here then we didn't send early data, or we didn't resume
-         * using the first identity so the server should not be accepting it.
+         * using the first identity, or the SNI/ALPN is not consistent so the
+         * server should not be accepting it.
          */
         *al = SSL_AD_ILLEGAL_PARAMETER;
         return 0;
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 8c2a4e97ac..a3a6bdf6cb 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1962,14 +1962,26 @@ int tls_handle_alpn(SSL *s, int *al)
             s->s3->npn_seen = 0;
 #endif
 
-            /* Check ALPN is consistent with early_data */
-            if (s->ext.early_data_ok
-                    && (s->session->ext.alpn_selected == NULL
+            /* Check ALPN is consistent with session */
+            if (s->session->ext.alpn_selected == NULL
                         || selected_len != s->session->ext.alpn_selected_len
                         || memcmp(selected, s->session->ext.alpn_selected,
-                                  selected_len) != 0))
+                                  selected_len) != 0) {
+                /* Not consistent so can't be used for early_data */
                 s->ext.early_data_ok = 0;
 
+                if (!s->hit) {
+                    /* If a new session update it with the new ALPN value */
+                    s->session->ext.alpn_selected = OPENSSL_memdup(selected,
+                                                                   selected_len);
+                    if (s->session->ext.alpn_selected == NULL) {
+                        *al = SSL_AD_INTERNAL_ERROR;
+                        return 0;
+                    }
+                    s->session->ext.alpn_selected_len = selected_len;
+                }
+            }
+
             return 1;
         } else if (r != SSL_TLSEXT_ERR_NOACK) {
             *al = SSL_AD_NO_APPLICATION_PROTOCOL;
@@ -1981,9 +1993,11 @@ int tls_handle_alpn(SSL *s, int *al)
          */
     }
 
-    /* Check ALPN is consistent with early_data */
-    if (s->ext.early_data_ok && s->session->ext.alpn_selected != NULL)
+    /* Check ALPN is consistent with session */
+    if (s->session->ext.alpn_selected != NULL) {
+        /* Not consistent so can't be used for early_data */
         s->ext.early_data_ok = 0;
+    }
 
     return 1;
 }