Split configuration of TLSv1.3 ciphers from older ciphers #5392

mattcaswell · 2018-02-16T15:59:53Z

With the current mechanism, old cipher strings that used to work in 1.1.0,
may inadvertently disable all TLSv1.3 ciphersuites causing connections to
fail. This is confusing for users.

In reality TLSv1.3 are quite different to older ciphers. They are much
simpler and there are only a small number of them so, arguably, they don't
need the same level of control that the older ciphers have.

This change splits the configuration of TLSv1.3 ciphers from older ones.
By default the TLSv1.3 ciphers are on, so you cannot inadvertently disable
them through your existing config.

Fixes #5359

A few things to note about this (which may or may not be controversial):

I have taken the opportunity to replace the OpenSSL specific names for the ciphersuites with the RFC standard names. The only reason to have OpenSSL specific ones was to make them a bit more consistent with the rest of the ciphersuite names. Now that they are treated differently anyway that reason seems less compelling (if it ever was).
This PR also changes the default ordering of the TLSv1.3 ciphersuites. Previously the TLSv1.3 ciphersuites were mixed in with the rest of the ciphersuites in the order "TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256". Now they are at the top of the ciphersuites list (on the basis that we would rather use these TLSv1.3 ciphersuites than older ones) and in the order "TLS_AES_128_GCM_SHA256:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_256_GCM_SHA384". This is on the basis that AES-256 seems overkill for the default choice (it also has the side benefit that the default hash will now be SHA-256 which is one step towards easing the PSK transition as discussed in Likely upgrade issues with PSK use in TLS 1.3 #5378). I'm not sure which we would prefer as the top choice out of the AES vs chacha20 ciphersuites. Note this is slightly inconsistent with the default ordering of the older ciphersuites which are sorted by strength (so AES-256 is preferred over AES-128). Since we are handling the two lists differently, I kind of think that is ok.
I have added new functions SSL_CTX_set_ciphersuites()/SSL_set_ciphersuites() for configuring the TLSv1.3 specific ciphersuites. There is also a new "Ciphersuites" SSL_CONF parameter, as well as a new "-ciphersuites" command line option for s_server/s_client/ciphers.
We possibly need SSL_CTX_get0_ciphersuites() and SSL_get0_ciphersuites() too.
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.

Checklist

documentation is added or updated
tests are added or updated

mattcaswell · 2018-02-16T16:35:10Z

Pushed a fix for the Travis failure. Still investigating the AppVeyor failure.

richsalz · 2018-02-16T16:36:30Z

I think using "ciphersuites" is confusing, maybe "tls13ciphers" ?
Why does apps/s_time.c have a getenv(SSL_CIPHERS) ?

mattcaswell · 2018-02-16T16:41:19Z

I think using "ciphersuites" is confusing, maybe "tls13ciphers" ?

I was deliberately trying to use something which doesn't include the string tls1.3 in it. That will live with us forever even when it is no longer relevant.

Why does apps/s_time.c have a getenv(SSL_CIPHERS) ?

No idea. It doesn't seem documented. Although I should at least add an option to s_time to set the TLSv1.3 ciphersuites.

richsalz · 2018-02-16T16:50:20Z

Yes, add the flag to s_time and remove the option when you do so.
As for using/notusing tls1.3 in the name, I'm curious what others think.

t-j-h · 2018-02-16T20:38:10Z

Don't use tls1.3 in the name - as we want this to be used in future.

kroeckx · 2018-02-17T14:44:13Z

I prefer that we default AES-256 over AES-128

kroeckx · 2018-02-17T14:58:02Z

I'm wondering if there will be any interaction between this new setting and older TLS protocols.

FdaSilvaYY · 2018-02-17T20:28:07Z

include/openssl/tls1.h

Why do you keep this last one ? ;)

Oops. How did I miss that one? Fixed now.

FdaSilvaYY · 2018-02-17T20:30:07Z

ssl/ssl_conf.c

style: blank line after decl.

mattcaswell · 2018-02-19T13:45:47Z

Yes, add the flag to s_time and remove the option when you do so.

I added the flag. I didn't remove the environment variable thing. If people are relying on that it could break things - which seems unnecessary.

mattcaswell · 2018-02-19T13:47:52Z

I prefer that we default AES-256 over AES-128

What do other people think about this? BTW I realised that I had inadvertently also changed the ordering of the chacha ciphersuite vs the AES-128 one. I've now swapped those two around. I've currently kept aes-128/chacha being negotiated in favour of AES-256 for now until I get further feedback.

mattcaswell · 2018-02-19T13:48:22Z

I'm wondering if there will be any interaction between this new setting and older TLS protocols.

I'm not sure what sort of thing you're worried about here?

richsalz · 2018-02-19T16:02:47Z

I do not think we should be setting the preference; it goes against our philosophy. We can also expect that some users -- e.g., FIPS -- will need to turn off some ciphers -- e,.g., ChaCha -- and we should allow them to otherwise use TLS 1.3

mattcaswell · 2018-02-19T16:08:14Z

I do not think we should be setting the preference; it goes against our philosophy.

I'm not sure I understand what you are trying to say? What preference are you talking about here?

We can also expect that some users -- e.g., FIPS -- will need to turn off some ciphers -- e,.g., ChaCha -- and we should allow them to otherwise use TLS 1.3

This PR enables people to turn off what ever ciphers they want to?

richsalz · 2018-02-19T16:23:34Z

Maybe I misunderstand, I did skim the code but perhaps not read closely enough. Comments like "kept aes 128 over 256" implies there is a specific ordering of ciphers. No?

mattcaswell · 2018-02-19T16:30:22Z

Maybe I misunderstand, I did skim the code but perhaps not read closely enough. Comments like "kept aes 128 over 256" implies there is a specific ordering of ciphers. No?

Well yes. In the code before this PR (and before TLSv1.3) we order our ciphersuites by preference. This is the DEFAULT cipher list. It is a requirement that we know what that order is for us to construct or process a ClientHello. This PR just separates out the configuration of TLSv1.3 ciphersuites from TLSv1.2 and below. We still need to order the ciphersuites as we did before this PR.

Prior to this PR the ordering was:

"TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256"

As part of this PR I changed the order to:

"TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384"

On the basis that AES256 as a default first preference seems overkill. Kurt thinks we should keep the ordering the same as it was before. It is that issue I am asking for comment on.

richsalz · 2018-02-19T17:25:56Z

Can I, as an application, configure which ciphers I want to use, and in which order?

mattcaswell · 2018-02-19T17:32:06Z

Can I, as an application, configure which ciphers I want to use, and in which order?

Yes, absolutely. We're just talking about the default here if an application doesn't specify anything different.

kroeckx · 2018-02-19T17:34:16Z

On Mon, Feb 19, 2018 at 01:48:26PM +0000, mattcaswell wrote: > I'm wondering if there will be any interaction between this new setting and older TLS protocols. I'm not sure what sort of thing you're worried about here?

I'm wondering if in the new configuration you say "AES", or "AES-256-GCM-SHA384", that you're going to enable the TLS 1.2 ciphers that match that or not, or if this is going to be a TLS 1.3 setting only. Do we also have (and need) settings for the key exchange and authentication?

mattcaswell · 2018-02-19T18:41:55Z

I'm wondering if in the new configuration you say "AES", or
"AES-256-GCM-SHA384", that you're going to enable the TLS 1.2
ciphers that match that or not, or if this is going to be a TLS
1.3 setting only.

No. The new configuration settings for TLSv1.3 are a simple list of the allowed ciphersuites. It does not have all the functionality of the TLSv1.2 cipher list (it's not needed - there are currently only 5 permissible values). It is not possible to specify "AES" in the list for TLSv1.3 because it is not a ciphersuite name.

kroeckx · 2018-02-19T21:37:35Z

My question is if it's useful to actually support it, that we split the current setting in 3 settings and that it's still general purpose. We could then deprecate the old one. You could then for instance have 3DES in it, and it would select the 3DES ciphers for older versions but since we don't have any ciphers for TLS 1.3 with 3DES it wouldn't do anything for TLS 1.3.

t-j-h · 2018-02-19T21:47:25Z

I think we should keep the ordering as it was - ordered by strength (as that is what we have as our DEFAULT) for TLSv1.2 - why would we elected to pick a different order for TLSv1.3 - i.e. not by strength - don't we want to selected the "strongest" available as our default?

i.e. I think I agree with @kroeckx

kroeckx · 2018-02-20T08:25:20Z

I'm not sure at all how my proposal to split it up in 3 and work for both TLS 1.2 and TLS 1.2 is supposed to work.

mattcaswell · 2018-02-21T15:48:29Z

I'm not sure at all how my proposal to split it up in 3 and work
for both TLS 1.2 and TLS 1.2 is supposed to work.

No, I don't know either, although it might be interesting to explore it. However, I think it is probably dealing with a separate problem to the one we are addressing here.

I think your proposal only really works if applications switch to your new 3-way configuration, and ditch any existing cipher_list configuration - otherwise how do you resolve conflicts between the two configuration approaches?

That's fine, and you could have the two things running in parallel, as long as applications only use one of them. But the problem this PR is trying to address is the backwards compatibility issue where applications have set a cipher_list that excludes the possibility of TLSv1.3 ciphersuites and expect it to just work, i.e. we need a solution which allows TLSv1.3 to work in the presence of an existing cipher list configuration.

So - in other words, I think your proposal is interesting, and might be worth exploring, but doesn't impact this PR.

mattcaswell · 2018-02-21T16:28:57Z

Fix up commit pushed to change the order back to preferring AES-256 over the AES-128/CHACHA20

kroeckx · 2018-02-21T18:13:40Z

I guess the way to split it is ignoring the new ones if the old one is set.

mattcaswell · 2018-02-21T18:18:50Z

I guess the way to split it is ignoring the new ones if the old one is set.

Right. But that doesn't help with the problem at hand: legacy client cipher strings can disable all TLSv1.3 ciphersuites. So I think we still need this PR. Your proposal still has merit but is orthogonal to this change.

mattcaswell · 2018-02-21T18:20:10Z

Pushed a new commit to try and keep the CIs happy. Lets see if it worked. But, anyway, I'm taking this out of WIP. Please review!

t-j-h · 2018-03-14T10:28:51Z

doc/man1/ciphers.pod

+ TLS_CHACHA20_POLY1305_SHA256               TLS_CHACHA20_POLY1305_SHA256
+ TLS_AES_128_CCM_SHA256                     TLS_AES_128_CCM_SHA256
+ TLS_AES_128_CCM_8_SHA256                   TLS_AES_128_CCM_8_SHA256



I don't get this - a table of two columns which are identical ...

This follows on in the same format as for all of the other ciphersuites listed just before this section. The first column is the standard RFC name. The second column is the OpenSSL name. As I mentioned in the initial commentary for this PR:

I have taken the opportunity to replace the OpenSSL specific names for the ciphersuites with the RFC standard names. The only reason to have OpenSSL specific ones was to make them a bit more consistent with the rest of the ciphersuite names. Now that they are treated differently anyway that reason seems less compelling (if it ever was).

t-j-h · 2018-03-14T10:36:53Z

ssl/ssl_lib.c

+            "TLS_AES_256_GCM_SHA384:"
+            "TLS_CHACHA20_POLY1305_SHA256:"
+            "TLS_AES_128_GCM_SHA256"))
+        goto err;


That default string should be coming out of a define in ssl.h - i.e. TLS_DEFAULT_CIPHERSUITES to match SSL_DEFAULT_CIPHER_LIST ?

Good idea. Done.

t-j-h · 2018-03-14T10:39:47Z

I've done a pass through - just a couple of minor comments.
I also think we should allow the TLS_ prefix on the ciphersuites to be optional - i.e. work without that being specified. It just really seems out of place to force its usage (and it is redundant in the context where used).

I have no issue with the strings being there - just suggesting we should also allow the non-"TLS_" versions to work.

Other than the noted comments and the suggestion above, I think this is ready.

mattcaswell · 2018-03-14T10:50:32Z

I also think we should allow the TLS_ prefix on the ciphersuites to be optional - i.e. work without that being specified. It just really seems out of place to force its usage (and it is redundant in the context where used).

I have no issue with the strings being there - just suggesting we should also allow the non-"TLS_" versions to work.

I disagree with this. Having "TLS_" on the front doesn't seem a huge hardship and I think having two names for the same thing is needlessly confusing (it would not necessarily be obvious to most users that TLS_AES_256_GCM_SHA384 and AES_256_GCM_SHA384 are in fact the same thing).

mattcaswell · 2018-03-14T10:51:02Z

I have responded to all the latest comments above, and have pushed a new commit to address one of them.

t-j-h

That TLS_ prefix is going to grate for me at least. But we will see what others think once it is in as no one else has commented on that.

mattcaswell · 2018-03-14T11:05:13Z

Pushed. Thanks!

With the current mechanism, old cipher strings that used to work in 1.1.0, may inadvertently disable all TLSv1.3 ciphersuites causing connections to fail. This is confusing for users. In reality TLSv1.3 are quite different to older ciphers. They are much simpler and there are only a small number of them so, arguably, they don't need the same level of control that the older ciphers have. This change splits the configuration of TLSv1.3 ciphers from older ones. By default the TLSv1.3 ciphers are on, so you cannot inadvertently disable them through your existing config. Fixes #5359 Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

A place in clienthellotest was missed in converting to the new mechanism for configuration of TLSv1.3 ciphersuites. Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

tmshort · 2018-03-14T11:56:24Z

Would it make any sense to have the default cipher strings be the return value of a function? Perhaps not now, but in 1.2?

FdaSilvaYY reviewed Feb 17, 2018

View reviewed changes

mattcaswell mentioned this pull request Feb 19, 2018

1.1.1-pre1: s_client: PSK ciphersuites no longer usable even with TLS 1.2 #5406

Closed

mattcaswell force-pushed the separate-tls13-ciphers branch from 3ccb631 to ffc3d74 Compare February 19, 2018 13:43

kaduk mentioned this pull request Feb 19, 2018

Remove undocumented SSL_CIPHER environment variable from s_time #5411

Open

t-j-h reviewed Mar 14, 2018

View reviewed changes

Put the default set of TLSv1.3 ciphersuites in a header file

7c50941

t-j-h approved these changes Mar 14, 2018

View reviewed changes

mattcaswell closed this Mar 14, 2018

levitte pushed a commit that referenced this pull request Mar 14, 2018

Update s_time to be allow configuration of TLSv1.3 ciphersuites

e54b3cc

Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

levitte pushed a commit that referenced this pull request Mar 14, 2018

Add documentation for TLSv1.3 ciphersuite configuration

9d2674c

Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

levitte pushed a commit that referenced this pull request Mar 14, 2018

Update CHANGES with details of TLSv1.3 ciphersuite configuration

2b527b9

Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

levitte pushed a commit that referenced this pull request Mar 14, 2018

Put the default set of TLSv1.3 ciphersuites in a header file

fa25763

Reviewed-by: Tim Hudson <tjh@openssl.org> (Merged from #5392)

dcooper16 mentioned this pull request Mar 14, 2018

OpenSSL 1.1.1 and cipher suite lists testssl/testssl.sh#1013

Closed

dcooper16 mentioned this pull request Mar 23, 2018

s_client not working with TLSv1.3 CCM ciphers #5740

Closed

mattcaswell mentioned this pull request Mar 23, 2018

Included TLS 1.3 ciphers in "openssl ciphers -V <crappy_pattern>" #5736

Closed

This was referenced Mar 28, 2018

TLS 1.3 ciphers and protocol values mozilla/server-side-tls#191

Closed

Confusing documentation surrounding cipherstring and TLS 1.3 in blog post #5777

Closed

This was referenced May 18, 2018

Remove TLS13-* suites from DEFAULT_CIPHERS urllib3/urllib3#1386

Closed

Add binding for SSL_CTX_set_ciphersuites() pyca/cryptography#4255

Closed

"openssl ciphers UNSUPPORTED" no longer fails with OpenSSL 1.1.1-pre6 #6296

Closed

ArchangeGabriel mentioned this pull request Jul 11, 2018

TLS 1.3 Cipher Strength 90%? ssllabs/ssllabs-scan#636

Open

daurnimator mentioned this pull request Jul 13, 2018

Bind *_set_ciphersuites wahern/luaossl#141

Closed

h-vn mentioned this pull request Aug 24, 2018

Add SSL_set_ciphersuites and SSL_CTX_set_ciphersuites radiator-software/p5-net-ssleay#36

Closed

lukastribus mentioned this pull request May 13, 2019

Captcha issues Anorov/cloudflare-scrape#235

Closed

szmarczak mentioned this pull request Jul 25, 2022

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

Merged

gstrauss mentioned this pull request Nov 25, 2024

Explicitly set list of allowed TLSv1.3 ciphersuites for nginx Modern/Intermediate/Old configurations mozilla/ssl-config-generator#124

Closed

Uh oh!

Split configuration of TLSv1.3 ciphers from older ciphers #5392

Split configuration of TLSv1.3 ciphers from older ciphers #5392

Uh oh!

Conversation

mattcaswell commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mattcaswell commented Feb 16, 2018

Uh oh!

richsalz commented Feb 16, 2018

Uh oh!

mattcaswell commented Feb 16, 2018

Uh oh!

richsalz commented Feb 16, 2018

Uh oh!

t-j-h commented Feb 16, 2018

Uh oh!

kroeckx commented Feb 17, 2018

Uh oh!

kroeckx commented Feb 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Feb 19, 2018

Uh oh!

mattcaswell commented Feb 19, 2018

Uh oh!

mattcaswell commented Feb 19, 2018

Uh oh!

richsalz commented Feb 19, 2018

Uh oh!

mattcaswell commented Feb 19, 2018

Uh oh!

richsalz commented Feb 19, 2018

Uh oh!

mattcaswell commented Feb 19, 2018

Uh oh!

richsalz commented Feb 19, 2018

Uh oh!

mattcaswell commented Feb 19, 2018

Uh oh!

kroeckx commented Feb 19, 2018 via email

Uh oh!

mattcaswell commented Feb 19, 2018

Uh oh!

kroeckx commented Feb 19, 2018

Uh oh!

t-j-h commented Feb 19, 2018

Uh oh!

kroeckx commented Feb 20, 2018 via email

Uh oh!

mattcaswell commented Feb 21, 2018

Uh oh!

mattcaswell commented Feb 21, 2018

Uh oh!

kroeckx commented Feb 21, 2018 via email

Uh oh!

mattcaswell commented Feb 21, 2018

Uh oh!

mattcaswell commented Feb 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-j-h commented Mar 14, 2018

Uh oh!

mattcaswell commented Mar 14, 2018

mattcaswell commented Feb 16, 2018 •

edited

Loading