Improvement of the ciphersuite selection by OttoHollmann · Pull Request #10590 · openssl/openssl

OttoHollmann · 2019-12-08T23:15:14Z

I had read some issues (#5050, #541, ...) about the current ciphersuite specification and I decided to implement some improvements:
https://github.com/OttoHollmann/openssl/tree/Improvement-of-the-ciphersuite-selection

Operator | for specifying version of each ciphersuite/alias
There are these versions: SSLv3, TLSv1, TLSv1.2, TLSv1.3, ALL and multiple versions can be used at one time: CHACHA20|TLSv1.2|TLSv1.3
Parameter for setting default version (-version_mask). If no version operator | or version alias (e.g. TLSv1.2) or specific ciphersuite is set, the default version mask will be applied. If no version_mask is provided, it will be set to all versions up to TLSv1.2 (for backwards compatibility).

e.g. TLSv1.3:AES128|ALL:ECDH+AES|TLSv1.2|TLSv1:DH with no version_mask will enable:
- TLSv1.3 - all TLSv1.3 ciphers (because this alias represent version)
- AES128|ALL - all ciphersuites using AES128 from all versions
- ECDH+AES|TLSv1.2|TLSv1 - ECDH+AES from versions TLSv1.2 or TLS1
- DH - DH from versions except TLSv1.3 (because of default value of version mask)
Allowing to set TLSv1.3 ciphersuites using cipher list interface.
If at least one valid TLSv1.3 ciphersuite is in the cipher list, or parameter version_mask is used, all ciphers entered using ciphersuites interface will be erased (ciphersuites will be set to empty string).
Add operator ^ for moving cipher at the top of the list (opposite of +)
Operator * for prioritizing cipher depending on client's list. If server ciphers are prioritized and a cipher has this flag and it is at the top of the client list, it will be selected.

e.g. (suppose only TLSv1.3 ciphersuites):
- server: TLSv1.3:+TLS_CHACHA20_POLY1305_SHA256:*CHACHA20|ALL
  - TLS_AES_256_GCM_SHA384
  - TLS_AES_128_GCM_SHA256
  - TLS_AES_128_CCM_8_SHA256
  - TLS_AES_128_CCM_SHA256
  - *TLS_CHACHA20_POLY1305_SHA256
- client: DEFAULT|TLSv1.3:^TLS_CHACHA20_POLY1305_SHA256
  - TLS_CHACHA20_POLY1305_SHA256
  - TLS_AES_256_GCM_SHA384
  - TLS_AES_128_GCM_SHA256
Will result into selecting TLS_CHACHA20_POLY1305_SHA256 because it's at the top of the client's list and has priority flag in the server list.
This feature was modified to check first common ciphersuite in client list instead of first ciphersuites.
Equal-preference groups - group is defined by these brackets [ ] and can contain ciphersuites of cipher alias (even with specified version)
- operators +^ can not be used inside of group
- operator * used inside of group will affect at most all ciphers in current group
- operators +^* used outside of group will not affect ciphersuites inside group
- operators @SECLEVEL and @STRENGTH can not be used with groups
  e.g. command openssl ciphers -v -version_mask ALL '[TLSv1.3:-CHACHA20]:AES256-CCM:CHACHA20:*CHACHA20' returns this (encryption column omitted):

┍   TLS_AES_256_GCM_SHA384         TLSv1.3 Kx=any      Au=any
│   TLS_AES_128_GCM_SHA256         TLSv1.3 Kx=any      Au=any
│   TLS_AES_128_CCM_8_SHA256       TLSv1.3 Kx=any      Au=any
┕   TLS_AES_128_CCM_SHA256         TLSv1.3 Kx=any      Au=any
    AES256-CCM                     TLSv1.2 Kx=RSA      Au=RSA
  * TLS_CHACHA20_POLY1305_SHA256   TLSv1.3 Kx=any      Au=any
  * ECDHE-ECDSA-CHACHA20-POLY1305  TLSv1.2 Kx=ECDH     Au=ECDSA
  * ECDHE-RSA-CHACHA20-POLY1305    TLSv1.2 Kx=ECDH     Au=RSA
  * DHE-RSA-CHACHA20-POLY1305      TLSv1.2 Kx=DH       Au=RSA
  * RSA-PSK-CHACHA20-POLY1305      TLSv1.2 Kx=RSAPSK   Au=RSA
  * DHE-PSK-CHACHA20-POLY1305      TLSv1.2 Kx=DHEPSK   Au=PSK
  * ECDHE-PSK-CHACHA20-POLY1305    TLSv1.2 Kx=ECDHEPSK Au=PSK
  * PSK-CHACHA20-POLY1305          TLSv1.2 Kx=PSK      Au=PSK

~~Documentation and tests are missing but will be added soon.~~

Any feedback will be greatly appreciated.

…cipher list interface.

…for each cipher.

…unction.

…st cipher.

…for each cipher.

…cipher list interface.

…r list format.

ssl/s3_lib.c

ssl/ssl_conf.c

ssl/ssl_lib.c

doc/man3/SSL_CTX_set_cipher_list.pod

openssl-machine · 2020-06-03T07:44:59Z

This PR is waiting for the creator to make requested changes but it has not been updated for 148 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2020-07-04T00:07:56Z

This PR is waiting for the creator to make requested changes but it has not been updated for 179 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2020-08-04T00:08:04Z

This PR is waiting for the creator to make requested changes but it has not been updated for 210 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2020-09-04T00:07:52Z

This PR is waiting for the creator to make requested changes but it has not been updated for 241 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2020-10-05T00:07:41Z

This PR is waiting for the creator to make requested changes but it has not been updated for 272 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

OttoHollmann · 2020-10-20T09:58:44Z

Can we move forward please? Which changes are needed to accept this PR? I am looking forward to your response!

mattcaswell · 2020-10-20T11:08:52Z

I have to admit to being quite sceptical about this PR. Do we really need to make ciphersuite selection more complicated? It's already quite complicated and I'm not sure adding more operators and ways to do things particularly helps most users.

I'm not at all convinced that version based selection is a good idea. People are already very confused by what the versions associated with ciphersuites actually mean and try to control the negotiated versions via ciphersuites.

I'm also not convinced by the proposal to specify TLSv1.3 ciphersuites via the old style cipher list. We had many discussions about how to do TLSv1.3 ciphersuite configuration during the 1.1.1 development phase and many different proposals were made (and more than one implemented in the code and then backed out again). Having decided on an approach and pushing it out, I'm unconvinced that changing tack now is a good plan.

richsalz · 2020-10-20T13:25:24Z

Merging the version into the list of ciphers makes this very very complicated. Understanding what happens to the version flags (or OP settings) and their interaction with the cipher list is tricky and hard to understand.

This shouldn't be accepted.

OttoHollmann · 2020-10-20T19:16:48Z

Yes, I would agree, that the ciphersuite selection process is quite complicated - this was the main reason which prompted the idea behind this PR.

I didn't know about previous discussions and attempts. But I noticed, that ciphersuites were entered using old cipher list interface in past. Because it can break some existing configuration, ciphersuites interface was created.

The main goal of this PR was not introducing additional way of doing things but to introduce an unified way. I know a few people who would appreciate this an unified interface.

And just a side note: I don't know any other crypto library which has two interfaces for specifying ciphers. This results into an issue with third party applications which have to implement a second interface otherwise you are unable to modify ciphersuites (e.g. nginx). I believe this makes users much more confused.

We don't have to accept this PR, but I'll be very happy, if we at least come up with a unified interface before closing this PR.

openssl-machine · 2020-11-20T00:07:51Z

This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2020-12-21T00:08:28Z

This PR is waiting for the creator to make requested changes but it has not been updated for 61 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2021-01-21T00:07:57Z

This PR is waiting for the creator to make requested changes but it has not been updated for 92 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

paulidale · 2021-01-21T06:56:47Z

Does anyone object to this being closed?

openssl-machine · 2021-02-21T00:07:11Z

This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2021-03-24T00:07:23Z

This PR is waiting for the creator to make requested changes but it has not been updated for 61 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2021-04-24T00:07:31Z

This PR is waiting for the creator to make requested changes but it has not been updated for 92 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2021-05-22T00:07:05Z

This PR has been closed. It was waiting for the creator to make requested changes but it has not been updated for 120 days.

OttoHollmann added 30 commits September 18, 2019 08:19

Add function for parsing version string.

3c293d3

Add parameter version_mask to function ssl_cipher_apply_rule.

f0e23dd

Modify cipher_list parser to parse version specification.

1be0365

Add parameter for specifying default version mask in cipher_list.

a30ff7d

Add function for counting ciphers by version, which were entered via …

99bfc9a

…cipher list interface.

Add operator for prioritizing ciphers depending on client's preference.

506f5f9

Add operator for moving ciphers to the beginning of the cipher_list.

e627da2

Replaced cipher_list with structure containing cipher_list and flags …

7bfd232

…for each cipher.

Add TLSv1.3 ciphers to the cipher list configuration interface.

ac095ca

Move code for checking ciphers into separate function.

8f6dc7f

Add a condition to check order of configuration parameters.

9c24855

Add parser and errors for equal-preference groups.

3810d30

Add support for the equal-preference groups to the cipher selection f…

7c2062e

…unction.

Remove ciphers entered from ciphersuites if they are not needed.

d21d10b

Merge cipher table tls13_ciphers with ssl3_ciphers.

fb9844d

Check preference flag for client's first common cipher instead of fir…

031bedd

…st cipher.

Fixed behavior of operators in equal-preference group.

f5275ff

Add a function for cipher flags description.

e396459

Fix coverity-scan errors.

720d2e5

Fix version mask

4d4b6da

Add function for parsing version string.

2b75a14

Add parameter version_mask to function ssl_cipher_apply_rule.

fbe9dee

Modify cipher_list parser to parse version specification.

3e085ea

Add parameter for specifying default version mask in cipher_list.

bd087e3

Add operator for moving ciphers to the beginning of the cipher_list.

61b2209

Replaced cipher_list with structure containing cipher_list and flags …

4c2d751

…for each cipher.

Add function for counting ciphers by version, which were entered via …

49521ec

…cipher list interface.

Add TLSv1.3 ciphers to the cipher list configuration interface.

7e795d7

Move code for checking ciphers into separate function.

ee0f9d6

Add operator for prioritizing ciphers depending on client's preference.

72fc89e

OttoHollmann added 3 commits December 21, 2019 15:39

Documentation update for equal-preference groups a and improved ciphe…

727e236

…r list format.

Add tests for equal-preference groups and priority flag.

b9170de

Renamed variable cipher_list_flags and fixed errors.

0ed8860

OttoHollmann changed the title ~~WIP: Improvement of the ciphersuite selection~~ Improvement of the ciphersuite selection Dec 22, 2019

Merge branch 'master' into Improvement-of-the-ciphersuite-selection

29ae55c

FdaSilvaYY suggested changes Dec 22, 2019

View reviewed changes

ssl/s3_lib.c Outdated Show resolved Hide resolved

ssl/ssl_conf.c Show resolved Hide resolved

ssl/ssl_lib.c Outdated Show resolved Hide resolved

doc/man3/SSL_CTX_set_cipher_list.pod Outdated Show resolved Hide resolved

doc/man3/SSL_CTX_set_cipher_list.pod Outdated Show resolved Hide resolved

OttoHollmann added 2 commits December 22, 2019 21:15

Fixed typo, coding style and build errors.

ae79d97

Fixed shadowed declaration and missing prototypes.

fa8960c

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Dec 25, 2019

OttoHollmann added 2 commits January 6, 2020 11:37

Fix displaying flags when using only supported ciphers.

d41e4f8

Fixed build error - uninitialized value.

9580968

OttoHollmann mentioned this pull request Jun 10, 2020

Fix set_ciphersuites ignore unknown ciphers. #12100

Closed

openssl-machine closed this May 22, 2021

Uh oh!

Comments

Conversation

OttoHollmann commented Dec 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openssl-machine commented Jun 3, 2020

Uh oh!

openssl-machine commented Jul 4, 2020

Uh oh!

openssl-machine commented Aug 4, 2020

Uh oh!

openssl-machine commented Sep 4, 2020

Uh oh!

openssl-machine commented Oct 5, 2020

Uh oh!

OttoHollmann commented Oct 20, 2020

Uh oh!

mattcaswell commented Oct 20, 2020

Uh oh!

richsalz commented Oct 20, 2020

Uh oh!

OttoHollmann commented Oct 20, 2020

Uh oh!

openssl-machine commented Nov 20, 2020

Uh oh!

openssl-machine commented Dec 21, 2020

Uh oh!

openssl-machine commented Jan 21, 2021

Uh oh!

paulidale commented Jan 21, 2021

Uh oh!

openssl-machine commented Feb 21, 2021

Uh oh!

openssl-machine commented Mar 24, 2021

Uh oh!

openssl-machine commented Apr 24, 2021

Uh oh!

openssl-machine commented May 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

OttoHollmann commented Dec 8, 2019 •

edited

Loading