-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
Description
Affected URL(s)
https://nodejs.org/docs/latest-v18.x/api/tls.html
Description of the problem
What is the correct, non-deprecated way to use the new tls.TLSSocket(...) constructor to establish a secure connection? Context: GHSA-2cpx-6pqp-wf35
According to two unmerged docs PRs, when directly calling new tls.TLSSocket(...) it is the user's responsibility to validate peer certificates and identity.
In #10846 it says:
Warning: When directly constructing a
tls.TLSSocketinstead of using
[tls.connect()][] it is the caller's responsibility to:
- manage the lifetime of the the underlying socket, including connecting it;
- validate the peer certificate and identity, see the [
'secure'][] event.
Before using the connection, the user must make the following
checks or the connection should be considered completely insecure:
- Verify that the peer certificate is valid, see [
ssl.verifyError()][].- Verify that the peer certificate is for the expected host, see
[tls.checkServerIdentity()][] and [tls.TLSSocket.getPeerCertificate()][].
In #23915 it says:
It is important to remember, however,
that it is the caller's responsibility to manage the lifecycle of the provided
net.Socket, including establishing the connection and validating peer
certificates and identity. See the ['secure'][] event.
And includes an example:
tlsSocket.on('secure', function() {
const err = this.verifyError() ||
tls.checkServerIdentity(hostname, this.getPeerCertificate());
if (err)
this.destroy(err);
});Both PRs demonstrate how to do this validation, but require use of:
- The
'secure'event. In the current Node.js documentation, the only mention of'secure'is under the deprecatedtls.SecurePair, and is itself deprecated. It is also not clear that the'secure'event is also emitted ontls.TLSSocket.
https://nodejs.org/docs/latest-v18.x/api/tls.html#event-secure tlsSocket.ssl.verifyError(), which does not appear at all in the current documentation. Furthermore, according to TLSCallbacks => TLSWrap, better TLS inception #840 (comment)tlsSocket.sslis a "legacy property".
Note that the described validation steps appear to be consistent with internal use
Line 1106 in 5fbf33e
| socket.on('secure', onServerSocketSecure); |
Lines 1044 to 1055 in 5fbf33e
| function onServerSocketSecure() { | |
| if (this._requestCert) { | |
| const verifyError = this._handle.verifyError(); | |
| if (verifyError) { | |
| this.authorizationError = verifyError.code; | |
| if (this._rejectUnauthorized) | |
| this.destroy(); | |
| } else { | |
| this.authorized = true; | |
| } | |
| } |
This leaves me with two concerns:
- The current documentation does not indicate that using
new tls.TLSSocket(...)by itself does not result in a secure connection. - As far as I can tell it is impossible to use
new tls.TLSSocket(...)to establish a secure connection without relying on APIs that are undocumented, deprecated, and/or legacy.