getDefaultEncoding() always returns 'buffer' in Node.js 20.
In diffiehellman.js, this value is always used as input to either
toBuf(), encode(), or getArrayBufferOrView(). All of these functions
treat any falsy encoding just like 'buffer', so we can safely remove the
calls to getDefaultEncoding().
Refs: https://github.com/nodejs/node/pull/47182
PR-URL: https://github.com/nodejs/node/pull/49169
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
This (not particularly elegant) native addon tests the effect of
UV_THREADPOOL_SIZE on node-api. The test fails if Node.js allows more
than UV_THREADPOOL_SIZE async tasks to run concurrently, or if it limits
the number of concurrent async tasks to anything less than
UV_THREADPOOL_SIZE.
PR-URL: https://github.com/nodejs/node/pull/49165
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
getDefaultEncoding() always returns 'buffer' in Node.js 20. It requires
some careful justification but the default encoding can be eliminated
from sig.js entirely.
In Sign.prototype.update, we can safely remove the conditional
assignment of getDefaultEncoding() to encoding. This is because
SignUpdate() in crypto_sig.cc internally calls node::crypto::Decode,
which returns UTF8 for falsy encoding values. In other words, with the
conditional assignment, StringBytes::Write() ultimately receives the
encoding BUFFER, and without the conditional assignment, it receives the
encoding UTF8. However, StringBytes::Write() treats both encodings
identically, so there is no need to deviate from the internal default
encoding UTF8.
In Sign.prototype.sign, we can also safely remove the conditional
assignment of getDefaultEncoding() to encoding. Whether encoding is
falsy or 'buffer' makes no difference.
In Verify.prototype.verify, we can also safely remove the conditional
assignment of getDefaultEncoding() to sigEncoding. This is because the
function passes the sigEncoding to getArrayBufferOrView(), which passes
it to Buffer.from(). If sigEncoding is 'buffer', getArrayBufferOrView()
instead passes 'utf8' to Buffer.from(). Because the default encoding of
Buffer.from() is 'utf8', passing a falsy encoding to
getArrayBufferOrView() instead of 'buffer' results in the same behavior.
Refs: https://github.com/nodejs/node/pull/47182
PR-URL: https://github.com/nodejs/node/pull/49145
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
When the experimental permission model is enabled, the running
JavaScript code is subject to certain restrictions, all of which can
be bypassed or even disabled by native code due to the nature of the
permission model implementation. That is why Node.js native addons
are disabled by default when the permission model is enabled. However,
the built-in crypto, https, and tls modules still allow loading
custom OpenSSL engines. Because OpenSSL engines can execute arbitrary
(native) code while being loaded by Node.js, this has the same security
implications as allowing native addons. In other words, allowing user
code to load OpenSSL engines at runtime effectively enables bypassing
any supposed security restrictions.
This patch adds appropriate checks before attempting to dynamically
load an OpenSSL engine that throw an error if the permission model is
enabled.
PR-URL: https://github.com/nodejs-private/node-private/pull/409
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
CVE-ID: CVE-2023-30586
The examples for generateKey() and generateKeySync() generate 64-bit
HMAC keys. That is inadequate for virtually any HMAC instance. As per
common NIST recommendations, the minimum should be roughly 112 bits, or
more commonly 128 bits.
Due to the design of HMAC itself, it is not unreasonable to choose the
underlying hash function's block size as the key length. For many
popular hash functions (SHA-256, SHA-224, SHA-1, MD5, ...) this happens
to be 64 bytes (bytes, not bits!). This is consistent with the HMAC
implementation in .NET, for example, even though it provides virtually
no benefit over a 256-bit key.
PR-URL: https://github.com/nodejs/node/pull/48052
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The error is not necessarily due to a client certificate engine. For
example, the `privateKeyEngine` option might just as well cause this
error and is independent of the client certificate.
Also mention that this is likely due to a compile-time option of OpenSSL
itself and not due to any particular engine.
PR-URL: https://github.com/nodejs/node/pull/47976
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
When OpenSSL is configured with OPENSSL_NO_ENGINE, setEngine() currently
throws an internal error because the C++ binding does not export the
relevant function, which causes _setEngine() to be undefined within JS.
Instead, match the behavior of tls/secure-context.js and throw the
existing error code ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED when OpenSSL
has been configured with OPENSSL_NO_ENGINE.
PR-URL: https://github.com/nodejs/node/pull/47977
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
SKIP_CHECK_VALUE is a string literal, so its size is the length of the
string in chars plus one. The buffer buf is also always null-terminated,
so its size should match the size of SKIP_CHECK_VALUE, which is _not_
SKIP_CHECK_SIZE.
Rename SKIP_CHECK_SIZE to be consistent with C/C++ terminology.
PR-URL: https://github.com/nodejs/node/pull/47845
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
The http2 implementation uses the deprecated function
nghttp2_session_callbacks_set_error_callback, which does not supply an
error code but only an error message. This so far forced node's error
callback to rely on the error message in order to distinguish between
different errors, which is fragile and inefficient.
Use the newer nghttp2_session_callbacks_set_error_callback2 function
instead, which is not deprecated and which provides the exact error code
to node's error callback.
PR-URL: https://github.com/nodejs/node/pull/47840
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
In general, we assume that the tmpdir will provide sufficient space for
most tests. Some tests, however, require hundreds of megabytes or even
gigabytes of space, which often causes them to fail, especially on our
macOS infrastructure. The most recent reliability report contains more
than 20 related CI failures.
This change adds a new function hasEnoughSpace() to the tmpdir module
that uses statfsSync() to guess whether allocating a certain amount of
space within the temporary directory will succeed.
This change also updates the most frequently failing tests to use the
new function such that the relevant parts of the tests are skipped if
tmpdir has insufficient space.
Refs: https://github.com/nodejs/reliability/issues/549
PR-URL: https://github.com/nodejs/node/pull/47767
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Richard Lau <rlau@redhat.com>
The documentation appears to still be wrong w.r.t. the meaning of the
concurrency option of the test() function. The implementation appears to
default to Infinity when the option is set to true. Is that intended or
a good idea? I don't know. It certainly makes more sense than what the
documentation says (which is basing the number of concurrent tasks
within a single thread on the number of CPU cores).
This changes the documentation to hopefully match the implementation and
adds a test that rules out the (rather arbitrary) behavior described in
the documentation.
Refs: https://github.com/nodejs/node/issues/47365
Refs: https://github.com/nodejs/node/pull/47642
PR-URL: https://github.com/nodejs/node/pull/47734
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
CSPRNG implicitly casts the size_t length argument to a signed int when
calling RAND_bytes(), which leaves it up to the caller to ensure that
the length argument actually fits into such a signed int. However, not
all call sites explicitly ensure that, which could lead to subtle bugs.
In OpenSSL 3, use RAND_bytes_ex() instead, which does not require
casting the length to a signed int.
In OpenSSL 1.1.1, RAND_bytes_ex() is not supported, thus we have to
process blocks of size INT_MAX one by one.
PR-URL: https://github.com/nodejs/node/pull/47515
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The JS layer already uses validateInt32(keylen, 'keylen', 0) to ensure
that the keylen argument fits into a signed 32-bit integer, thus, the
THROW statement in C++ is unreachable (unless the binding is accessed
directly, of course).
PR-URL: https://github.com/nodejs/node/pull/47407
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
SnapshotSerializerDeserializer::GetName() appears to confuse static
analysis such as Coverity.
This changes the function structure to a sequence of if-else blocks and
marks all branch conditions as constexpr. (Unfortunately, this results
in a dangling 'else' keyword in the V macro.)
As per a request in the PR discussion, this change does _not_ ensure
that GetName<T>() can only be called for known types T and instead still
returns an empty string in that case.
Also use std::is_unsigned_v instead of !std::is_signed_v.
PR-URL: https://github.com/nodejs/node/pull/46509
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
signtool still defaults to SHA1, which is vulnerable to certain
collisions. This switches to SHA256, which is stronger and which also
matches the hash function used by the signing certificate.
Technically, `/fd certHash` would be a better choice, but I don't know
if it is widely supported.
PR-URL: https://github.com/nodejs/node/pull/47206
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The JS layer already verifies that divisor_bits is either a non-negative
32-bit signed integer or null/undefined, in which case it passes -1 to
the C++ layer. In either case, the C++ layer receives a 32-bit signed
integer greater than or equal to -1.
PR-URL: https://github.com/nodejs/node/pull/47168
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Previously, the JS layer would validate that the value of the 'checks'
option was an unsigned 32-bit integer, otherwise throwing an appropriate
error but with a slightly misleading error message. Then the C++ layer
would validate that the value was an unsigned 31-bit integer, otherwise
throwing an appropriate error, but with a different (and even less
helpful) error message.
Instead, make the JS layer aware of the 31-bit restriction so that no
validation in C++ is necessary and so that the error message always
matches the exact requirement.
PR-URL: https://github.com/nodejs/node/pull/47165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
None of these constants have any effect in recent OpenSSL versions, not
even in Node.js release lines that still use OpenSSL 1.1.1.
It is likely rare that these options are still used (intentionally), and
removing them is unlikely to break any existing applications. These
constants can only be passed to the secureOptions option of
tls.createSecureContext() and related APIs, and a value of undefined
will be ignored. Similarly, if a bitwise combination of multiple options
is used, undefined constants will not change the behavior because
(a | undefined | b) === (a | b) for (small) integers a and b.
Refs: https://github.com/nodejs/node/pull/46954
Refs: https://github.com/nodejs/node/pull/47066
PR-URL: https://github.com/nodejs/node/pull/47073
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This test had two problems:
* The first argument was a number in both cases, which is what caused
the (expected) ERR_INVALID_ARG_TYPE error -- the validity of the
'checks' option was not actually verified at all. Thus, the first
argument should be valid for this particular test.
* The function returned by common.mustNotCall() was passed to
assert.throws() as a third argument instead of being passed to
checkPrime() as a third argument. (Isn't JavaScript great?) This again
led to the (expected) ERR_INVALID_ARG_TYPE error, but again, the
validity of the 'checks' option was not verified.
Fix both issues by ensuring that all arguments except the 'checks'
option are valid.
Refs: https://github.com/nodejs/node/pull/36997
PR-URL: https://github.com/nodejs/node/pull/47139
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This constant was likely introduced for feature detection, but it has
been pointless for a long time.
1. I am not aware of any possible Node.js build configuration (on any
recent/supported release line) that would have crypto.constants but
not crypto.constants.ALPN_ENABLED.
2. There is no evidence of this constant being used for feature
detection in the ecosystem. In fact, both internal and external type
definitions for crypto.constants simply assume that the constant
exists.
3. There is no good reason for any modern TLS stack to not support ALPN.
It looks like ALPN might have been optional in much earlier versions
of OpenSSL, but all recent versions of OpenSSL unconditionally
support ALPN as far as I can tell.
Refs: https://github.com/nodejs/node/pull/46956
PR-URL: https://github.com/nodejs/node/pull/47028
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Node.js has so far only supported user-defined DHE parameters and even
recommended generating custom parameters. This change lets users set the
dhparam option to 'auto' instead, in which case DHE parameters of
sufficient strength are selected automatically (from a small set of
well-known parameters). This has been recommended by OpenSSL for quite a
while, and it makes it much easier for Node.js TLS servers to properly
support DHE-based perfect forward secrecy.
This also updates the documentation to prioritize ECDHE over DHE, mostly
because the former tends to be more efficient and is enabled by default.
PR-URL: https://github.com/nodejs/node/pull/46978
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
TLSEXT_TYPE_application_layer_protocol_negotiation is always defined in
all supported OpenSSL versions. We also use this macro elsewhere without
guards, so if it did not exist, compilation would have already failed.
Besides that, it makes little sense to provide a TLS 1.3 implementation
that does not support ALPN.
PR-URL: https://github.com/nodejs/node/pull/46956
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The dhparam option is not required for perfect forward secrecy in
general. It is only required for non-ECDHE secrecy, but ECDHE-based
secrecy is generally preferred anyway.
PR-URL: https://github.com/nodejs/node/pull/46949
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
If this test fails, e.g., if the s_client output does not match the
expectation, the previous implementation would not produce any helpful
error messages. Rework the control flow to be more idiomatic. Avoid
callback chaining and stream operations.
Also, the TLS server 'close' event does not pass an error to the event
handler, so remove the respective assertion.
PR-URL: https://github.com/nodejs/node/pull/46751
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Even though most compiler should not actually emit FPU instructions, it
is unnecessary to use floating-point arithmetic for powers of 2.
Also change some signed counters to unsigned integers.
PR-URL: https://github.com/nodejs/node/pull/46326
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This function base64-decodes a given JavaScript string to obtain the
secret key, whose length must not exceed INT_MAX. However, because
JavaScript strings are limited to v8::String::kMaxLength chars and
because base64 decoding never yields more bytes than input chars, the
size of the decoded key must be strictly less than
v8::String::kMaxLength bytes. Therefore, it is sufficient to statically
assert that String::kMaxLength <= INT_MAX (which is always true because
String::kMaxLength itself is an int).
Aside from being unreachable, Coverity considers the current code
"suspicious" because it indicates that buffers larger than INT_MAX might
actually be allocated.
PR-URL: https://github.com/nodejs/node/pull/46209
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
It is not true that all TLS 1.3 cipher suites use ECDH.
TLS 1.3 allows DH over finite fields instead of elliptic curves.
Also, the key exchange mechanism is independent of the cipher suite
in TLS 1.3.
PR-URL: https://github.com/nodejs/node/pull/46114
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Tobias Nießen