Ensure s_client sends an SNI extension by default

Enforcement of an SNI extension in the initial ClientHello is becoming
increasingly common (e.g. see GitHub issue #2580). This commit changes
s_client so that it adds SNI be default, unless explicitly told not to via
the new "-noservername" option.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2614)

6 files changed, 58 insertions, 18 deletions

diff --git a/CHANGES b/CHANGES
index b1476d8ba6..7bd0f924f1 100644
--- a/CHANGES
+++ b/CHANGES
@@ -49,6 +49,12 @@
   *) Add support for ARIA
      [Paul Dale]
 
+  *) s_client will now send the Server Name Indication (SNI) extension by
+     default unless the new "-noservername" option is used. The server name is
+     based on the host provided to the "-connect" option unless overridden by
+     using "-servername".
+     [Matt Caswell]
+
   *) Add support for SipHash
      [Todd Short]
 
diff --git a/apps/s_client.c b/apps/s_client.c
index efdc8e3ef3..c544d49b6d 100644
--- a/apps/s_client.c
+++ b/apps/s_client.c
@@ -544,7 +544,7 @@ typedef enum OPTION_choice {
         OPT_VERIFYCAPATH,
     OPT_KEY, OPT_RECONNECT, OPT_BUILD_CHAIN, OPT_CAFILE, OPT_NOCAFILE,
     OPT_CHAINCAFILE, OPT_VERIFYCAFILE, OPT_NEXTPROTONEG, OPT_ALPN,
-    OPT_SERVERINFO, OPT_STARTTLS, OPT_SERVERNAME,
+    OPT_SERVERINFO, OPT_STARTTLS, OPT_SERVERNAME, OPT_NOSERVERNAME,
     OPT_USE_SRTP, OPT_KEYMATEXPORT, OPT_KEYMATEXPORTLEN, OPT_SMTPHOST,
     OPT_ASYNC, OPT_SPLIT_SEND_FRAG, OPT_MAX_PIPELINES, OPT_READ_BUF,
     OPT_KEYLOG_FILE, OPT_EARLY_DATA, OPT_REQCAFILE,
@@ -652,6 +652,8 @@ const OPTIONS s_client_options[] = {
     {"nocommands", OPT_NOCMDS, '-', "Do not use interactive command letters"},
     {"servername", OPT_SERVERNAME, 's',
      "Set TLS extension servername in ClientHello"},
+    {"noservername", OPT_NOSERVERNAME, '-',
+     "Do not send the server name (SNI) extension in the ClientHello"},
     {"tlsextdebug", OPT_TLSEXTDEBUG, '-',
      "Hex dump of all TLS extensions received"},
 #ifndef OPENSSL_NO_OCSP
@@ -872,6 +874,7 @@ int s_client_main(int argc, char **argv)
     struct timeval tv;
 #endif
     char *servername = NULL;
+    int noservername = 0;
     const char *alpn_in = NULL;
     tlsextctx tlsextcbp = { NULL, 0 };
     const char *ssl_config = NULL;
@@ -1359,6 +1362,9 @@ int s_client_main(int argc, char **argv)
         case OPT_SERVERNAME:
             servername = opt_arg();
             break;
+        case OPT_NOSERVERNAME:
+            noservername = 1;
+            break;
         case OPT_USE_SRTP:
             srtp_profiles = opt_arg();
             break;
@@ -1399,6 +1405,20 @@ int s_client_main(int argc, char **argv)
         BIO_printf(bio_err, "%s: Can't use both -4 and -6\n", prog);
         goto opthelp;
     }
+    if (noservername) {
+        if (servername != NULL) {
+            BIO_printf(bio_err,
+                       "%s: Can't use -servername and -noservername together\n",
+                       prog);
+            goto opthelp;
+        }
+        if (dane_tlsa_domain != NULL) {
+            BIO_printf(bio_err,
+               "%s: Can't use -dane_tlsa_domain and -noservername together\n",
+               prog);
+            goto opthelp;
+        }
+    }
     argc = opt_num_rest();
     if (argc != 0)
         goto opthelp;
@@ -1720,7 +1740,7 @@ int s_client_main(int argc, char **argv)
     if (!set_cert_key_stuff(ctx, cert, key, chain, build_chain))
         goto end;
 
-    if (servername != NULL) {
+    if (!noservername) {
         tlsextcbp.biodebug = bio_err;
         SSL_CTX_set_tlsext_servername_callback(ctx, ssl_servername_cb);
         SSL_CTX_set_tlsext_servername_arg(ctx, &tlsextcbp);
@@ -1793,7 +1813,9 @@ int s_client_main(int argc, char **argv)
     if (fallback_scsv)
         SSL_set_mode(con, SSL_MODE_SEND_FALLBACK_SCSV);
 
-    if (servername != NULL) {
+    if (!noservername && (servername != NULL || dane_tlsa_domain == NULL)) {
+        if (servername == NULL)
+            servername = (host == NULL) ? "localhost" : host;
         if (!SSL_set_tlsext_host_name(con, servername)) {
             BIO_printf(bio_err, "Unable to set TLS servername extension.\n");
             ERR_print_errors(bio_err);
@@ -2459,7 +2481,7 @@ int s_client_main(int argc, char **argv)
             if (in_init) {
                 in_init = 0;
 
-                if (servername != NULL && !SSL_session_reused(con)) {
+                if (!noservername && !SSL_session_reused(con)) {
                     BIO_printf(bio_c_out,
                                "Server did %sacknowledge servername extension.\n",
                                tlsextcbp.ack ? "" : "not ");
diff --git a/doc/man1/s_client.pod b/doc/man1/s_client.pod
index cc2ad4f1f2..16538567bc 100644
--- a/doc/man1/s_client.pod
+++ b/doc/man1/s_client.pod
@@ -14,6 +14,7 @@ B<openssl> B<s_client>
 [B<-4>]
 [B<-6>]
 [B<-servername name>]
+[B<-noservername>]
 [B<-verify depth>]
 [B<-verify_return_error>]
 [B<-cert filename>]
@@ -156,7 +157,17 @@ Use IPv6 only.
 
 =item B<-servername name>
 
-Set the TLS SNI (Server Name Indication) extension in the ClientHello message.
+Set the TLS SNI (Server Name Indication) extension in the ClientHello message to
+the given value.
+
+=item B<-noservername>
+
+Suppresses sending of the SNI (Server Name Indication) extension in the
+ClientHello message. Cannot be used in conjunction with the B<-servername> or
+<-dane_tlsa_domain> options. If this option is not given then the hostname
+provided to the B<-connect> option is used in the SNI extension, or "localhost"
+if B<-connect> has not been supplied. Note that an SNI name should normally be a
+DNS name and not an IP address.
 
 =item B<-cert certname>
 
diff --git a/test/recipes/70-test_sslmessages.t b/test/recipes/70-test_sslmessages.t
index a6278dc630..a763486f5b 100644
--- a/test/recipes/70-test_sslmessages.t
+++ b/test/recipes/70-test_sslmessages.t
@@ -222,22 +222,23 @@ checkhandshake($proxy, checkhandshake::RENEG_HANDSHAKE,
                checkhandshake::DEFAULT_EXTENSIONS,
                "Rengotiation handshake test");
 
-#Test 8: Server name handshake (client request only)
+#Test 8: Server name handshake (no client request)
 $proxy->clear();
-$proxy->clientflags("-no_tls1_3 -servername testhost");
+$proxy->clientflags("-no_tls1_3 -noservername");
 $proxy->start();
 checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE,
                checkhandshake::DEFAULT_EXTENSIONS
-               | checkhandshake::SERVER_NAME_CLI_EXTENSION,
+               & ~checkhandshake::SERVER_NAME_CLI_EXTENSION,
                "Server name handshake test (client)");
 
 #Test 9: Server name handshake (server support only)
 $proxy->clear();
-$proxy->clientflags("-no_tls1_3");
+$proxy->clientflags("-no_tls1_3 -noservername");
 $proxy->serverflags("-servername testhost");
 $proxy->start();
 checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE,
-               checkhandshake::DEFAULT_EXTENSIONS,
+               checkhandshake::DEFAULT_EXTENSIONS
+               & ~checkhandshake::SERVER_NAME_CLI_EXTENSION,
                "Server name handshake test (server)");
 
 #Test 10: Server name handshake (client and server)
@@ -247,7 +248,6 @@ $proxy->serverflags("-servername testhost");
 $proxy->start();
 checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE,
                checkhandshake::DEFAULT_EXTENSIONS
-               | checkhandshake::SERVER_NAME_CLI_EXTENSION
                | checkhandshake::SERVER_NAME_SRV_EXTENSION,
                "Server name handshake test");
 
diff --git a/test/recipes/70-test_tls13messages.t b/test/recipes/70-test_tls13messages.t
index 0d25beffa9..c4e20b7614 100644
--- a/test/recipes/70-test_tls13messages.t
+++ b/test/recipes/70-test_tls13messages.t
@@ -200,21 +200,23 @@ checkhandshake($proxy, checkhandshake::CLIENT_AUTH_HANDSHAKE,
                checkhandshake::DEFAULT_EXTENSIONS,
                "Client auth handshake test");
 
-#Test 7: Server name handshake (client request only)
+#Test 7: Server name handshake (no client request)
 $proxy->clear();
-$proxy->clientflags("-servername testhost");
+$proxy->clientflags("-noservername");
 $proxy->start();
 checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE,
                checkhandshake::DEFAULT_EXTENSIONS
-               | checkhandshake::SERVER_NAME_CLI_EXTENSION,
+               & ~checkhandshake::SERVER_NAME_CLI_EXTENSION,
                "Server name handshake test (client)");
 
 #Test 8: Server name handshake (server support only)
 $proxy->clear();
+$proxy->clientflags("-noservername");
 $proxy->serverflags("-servername testhost");
 $proxy->start();
 checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE,
-               checkhandshake::DEFAULT_EXTENSIONS,
+               checkhandshake::DEFAULT_EXTENSIONS
+               & ~checkhandshake::SERVER_NAME_CLI_EXTENSION,
                "Server name handshake test (server)");
 
 #Test 9: Server name handshake (client and server)
@@ -224,7 +226,6 @@ $proxy->serverflags("-servername testhost");
 $proxy->start();
 checkhandshake($proxy, checkhandshake::DEFAULT_HANDSHAKE,
                checkhandshake::DEFAULT_EXTENSIONS
-               | checkhandshake::SERVER_NAME_CLI_EXTENSION
                | checkhandshake::SERVER_NAME_SRV_EXTENSION,
                "Server name handshake test");
 
diff --git a/test/testlib/checkhandshake.pm b/test/testlib/checkhandshake.pm
index 43efe81327..d5d0e29ee5 100644
--- a/test/testlib/checkhandshake.pm
+++ b/test/testlib/checkhandshake.pm
@@ -31,8 +31,8 @@ use constant {
 };
 
 use constant {
-    #DEFAULT ALSO INCLUDES SESSION_TICKET_SRV_EXTENSION
-    DEFAULT_EXTENSIONS => 0x00000003,
+    #DEFAULT also includes SESSION_TICKET_SRV_EXTENSION and SERVER_NAME_CLI
+    DEFAULT_EXTENSIONS => 0x00000007,
     SESSION_TICKET_SRV_EXTENSION => 0x00000002,
     SERVER_NAME_CLI_EXTENSION => 0x00000004,
     SERVER_NAME_SRV_EXTENSION => 0x00000008,