Fix node crashing due to endless loop #37757 by nils91 · Pull Request #37990 · nodejs/node · GitHub

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

@nodejs/crypto

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

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

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

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

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

@nodejs/crypto @mhdawson (for FIPS)

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

@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 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

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.

Refs: nodejs#37757 Refs: nodejs#37889

Fixes: nodejs#37757 Refs: nodejs#37889

This reverts commit bd978e1.

Landed in fa6d084

Refs: #37757 Refs: #37889 PR-URL: #37990 Fixes: #37757 Reviewed-By: James M Snell <jasnell@gmail.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

- 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: 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: #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>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 30, 2021

jasnell approved these changes Mar 30, 2021

View reviewed changes

addaleax reviewed Mar 30, 2021

View reviewed changes

Comment thread src/crypto/crypto_common.cc Outdated

Comment thread test/sequential/test-https-selfsigned-no-keycertsign-no-crash.js Outdated

Comment thread test/sequential/test-https-selfsigned-no-keycertsign-no-crash.js Outdated

nils91 force-pushed the master branch 3 times, most recently from f0020fb to bd978e1 Compare March 31, 2021 17:12

nils91 force-pushed the master branch from fd97bc5 to 1e0cc8e Compare April 4, 2021 11:25

nils91 force-pushed the master branch from 1e0cc8e to 02ae1b5 Compare April 14, 2021 13:41

nils91 added 9 commits April 23, 2021 11:16

test: ensure node doesnt crash with some selfsigned certificates

0a3aa58

Refs: nodejs#37757 Refs: nodejs#37889

test: add empty line for readability

00aca61

test: let server choose a port to use

d4dd0b4

test: move https selfsigned to parallel

832811d

test: wrap callbacks in common.must[Not]Call

5dca179

crypto: exit potentially infinite loop in GetLastIssuedCert if stuck

e3bd5f5

Fixes: nodejs#37757 Refs: nodejs#37889

crypto: more compact solution to detect endless loop

0734ad0

Revert "crypto: more compact solution to detect endless loop"

374335d

This reverts commit bd978e1.

crypto: put the condition at the very end of the loop

fe0fcbb

jasnell closed this Apr 27, 2021

jasnell pushed a commit that referenced this pull request Apr 27, 2021

crypto: don't crash with some selfsigned certs

fa6d084

Refs: #37757 Refs: #37889 PR-URL: #37990 Fixes: #37757 Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request Apr 29, 2021

crypto: don't crash with some selfsigned certs

195f679

Refs: #37757 Refs: #37889 PR-URL: #37990 Fixes: #37757 Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request May 3, 2021

v16.1.0 release proposal #38522

Merged

targos added dont-land-on-v14.x and removed dont-land-on-v14.x labels May 30, 2021

tniessen mentioned this pull request Mar 15, 2022

test: improve https_renew_cert.sh script #42343

Merged

tniessen mentioned this pull request May 16, 2022

src: fix static analysis warning and use smart ptr #43117

Merged

Fix node crashing due to endless loop #37757 by nils91 · Pull Request #37990 · nodejs/node · GitHub

Uh oh!

Uh oh!

Fix node crashing due to endless loop #37757 - #37990

Fix node crashing due to endless loop #37757#37990nils91 wants to merge 14 commits intonodejs:masternodejs/node:masterfrom nils91:masternils91/node:masterCopy head branch name to clipboard

Conversation

nils91 commented Mar 30, 2021

Uh oh!

targos commented Mar 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 1, 2021

Uh oh!

Trott commented Apr 3, 2021

Uh oh!

nils91 commented Apr 3, 2021

Uh oh!

nils91 commented Apr 3, 2021

Uh oh!

nils91 commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Apr 5, 2021

Uh oh!

mhdawson commented Apr 8, 2021

Uh oh!

danbev commented Apr 11, 2021

Uh oh!

danbev commented Apr 13, 2021

Uh oh!

nils91 commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Fix node crashing due to endless loop #37757#37990
nils91 wants to merge 14 commits into
nodejs:masterfrom
nils91:master

nils91 commented Apr 3, 2021 •
edited
Loading

nils91 commented Apr 16, 2021 •
edited
Loading