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

crypto: don't disable TLS 1.3 without suites #43427

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

AdamMajer
Copy link
Contributor

In the manual page, there is a stement that ciphersuites contain
explicit default settings - all TLS 1.3 ciphersuites are available.
In node, we assume that an empty setting mean no ciphersuites and
we disable TLS 1.3. A correct approach to disabling TLS 1.3 is to
disable TLS 1.3 and by not override the default ciphersuits
with an empty string.

So, only override OpenSSL's TLS 1.3 ciphersuites with an explicit
list of ciphers. If none are acceptible, the correct approach is
to disable TLS 1.3 instead elsewhere.

Fixes: #43419

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Jun 14, 2022
@AdamMajer
Copy link
Contributor Author

A few tests will require fixes.... but it would be nice to have feedback especially when these ciphers are set externally to node itself.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AdamMajer
Copy link
Contributor Author

I've fixed some of the unit tests now but will finish the rest next week. Mostly it's just involves clamping the maxVersion to TLSv1.2 when there are no TLS 1.3 cipher suites set.

@AdamMajer
Copy link
Contributor Author

This should now pass CI. Fix in parallel/test-tls-set-ciphers.js is rather ugly but it just re-iterates previous assumptions.

In the manual page, there is a statement that ciphersuites contain
explicit default settings - all TLS 1.3 ciphersuites enabled.
In node, we assume that an empty setting mean no ciphersuites and
we disable TLS 1.3. A correct approach to disabling TLS 1.3 is to
disable TLS 1.3 and by not override the default ciphersuits
with an empty string.

So, only override OpenSSL's TLS 1.3 ciphersuites with an explicit
list of ciphers. If none are acceptable, the correct approach is
to disable TLS 1.3 instead elsewhere.

Fixes: nodejs#43419
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 24, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Jun 25, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/43427
✔  Done loading data for nodejs/node/pull/43427
----------------------------------- PR info ------------------------------------
Title      crypto: don't disable TLS 1.3 without suites (#43427)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     AdamMajer:ciphersuites -> nodejs:main
Labels     tls, commit-queue-squash
Commits    1
 - crypto: don't disable TLS 1.3 without suites
Committers 1
 - Adam Majer 
PR-URL: https://github.com/nodejs/node/pull/43427
Fixes: https://github.com/nodejs/node/issues/43419
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43427
Fixes: https://github.com/nodejs/node/issues/43419
Reviewed-By: Matteo Collina 
Reviewed-By: Paolo Insogna 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - crypto: don't disable TLS 1.3 without suites
   ℹ  This PR was created on Tue, 14 Jun 2022 15:11:49 GMT
   ✔  Approvals: 3
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/43427#pullrequestreview-1006059888
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/43427#pullrequestreview-1006070148
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43427#pullrequestreview-1006073727
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-06-24T23:13:05Z: https://ci.nodejs.org/job/node-test-pull-request/44842/
- Querying data for job/node-test-pull-request/44842/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2560676043

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 25, 2022
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit 9cde7a0 into nodejs:main Jun 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 9cde7a0

targos pushed a commit that referenced this pull request Jul 12, 2022
In the manual page, there is a statement that ciphersuites contain
explicit default settings - all TLS 1.3 ciphersuites enabled.
In node, we assume that an empty setting mean no ciphersuites and
we disable TLS 1.3. A correct approach to disabling TLS 1.3 is to
disable TLS 1.3 and by not override the default ciphersuits
with an empty string.

So, only override OpenSSL's TLS 1.3 ciphersuites with an explicit
list of ciphers. If none are acceptable, the correct approach is
to disable TLS 1.3 instead elsewhere.

Fixes: #43419

PR-URL: #43427
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jul 20, 2022
In the manual page, there is a statement that ciphersuites contain
explicit default settings - all TLS 1.3 ciphersuites enabled.
In node, we assume that an empty setting mean no ciphersuites and
we disable TLS 1.3. A correct approach to disabling TLS 1.3 is to
disable TLS 1.3 and by not override the default ciphersuits
with an empty string.

So, only override OpenSSL's TLS 1.3 ciphersuites with an explicit
list of ciphers. If none are acceptable, the correct approach is
to disable TLS 1.3 instead elsewhere.

Fixes: #43419

PR-URL: #43427
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@szmarczak
Copy link
Member

OpenSSL docs for reference:

-ciphersuites val Sets the list of TLSv1.3 ciphersuites.
cipherlist A cipher list of TLSv1.2 and below ciphersuites

node --help shows:

  --tls-cipher-list=...       use an alternative default TLS cipher
                              list

Node.js' TLS cipher list covers all TLS versions (in contrary to the OpenSSL definition).


With this PR, if we specify a single TLSv1.2 ciphersuite (regardless whether through --tls-cipher-list or ciphers, all default TLSv1.3 ciphersuites remain enabled.

The change is quite breaking, so I believe this should have been released in the next major release instead.

@AdamMajer
Copy link
Contributor Author

This is correct - the TLSv1.3 ciphersuites are for TLSv1.3, so the correct way is to set max TLS version to 1.2 if you only want TLS 1.2 used. This fix breaks the assumption that TLSv1.3 should be disabled when cipher suites are not explicitly present because TLSv1.3 has a default set of cipher suites already. This was not the case for TLSv1.2 and prior.

But at the same time, the way that cipher suites were selected prior to this, having disabled TLSv1.3 whenever distros used crypto policies or other OpenSSL based ciphers - this is a bug. For example, setting ciphers list to DEFAULT_SUSE or to PROFILE=SYSTEM and then have TLSv1.3 disabled ... that's a problem.

Personally, I will have to backport this to our nodejs16 irrespective if it gets backported here. I believe that disabling TLSv1.3 by accident is worse than having it enabled by accident 😉

@AdamMajer AdamMajer deleted the ciphersuites branch July 24, 2022 13:45
@szmarczak
Copy link
Member

For example, setting ciphers list to DEFAULT_SUSE or to PROFILE=SYSTEM and then have TLSv1.3 disabled ... that's a problem.

That is a separate case and should not (I believe) be confused with providing a single ciphersuite. I think the problem we have here is that a single option is used to control both <TLSv1.3 and TLSv1.3 ciphersuites. OpenSSL separates them and I believe that Node.js should too 🤔

@AdamMajer
Copy link
Contributor Author

Providing different options now for both would be even larger change. Upstream has provided the 2nd option because they didn't want people to accidentally disable their TLSv1.3 support by not supplying TLS1.3 ciphers ;)

openssl/openssl#11899 (comment)

In Node, the difference between cipher set and cipher suite is whether it uses standard names or not. So, TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384 would be sent to cipher suite function and ECDHE-PSK-AES256-CBC-SHA384 to the cipher set function for TLS 1.2 and below. But both set TLS 1.2 and below ciphers! For a second, I thought this would be a bug but look,

openssl ciphers -v -ciphersuites TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384 NONE
ECDHE-PSK-AES256-CBC-SHA384 TLSv1 Kx=ECDHEPSK Au=PSK  Enc=AES(256)  Mac=SHA384

OpenSSL surprises :-) So, having a single command line parameter for both makes sense, provided that this OpenSSL feature is not a bug 😆

Now, for the possible reason why TLSv1.3 was disabled in node when no ciphersuites were selected, is,

openssl s_client -ciphersuites TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 -connect google.com:443
CONNECTED(00000003)
140005711374144:error:141A90B5:SSL routines:ssl_cipher_list_to_bytes:no ciphers available:ssl/statem/statem_clnt.c:3800:No ciphers enabled for max supported SSL/TLS version

This will work if we explicitly only permit TLSv1.2,

openssl s_client -tls1_2 -ciphersuites TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 -cipher NONE -connect google.com:443
...
SSL-Session:
    Protocol  : TLSv1.2
    Cipher    : ECDHE-RSA-AES256-GCM-SHA384
...

And it makes sense because TLS Protocol version negotiation happens prior to cipher negotiation. And this was already found 2 years ago,

adrienverge/openfortivpn#687 (comment)

Of course, the core of the problem is that TLSv1.3 should not be disabled if cipher suites are not specified. This PR fixes this problem and restores upstream behaviour (as per OpenSSL's explicit design decision). This is why it should not be viewed as a major feature change and more of a bug fix.

@szmarczak
Copy link
Member

openssl/openssl#11899 (comment)

Ciphersuites in TLSv1.3 are completely different to TLSv1.2 ciphersuites and are mutually exclusive

Hence the need for two different options.

TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384

It's not a TLSv1.3 ciphersuite.

as per OpenSSL's explicit design decision

Their explicit design decision was to have two different options, not a single one. Despite this PR being a bug fix, it's a major breaking change. I had code that was relying on the previous behavior and it stopped working with this PR.

@AdamMajer
Copy link
Contributor Author

Sure, but despite what it says, openssl is still setting TLSv1.2 cipher with ciphersuite option, as I demonstrated. 🤷

I had code that was relying on the previous behavior and it stopped working with this PR.

Did you expect that TLSv1.3 would be disabled implicitly if you didn't specify the ciphersuite in a cipher set? That's the only thing that is really changed here. --tls-max-v1.2 should be used instead in these cases.

@szmarczak
Copy link
Member

openssl is still setting TLSv1.2 cipher with ciphersuite option, as I demonstrated

It was just implemented like that:

Under the hood the TLSv1.3 ciphersuites still get added into the same cipher_list as all the others. This is visible to an application if it calls SSL_get_ciphers() et al.

despite what it says

That doesn't mean the option should be misused, no?

Did you expect that TLSv1.3 would be disabled implicitly if you didn't specify the ciphersuite in a cipher set?

Correct. I expected that because Node.js' option manages both <TLSv1.3 and TLSv1.3. Autofilling other ciphers where I explicitly set just one sounds like magic behavior to me.

--tls-max-v1.2 should be used instead in these cases.

That works, however that is a breaking change since it requires a change in my code. To sum up, since this has already landed I'm okay with this change ~ maybe someday we'll have a more strict way to set ciphers :)

targos pushed a commit that referenced this pull request Jul 31, 2022
In the manual page, there is a statement that ciphersuites contain
explicit default settings - all TLS 1.3 ciphersuites enabled.
In node, we assume that an empty setting mean no ciphersuites and
we disable TLS 1.3. A correct approach to disabling TLS 1.3 is to
disable TLS 1.3 and by not override the default ciphersuits
with an empty string.

So, only override OpenSSL's TLS 1.3 ciphersuites with an explicit
list of ciphers. If none are acceptable, the correct approach is
to disable TLS 1.3 instead elsewhere.

Fixes: #43419

PR-URL: #43427
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
In the manual page, there is a statement that ciphersuites contain
explicit default settings - all TLS 1.3 ciphersuites enabled.
In node, we assume that an empty setting mean no ciphersuites and
we disable TLS 1.3. A correct approach to disabling TLS 1.3 is to
disable TLS 1.3 and by not override the default ciphersuits
with an empty string.

So, only override OpenSSL's TLS 1.3 ciphersuites with an explicit
list of ciphers. If none are acceptable, the correct approach is
to disable TLS 1.3 instead elsewhere.

Fixes: nodejs/node#43419

PR-URL: nodejs/node#43427
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--tls-cipher-list=DEFAULT@SECLEVEL=0 doesn't compatible with tls1.3
7 participants