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

SECLEVEL set via ciphers option is applied too late in tls.createSecureContext #36655

Closed
Hornwitser opened this issue Dec 28, 2020 · 14 comments
Closed
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@Hornwitser
Copy link

  • Version: v14.9.0
  • Platform: Linux box 5.8.5-arch1-1 #1 SMP PREEMPT Thu, 27 Aug 2020 18:53:02 +0000 x86_64 GNU/Linux
  • Subsystem: tls

What steps will reproduce the bug?

Creating secure context with a certificate who's key bits are too small for the default openssl SECLEVEL causes the operation to throw a key "too small error" even if the SECLEVEL is overridden to an acceptable value via the ciphers option. For example the following code throws on my system (note requires node-forge to generate the certificate, install it via npm):

const util = require("util");
const tls = require("tls");
const forge = require("node-forge");

async function test() {
    // Generate a TLS certificate with too few bits
    let keypair = forge.pki.rsa.generateKeyPair(512);
    let cert = forge.pki.createCertificate();
    cert.publicKey = keypair.publicKey;
    cert.serialNumber = "01";
    cert.validity.notBefore = new Date();
    cert.validity.notAfter = new Date(cert.validity.notBefore);
    cert.validity.notAfter.setFullYear(cert.validity.notAfter.getFullYear() + 1);
    let attrs = [{ name: "commonName", value: "localhost" }];
    cert.setSubject(attrs);
    cert.setIssuer(attrs);
    cert.sign(keypair.privateKey);
    let certPem = forge.pki.certificateToPem(cert);
    let keyPem = forge.pki.privateKeyToPem(keypair.privateKey);

    let serverOpts = {
        key: keyPem,
        cert: certPem,
        ciphers: "DEFAULT@SECLEVEL=0",
    }

    let clientOpts = {
        ciphers: "DEFAULT@SECLEVEL=0",
        ca: certPem,
    };

    // Create TLS server, this is the operation that fails.
    let server = tls.createServer(serverOpts, socket => {
        socket.on("data", (chunk) => {
            console.log("server pong");
            socket.end("pong");
        });
    });
    server.unref();
    await util.promisify(server.listen.bind(server))();

    // Try connecting to the server
    let addr = `https://localhost:${server.address().port}/`;
    let socket = tls.connect(server.address().port, "localhost", clientOpts, () => {
        socket.on("data", (chunk) => { });
        console.log("client ping");
        socket.write("ping");
    });
}

test().catch(console.log);

With the following error:

Error: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small
    at Object.createSecureContext (_tls_common.js:129:17)
    at Server.setSecureContext (_tls_wrap.js:1323:27)
    at new Server (_tls_wrap.js:1181:8)
    at Object.createServer (_tls_wrap.js:1224:10)
    at test (bug.js:33:22)
    at Object.<anonymous> (bug.js:51:1)
    at Module._compile (internal/modules/cjs/loader.js:1075:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1096:10)
    at Module.load (internal/modules/cjs/loader.js:940:32)
    at Function.Module._load (internal/modules/cjs/loader.js:781:14) {
  library: 'SSL routines',
  function: 'SSL_CTX_use_certificate',
  reason: 'ee key too small',
  code: 'ERR_SSL_EE_KEY_TOO_SMALL'
}

(if this does not reproduce the issue it might be possible that the system default SECLEVEL is 0, in this case it's possible to reproduce the issue by adding the following to a .cnf file

openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
CipherString = DEFAULT:@SECLEVEL=1

And then pointing node to it via the --openssl-config option.)

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

Whenever the default/configured SECLEVEL for openssl is greater than the one requested via the ciphers and this level is more strict than the certificate used requires.

What is the expected behavior?

Setting SECLEVEL via the ciphers option as Documented in the Ciphers List Format should allow a weaker certificate to be used than the system default SECLEVEL allows.

What do you see instead?

Node.js tries to add the certificate to the secure context before the ciphers option is process, which causes the default SECLEVEL to be used when evaluating the certificate. I know this to be the case as I tested reordering the certificate being added to the security context by using the following monkey patch:

const origCreateSecureContext = tls.createSecureContext;
tls.createSecureContext = function(options) {
    if (!options.cert || !options.key) {
        return origCreateSecureContext(options);
    }

    let lessOptions = { ...options };
    delete lessOptions.key;
    delete lessOptions.cert;
    let ctx = origCreateSecureContext(lessOptions);
    ctx.context.setCert(options.cert);
    ctx.context.setKey(options.key, undefined);
    return ctx;
};

If this is added to the reproduction code above it no longer throws a key too small error.

Additional information

Being able to set the security level in order to use certificates weaken than the default security setting allows is useful for automated testing. A viable workaround is to set the level through an openssl config file like shown above, but it's a bit of a sledgehammer approach. It looks like there's a SSL_CTX_set_security_level function to directly set the level instead of doing through the ciphers option, which might be a better alternative to setting it via the ciphers option.

Note that the reverse situation also does not work correctly: if SECLEVEL is increased in the ciphers option the certificate is still accepted, but handshakes with the server fails with Error: Client network socket disconnected before secure TLS connection was established.

@targos targos added the tls Issues and PRs related to the tls subsystem. label Dec 28, 2020
@targos
Copy link
Member

targos commented Dec 28, 2020

@nodejs/crypto

@trentm
Copy link
Contributor

trentm commented Apr 27, 2021

FWIW, I'm hitting:

node:internal/tls:91
   context.setCert(cert);
            ^

Error: error:140AB18F:SSL routines:SSL_CTX_use_certificate:ee key too small
    at node:internal/tls:91:13
    ...

in tests using the newly released "node:16" docker image because the default SECLEVEL has increased since the "node:14" docker image:

% docker run --rm -ti node:14 grep -r SECLEVEL /etc/ssl
% docker run --rm -ti node:16 grep -r SECLEVEL /etc/ssl
/etc/ssl/openssl.cnf:CipherString = DEFAULT@SECLEVEL=2

IIUC, without this bug I would have been able to workaround with using ciphers: "DEFAULT@SECLEVEL=0" in my test code. I have other workarounds available for my testing. I mention this mainly because that SECLEVEL change in the "node:16" docker image might make this bug more prevalent for node reports.

@Hornwitser Thanks very much for this bug report.

@kumarrishav
Copy link
Contributor

Facing the same issue.

this workaround did the job

const origCreateSecureContext = tls.createSecureContext;
tls.createSecureContext = function(options) {
    if (!options.cert || !options.key) {
        return origCreateSecureContext(options);
    }

    let lessOptions = { ...options };
    delete lessOptions.key;
    delete lessOptions.cert;
    let ctx = origCreateSecureContext(lessOptions);
    ctx.context.setCert(options.cert);
    ctx.context.setKey(options.key, undefined);
    return ctx;
};

@kumarrishav
Copy link
Contributor

Do we have a fix for this? Else, it needs to do that workaround patch fix for tls.createSecureContext

@kumarrishav
Copy link
Contributor

kumarrishav commented Sep 7, 2023

with node.js 18, When node.js app is a server, it throws the following error (when a client makes a connection) when I try load the cert later

Error: write EPROTO 80601EE501000000:error:0A000410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../deps/openssl/openssl/ssl/record/rec_layer_s3.c

    at TLSWrap.loadSession [as onclienthello] (node:_tls_wrap:205:19)
    at TLSWrap.callbackTrampoline (node:internal/async_hooks:130:17) {
  library: 'SSL routines',
  reason: 'no shared cipher',
  code: 'ERR_SSL_NO_SHARED_CIPHER'
}
9/7/2023, 7:22:46 PM UNCAUGHTEXCEPTION Error: write EPROTO 80601EE501000000:error:0A000410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1600:SSL alert number 40

It seems like the server is unable to find the right cipher and unable to finish serverHello

I get the same error mentioned below in node 16 as well if I do the above (i.e load cert after creating securecontext with ciphers). why there is error if i load the cert later

i.e

        ...
        tlsOptions.ciphers = ciphers;
	const secureContext = Tls.createSecureContext(tlsOptions);
	secureContext.context.setCert(cert);
        options.secureContext = secureContext;
        .....
       delete options.ciphers
       delete options.cert
       Https.createServer(options)

Error

TLS 40052: server emit tlsClientError: Error: 8138940544:error:1417A0C1:SSL routines:tls_post_process_client_hello:no shared cipher:../deps/openssl/openssl/ssl/statem/statem_srvr.c:2313:

    at TLSWrap.loadSession [as onclienthello] (node:_tls_wrap:205:19)
    at TLSWrap.callbackTrampoline (node:internal/async_hooks:130:17) {
  library: 'SSL routines',
  function: 'tls_post_process_client_hello',
  reason: 'no shared cipher',
  code: 'ERR_SSL_NO_SHARED_CIPHER'
}
9/7/2023, 7:46:47 PM UNCAUGHTEXCEPTION Error: write EPROTO 8138940544:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1565:SSL alert number 40

@kumarrishav
Copy link
Contributor

any pointer here @Hornwitser ?

@Hornwitser
Copy link
Author

I don't know Node.js TLS internals, and they might change and break code relying on them with any update, so I wouldn't try making it work like that if I were you. The original post describes how to set up an OpenSSL config file with SECLEVEL set in it, pass that in with --openssl-config when starting node and it should work.

@kumarrishav
Copy link
Contributor

kumarrishav commented Sep 11, 2023

@Hornwitser I tried this node --openssl-config=openssl.cnf index.js but still seeing UNCAUGHTEXCEPTION Error: error:0A00018E:SSL routines::ca md too weak. could it be because ciphers are being overridden in node.js tls call in the application and hence these config are not being honored??

where openssl.cnf is

openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
openssl_conf = openssl_init
security_level = 0

tried this too

openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
CipherString = DEFAULT:@SECLEVEL=0

@richardlau
Copy link
Member

Node.js won't read openssl_conf by default since #43124 -- it will instead look for nodejs_conf. There's a command line option if you'd like Node.js to read openssl_conf.

@kumarrishav
Copy link
Contributor

Still the same result @richardlau

node --openssl-shared-config --openssl-config=openssl.cnf index.js

openssl_conf = openssl_init

[openssl_init]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
CipherString = DEFAULT:@SECLEVEL=0

Tried this too

node --openssl-config=openssl.cnf index.js

nodejs_conf = openssl_init

[openssl_init]
ssl_conf = ssl_sect

[ssl_sect]
system_default = system_default_sect

[system_default_sect]
CipherString = DEFAULT:@SECLEVEL=0

@kumarrishav
Copy link
Contributor

@Hornwitser @richardlau any idea why does this not work
#49549 (comment)

@richardlau
Copy link
Member

I do not.

@bnoordhuis
Copy link
Member

I suspect this is an ordering issue that can be fixed with the patch below. Feel free to take it and turn it into a pull request.

diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js
index 0fa3098ffa..40d995f989 100644
--- a/lib/internal/tls/secure-context.js
+++ b/lib/internal/tls/secure-context.js
@@ -148,6 +148,31 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
     ticketKeys,
   } = options;
 
+  // Set the cipher list and cipher suite before anything else because
+  // @SECLEVEL=<n> changes the security level and that affects subsequent
+  // operations.
+  if (ciphers !== undefined && ciphers !== null)
+    validateString(ciphers, `${name}.ciphers`);
+
+  // Work around an OpenSSL API quirk. cipherList is for TLSv1.2 and below,
+  // cipherSuites is for TLSv1.3 (and presumably any later versions). TLSv1.3
+  // cipher suites all have a standard name format beginning with TLS_, so split
+  // the ciphers and pass them to the appropriate API.
+  const {
+    cipherList,
+    cipherSuites,
+  } = processCiphers(ciphers, `${name}.ciphers`);
+
+  if (cipherSuites !== '')
+    context.setCipherSuites(cipherSuites);
+  context.setCiphers(cipherList);
+
+  if (cipherList === '' &&
+      context.getMinProto() < TLS1_3_VERSION &&
+      context.getMaxProto() > TLS1_2_VERSION) {
+    context.setMinProto(TLS1_3_VERSION);
+  }
+
   // Add CA before the cert to be able to load cert's issuer in C++ code.
   // NOTE(@jasnell): ca, cert, and key are permitted to be falsy, so do not
   // change the checks to !== undefined checks.
@@ -218,28 +243,6 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
     }
   }
 
-  if (ciphers !== undefined && ciphers !== null)
-    validateString(ciphers, `${name}.ciphers`);
-
-  // Work around an OpenSSL API quirk. cipherList is for TLSv1.2 and below,
-  // cipherSuites is for TLSv1.3 (and presumably any later versions). TLSv1.3
-  // cipher suites all have a standard name format beginning with TLS_, so split
-  // the ciphers and pass them to the appropriate API.
-  const {
-    cipherList,
-    cipherSuites,
-  } = processCiphers(ciphers, `${name}.ciphers`);
-
-  if (cipherSuites !== '')
-    context.setCipherSuites(cipherSuites);
-  context.setCiphers(cipherList);
-
-  if (cipherList === '' &&
-      context.getMinProto() < TLS1_3_VERSION &&
-      context.getMaxProto() > TLS1_2_VERSION) {
-    context.setMinProto(TLS1_3_VERSION);
-  }
-
   validateString(ecdhCurve, `${name}.ecdhCurve`);
   context.setECDHCurve(ecdhCurve);
 

kumarrishav added a commit to kumarrishav/node that referenced this issue Oct 13, 2023
kumarrishav added a commit to kumarrishav/node that referenced this issue Oct 14, 2023



Set the cipher list and cipher suite before anything else because @SECLEVEL=<n> changes the security level and that affects subsequent operations.

Fixes: nodejs#36655 nodejs#49549

Refs: https://github.com/orgs/nodejs/discussions/49634 https://github.com/orgs/nodejs/discussions/46545
@kumarrishav
Copy link
Contributor

@bnoordhuis Thanks for the guidance. Yes, it seems that was the issue. I made the suggested change and built a binary from the source. It seems working now.

raised the PR here: #50186

kumarrishav added a commit to kumarrishav/node that referenced this issue Oct 16, 2023
Set the cipher list and cipher suite before anything else because
@SECLEVEL=<n>changes the security level and that affects
subsequent operations.

Fixes: nodejs#36655
nodejs#49549

Refs: https://github.com/orgs/nodejs/discussions/49634
https://github.com/orgs/nodejs/discussions/46545
kumarrishav added a commit to kumarrishav/node that referenced this issue Oct 16, 2023
targos pushed a commit that referenced this issue Nov 23, 2023
Set the cipher list and cipher suite before anything else
because @SECLEVEL=<n> changes the security level and
that affects subsequent operations.

Fixes: #36655
Fixes: #49549
Refs: https://github.com/orgs/nodejs/discussions/49634
Refs: https://github.com/orgs/nodejs/discussions/46545
Refs: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_security_level.html
PR-URL: #50186
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
martenrichter pushed a commit to martenrichter/node that referenced this issue Nov 26, 2023
Set the cipher list and cipher suite before anything else
because @SECLEVEL=<n> changes the security level and
that affects subsequent operations.

Fixes: nodejs#36655
Fixes: nodejs#49549
Refs: https://github.com/orgs/nodejs/discussions/49634
Refs: https://github.com/orgs/nodejs/discussions/46545
Refs: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_security_level.html
PR-URL: nodejs#50186
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
UlisesGascon pushed a commit that referenced this issue Dec 11, 2023
Set the cipher list and cipher suite before anything else
because @SECLEVEL=<n> changes the security level and
that affects subsequent operations.

Fixes: #36655
Fixes: #49549
Refs: https://github.com/orgs/nodejs/discussions/49634
Refs: https://github.com/orgs/nodejs/discussions/46545
Refs: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_security_level.html
PR-URL: #50186
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
UlisesGascon pushed a commit that referenced this issue Dec 19, 2023
Set the cipher list and cipher suite before anything else
because @SECLEVEL=<n> changes the security level and
that affects subsequent operations.

Fixes: #36655
Fixes: #49549
Refs: https://github.com/orgs/nodejs/discussions/49634
Refs: https://github.com/orgs/nodejs/discussions/46545
Refs: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_security_level.html
PR-URL: #50186
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
richardlau pushed a commit that referenced this issue Mar 19, 2024
Set the cipher list and cipher suite before anything else
because @SECLEVEL=<n> changes the security level and
that affects subsequent operations.

Fixes: #36655
Fixes: #49549
Refs: https://github.com/orgs/nodejs/discussions/49634
Refs: https://github.com/orgs/nodejs/discussions/46545
Refs: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_security_level.html
PR-URL: #50186
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants