Unexpected openssl error with Node 12.18.0 custom build

[liweixi100]

• Version: 12.18.0 (built with "--openssl-system-ca-path")
• Platform: Redhat Linux 7.6
• Subsystem: Linux 3.10.0-957.35.2.el7.x86_64 deps: update openssl to 1.0.1j #1 SMP Wed Sep 18 05:51:28 EDT 2019 x86_64 x86_64 x86_64 GNU/Linux

What steps will reproduce the bug?

Our app would crash at runtime if the --openssl-system-ca-path referenced path did not exist.

How often does it reproduce? Is there a required condition?

The crash was observed every time with our app. However, we could not reproduce the crash with a simple script. Our app had a much more complicated dependencies.

What is the expected behavior?

Before 12.18.0, even if the --openssl-system-ca-path referenced path did not exist, the app would run just fine.

What do you see instead?

Our app crashed every time with following traces:

`Error: error:02001002:system library:fopen:No such file or directory
    at Sign.sign (internal/crypto/sig.js:105:29)
    at SAML.signRequest (/mybackend/node_modules/passport-saml/lib/passport-saml/saml.js:137:38)
    at DeflateRaw.requestToUrlHelper [as cb] (/mybackend/node_modules/passport-saml/lib/passport-saml/saml.js:313:12)
    at DeflateRaw.zlibBufferOnEnd (zlib.js:149:10)
    at DeflateRaw.emit (events.js:327:22)
    at endReadableNT (_stream_readable.js:1221:12)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  opensslErrorStack: [
    'error:0B084002:x509 certificate routines:X509_load_cert_crl_file:system lib',
    'error:2006D080:BIO routines:BIO_new_file:no such file'
  ],
  library: 'system library',
  function: 'fopen',
  reason: 'No such file or directory',
  code: 'ERR_OSSL_SYS_NO_SUCH_FILE_OR_DIRECTORY'
}

Additional information

We have been making a custom build of node using the --openssl-system-ca-path configure option. Recently we noticed the crash when we upgraded from 12.16.3 to 12.18.0.

It turned out the crash was related to openssl-system-ca-path build config --- if the environment did not contain a valid path for the cert, our app would crash. If the cert path existed, it would not crash.

However, that was not the behavior we saw prior to the 12.18.0 version.

We also found a workaround: if the process was started with NODE_EXTRA_CA_CERTS=/path/to/cert.pem, then the crash would go away. This was true even if the path did not exist at all; as long as the NODE_EXTRA_CA_CERTS env was set to anything, it would not crash.

This was the PR for the openssl-system-ca-path feature that was created by @danbev

We'd like to clarify:

1. Does the --openssl-system-ca-path build option compile in the content of the certificate, or does it only compile in the path of it? (I think it was the latter, but wanted to be sure.)

2. What is the expected runtime behavior of custom build made with openssl-system-ca-path: if the cert does not exist, shouldn't it fallback to the built-in CA?

3. Could there be any recent changes that made the ca-path-not-exit error become a fatal exception?

[danbev]

@liweixi100 Sorry about this, I'll take a closer look at this on Monday.

[liweixi100]

Thank you Daniel. Really appreciate your insight.

[danbev]

We'd like to clarify:

1. Does the --openssl-system-ca-path build option compile in the content of the certificate, or does it only compile in the path of it? (I think it was the latter, but wanted to be sure.)

You are correct, it is only the path and it is then used here.

Could there be any recent changes that made the ca-path-not-exit error become a fatal exception?

One thing I noticed is that if the file specified using --openssl-system-ca-path is not found there will be an error produced by OpenSSL:

$ lldb -- ./node/out/Debug/node -p 'tls.rootCertificates'
(lldb) br s -f node_crypto.cc -l 978
(lldb) r
(lldb) expr ERR_reason_error_string(ERR_peek_error())
(const char *) $1 = 0x0000000003dd0fd8 "No such file or directory"

Perhaps what is happening is that the application is at some point calling a function that calls NewRootCertDir, causing this error to be on the OpenSSL error stack. When sign is then called it will check if there was an error which there now will be.

When specifying NODE_EXTRA_CA_CERTS that function uses ClearErrorOnReturn which will clear this error and could explain why you are not seeing this when specifying that flag.

I've tried to reproduce this using this example, and the output would be the following when run:

$ ./node/out/Debug/node lib/sign.js
internal/crypto/sig.js:105
  const ret = this[kHandle].sign(data, format, type, passphrase, rsaPadding,
                            ^

Error: error:02001002:system library:fopen:No such file or directory
    at Sign.sign (internal/crypto/sig.js:105:29)
    at Object.<anonymous> (/home/danielbevenius/work/nodejs/learning-nodejs/lib/sign.js:17:26)
    at Module._compile (internal/modules/cjs/loader.js:1138:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
    at Module.load (internal/modules/cjs/loader.js:986:32)
    at Function.Module._load (internal/modules/cjs/loader.js:879:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  opensslErrorStack: [
    'error:0B084002:x509 certificate routines:X509_load_cert_crl_file:system lib',
    'error:2006D080:BIO routines:BIO_new_file:no such file'
  ],
  library: 'system library',
  function: 'fopen',
  reason: 'No such file or directory',
  code: 'ERR_OSSL_SYS_NO_SUCH_FILE_OR_DIRECTORY'
}

What I should have done is clear the error in some way, perhaps by setting an error mark before calling X509_STORE_load_locations and then clearing it upon return:

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index f08d68d6b6..f6703a0ee5 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -971,7 +971,9 @@ static X509_STORE* NewRootCertStore() {
 
   X509_STORE* store = X509_STORE_new();
   if (*system_cert_path != '\0') {
+    ERR_set_mark();
     X509_STORE_load_locations(store, system_cert_path, nullptr);
+    ERR_pop_to_mark();
   }
 
   Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);

I think might be what is causing the error you are seeing. As to what has changed I'm not exactly sure but this is a mistake on my part and needs to be fixed. Thanks for creating this issue and making us aware of it! I can open a PR for this but please let me know if you would like to do that?

[liweixi100]

Hi Daniel. Thank you so much for your detailed analysis. Learned a lot from your code example and the patch. You patch looks great, feel free to submit it. I'd like to help but would have to jump through hoops in order to contribute to open source projects. I will help with the testing for sure. Thank you for your help! Best regards.

[liweixi100]

Thank you Daniel!