If user uses invalid type for `options.SNICallback` in
TLSSocket(), it's not internal issue of Node.js. So
validateFunction() is more proper than assert().
PR-URL: https://github.com/nodejs/node/pull/50530
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Without this, some heavy usage of TLS sockets can result in
MaxListenersExceededWarning firing, from the 'this.on('close', ...)'
line here.
These appear to come from reinitializeHandle, which calls _wrapHandle
repeatedly on the same socket instance.
PR-URL: https://github.com/nodejs/node/pull/50136
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This fixes a potential segfault, among various other likely-related
issues, which all occur because TLSSockets were not informed if their
underlying stream was closed in many cases.
This also significantly modifies an existing TLS test. With this change
in place, that test no longer works, as it tries to mess with internals
to trigger a race, and those internals are now cleaned up earlier. This
test has been simplified to a more general TLS shutdown test.
PR-URL: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Fixs two issues in `TLSWrap`, one of them is reported in
https://github.com/nodejs/node/issues/30896.
1. `TLSWrap` has exactly one `StreamListener`, however,
that `StreamListener` can be replaced. We have not been
rigorous enough here: if an active write has not been
finished before the transition, the finish callback of it
will be wrongly fired the successor `StreamListener`.
2. A `TLSWrap` does not allow more than one active write,
as checked in the assertion about current_write in
`TLSWrap::DoWrite()`.
However, when users make use of an existing `tls.TLSSocket`
to establish double TLS, by
either
tls.connect({socket: tlssock})
or
tlsServer.emit('connection', tlssock)
we have both of the user provided `tls.TLSSocket`, tlssock and
a brand new created `TLSWrap` writing to the `TLSWrap` bound to
tlssock, which easily violates the constranint because two writers
have no idea of each other.
The design of the fix is:
when a `TLSWrap` is created on top of a user provided socket,
do not send any data to the socket until all existing writes
of the socket are done and ensure registered callbacks of
those writes can be fired.
PR-URL: https://github.com/nodejs/node/pull/48969
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.
With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.
PR-URL: https://github.com/nodejs/node/pull/48255
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
It doesn't make sense from a performance perspective to retain an
arraybuffer with the ALPN byte string and look it up as a property on
the JS context object for every TLS handshake.
Store the byte string in the C++ TLSWrap object instead. That's both
a lot faster and a lot simpler.
PR-URL: https://github.com/nodejs/node/pull/44875
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
TLSWrap.onerror has a helpful debug() call built in to it. However in
case of a malformed TLSSocket object, where the `_tlsOptions` value is
an unexpected `undefined`, accessing `_tlsOptions.isServer` causes
a TypeError to be thrown.
This commit ensures that the debug() call properly logs the state as
'unknown', instead of the two 'server' and 'client' choices previously
available. Additionally, onerror branching is adjusted to allow such
`undefined` options object, by use of optional chaining.
Other methods are not being adjusted, as such a case of `undefined`
options is not viable during regular processing of the TLSSocket.
Fixes: https://github.com/nodejs/node/issues/41501
PR-URL: https://github.com/nodejs/node/pull/41523
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Prior to this patch `session` event was emitted after `secure` event on
TLSSocket, but before `secureConnect` event. This is problematic for
`https.Agent` because it must cache session only after verifying the
remote peer's certificate.
Connecting to a server that presents an invalid certificate resulted
in the session being cached after the handshake with the server and
evicted right after a certifiate validation error and socket's
destruction. A request initiated during this narrow window would pick
the faulty session, send it to the malicious server and skip the
verification of the server's certificate.
Fixes: https://hackerone.com/reports/811502
CVE-ID: CVE-2020-8172
PR-URL: https://github.com/nodejs-private/node-private/pull/200
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
There's a typo that causes only the first socket to be logged
(i.e. when the warning is emitted).
In addition, server sockets aren't logged because `keylog` events
are not emitted on tls.Server, not the socket. This behaviour is
counterintuitive and has caused more bugs in the past, so make all
sockets (server or client) emit 'keylog'. tls.Server will just
re-emit these events.
Refs: https://github.com/nodejs/node/pull/30055
PR-URL: https://github.com/nodejs/node/pull/33366
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.
This fix sets secureConnecting to false when a client TLS socket
has connected.
PR-URL: https://github.com/nodejs/node/pull/33209
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Aras Abbasi