Opened 3 years ago

Closed 7 months ago

#5514 enhancement closed fixed (fixed)

ssl web server allows insecure single-DES

Reported by: zooko Owned by: zooko
Priority: normal Milestone:
Component: core Keywords: security
Cc: davidsarah Branch:
Author: Launchpad Bug:

Description

This service to analyze your SSL server for you:

https://www.ssllabs.com/ssldb/analyze.html?d=twistedmatrix.com

says:

Cipher Suites (sorted by strength; we could not determine if server has a preference)
TLS_RSA_WITH_DES_CBC_SHA (0x9)   WEAK 	56
TLS_RSA_WITH_RC4_128_MD5 (0x4) 	128
TLS_RSA_WITH_RC4_128_SHA (0x5) 	128
TLS_RSA_WITH_AES_128_CBC_SHA (0x2f) 	128
TLS_RSA_WITH_3DES_EDE_CBC_SHA (0xa) 	168
TLS_RSA_WITH_AES_256_CBC_SHA (0x35) 	256

That means that the server will go ahead and do single-DES with a 56-bit key as its cipher. One particular reason not to allow single-DES is the possibility of "downgrade attacks" where the server prefers a strong cipher, and the client prefers a strong cipher but an attacker tricks the two of them into agreeing on a weak cipher. Better not to allow weak ciphers at all.

I looked for an API to control the cipher selection, but didn't find one. I looked in documents/current/api/twisted.internet.ssl.CertificateOptions.html, for example.

Attachments (1)

no-3des.patch (669 bytes) - added by exarkun 19 months ago.
a patch which turns off 3DES ciphers in twisted.protocols.tls

Download all attachments as: .zip

Change History (14)

comment:1 Changed 3 years ago by davidsarah

  • Cc davidsarah added
  • Keywords security added

Although SSL3 and TLS are supposed to be resistant to downgrade attacks, that's one of the areas of the protocol that is least well analysed and potentially most vulnerable to implementation weaknesses. I strongly agree with removing single DES; no recent client should be dependent on it.

comment:2 Changed 3 years ago by exarkun

  • Owner set to zooko

This is... bad? I guess?

I think this ticket needs some clarification, though.

Is the problem that the twistedmatrix.com website itself is deployed with this cipher enabled?

Or is the problem that twistd web will start up a server with this cipher enabled at all?

Or is the problem that CertificateOptions will create a context that has this cipher enabled?

Or is the problem something else?

Thanks!

comment:3 Changed 3 years ago by zooko

The problem is that I deployed an ssl-enabled web site on Twisted without changing the defaults, and my web site allows 56-bit keys:

https://www.ssllabs.com/ssldb/analyze.html?d=leastauthority.com

It identifies itself as "TwistedWeb/10.0.0". Here's the relevant code that we used to configure and launch it:

sslfactory = ssl.DefaultOpenSSLContextFactory(KEYFILE, CERTFILE)
reactor.listenSSL(port, site, sslfactory)

We did patch it to load cert chains, as described in #2061.

comment:4 Changed 3 years ago by exarkun

Thanks. I wonder if you could help me out a little bit more, too.

Should this be a Twisted issue, rather than an OpenSSL issue? OpenSSL is providing the TLS implementation, including the default ciphers list. Can we assume that OpenSSL should know best about good security settings? Why does OpenSSL ship with TLS_RSA_WITH_DES_CBC_SHA enabled by default?

I am all in favor of having secure Twisted-based applications, but this seems like either a deployment configuration issue, in which case it needs to be dealt with by the application using Twisted, or a TLS implementation issue, in which case it needs to be dealt with in OpenSSL.

Twisted appears to sit in between these two layers - not quite low enough to be making decisions about TLS protocol details like the default ciphers list, but not quite high enough to know what's appropriate for a particular application.

comment:5 Changed 2 years ago by davidsarah

OpenSSL is in general not as strict about security configuration as Twisted probably wants to be by default (different user communities, with different priorities for compatibility with very old browser clients). If there were an exposed Twisted API to make it more strict, that might not be a problem, but there isn't.

comment:6 Changed 2 years ago by exarkun

I have difficulty even finding an API in OpenSSL for this. SSL_CTX_set_cipher_list lets the ciphers a connection created with a particular context will allow, but there is no corresponding SSL_CTX_get_cipher_list. This seems to mean there is no way to use SSL_CTX_set_cipher_list to only change the configuration to disallow a particular cipher.

There is SSL_get_cipher_list (and SSL_get_ciphers - determining the difference is left as an exercise for the reader) as well as SSL_set_cipher_list. Perhaps this could be used to change the allowed ciphers only enough to forbid single-DES. However, it operates on a connection, not a context. This probably means it needs to be used as soon as the SSL* is created, but before the handshake is performed. I think that, as a historical accident, it is possible to achieve this timing by issuing the call in Protocol.makeConnection (should we leave it to the application).

Perhaps better would be to invent our own expanded context interface. This could support forbidding particular ciphers, and our own TLS layer could talk to it. I'm not quite sure what the details of the implementation for this look like, as OpenSSL still only gives us SSL_set_cipher_list. Perhaps it could be workable to interrogate the cipher list before the handshake, give the context an opportunity to prioritize and remove elements from it, and then set the result back on the connection before proceeding with the handshake.

Presently the assumption that the context object is a pyOpenSSL OpenSSL.SSL.Context is probably scattered rather liberally throughout Twisted (and potentially applications using Twisted). Finding some migration path away from that would be a necessary part of this enhancement.

comment:7 Changed 19 months ago by exarkun

Experimentally, this patch fixes the problem, though ties us more closely to the (py)OpenSSL Context API.

Changed 19 months ago by exarkun

a patch which turns off 3DES ciphers in twisted.protocols.tls

comment:8 Changed 19 months ago by exarkun

The condition in the patch is backwards, of course.

comment:9 Changed 16 months ago by zooko

The patch is changing 3DES, but the issue here is about just "DES", which means good old-fashioned single-DES, invented in the early 1970's. No kidding!

3DES is not actually a problem. It is secure! The only reason people don't use 3DES that much anymore is that AES is much faster than 3DES.

But good old-fashioned DES could probably be brute-force cracked by your cellphone.

comment:10 Changed 15 months ago by finncolman

We used to have code like this:
ssl_context_factory = DefaultOpenSSLContextFactory(ssl_private_key_path, ssl_server_cert_path)
client_gateway_svc.addService(internet.SSLServer(ssl_port, client_gateway_svc.factory, ssl_context_factory))

We disabled weak ciphers by doing something like this:
ssl_context_factory = DefaultOpenSSLContextFactory(ssl_private_key_path, ssl_server_cert_path)
ssl_context = ssl_context_factory.getContext()
ssl_context.set_cipher_list('ALL:!aNULL:!eNULL:!LOW:!EXP:RC4+RSA:+HIGH:+MEDIUM')
client_gateway_svc.addService(internet.SSLServer(ssl_port, client_gateway_svc.factory, ssl_context_factory))

This allows you to disable weak ciphers in a similar way to how it is done in Apache config, and doesn't involve any change to the base twisted code. It also only allows 'HIGH' and 'MEDIUM' ciphers, so you don't need to hard code cipher names. You just get the context off the ssl context factory and set the cipher list. It took me ages to discover this, as it isn't clearly documented anywhere that this is how weak ciphers are disabled in twisted.

comment:11 Changed 12 months ago by zooko

For what it is worth, a lot of other people have been talking about this topic, and we seem to be kind of converging.

  • The Qualys ssl analyzer:

https://www.ssllabs.com/ssltest/analyze.html?d=zooko.com

  • Mozilla recommendations:

https://bugzilla.mozilla.org/show_bug.cgi?id=927045

  • LeastAuthority.com:

https://github.com/LeastAuthority/leastauthority.com/issues/92

  • Hynek's web page:

http://hynek.me/articles/hardening-your-web-servers-ssl-ciphers/

comment:12 Changed 12 months ago by zooko

For what it is worth, I've been persuaded by Adam Langley and Nick Matthewson that GCM is hard to implement in a timing-leak-safe way, and I've decided that SHA-1 is just as good as SHA-256 when used in HMAC, so I tweaked my config string to this:

ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:DHE-RSA-AES128-SHA:DHE-ECDSA-AES128-SHA:DHE-RSA-AES128-SHA256:DHE-ECDSA-AES128-SHA256:DHE-RSA-AES128-GCM-SHA256:DHE-ECDSA-AES128-GCM-SHA256:DES-CBC3-SHA

comment:13 Changed 7 months ago by zooko

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

Fixed by [41218], #6663.

Note: See TracTickets for help on using tickets.