Do not reset SNI data in SSL_do_handshake()

PR #3783 introduce coded to reset the server side SNI state in
SSL_do_handshake() to ensure any erroneous config time SNI changes are
cleared. Unfortunately SSL_do_handshake() can be called mid-handshake
multiple times so this is the wrong place to do this and can mean that
any SNI data is cleared later on in the handshake too.

Therefore move the code to a more appropriate place.

Fixes #7014

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/7149)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 3d25da637d2bc793efa540ef1367f1f12448707c..d75158e30c4f1e934366ff79826fe9e3bcdaf092 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3559,12 +3559,6 @@ int SSL_do_handshake(SSL *s)
 
     s->method->ssl_renegotiate_check(s, 0);
 
-    if (SSL_is_server(s)) {
-        /* clear SNI settings at server-side */
-        OPENSSL_free(s->ext.hostname);
-        s->ext.hostname = NULL;
-    }
-
     if (SSL_in_init(s) || SSL_in_before(s)) {
         if ((s->mode & SSL_MODE_ASYNC) && ASYNC_get_current_job() == NULL) {
             struct ssl_async_args args;

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index cd4f078cf3d80622bf83a807e3b5f22534e4080e..8422161dc103679da683127a7e31ce043edffd1c 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -904,9 +904,13 @@ static int final_renegotiate(SSL *s, unsigned int context, int sent)
 
 static int init_server_name(SSL *s, unsigned int context)
 {
-    if (s->server)
+    if (s->server) {
         s->servername_done = 0;
 
+        OPENSSL_free(s->ext.hostname);
+        s->ext.hostname = NULL;
+    }
+
     return 1;
 }
 

diff --git a/test/build.info b/test/build.info
index 04014e72867c754b411df8b28add89e2087748f1..2c02ecc04010523ffad56e28839ce8a3bcef3171 100644 (file)
--- a/test/build.info
+++ b/test/build.info
@@ -364,7 +364,7 @@ INCLUDE_MAIN___test_libtestutil_OLB = /INCLUDE=MAIN
   INCLUDE[ciphername_test]=../include
   DEPEND[ciphername_test]=../libcrypto ../libssl libtestutil.a
 
-  SOURCE[servername_test]=servername_test.c
+  SOURCE[servername_test]=servername_test.c ssltestlib.c
   INCLUDE[servername_test]=../include
   DEPEND[servername_test]=../libcrypto ../libssl libtestutil.a
 

diff --git a/test/recipes/70-test_servername.t b/test/recipes/70-test_servername.t
index dae5d46baaa4a5b8515305673e6f6f426227989b..e6448aa88dcad787f2a4bfe9ac8116723ac30719 100644 (file)
--- a/test/recipes/70-test_servername.t
+++ b/test/recipes/70-test_servername.t
@@ -11,7 +11,7 @@ use strict;
 use warnings;
 
 use OpenSSL::Test::Simple;
-use OpenSSL::Test;
+use OpenSSL::Test qw/:DEFAULT srctop_file/;
 use OpenSSL::Test::Utils qw(alldisabled available_protocols);
 
 setup("test_servername");
@@ -19,4 +19,8 @@ setup("test_servername");
 plan skip_all => "No TLS/SSL protocols are supported by this OpenSSL build"
     if alldisabled(grep { $_ ne "ssl3" } available_protocols("tls"));
 
-simple_test("test_servername", "servername_test");
+plan tests => 1;
+
+ok(run(test(["servername_test", srctop_file("apps", "server.pem"),
+             srctop_file("apps", "server.pem")])),
+             "running servername_test");

diff --git a/test/servername_test.c b/test/servername_test.c
index e0c473e26793d82af323238d8f247663eccf77bf..6272baec189d4d6fd4602185e51e872afeab57a6 100644 (file)
--- a/test/servername_test.c
+++ b/test/servername_test.c
@@ -22,11 +22,15 @@
 
 #include "testutil.h"
 #include "internal/nelem.h"
+#include "ssltestlib.h"
 
 #define CLIENT_VERSION_LEN      2
 
 static const char *host = "dummy-host";
 
+static char *cert = NULL;
+static char *privkey = NULL;
+
 static int get_sni_from_client_hello(BIO *bio, char **sni)
 {
     long len;
@@ -176,45 +180,38 @@ end:
 
 static int server_setup_sni(void)
 {
-    SSL_CTX *ctx;
-    SSL *con = NULL;
-    BIO *rbio;
-    BIO *wbio;
-    int ret = 0;
+    SSL_CTX *cctx = NULL, *sctx = NULL;
+    SSL *clientssl = NULL, *serverssl = NULL;
+    int testresult = 0;
 
-    /* use TLS_server_method to choose 'server-side' */
-    ctx = SSL_CTX_new(TLS_server_method());
-    if (!TEST_ptr(ctx))
-        goto end;
-
-    con = SSL_new(ctx);
-    if (!TEST_ptr(con))
-        goto end;
-
-    rbio = BIO_new(BIO_s_mem());
-    wbio = BIO_new(BIO_s_mem());
-    if (!TEST_ptr(rbio)|| !TEST_ptr(wbio)) {
-        BIO_free(rbio);
-        BIO_free(wbio);
+    if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(),
+                                       TLS_client_method(),
+                                       TLS1_VERSION, TLS_MAX_VERSION,
+                                       &sctx, &cctx, cert, privkey))
+            || !TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl,
+                                             NULL, NULL)))
         goto end;
-    }
-
-    SSL_set_bio(con, rbio, wbio);
 
     /* set SNI at server side */
-    SSL_set_tlsext_host_name(con, host);
+    SSL_set_tlsext_host_name(serverssl, host);
 
-    if (!TEST_int_le(SSL_accept(con), 0))
-        /* This shouldn't succeed because we have nothing to listen on */
+    if (!TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE)))
         goto end;
-    if (!TEST_ptr_null(SSL_get_servername(con, TLSEXT_NAMETYPE_host_name)))
-        /* SNI should be cleared by SSL_accpet */
+
+    if (!TEST_ptr_null(SSL_get_servername(serverssl,
+                                          TLSEXT_NAMETYPE_host_name))) {
+        /* SNI should have been cleared during handshake */
         goto end;
-    ret = 1;
+    }
+    
+    testresult = 1;
 end:
-    SSL_free(con);
-    SSL_CTX_free(ctx);
-    return ret;
+    SSL_free(serverssl);
+    SSL_free(clientssl);
+    SSL_CTX_free(sctx);
+    SSL_CTX_free(cctx);
+
+    return testresult;
 }
 
 typedef int (*sni_test_fn)(void);
@@ -236,6 +233,10 @@ static int test_servername(int test)
 
 int setup_tests(void)
 {
+    if (!TEST_ptr(cert = test_get_argument(0))
+            || !TEST_ptr(privkey = test_get_argument(1)))
+        return 0;
+
     ADD_ALL_TESTS(test_servername, OSSL_NELEM(sni_test_fns));
     return 1;
 }