Fix SSL_get_servername() and SNI behaviour

The SNI behaviour for TLSv1.3 and the behaviour of SSL_get_servername()
was not quite right, and not entirely consistent with the RFC.

The TLSv1.3 RFC explicitly says that SNI is negotiated on each handshake
and the server is not required to associate it with the session. This was
not quite reflected in the code so we fix that.

Additionally there were some additional checks around early_data checking
that the SNI between the original session and this session were
consistent. In fact the RFC does not require any such checks, so they are
removed.

Finally the behaviour of SSL_get_servername() was not quite right. The
behaviour was not consistent between resumption and normal handshakes,
and also not quite consistent with historical behaviour. We clarify the
behaviour in various scenarios and also attempt to make it match historical
behaviour as closely as possible.

Fixes #8822

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

2 files changed, 18 insertions, 8 deletions

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index e2e704543e..d37accac18 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -948,7 +948,6 @@ static int final_server_name(SSL *s, unsigned int context, int sent)
      * was successful.
      */
     if (s->server) {
-        /* TODO(OpenSSL1.2) revisit !sent case */
         if (sent && ret == SSL_TLSEXT_ERR_OK && !s->hit) {
             /* Only store the hostname in the session if we accepted it. */
             OPENSSL_free(s->session->ext.hostname);
diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 194b521877..a2a4ae8a6e 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -127,6 +127,10 @@ int tls_parse_ctos_server_name(SSL *s, PACKET *pkt, unsigned int context,
         return 0;
     }
 
+    /*
+     * In TLSv1.2 and below the SNI is associated with the session. In TLSv1.3
+     * we always use the SNI value from the handshake.
+     */
     if (!s->hit || SSL_IS_TLS13(s)) {
         if (PACKET_remaining(&hostname) > TLSEXT_MAXLEN_host_name) {
             SSLfatal(s, SSL_AD_UNRECOGNIZED_NAME,
@@ -155,8 +159,12 @@ int tls_parse_ctos_server_name(SSL *s, PACKET *pkt, unsigned int context,
         }
 
         s->servername_done = 1;
-    }
-    if (s->hit) {
+    } else {
+        /*
+         * In TLSv1.2 and below we should check if the SNI is consistent between
+         * the initial handshake and the resumption. In TLSv1.3 SNI is not
+         * associated with the session.
+         */
         /*
          * TODO(openssl-team): if the SNI doesn't match, we MUST
          * fall back to a full handshake.
@@ -164,9 +172,6 @@ int tls_parse_ctos_server_name(SSL *s, PACKET *pkt, unsigned int context,
         s->servername_done = (s->session->ext.hostname != NULL)
             && PACKET_equal(&hostname, s->session->ext.hostname,
                             strlen(s->session->ext.hostname));
-
-        if (!s->servername_done && s->session->ext.hostname != NULL)
-            s->ext.early_data_ok = 0;
     }
 
     return 1;
@@ -1333,8 +1338,14 @@ EXT_RETURN tls_construct_stoc_server_name(SSL *s, WPACKET *pkt,
                                           unsigned int context, X509 *x,
                                           size_t chainidx)
 {
-    if (s->hit || s->servername_done != 1
-            || s->ext.hostname == NULL)
+    if (s->servername_done != 1)
+        return EXT_RETURN_NOT_SENT;
+
+    /*
+     * Prior to TLSv1.3 we ignore any SNI in the current handshake if resuming.
+     * We just use the servername from the initial handshake.
+     */
+    if (s->hit && !SSL_IS_TLS13(s))
         return EXT_RETURN_NOT_SENT;
 
     if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_server_name)