-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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.
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
Fix node crashing due to endless loop #37757 #37990
Conversation
@nodejs/crypto |
test/sequential/test-https-selfsigned-no-keycertsign-no-crash.js
Outdated
Show resolved
Hide resolved
test/sequential/test-https-selfsigned-no-keycertsign-no-crash.js
Outdated
Show resolved
Hide resolved
f0020fb
to
bd978e1
Compare
It looks like the test fails when FIPS is enabled?
|
The |
If i run the test without changes and with
|
I´ve modified the test so that it skips if OpenSSL is |
@nodejs/crypto @mhdawson (for FIPS) |
@danbev could you take a look at the FIPS issue ? |
I'll try to take a look at this next week 👍 |
I've taken a look at below are some details about this, but for those that don't want to read through it I think it would be enough to skip this test when linking against a FIPS compatible OpenSSL library: if (process.config.variables.openssl_is_fips)
common.skip('Skipping as test uses non-fips compliant EC curve'); We use this configuration property, instead of detailsThis error is specifically on UBI8 which uses a shared/dynamically linked $ openssl version
OpenSSL 1.1.1g FIPS 21 Apr 2020 And node would be configured using: ./configure --shared-openssl --openssl-is-fips --debug The test $ out/Debug/node test/parallel/test-https-selfsigned-no-keycertsign-no-crash.js
node:assert:162
throw err;
^
AssertionError [ERR_ASSERTION]: function should not have been called at /home/danielbevenius/work/nodejs/node/test/parallel/test-https-selfsigned-no-keycertsign-no-crash.js:57
called with arguments: Error: unable to verify the first certificate
at TLSSocket.onConnectSecure (node:_tls_wrap:1532:34)
at TLSSocket.emit (node:events:369:20)
at TLSSocket._finishInit (node:_tls_wrap:946:8)
at TLSWrap.ssl.onhandshakedone (node:_tls_wrap:720:12) {
code: 'UNABLE_TO_VERIFY_LEAF_SIGNATURE'
}
at ClientRequest.mustNotCall (/home/danielbevenius/work/nodejs/node/test/common/index.js:452:12)
at ClientRequest.emit (node:events:369:20)
at TLSSocket.socketErrorListener (node:_http_client:447:9)
at TLSSocket.emit (node:events:369:20)
at emitErrorNT (node:internal/streams/destroy:195:8)
at emitErrorCloseNT (node:internal/streams/destroy:160:3)
at processTicksAndRejections (node:internal/process/task_queues:83:21) {
generatedMessage: false,
code: 'ERR_ASSERTION',
actual: undefined,
expected: undefined,
operator: 'fail'
} If we take look at where the error originates from we can see that is in function onConnectSecure() {
const options = this[kConnectOptions];
// Check the size of DHE parameter above minimum requirement
// specified in options.
const ekeyinfo = this.getEphemeralKeyInfo(); $ lldb -- out/Debug/node test/parallel/test-https-selfsigned-no-keycertsign-no-crash.js
(lldb) br s -n TLSWrap::GetEphemeralKeyInfo
(lldb) br s -n TLSWrap::VerifyError
(lldb) r -> 1954 args.GetReturnValue().Set(GetEphemeralKey(env, w->ssl_)
1955 .FromMaybe(Local<Value>()));
ocal<Object> info = Object::New(env->isolate());
if (!SSL_get_server_tmp_key(ssl.get(), &raw_key))
return scope.Escape(info);
Local<Context> context = env->context();
crypto::EVPKeyPointer key(raw_key);
int kid = EVP_PKEY_id(key.get());
int bits = EVP_PKEY_bits(key.get());
switch (kid) {
...
case EVP_PKEY_EC:
case EVP_PKEY_X25519:
case EVP_PKEY_X448:
{
const char* curve_name;
if (kid == EVP_PKEY_EC) {
ECKeyPointer ec(EVP_PKEY_get1_EC_KEY(key.get()));
int nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec.get()));
curve_name = OBJ_nid2sn(nid);
} else {
curve_name = OBJ_nid2sn(kid);
}
if (!Set<String>(context,
info,
env->type_string(),
env->ecdh_string()) ||
!Set<String>(context,
info,
env->name_string(),
OneByteString(env->isolate(), curve_name)) ||
!Set<Integer>(context,
info,
env->size_string(),
Integer::New(env->isolate(), bits))) {
return MaybeLocal<Object>();
}
}
break;
}
return scope.Escape(info);
} (lldb) expr kid
(int) $2 = 1034
(lldb) expr curve_name
(const char *) $3 = 0x00007ffff7f03c74 "X25519" And we can verify that this matches the define in OpenSSL: #define SN_X25519 "X25519"
#define NID_X25519 1034 If we peek at the OpenSSL errors we find that there have not been any errors (lldb) expr (int) ERR_peek_error()
(int) $7 = 0 And we can inspect the escaped v8::Object: (lldb) jlh info
0x7a3ad498931: [JS_OBJECT_TYPE]
- map: 0x2fd59916cdf9 <Map(HOLEY_ELEMENTS)> [FastProperties]
- prototype: 0x0435f3ca0899 <Object map = 0x3dae346c1239>
- elements: 0x1060a21c1309 <FixedArray[0]> [HOLEY_ELEMENTS]
- properties: 0x1060a21c1309 <FixedArray[0]>
- All own properties (excluding elements): {
0x1060a21c3d71: [String] in ReadOnlySpace: #type: 0x064eda0aa101 <String[4]: #ECDH> (const data field 0), location: in-object
0x1060a21c4bb1: [String] in ReadOnlySpace: #name: 0x07a3ad498999 <String[6]: "X25519"> (const data field 1), location: in-object
0x1ab68d856751: [String] in ReadOnlySpace: #size: 253 (const data field 2), location: in-object
}
If we continue in the debugging session the next function to be called will unction onConnectSecure() {
const options = this[kConnectOptions];
// Check the size of DHE parameter above minimum requirement
// specified in options.
const ekeyinfo = this.getEphemeralKeyInfo();
if (ekeyinfo.type === 'DH' && ekeyinfo.size < options.minDHSize) {
const err = new ERR_TLS_DH_PARAM_SIZE(ekeyinfo.size);
debug('client emit:', err);
this.emit('error', err);
this.destroy();
return;
}
let verifyError = this._handle.verifyError(); This will be trapped by our break point. void TLSWrap::VerifyError(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
TLSWrap* w;
ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
// XXX(bnoordhuis) The UNABLE_TO_GET_ISSUER_CERT error when there is no
// peer certificate is questionable but it's compatible with what was
// here before.
long x509_verify_error = // NOLINT(runtime/int)
VerifyPeerCertificate(
w->ssl_,
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT);
if (x509_verify_error == X509_V_OK)
return args.GetReturnValue().SetNull();
const char* reason = X509_verify_cert_error_string(x509_verify_error);
const char* code = X509ErrorCode(x509_verify_error);
Local<Object> exception =
Exception::Error(OneByteString(env->isolate(), reason))
->ToObject(env->isolate()->GetCurrentContext())
.FromMaybe(Local<Object>());
if (Set(env, exception, env->code_string(), code))
args.GetReturnValue().Set(exception);
} When in FIPS 140-2 compliance mode, only the following ciphersuites may be
It seems to be the case that the if (process.config.variables.openssl_is_fips)
common.skip('Skipping as test uses non-fips compliant EC curve'); We use this configuration property to detect if the OpenSSL library is FIPS $ out/Debug/node test/parallel/test-https-selfsigned-no-keycertsign-no-crash.js
1..0 # Skipped: Skipping as test uses non-fips compliant EC curve |
I´ve added the check for FIPS to the test. Now
Seems unrelated. |
Landed in fa6d084 |
To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. Extend the certificate validity from 1 year to 10 years. Show a text representation of the issued certificate upon completion such that the user can verify the validity. Refs: nodejs#42342 Refs: nodejs#37990
To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. Remove an unnecessary conversion step using openssl rsa. Extend the certificate validity from 1 year to 10 years. Show a text representation of the issued certificate upon completion such that the user can verify the validity. Refs: nodejs#42342 Refs: nodejs#37990
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: nodejs#42342 Refs: nodejs#37990
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: nodejs#42342 Refs: nodejs#37990
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: #42342 Refs: #37990 PR-URL: #42343 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: #42342 Refs: #37990 PR-URL: #42343 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: nodejs#42342 Refs: nodejs#37990 PR-URL: nodejs#42343 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: #42342 Refs: #37990 PR-URL: #42343 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: #42342 Refs: #37990 PR-URL: #42343 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: #42342 Refs: #37990 PR-URL: #42343 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
- To avoid unnecessarily large diffs, only generate a new private key if necessary. Otherwise, reuse the existing private key and only issue a new certificate. - Remove an unnecessary conversion step using openssl rsa and the intermediate rsa.pem and csr.pem files. - Extend the certificate validity from 1 year to 10 years. - Show a text representation of the issued certificate upon completion such that the user can verify the validity. - Make the script executable. - Use "#!/usr/bin/env bash" instead of "#!/bin/bash". - Allow the script to be called from any directory. Refs: nodejs#42342 Refs: nodejs#37990 PR-URL: nodejs#42343 Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: nodejs#37990
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: nodejs#37990
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: #37990 PR-URL: #43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: #37990 PR-URL: #43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: #37990 PR-URL: #43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: #37990 PR-URL: #43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: #37990 PR-URL: #43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: #37990 PR-URL: #43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Coverity issues a warning about `value_before_reset == ca`, where value_before_reset is a pointer that may not be valid. While comparing the pointer itself should still work, we should really be using smart pointers here so that this particular check can be simplified without running into a memory leak. Refactor SSL_CTX_get_issuer to return a smart pointer and update the call sites accordingly. Note that we might have to change that in the future once we improve error handling throughout crypto/tls. Refs: nodejs/node#37990 PR-URL: nodejs/node#43117 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Node ran into an endless loop (and crashed when it ran out of memory) when using a self signed certificate without
keyCertSign
bit. This fixes the crash by adding a way to detect the endless loop and leaving it in that case.Fixes: #37757
Refs: #37889