Opened 6 years ago

Closed 6 years ago

#11513 closed defect (fixed)

Make UNRESTRICTED_SERVER_CIPHER_LIST non-stupid

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, 024-backport, tls, nickm-backport-02422
Cc: isis@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This is a parent ticket for the sub-tickets we've accumulated about fixing our the list of ciphersuites that servers prefer and pick from.

Child Tickets

TicketStatusOwnerSummaryComponent
#11492closedRelay will never choose TLS1_TXT_ECDHE_RSA_WITH_AES_256_CBC_SHA ciphersCore Tor/Tor
#11498closedAllow to use TLS1_TXT_ECDHE_RSA_WITH_AES_256_SHA384 cipher suite by relayCore Tor/Tor
#11499closedSort list of cipher suites by clear goalsCore Tor/Tor

Attachments (2)

gen_server_ciphers.py (2.6 KB) - added by nickm 6 years ago.
gen_server_ciphers.patch (1.3 KB) - added by cypherpunks 6 years ago.
Patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 6 years ago by nickm

Here's the FULL list of adequate ciphers provided by openssl 1.0.1:

[659]$ grep '\(TLS1\|SSL3\)_TXT' /usr/local/opt/openssl/include/openssl/*.h | grep RSA | grep -v CAMEL |grep -v RC4 |  grep 'DHE\|EDH' |grep -v SEED |grep -v NULL | grep -v EXP |grep -v DES_64 | sed -e 's/^.*://'  
#define SSL3_TXT_EDH_RSA_DES_192_CBC3_SHA	"EDH-RSA-DES-CBC3-SHA"
#define TLS1_TXT_DHE_RSA_WITH_AES_128_SHA		"DHE-RSA-AES128-SHA"
#define TLS1_TXT_DHE_RSA_WITH_AES_256_SHA		"DHE-RSA-AES256-SHA"
#define TLS1_TXT_ECDHE_RSA_WITH_DES_192_CBC3_SHA        "ECDHE-RSA-DES-CBC3-SHA"
#define TLS1_TXT_ECDHE_RSA_WITH_AES_128_CBC_SHA         "ECDHE-RSA-AES128-SHA"
#define TLS1_TXT_ECDHE_RSA_WITH_AES_256_CBC_SHA         "ECDHE-RSA-AES256-SHA"
#define TLS1_TXT_DHE_RSA_WITH_AES_128_SHA256		"DHE-RSA-AES128-SHA256"
#define TLS1_TXT_DHE_RSA_WITH_AES_256_SHA256		"DHE-RSA-AES256-SHA256"
#define TLS1_TXT_DHE_RSA_WITH_AES_128_GCM_SHA256	"DHE-RSA-AES128-GCM-SHA256"
#define TLS1_TXT_DHE_RSA_WITH_AES_256_GCM_SHA384	"DHE-RSA-AES256-GCM-SHA384"
#define TLS1_TXT_ECDHE_RSA_WITH_AES_128_SHA256      "ECDHE-RSA-AES128-SHA256"
#define TLS1_TXT_ECDHE_RSA_WITH_AES_256_SHA384      "ECDHE-RSA-AES256-SHA384"
#define TLS1_TXT_ECDHE_RSA_WITH_AES_128_GCM_SHA256      "ECDHE-RSA-AES128-GCM-SHA256"
#define TLS1_TXT_ECDHE_RSA_WITH_AES_256_GCM_SHA384      "ECDHE-RSA-AES256-GCM-SHA384"

As implicit in that command line, I'm excluding the SSL2 protocol and all export ciphersuites; I'm excluding the CAMELLIA, SEED, RC4, single-DES, and NULL ciphers; and I'm requiring ephemeral keys for forward secrecy. (I tried this with openssl master and openssl 1.0.2, and got the same lists.)

So our degrees of freedom are: AES vs 3DES, ECDHE vs DHE, GCM vs CBC, and SHA256 vs SHA384 vs SHA1. We also need to order those by priority.

comment:2 Changed 6 years ago by nickm

So, obviously:

  • AES is better than 3DES.
  • ECDHE is stronger / faster than DHE.
  • GCM is faster than CBC-SHA, and lets us avoid all the ugly "mac-then-encryot"-related CBC idiocy, so it may be stronger too.
  • SHA384 is faster than SHA256 on 64-bit platforms. SHA1 is weaker than the other two, though probably not yet in a way that matters for TLS.

I suggest that we prioritize those ciphersuites in that order too. So all AES before any 3DES. Within a given cipher, all ECDHE before any DHE. Within a cipher-group combination, GCM before CBC-SHA. And last, prefer SHA384 over SHA256 over SHA1.

comment:3 Changed 6 years ago by nickm

Oh, the above didn't rank AES256 vs AES128. I claim that the weaknesses in the AES256 key schedule don't matter in practice, so let's prefer AES256. Though rational people might decide that we don't need so many rounds, and a 256 bit key doesn't help. I think that the key length is less important than any of the other issues.

I've written a python program to do the sorting for me, since there's a bad track record for building these things by hand (#11498); attaching it here.

Changed 6 years ago by nickm

Attachment: gen_server_ciphers.py added

Changed 6 years ago by cypherpunks

Attachment: gen_server_ciphers.patch added

Patch

comment:4 Changed 6 years ago by cypherpunks

Patch for gen_server_ciphers.py​ attached.

comment:5 Changed 6 years ago by nickm

Found another bug too:

#ifdef TLS1_TXT_ECDHE_RSA_WITH_AES_128_GCM_SHA256
       TLS1_TXT_ECDHE_RSA_WITH_AES_128_GCM_SHA256
#endif

There's a missing colon there, so it will get squished together with TLS1_TXT_DHE_RSA_WITH_AES_256_SHA .

comment:6 Changed 6 years ago by nickm

There are lots more bugs in the script I posted; I'll have a new one soon.

comment:7 Changed 6 years ago by nickm

Status: newneeds_review

Patch for all these issues, plus revised script, in branch "bug11513_024" in my public repository.

(It's a good day when I can write code to write code.)

comment:8 Changed 6 years ago by isis

I reviewed nickm's src/common/gen_server_ciphers.py script in commit bd3db82906a2efcd678b5f4b61fef26c93828777.

Looks good to me. I had a couple slight "wait-what-are-you-doing-with-that-list-indexing-and-those-dictionary-lookups-in-that-generator" doubletakes, but overall the code is easy to read and neatly organised. I tested and it and seems to sort correctly on my 1.0.2-stable and 1.0.1g OpenSSLs, according to the above parameters. (Also, I agree with the sorting rationale.)

comment:9 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final

Thanks for the review, Isis! (And thanks to the other folks who've read this over for me.) I'm merging it into 0.2.5 and marking for possible 0.2.4 backport.

comment:10 Changed 6 years ago by cypherpunks

By default server follows client's preference. It depends SSL_OP_CIPHER_SERVER_PREFERENCE option.
Is it worth to prevent any possible client's insecure choice or to allow client to chose it's own destiny? (if something wrong with one of cipher then client's software would be updated faster)

Either way, server's cipher list should be ordered for clarity, just in case and for future.

comment:11 Changed 6 years ago by nickm

Good point; opened #11528 for that.

comment:12 Changed 6 years ago by isis

Cc: isis@… added

comment:13 Changed 6 years ago by nickm

Recommendation: backport along with other TLS ciphersuite improvements. Anything that can ever make users stick on DHE1024 instead of ECDHE is a security bug we should fix IMO

comment:14 Changed 6 years ago by nickm

Keywords: nickm-backport-02422 added

Adding a tag for tickets I think we should backport into 0.2.4.22. Omitting ones where I said "unsure"

comment:15 Changed 6 years ago by nickm

Current status: these involve very little change to our actual code, and have potential to improve security a great deal, while making more invasive backports (like #9386) less urgent. I am planning to merge them in 0.2.4.22

comment:16 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged into maint-0.2.4 for inclusion in 0.2.4.22

Note: See TracTickets for help on using tickets.