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 version
OpenSSL 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.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
_tls_wrap.js:

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>()));

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:                                                            
      {                                                                            
        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
raised at this point:

(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
 }

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:

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
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.js
1..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