Opened 6 years ago

Closed 6 years ago

#11528 closed defect (fixed)

Consider using ​SSL_OP_CIPHER_SERVER_PREFERENCE

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

Description

With #11513, we gave the servers a reasonable set of ciphers to allow. On that ticket, cypherpunks notes:

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.

So to be clear, my understanding is that the algorithm is to take the intersection of the client's list and the server's list, and then pick the item in the intersection that appeared first on the client's order (by default) or the item in the intersection that appeared first on the server's list (if SSL_OP_CIPHER_SERVER_PREFERENCE is set on the server).

Which way shall we do it?

Child Tickets

Change History (11)

comment:1 Changed 6 years ago by nickm

Here are the ciphers that appear on the current client and server lists, sorted by client preference order:

   XCIPHER(0xc02f, TLS1_TXT_ECDHE_RSA_WITH_AES_128_GCM_SHA256)
   XCIPHER(0xc013, TLS1_TXT_ECDHE_RSA_WITH_AES_128_CBC_SHA)
   XCIPHER(0xc014, TLS1_TXT_ECDHE_RSA_WITH_AES_256_CBC_SHA)
   XCIPHER(0xc012, TLS1_TXT_ECDHE_RSA_WITH_DES_192_CBC3_SHA)
   XCIPHER(0x0033, TLS1_TXT_DHE_RSA_WITH_AES_128_SHA)
   XCIPHER(0x0039, TLS1_TXT_DHE_RSA_WITH_AES_256_SHA)
   XCIPHER(0x0016, SSL3_TXT_EDH_RSA_DES_192_CBC3_SHA)

Here are the ciphers that appear on the old client and current server lists, sorted by client preference order:

   XCIPHER(0xc014, TLS1_TXT_ECDHE_RSA_WITH_AES_256_CBC_SHA)
   XCIPHER(0x0039, TLS1_TXT_DHE_RSA_WITH_AES_256_SHA)
   XCIPHER(0xc013, TLS1_TXT_ECDHE_RSA_WITH_AES_128_CBC_SHA)
   XCIPHER(0x0033, TLS1_TXT_DHE_RSA_WITH_AES_128_SHA)
   XCIPHER(0xc012, TLS1_TXT_ECDHE_RSA_WITH_DES_192_CBC3_SHA)
   XCIPHER(0x0016, SSL3_TXT_EDH_RSA_DES_192_CBC3_SHA)

(I'm only considering the new server list from #11513, since we wouldn't merge a patch for this to any series without also merging #11513 .)

comment:2 Changed 6 years ago by nickm

(The "new" and "old" client ciphers lists above are the ones before and after #11438)

comment:3 Changed 6 years ago by nickm

Keywords: nickm-review-0254 added

comment:4 Changed 6 years ago by nickm

Status: newneeds_review

After discussion, I think that the right move here is to apply this flag. The new server cipher list fits our needs much better than the old client list or even the new one.

Trivial patch in bug11528_024. (Yes, I have confirmed that every OpenSSL we support has SSL_OP_CIPHER_SERVER_PREFERENCE)

comment:5 Changed 6 years ago by nickm

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

Merging to master, marking for 0.2.4 backport

comment:6 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:7 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:8 Changed 6 years ago by arma

I don't see a bug11528_024 branch from you?

comment:9 Changed 6 years ago by nickm

Oops, should be ticket11528_024.

comment:10 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:11 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.