Fix node crashing due to endless loop #37757

[nils91]

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

[targos]

@nodejs/crypto

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/37086/

[Trott]

It looks like the test fails when FIPS is enabled?

https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubi81_sharedlibs_openssl111fips_x64/26072/testReport/junit/(root)/test/parallel_test_https_selfsigned_no_keycertsign_no_crash/

08:06:48 not ok 1587 parallel/test-https-selfsigned-no-keycertsign-no-crash 08:06:48 --- 08:06:48 duration_ms: 0.144 08:06:48 severity: fail 08:06:48 exitcode: 1 08:06:48 stack: |- 08:06:48 node:assert:162 08:06:48 throw err; 08:06:48 ^ 08:06:48 08:06:48 AssertionError [ERR_ASSERTION]: (e) =>{08:06:48 httpsServer.close(); 08:06:48 } at /home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-https-selfsigned-no-keycertsign-no-crash.js:88 08:06:48 called with arguments: Error: unable to verify the first certificate 08:06:48 at TLSSocket.onConnectSecure (node:_tls_wrap:1531:34) 08:06:48 at TLSSocket.emit (node:events:369:20) 08:06:48 at TLSSocket._finishInit (node:_tls_wrap:945:8) 08:06:48 at TLSWrap.ssl.onhandshakedone (node:_tls_wrap:719:12){08:06:48 code: 'UNABLE_TO_VERIFY_LEAF_SIGNATURE' 08:06:48 } 08:06:48 at ClientRequest.mustNotCall (/home/iojs/build/workspace/node-test-commit-linux-containered/test/common/index.js:448:12) 08:06:48 at ClientRequest.emit (node:events:369:20) 08:06:48 at TLSSocket.socketErrorListener (node:_http_client:444:9) 08:06:48 at TLSSocket.emit (node:events:369:20) 08:06:48 at emitErrorNT (node:internal/streams/destroy:195:8) 08:06:48 at emitErrorCloseNT (node:internal/streams/destroy:160:3) 08:06:48 at processTicksAndRejections (node:internal/process/task_queues:81:21){08:06:48 generatedMessage: false, 08:06:48 code: 'ERR_ASSERTION', 08:06:48 actual: undefined, 08:06:48 expected: undefined, 08:06:48 operator: 'fail' 08:06:48 } 08:06:48 ... 

[nils91]

The common module has the method hasFipsCrypto. Would skipping the test be an option?

[nils91]

If i run the test without changes and with node --enable-fips this is what i´m getting:

node:internal/bootstrap/loaders:162 mod = bindingObj[module] = getInternalBinding(module); ^ Error: error:0F06D065:common libcrypto routines:FIPS_mode_set:fips mode not supported at internalBinding (node:internal/bootstrap/loaders:162:34) at node:tls:58:48 at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:307:7) at nativeModuleRequire (node:internal/bootstrap/loaders:336:14) at node:https:39:13 at NativeModule.compileForInternalLoader (node:internal/bootstrap/loaders:307:7) at NativeModule.compileForPublicLoader (node:internal/bootstrap/loaders:247:10) at loadNativeModule (node:internal/modules/cjs/helpers:40:9) at Function.Module._load (node:internal/modules/cjs/loader:799:15) at Module.require (node:internal/modules/cjs/loader:1012:19){library: 'common libcrypto routines', function: 'FIPS_mode_set', reason: 'fips mode not supported', code: 'ERR_OSSL_CRYPTO_FIPS_MODE_NOT_SUPPORTED' } 

[nils91]

I´ve modified the test so that it skips if OpenSSL is <= 1.1.1h < 1.1.1h. The failing jenkins ci run builds with --shared-openssl, which seems to use the system´s openssl. Not sure if theres an old openssl version installed, so that might not work. I have been able to reproduce the error message (and verify that the test works as intended) when building with --shared-openssl and an older openssl version though...

[Trott]

@nodejs/crypto @mhdawson (for FIPS)

[mhdawson]

@danbev could you take a look at the FIPS issue ?

[danbev]

@danbev could you take a look at the FIPS issue ?

I'll try to take a look at this next week 👍

[danbev]

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 common.hasFips(), to detect if the OpenSSL library is FIPS
compatible, regardless if it has been enabled or not.

details

This error is specifically on UBI8 which uses a shared/dynamically linked
OpenSSL 1.1.1 version with FIPS enabled. This version is provided/maintained
by Red Hat and contains patches onto of OpenSSL to get FIPS support.

$ openssl versionOpenSSL 1.1.1g FIPS 21 Apr 2020

And node would be configured using:

./configure --shared-openssl --openssl-is-fips --debug

The test test-https-selfsigned-no-keycertsign-no-crash.js fails with the
following error:

$ out/Debug/node test/parallel/test-https-selfsigned-no-keycertsign-no-crash.jsnode: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:57called 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
_tls_wrap.js:

functiononConnectSecure(){constoptions=this[kConnectOptions];// Check the size of DHE parameter above minimum requirement // specified in options. constekeyinfo=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>()));

GetEphemeralKey can be found in crypto_common.cc.

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:{constchar* 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:

#defineSN_X25519 "X25519" #defineNID_X25519 1034 

If we peek at the OpenSSL errors we find that there have not been any errors
raised at this point:

(lldb) expr (int) ERR_peek_error()(int) $7 = 0

And we can inspect the escaped v8::Object:

(lldb) jlh info0x7a3ad498931: [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 }

X25519 is an elliptic curve that has 128 bits of security and uses a 256-bit
size key size.

If we continue in the debugging session the next function to be called will
be verifyError:

unctiononConnectSecure(){constoptions=this[kConnectOptions];// Check the size of DHE parameter above minimum requirement // specified in options. constekeyinfo=this.getEphemeralKeyInfo();if(ekeyinfo.type==='DH'&&ekeyinfo.size<options.minDHSize){consterr=newERR_TLS_DH_PARAM_SIZE(ekeyinfo.size);debug('client emit:',err);this.emit('error',err);this.destroy();return;}letverifyError=this._handle.verifyError();

This will be trapped by our break point.

voidTLSWrap::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(); constchar* reason = X509_verify_cert_error_string(x509_verify_error); constchar* 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
used for TLS communications:

ECDHE-RSA-AES256-SHA384 DHE-RSA-AES256-SHA256 DH-RSA-AES256-SHA256 ECDH-RSA-AES256-SHA384 AES256-SHA256 AES256-SHA 

It seems to be the case that the X25519 curve is not allowed when FIPS is
enable which is the case here. Perhaps we could just skip this test if fips is
enabled.

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
compatible, regardless if it has been enabled or not.

$ out/Debug/node test/parallel/test-https-selfsigned-no-keycertsign-no-crash.js1..0 # Skipped: Skipping as test uses non-fips compliant EC curve

[nils91]

I´ve added the check for FIPS to the test. Now test-tarball-linux on github fails with

=== release test === Path: js-native-api/test_object/test Command: out/Release/node /home/runner/work/node/node/node-v16.0.0-nightly2021-04-1435da878895/test/js-native-api/test_object/test.js --- CRASHED (Signal: 11) --- 

Seems unrelated.

[jasnell]

Landed in fa6d084