Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tls: stronger validation for 'servername' in server.addContext #33943

Closed
wants to merge 5 commits into from
Closed

tls: stronger validation for 'servername' in server.addContext #33943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
00b7b9f
tls: stronger validation for 'servername' in server.addContext
mkrawczuk Jun 18, 2020
6532918
fixup! remove a nasty backtick.
mkrawczuk Jun 18, 2020
38d50aa
fixup! correct doc link.
mkrawczuk Jun 19, 2020
9149d3b
fixup! add deleted error into legacy codes.
mkrawczuk Jun 19, 2020
cb77037
fixup!
mkrawczuk Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
mkrawczuk marked this conversation as resolved.
Show resolved Hide resolved

<a id="ERR_TLS_SESSION_ATTACK"></a>
### `ERR_TLS_SESSION_ATTACK`

Expand Down Expand Up @@ -2534,6 +2528,16 @@ 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`
mkrawczuk marked this conversation as resolved.
Show resolved Hide resolved
<!-- 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.

<a id="ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER"></a>
### `ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER`
<!-- YAML
Expand Down
9 changes: 5 additions & 4 deletions doc/api/tls.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think hostname is better. This is how TLS defines a "server name":

struct {
  NameType name_type;
  select (name_type) {
    case host_name: HostName;
  } name;
} ServerName;

It is correct that TLS does not allow IP addresses in the HostName field, but that is not necessarily true for ServerName. TLS simply doesn't support other NameTypes so far, but it might do so in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the point, but... the extension has never changed and "server name" === "host name server name" in everyone's minds. I personally don't think it's worth it to expose this detail to users, nor have I seen any other API do it.

So I'm personally +1 for naming the parameter servername and stating things like "Setting the TLS ServerName to an IP address is not permitted", as we currently do.

<!-- 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
Expand Down Expand Up @@ -1953,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
Expand Down
16 changes: 14 additions & 2 deletions lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it semver-major, I think? (Meaning that it cannot land on any of the active release lines.)

}

if (typeof servername !== 'string') {
throw new ERR_INVALID_ARG_TYPE('servername', 'string', servername);
}

if (net.isIP(servername)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what happens if servername is neither an IP nor a DNS hostname?

throw new ERR_INVALID_ARG_VALUE(
'servername',
servername,
'must not be an IP address'
);
}

const re = new RegExp('^' +
Expand Down
3 changes: 0 additions & 3 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-tls-sni-server-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 \'::\''
});