Skip to content
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

Closed
wants to merge 14 commits into from
Closed

Conversation

nils91
Copy link
Contributor

@nils91 nils91 commented Mar 30, 2021

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-github-bot 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
@targos
Copy link
Member

targos commented Mar 30, 2021

@nodejs/crypto

@nils91 nils91 force-pushed the master branch 3 times, most recently from f0020fb to bd978e1 Compare March 31, 2021 17:12
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Apr 3, 2021

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
Copy link
Contributor Author

nils91 commented Apr 3, 2021

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

@nils91
Copy link
Contributor Author

nils91 commented Apr 3, 2021

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
Copy link
Contributor Author

nils91 commented Apr 3, 2021

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
Copy link
Member

Trott commented Apr 5, 2021

@nodejs/crypto @mhdawson (for FIPS)

@mhdawson
Copy link
Member

mhdawson commented Apr 8, 2021

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

@danbev
Copy link
Contributor

danbev commented Apr 11, 2021

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

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

@danbev
Copy link
Contributor

danbev commented Apr 13, 2021

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
Copy link
Contributor Author

nils91 commented Apr 16, 2021

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
Copy link
Member

jasnell commented Apr 27, 2021

Landed in fa6d084

@jasnell jasnell closed this Apr 27, 2021
jasnell pushed a commit that referenced this pull request Apr 27, 2021
Refs: #37757
Refs: #37889

PR-URL: #37990
Fixes: #37757
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Apr 29, 2021
Refs: #37757
Refs: #37889

PR-URL: #37990
Fixes: #37757
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 3, 2021
tniessen added a commit to tniessen/node that referenced this pull request Mar 15, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request Mar 15, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request Mar 15, 2022
- 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
tniessen added a commit to tniessen/node that referenced this pull request Mar 15, 2022
- 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
nodejs-github-bot pushed a commit that referenced this pull request Mar 17, 2022
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
bengl pushed a commit that referenced this pull request Mar 21, 2022
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Apr 21, 2022
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 24, 2022
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
- 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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request May 16, 2022
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
tniessen added a commit to tniessen/node that referenced this pull request May 16, 2022
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
nodejs-github-bot pushed a commit that referenced this pull request May 18, 2022
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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit that referenced this pull request May 30, 2022
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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
juanarbol pushed a commit that referenced this pull request May 31, 2022
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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
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 <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodejs runs out of memory trying to connect to HTTPS host with self-signed certificate
8 participants