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

"ERR_SSL_NO_CIPHER_MATCH" error when specifying TLS cipher suites #49699

Closed
jloleysens opened this issue Sep 18, 2023 · 1 comment · Fixed by #49712
Closed

"ERR_SSL_NO_CIPHER_MATCH" error when specifying TLS cipher suites #49699

jloleysens opened this issue Sep 18, 2023 · 1 comment · Fixed by #49712
Labels
confirmed-bug Issues with confirmed bugs. https Issues or PRs related to the https subsystem. tls Issues and PRs related to the tls subsystem.

Comments

@jloleysens
Copy link

jloleysens commented Sep 18, 2023

Version

v18.17.1

Platform

Darwin 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul 5 22:22:05 PDT 2023; root:xnu-8796.141.3~6/RELEASE_ARM64_T6000 arm64

Subsystem

tls

What steps will reproduce the bug?

Run

require('node:https').createServer({ minVersion: 'TLSv1.3', ciphers: 'TLS_AES_256_GCM_SHA384:!TLS_CHACHA20_POLY1305_SHA256'})

Weirdly, setting ECDHE-RSA-AES128-GCM-SHA256 at the end like 'TLS_AES_256_GCM_SHA384:!TLS_CHACHA20_POLY1305_SHA256:ECDHE-RSA-AES128-GCM-SHA256' causes it work work even though 'ECDHE-RSA-AES128-GCM-SHA256' is from TLSv1.2.

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

Always

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

The call to https.createServer should succeed and use the cipher suites specified AND (bonus) disallow specifying cipher suites not supported by the current protocol.

What do you see instead?

Uncaught Error: error:0A0000B9:SSL routines::no cipher match
    at configSecureContext (node:internal/tls/secure-context:231:11)
    at Object.createSecureContext (node:_tls_common:117:3)
    at Server.setSecureContext (node:_tls_wrap:1362:27)
    at Server (node:_tls_wrap:1226:8)
    at new Server (node:https:74:3)
    at Object.createServer (node:https:112:10) {
  library: 'SSL routines',
  reason: 'no cipher match',
  code: 'ERR_SSL_NO_CIPHER_MATCH'
}

Additional information

> tls.DEFAULT_CIPHERS
'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA256:ECDHE-RSA-AES256-SHA384:DHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA256:HIGH:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!SRP:!CAMELLIA'
@tniessen tniessen added tls Issues and PRs related to the tls subsystem. https Issues or PRs related to the https subsystem. labels Sep 18, 2023
@bnoordhuis
Copy link
Member

This is the result of node trying to make the ciphers option work for both TLSv1.2 and 1.3 because openssl has different APIs for them (SSL_CTX_set_cipher_list vs. SSL_CTX_set_ciphersuites.)

Node parses the string but it doesn't understand bang syntax and treats it as a TLSv1.2 cipher. PR welcome. Untested, but I think the fix looks something like this:

diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js
index 36d33e6ac8..0fa3098ffa 100644
--- a/lib/internal/tls/secure-context.js
+++ b/lib/internal/tls/secure-context.js
@@ -99,21 +99,25 @@ function processCiphers(ciphers, name) {
   const cipherList =
     ArrayPrototypeJoin(
       ArrayPrototypeFilter(
         ciphers,
         (cipher) => {
-          return cipher.length > 0 &&
-            !StringPrototypeStartsWith(cipher, 'TLS_');
+          if (cipher.length === 0) return false;
+          if (StringPrototypeStartsWith(cipher, 'TLS_')) return false;
+          if (StringPrototypeStartsWith(cipher, '!TLS_')) return false;
+          return true;
         }), ':');
 
   const cipherSuites =
     ArrayPrototypeJoin(
       ArrayPrototypeFilter(
         ciphers,
         (cipher) => {
-          return cipher.length > 0 &&
-            StringPrototypeStartsWith(cipher, 'TLS_');
+          if (cipher.length === 0) return false;
+          if (StringPrototypeStartsWith(cipher, 'TLS_')) return true;
+          if (StringPrototypeStartsWith(cipher, '!TLS_')) return true;
+          return false;
         }), ':');
 
   // Specifying empty cipher suites for both TLS1.2 and TLS1.3 is invalid, its
   // not possible to handshake with no suites.
   if (cipherSuites === '' && cipherList === '')

@bnoordhuis bnoordhuis added the confirmed-bug Issues with confirmed bugs. label Sep 18, 2023
atlowChemi added a commit to atlowChemi/node that referenced this issue Sep 19, 2023
atlowChemi added a commit to atlowChemi/node that referenced this issue Sep 21, 2023
atlowChemi added a commit to atlowChemi/node that referenced this issue Sep 21, 2023
atlowChemi added a commit to atlowChemi/node that referenced this issue Sep 30, 2023
atlowChemi added a commit to atlowChemi/node that referenced this issue Sep 30, 2023
nodejs-github-bot pushed a commit that referenced this issue Oct 4, 2023
Fixes: #49699
PR-URL: #49712
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Fixes: nodejs#49699
PR-URL: nodejs#49712
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
targos pushed a commit that referenced this issue Nov 11, 2023
Fixes: #49699
PR-URL: #49712
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
Fixes: nodejs#49699
PR-URL: nodejs#49712
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. https Issues or PRs related to the https subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants