Opened 11 months ago

Closed 9 months ago

#6801 enhancement closed fixed (fixed)

Set OP_NO_COMPRESSION & OP_CIPHER_SERVER_PREFERENCE on TLS contexts

Reported by: hynek Owned by: hynek
Priority: normal Milestone:
Component: core Keywords:
Cc: tobias.oberstein@… Branch: branches/harden-tls-options-6801
(diff, github, buildbot, log)
Author: hynek Launchpad Bug:

Description

There is a broad consensus that there are two options that should always be set:

  • OP_NO_COMPRESSION (CRIME attacks, seems like most OpenSSLs set that as default nowadays but still)
  • OP_CIPHER_SERVER_PREFERENCE (if we push for secure ciphers, we should make sure they get used too)

Both are missing in pyOpenSSL at the moment but we can easily wing it for now.

Confusingly, OP_NO_COMPRESSION is documented in my self-built pyOpenSSL docs ( http://glui.me/?i=3b37w0j9wemcrsl/2013-10-25_at_14.30.png – maybe I’ve built them from a bzr checkout? been a while) and my OpenSSL should have support for it but I can’t find it neither in the official docs at http://pythonhosted.org/pyOpenSSL/openssl-ssl.html nor in the actual library. In any case we’d have to take into account that the underlying (Py)OpenSSL may not support either.

Implementation is trivial, I’d just wait for #6772 to land in trunk because it’s laying groundwork for setting options.

Change History (6)

comment:1 Changed 11 months ago by oberstet

  • Cc tobias.oberstein@… added

Probably these 2 should be set also:

SSL.OP_SINGLE_ECDH_USE = 0x00080000L
SSL.OP_SINGLE_DH_USE = 0x00100000L

comment:2 Changed 9 months ago by hynek

  • Author set to hynek
  • Branch set to branches/harden-tls-options-6801

(In [41219]) Branching to harden-tls-options-6801.

comment:3 Changed 9 months ago by hynek

  • Keywords review added

I’ve done the following:

  1. Consolidated options handling by baking them into self._options on __init__ to avoid repeated calls to set_options in _makeContext.
  2. Set OP_NO_COMPRESSION and OP_CIPHER_SERVER_PREFERENCE unconditionally.
  3. Bonus: Added tests that OP_SINGLE_DH_USE is set if enableSingleUseKeys is active and also added OP_SINGLE_ECDH_USEin the same case so we don't forget it once we actually add ECDH support.

Buildbots are green, please let me know what you think.

comment:4 Changed 9 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:5 Changed 9 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to hynek
  • Status changed from assigned to new
  1. The docstring for test_singleUseKeys says what the test is doing rather than what the implementation is expected to do (though it does so without using the word should for which I guess you get a few points :).
  2. The news fragment says "and makes servers choose the cipher". I think this might be a little ambiguous wrt CertificateOptions usage for clients. I think that OP_CIPHER_SERVER_PREFERENCE is a no-op on clients? So maybe "and, for servers, uses server preference to choose the cipher." That's a little awkward due to the repeated word "server" but maybe it's an improvement anyway; I'll leave it up to you.

Otherwise looks great, please address those issues and merge. Thanks a lot!

comment:6 Changed 9 months ago by hynek

  • Resolution set to fixed
  • Status changed from new to closed

(In [41286]) Merge harden-tls-options-6801

Author: hynek
Reviewer: exarkun
Fixes: #6801

twisted.internet.ssl.CertificateOptions now disables TLS compression to avoid
CRIME attacks and, for servers, uses server preference to choose the cipher.

Note: See TracTickets for help on using tickets.