PR-URL: https://github.com/nodejs/node/pull/53636
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/52625
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
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>
If user sets invalid type for options.minDHSize in
tls.connect(), it's not internal issue of Node.js. So
validateNumber() is more proper than assert(). Plus,
set min of validateNumber() as 1 to check minDHSize
is positive.
Refs: https://github.com/nodejs/node/pull/49896
PR-URL: https://github.com/nodejs/node/pull/49973
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
If user uses invalid type for `options.checkServerIdentity`
in tls.connect(), it's not internal issue of Node.js. So
validateFunction() is more proper than assert().
Fixes: https://github.com/nodejs/node/issues/49839
PR-URL: https://github.com/nodejs/node/pull/49896
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Remove redundant code by moving it to outside of `if/else`.
PR-URL: https://github.com/nodejs/node/pull/49457
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Use validateFunction to remove duplicate implementation.
Plus, remove `assert()` statement because validateFunction
has the same purpose.
PR-URL: https://github.com/nodejs/node/pull/49422
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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>
PR-URL: https://github.com/nodejs/node/pull/45190
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
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>
Do not call tls.createSecureContext() if the context provided
is already an instance of tls.SecureContext.
Fixes: https://github.com/nodejs/node/issues/47408
PR-URL: https://github.com/nodejs/node/pull/47570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Throwing error after checking type is repeated. So replace
it with validate function.
PR-URL: https://github.com/nodejs/node/pull/46101
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/44731
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.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>
On the 'tlsClientError' event, the `tlsSocket` instance is passed as
`closed` status. Thus, users can't get information such as `remote
address`, `remoteFamily`, and so on.
This adds a flag to close a socket after emitting an `error` event.
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
PR-URL: https://github.com/nodejs/node/pull/44021
Fixes: https://github.com/nodejs/node/issues/43963
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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>
Incomplete validation of rejectUnauthorized parameter (Low)
If the Node.js https API was used incorrectly and "undefined" was passed
in for the "rejectUnauthorized" parameter, no error was returned and
connections to servers with an expired certificate would have been
accepted.
CVE-ID: CVE-2021-22939
Refs: https://nvd.nist.gov/vuln/detail/CVE-2021-22939
Refs: https://hackerone.com/reports/1278254
PR-URL: https://github.com/nodejs-private/node-private/pull/276
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Akshay K <iit.akshay@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: https://github.com/nodejs/node/pull/38608
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Add documentation for net.connect AbortSignal,
and add the support to tls.connect as well
PR-URL: https://github.com/nodejs/node/pull/37735
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/37070
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/37028
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
We have a few places where we individually forward each
parameter to tls.createSecureContext(). In #28973 and others,
we added new SecureContext options but forgot to keep these
places up to date.
As per https.Agent#getName, I understand that at least
`privateKeyIdentifier` and `privateKeyEngine` should be
added too, since they're a substitute for `key`. I've
also added sigalgs.
Fixes: https://github.com/nodejs/node/issues/36322
Refs: https://github.com/nodejs/node/pull/28973
PR-URL: https://github.com/nodejs/node/pull/36416
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Added options: `ticketKeys` and `sessionTimeout`, that are honored by
`createServer`, that calls `createSecureContext`.
This also introduces a minor code simplification.
PR-URL: https://github.com/nodejs/node/pull/33974
Fixes: https://github.com/nodejs/node/issues/20908
Reviewed-By: Alba Mendez <me@alba.sh>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Wrapped streams are expected to behave the same as socket with handle.
Remove unnecessary difference in handling.
PR-URL: https://github.com/nodejs/node/pull/34105
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@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>
