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

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

Closed

Conversation

@mattcaswell
Copy link
Member

commented Feb 16, 2018

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 #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

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2018

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

@richsalz

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

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

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 16, 2018

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

@kroeckx

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

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

@kroeckx

This comment has been minimized.

Copy link
Member

commented Feb 17, 2018

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

# define TLS1_3_TXT_AES_128_GCM_SHA256 "TLS13-AES-128-GCM-SHA256"
# define TLS1_3_TXT_AES_256_GCM_SHA384 "TLS13-AES-256-GCM-SHA384"
# define TLS1_3_TXT_CHACHA20_POLY1305_SHA256 "TLS13-CHACHA20-POLY1305-SHA256"
# define TLS1_3_TXT_AES_128_CCM_SHA256 "TLS13-AES-128-CCM-SHA256"
# define TLS1_3_TXT_AES_128_CCM_8_SHA256 "TLS13-AES-128-CCM-8-SHA256"

This comment has been minimized.

Copy link
@FdaSilvaYY

FdaSilvaYY Feb 17, 2018

Contributor

Why do you keep this last one ? ;)

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Feb 19, 2018

Author Member

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

@@ -263,6 +263,16 @@ static int cmd_CipherString(SSL_CONF_CTX *cctx, const char *value)
return rv > 0;
}

static int cmd_Ciphersuites(SSL_CONF_CTX *cctx, const char *value)
{
int rv = 1;

This comment has been minimized.

Copy link
@FdaSilvaYY

FdaSilvaYY Feb 17, 2018

Contributor

style: blank line after decl.

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Feb 19, 2018

Author Member

Fixed

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

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

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 19, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2018

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

@kroeckx

This comment has been minimized.

Copy link
Member

commented Feb 21, 2018

@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2018

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!

@mattcaswell mattcaswell force-pushed the mattcaswell:separate-tls13-ciphers branch from 2e470d9 to 4d71a7d Mar 14, 2018
@mattcaswell

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

Rebased again to resolve more conflicts. Ping @t-j-h?

TLS_AES_256_GCM_SHA384 TLS_AES_256_GCM_SHA384
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

This comment has been minimized.

Copy link
@t-j-h

t-j-h Mar 14, 2018

Member

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

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Mar 14, 2018

Author Member

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).

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

This comment has been minimized.

Copy link
@t-j-h

t-j-h Mar 14, 2018

Member

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

This comment has been minimized.

Copy link
@mattcaswell

mattcaswell Mar 14, 2018

Author Member

Good idea. Done.

@t-j-h

This comment has been minimized.

Copy link
Member

commented Mar 14, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

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

@t-j-h
t-j-h approved these changes Mar 14, 2018
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2018

Pushed. Thanks!

levitte pushed a commit that referenced this pull request Mar 14, 2018
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)
levitte pushed a commit that referenced this pull request Mar 14, 2018
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #5392)
levitte pushed a commit that referenced this pull request Mar 14, 2018
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #5392)
levitte pushed a commit that referenced this pull request Mar 14, 2018
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #5392)
levitte pushed a commit that referenced this pull request Mar 14, 2018
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)
levitte pushed a commit that referenced this pull request Mar 14, 2018
Reviewed-by: Tim Hudson <tjh@openssl.org>
(Merged from #5392)
@tmshort

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.