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

NODE_EXTRA_CA_CERTS option seems to conflict with custom OpenSSL config #48143

Closed
ckcr4lyf opened this issue May 23, 2023 · 4 comments
Closed

Comments

@ckcr4lyf
Copy link
Contributor

ckcr4lyf commented May 23, 2023

Version

v18.16.0, v20.2.0

Platform

Linux 15cd296152a7 6.3.2-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 11 May 2023 16:40:42 +0000 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Adding custom options to OpenSSL config for node, it works as expected. (In my case, adding option to allow UnsafeLegacyRenegotiation (https://github.com/ckcr4lyf/no-rfc5746/blob/23c9abf620b37754a8eb4c206e6c8d37646a7c91/openssl.cnf)

When connecting to a TLS server that doesn't support the renegotiation extension, it DOES NOT throw an error, which implies that it is correctly reading the custom config. (If the config is missing it will fail)

However, if the NODE_EXTRA_CA_CERTS var is set, then it WILL throw an error.

Since it is non-trivial to create a TLS server which does not support renegotiation, I have made a custom TCP socket which replies with a TLS ServerHello with no extensions, causing clients to fail: https://github.com/ckcr4lyf/no-rfc5746/actions/runs/5059898372/jobs/9082065602#step:5:30

I've also packaged the node PoC of this bug w/ node 18 & node 20 into a docker container, so anyone who wants to can easily test it, or open a shell in it and poke around. You an check a Github run of it, to see the difference in when the option is not set vs. when it is: https://github.com/ckcr4lyf/no-rfc5746/actions/runs/5060085507/jobs/9082524060

Example of running the docker container to PoC the normal case:

$ docker run --rm ghcr.io/ckcr4lyf/node-sslconf-poc-node18:master@sha256:66be176fe027b5b98ca4636b2a6b0ad084c603ca51344725f0a47c046bd7e856
started on :4433
Success

And failure case when NODE_EXTRA_CA_CERTS is set:

$ docker run --rm -e NODE_EXTRA_CA_CERTS="/etc/ssl/certs/ca-certificates.crt" ghcr.io/ckcr4lyf/node-sslconf-poc-node18:master@sha256:66be176fe027b5b98ca4636b2a6b0ad084c603ca51344725f0a47c046bd7e856
started on :4433
[Error: C0E7F4C1AE7F0000:error:0A000152:SSL routines:final_renegotiate:unsafe legacy renegotiation disabled:../deps/openssl/openssl/ssl/statem/extensions.c:922:
] {
  library: 'SSL routines',
  reason: 'unsafe legacy renegotiation disabled',
  code: 'ERR_SSL_UNSAFE_LEGACY_RENEGOTIATION_DISABLED'
}

You can use -e STRACE=true to view strace for openat,read calls.

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

Seems to be 100% of the time, if NODE_EXTRA_CA_CERTS is set.

What is the expected behavior? Why is that the expected behavior?

It should still respect the custom SSL config, since there is no obvious conflict documented between NODE_EXTRA_CA_CERTS and the openssl config, and strace implies it is still read

openat(AT_FDCWD, "/etc/ssl/openssl.cnf", O_RDONLY) = 3
read(3, "nodejs_conf = openssl_init\n\n[ope"..., 4096) = 171
read(3, "", 4096)                       = 0
openat(AT_FDCWD, "/etc/ssl/certs/ca-certificates.crt", O_RDONLY) = 3
read(3, "-----BEGIN CERTIFICATE-----\nMIIH"..., 4096) = 4096
read(3, "8B1\nRXxlDPiyN8+sD8+Nb/kZ94/sHvJw"..., 4096) = 4096
[...truncated]

What do you see instead?

It did not respect the custom SSL config, and failed due to unsafe option (https://github.com/ckcr4lyf/no-rfc5746/actions/runs/5059898380/jobs/9082077770#step:4:8)

[Error: C097DA8F8B7F0000:error:0A000152:SSL routines:final_renegotiate:unsafe legacy renegotiation disabled:../deps/openssl/openssl/ssl/statem/extensions.c:922:
] {
  library: 'SSL routines',
  reason: 'unsafe legacy renegotiation disabled',
  code: 'ERR_SSL_UNSAFE_LEGACY_RENEGOTIATION_DISABLED'
}

Additional information

I tried poking around node source, but I am no expert in C/C++, config and build systems, so I am not sure. Hopefully my conditions to reproduce can help someone else test it.

@ckcr4lyf
Copy link
Contributor Author

I'm hosting a website which supports the old TLS renegotiation to make it easier if you guys want to test: https://rfc5746.mywaifu.best:4433/

Example with curl:

$ curl -v https://rfc5746.mywaifu.best:4433
*   Trying 152.67.8.255:4433...
* Connected to rfc5746.mywaifu.best (152.67.8.255) port 4433 (#0)
* ALPN: offers h2,http/1.1
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
*  CAfile: /etc/ssl/certs/ca-certificates.crt
*  CApath: none
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (OUT), TLS alert, handshake failure (552):
* OpenSSL/3.0.8: error:0A000152:SSL routines::unsafe legacy renegotiation disabled
* Closing connection 0
curl: (35) OpenSSL/3.0.8: error:0A000152:SSL routines::unsafe legacy renegotiation disabled

@bnoordhuis
Copy link
Member

I suspect that's due to init order. At the moment NODE_EXTRA_CA_CERTS is processed before OPENSSL_CONF but maybe it shouldn't be. Does it work when you apply this patch?

diff --git a/src/node.cc b/src/node.cc
index 1806693d4bd..0d63be9a268 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -961,11 +961,6 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& args,
       return ret;
     };

-    {
-      std::string extra_ca_certs;
-      if (credentials::SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))
-        crypto::UseExtraCaCerts(extra_ca_certs);
-    }
     // In the case of FIPS builds we should make sure
     // the random source is properly initialized first.
 #if OPENSSL_VERSION_MAJOR >= 3
@@ -1052,6 +1047,10 @@ InitializeOncePerProcessInternal(const std::vector<std::string>& args,
       CHECK(crypto::CSPRNG(buffer, length).is_ok());
       return true;
     });
+
+    std::string extra_ca_certs;
+    if (credentials::SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))
+      crypto::UseExtraCaCerts(extra_ca_certs);
 #endif  // HAVE_OPENSSL && !defined(OPENSSL_IS_BORINGSSL)
   }

@ckcr4lyf
Copy link
Contributor Author

Does it work when you apply this patch?

Thanks for the suggestion. When applying the patch to latest master, it works, even if I supply NODE_EXTRA_CA_CERTS , so this is definitely the cause of the issue.

@ckcr4lyf
Copy link
Contributor Author

The fix (#48159) has been merged and released in v20.3.0 , I can confirm it works now (https://github.com/ckcr4lyf/no-rfc5746/actions/runs/5228103413).

Closing this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@bnoordhuis @ckcr4lyf and others