From 00b7b9f723f7ea43075182c5e8ffb18734ffa2f0 Mon Sep 17 00:00:00 2001
From: Mateusz Krawczuk <krawczukmat@gmail.com>
Date: Thu, 18 Jun 2020 16:36:04 +0200
Subject: [PATCH 1/5] tls: stronger validation for 'servername' in
 server.addContext

If 'servername' is not provided, 'ERR_MISSING_ARGS is thrown.
If 'servername' is not a string, 'ERR_INVALID_ARG_TYPE' is thrown.
If 'servername' is an IP address, 'ERR_INVALID_ARG_VALUE' is thrown, since
literal IPv4 and IPv6 addresses are not permitted in SNI.

Fixed API documentation ('hostname' -> 'servername').

Also removed a redundant error 'ERR_TLS_REQUIRED_SERVER_NAME'.
---
 doc/api/errors.md                           |  6 ------
 doc/api/tls.md                              |  9 ++++----
 lib/_tls_wrap.js                            | 16 ++++++++++++--
 lib/internal/errors.js                      |  3 ---
 test/parallel/test-tls-sni-server-client.js | 23 +++++++++++++++++++++
 5 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index fdc3cc1e6ecb42..ab145661d289a3 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -2063,12 +2063,6 @@ attempt to set the `secureProtocol` explicitly. Use one mechanism or the other.
 
 An attempt was made to renegotiate TLS on a socket instance with TLS disabled.
 
-<a id="ERR_TLS_REQUIRED_SERVER_NAME"></a>
-### `ERR_TLS_REQUIRED_SERVER_NAME`
-
-While using TLS, the `server.addContext()` method was called without providing
-a host name in the first parameter.
-
 <a id="ERR_TLS_SESSION_ATTACK"></a>
 ### `ERR_TLS_SESSION_ATTACK`
 
diff --git a/doc/api/tls.md b/doc/api/tls.md
index 96e054e533a666..3027336339a74d 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -545,18 +545,19 @@ called:
 * `tlsSocket` {tls.TLSSocket} The `tls.TLSSocket` instance from which the
   error originated.
 
-### `server.addContext(hostname, context)`
+### `server.addContext(servername, context)`
 <!-- YAML
 added: v0.5.3
 -->
-
-* `hostname` {string} A SNI host name or wildcard (e.g. `'*'`)
+`
+* `servername` {string} A SNI server name or wildcard (e.g. `'*'`). Must not be
+  an IP address.
 * `context` {Object} An object containing any of the possible properties
   from the [`tls.createSecureContext()`][] `options` arguments (e.g. `key`,
   `cert`, `ca`, etc).
 
 The `server.addContext()` method adds a secure context that will be used if
-the client request's SNI name matches the supplied `hostname` (or wildcard).
+the client request's SNI name matches the supplied `servername` (or wildcard).
 
 ### `server.address()`
 <!-- YAML
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 02fd7b002651c3..cec7e325ff2ab0 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -60,13 +60,13 @@ const {
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_ARG_VALUE,
   ERR_INVALID_CALLBACK,
+  ERR_MISSING_ARGS,
   ERR_MULTIPLE_CALLBACK,
   ERR_SOCKET_CLOSED,
   ERR_TLS_DH_PARAM_SIZE,
   ERR_TLS_HANDSHAKE_TIMEOUT,
   ERR_TLS_INVALID_CONTEXT,
   ERR_TLS_RENEGOTIATION_DISABLED,
-  ERR_TLS_REQUIRED_SERVER_NAME,
   ERR_TLS_SESSION_ATTACK,
   ERR_TLS_SNI_FROM_SERVER,
   ERR_TLS_INVALID_STATE
@@ -1411,7 +1411,19 @@ Server.prototype.setOptions = deprecate(function(options) {
 // SNI Contexts High-Level API
 Server.prototype.addContext = function(servername, context) {
   if (!servername) {
-    throw new ERR_TLS_REQUIRED_SERVER_NAME();
+    throw new ERR_MISSING_ARGS('servername');
+  }
+
+  if (typeof servername !== 'string') {
+    throw new ERR_INVALID_ARG_TYPE('servername', 'string', servername);
+  }
+
+  if (net.isIP(servername)) {
+    throw new ERR_INVALID_ARG_VALUE(
+      'servername',
+      servername,
+      'must not be an IP address'
+    );
   }
 
   const re = new RegExp('^' +
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 34ef6368218aac..fba2d9cf679e3b 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -1445,9 +1445,6 @@ E('ERR_TLS_PROTOCOL_VERSION_CONFLICT',
 E('ERR_TLS_RENEGOTIATION_DISABLED',
   'TLS session renegotiation disabled for this socket', Error);
 
-// This should probably be a `TypeError`.
-E('ERR_TLS_REQUIRED_SERVER_NAME',
-  '"servername" is required parameter for Server.addContext', Error);
 E('ERR_TLS_SESSION_ATTACK', 'TLS session renegotiation attack detected', Error);
 E('ERR_TLS_SNI_FROM_SERVER',
   'Cannot issue SNI from a TLS server-side socket', Error);
diff --git a/test/parallel/test-tls-sni-server-client.js b/test/parallel/test-tls-sni-server-client.js
index 79f3601561ee19..9552665d470868 100644
--- a/test/parallel/test-tls-sni-server-client.js
+++ b/test/parallel/test-tls-sni-server-client.js
@@ -128,3 +128,26 @@ function test(options, clientResult, serverResult) {
     }));
   });
 }
+
+// Ensure an error is thrown if 'servername' is not specified.
+assert.throws(() =>
+  tls.createServer(serverOptions, () => {}).addContext(),
+              {
+                code: 'ERR_MISSING_ARGS'
+              });
+
+// Ensure an error is thrown is 'servername' is not a string.
+assert.throws(() =>
+  tls.createServer(serverOptions, () => {}).addContext(7),
+              {
+                code: 'ERR_INVALID_ARG_TYPE'
+              });
+
+// Ensure an error is thrown if 'servername' is an IP address.
+assert.throws(() =>
+  tls.createServer(serverOptions, () => {}).addContext('::'),
+              {
+                code: 'ERR_INVALID_ARG_VALUE',
+                message: 'The argument \'servername\' must not be an IP ' +
+                'address. Received \'::\''
+              });

From 65329185aedbbb8719639d15575b9d3f26c16c03 Mon Sep 17 00:00:00 2001
From: Mateusz Krawczuk <krawczukmat@gmail.com>
Date: Thu, 18 Jun 2020 18:03:42 +0200
Subject: [PATCH 2/5] fixup! remove a nasty backtick.

---
 doc/api/tls.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/api/tls.md b/doc/api/tls.md
index 3027336339a74d..c91d9a01db3772 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -549,7 +549,7 @@ called:
 <!-- YAML
 added: v0.5.3
 -->
-`
+
 * `servername` {string} A SNI server name or wildcard (e.g. `'*'`). Must not be
   an IP address.
 * `context` {Object} An object containing any of the possible properties

From 38d50aa5a8c6f79546dc35c54c16b569696b02bd Mon Sep 17 00:00:00 2001
From: Mateusz Krawczuk <krawczukmat@gmail.com>
Date: Fri, 19 Jun 2020 11:42:35 +0200
Subject: [PATCH 3/5] fixup! correct doc link.

---
 doc/api/tls.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/api/tls.md b/doc/api/tls.md
index c91d9a01db3772..738a51a5782815 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -1954,7 +1954,7 @@ where `secureSocket` has the same API as `pair.cleartext`.
 [`net.Server.address()`]: net.html#net_server_address
 [`net.Server`]: net.html#net_class_net_server
 [`net.Socket`]: net.html#net_class_net_socket
-[`server.addContext()`]: #tls_server_addcontext_hostname_context
+[`server.addContext()`]: #tls_server_addcontext_servername_context
 [`server.getTicketKeys()`]: #tls_server_getticketkeys
 [`server.listen()`]: net.html#net_server_listen
 [`server.setTicketKeys()`]: #tls_server_setticketkeys_keys

From 9149d3b5ad1c7ebe367ff0fab60227f196962111 Mon Sep 17 00:00:00 2001
From: Mateusz Krawczuk <krawczukmat@gmail.com>
Date: Fri, 19 Jun 2020 19:26:27 +0200
Subject: [PATCH 4/5] fixup! add deleted error into legacy codes.

---
 doc/api/errors.md | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index ab145661d289a3..904f344522ce74 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -2528,6 +2528,12 @@ removed: v10.0.0
 
 Used when a TLS renegotiation request has failed in a non-specific way.
 
+<a id="ERR_TLS_REQUIRED_SERVER_NAME"></a>
+### `ERR_TLS_REQUIRED_SERVER_NAME`
+
+While using TLS, the `server.addContext()` method was called without providing
+a host name in the first parameter.
+
 <a id="ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER"></a>
 ### `ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER`
 <!-- YAML

From cb77037cf82b9736d9528d80483d49f16a130189 Mon Sep 17 00:00:00 2001
From: Mateusz Krawczuk <krawczukmat@gmail.com>
Date: Sat, 20 Jun 2020 01:03:28 +0200
Subject: [PATCH 5/5] fixup!

---
 doc/api/errors.md | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 904f344522ce74..d19cdbdd439c8a 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -2530,6 +2530,10 @@ Used when a TLS renegotiation request has failed in a non-specific way.
 
 <a id="ERR_TLS_REQUIRED_SERVER_NAME"></a>
 ### `ERR_TLS_REQUIRED_SERVER_NAME`
+<!-- YAML
+added: v9.0.0
+removed: REPLACEME
+-->
 
 While using TLS, the `server.addContext()` method was called without providing
 a host name in the first parameter.