SECURITY: context_spec.cr specifies "only TLS 1.0" by calling tlsv1_method. OpenSSL says avoid tlsv1_method. TLS 1.0 and 1.1 are deprecated.

x448 commented 4 years ago

A. OpenSSL docs tell us to avoid calling tlsv1_method but crystal is calling it.

B. Calling tlsv1_method means a TLS/SSL connection will only understand TLS 1.0.

C. Current industry recommendation is to use least TLS 1.2:

June 30, 2018 deadline to drop TLS 1.0 - The PCI Council (Payment Card Industry) suggested that organizations migrate from TLS 1.0 to TLS 1.1 or higher before June 30, 2018.
March 2020 deadline to deprecate TLS 1.1 - In October 2018, Apple, Google, Microsoft, and Mozilla jointly announced they would deprecate TLS 1.0 and 1.1 in March 2020.

D. See Background (provided below) for timeline and some attacks on TLS for more context.

OpenSSL docs

OpenSSL 1.1.0 says use TLS_method__ and avoid TLSv1_method, TLSv1_1_method, etc. OpenSSL 1.0.2 says use SSLv23_method__ and avoid TLSv1_method, TLSv1_1_method, etc.

(click to expand) Quote from docs and links

>TLS_method(), TLS_server_method(), TLS_client_method() > >These are the general-purpose version-flexible SSL/TLS methods. The actual protocol version used will be negotiated to the highest version mutually supported by the client and the server. The supported protocols are SSLv3, TLSv1, TLSv1.1 and TLSv1.2. Applications should use these methods, and avoid the version-specific methods described below. > ... > TLSv1_2_method(), TLSv1_2_server_method(), TLSv1_2_client_method() > ... > TLSv1_1_method(), TLSv1_1_server_method(), TLSv1_1_client_method() > ... > TLSv1_method(), TLSv1_server_method(), TLSv1_client_method() > A TLS/SSL connection established with these methods will only understand the TLSv1 protocol. * https://www.openssl.org/docs/man1.1.0/man3/TLSv1_method.html * https://www.openssl.org/docs/man1.0.2/man3/TLSv1_method.html

Relevant code in crystal

https://github.com/crystal-lang/crystal/blob/fd0780c3a4ef972a6090e09abc9b09a0e39345ff/spec/std/openssl/ssl/context_spec.cr#L31

https://github.com/crystal-lang/crystal/blob/fd0780c3a4ef972a6090e09abc9b09a0e39345ff/spec/std/openssl/ssl/context_spec.cr#L48

Additionally, TLS 1.0 and TLS 1.1 should be disabled from being chosen during auto-negotiation. See Background (provided below).

Background

1999. TLS 1.0 was first defined in RFC 2246 in January 1999.

2006. TLS 1.1 was defined in RFC 4346 in April 2006.

2008. TLS 1.2 was defined in RFC 5246 in August 2008.

2014. TLS 1.0 allows downgrading the connection to SSL 3.0, thus weakening security (POODLE SSL Variant). Source:
https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_1.0

2014. TLS 1.0, TLS 1.1, and TLS 1.2 (if not implemented properly) are vulnerable to POODLE TLS Variant even if SSLv3 is disabled. Source:
https://en.wikipedia.org/wiki/POODLE

2017. Google Chrome set TLS 1.3 as the default version for a short time in 2017. It then removed it as the default, due to incompatible middleboxes such as Blue Coat web proxies. Source:
https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_1.3

2018. TLS 1.3 was defined in RFC 8446 in August 2018. It mandates use of AEAD ciphers, key exchanges that offer perfect forward secrecy, integrates session hash, and drops support for many insecure or obsolete features. Source:
https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_1.3

TLS Interception Appliances

POODLE from 2014 is not the only security issue to consider, see this paper:

To analyze TLS-encrypted data, network appliances implement a Man-in-the-Middle TLS proxy, by acting as the intended web server to a requesting client (e.g., a browser), and acting as the client to the actual/outside web server. Source:
The Sorry State of TLS Security in Enterprise - Interception Appliances (PDF, arxiv.org)

ysbaddaden commented 4 years ago

We follow the Mozilla recommendations. But sadly, we didn't keep them updated since. It's been stuck on 2016 recommended settings, which are really outdated. We should indeed disable TLS 1.0 and TLS 1.1, add/remove some ciphers, and a few more things.

See https://wiki.mozilla.org/Security/Server_Side_TLS

x448 commented 4 years ago

Thanks for confirming and taking time to share what happened.

I'm too new to crystal (still evaluating whether to install) -- otherwise, I'd ask to submit PR because context_spec.cr seems well-organized and easy to maintain.

BTW, crystal's high productivity syntax familiar to rubyists + aot compile to llvm + concurrency with channels & fibers. Great combo!

straight-shoota commented 4 years ago

Thanks for the detailed report @x448 . However, I don't think this is actually much of a security concern. tlsv1_method is only referenced in test code. It's just there to make sure the Crystal bindings can be used with a specific versioned method.

The actual library code uses the recommended TLS_method/SSLv23_method as default:

https://github.com/crystal-lang/crystal/blob/b3a3e1b94bd34d798bbdf47faee422571f89a066/src/openssl/ssl/context.cr#L5-L11

However, as per @ysbaddaden we should updated and validate the default configuration (such as supported ciphers) to the current standards.

ysbaddaden commented 4 years ago

@straight-shoota OpenSSL has insecure defaults.

For example in OpenSSL 1.1.0, by using TLS_method(3) we enable SSLv3, TLSv1, TLSv1.1 and TLSv1.2 by default, of which we only disable SSLv3. It was acceptable in 2016 but that's no longer recommended since 2018. OpenSSL 1.1.1 TLS_method(3) will also enable TLSv1.3 in addition to everything else (down to SSLv3).

Using TLS_method(3) in 2020 is still recommended by OpenSSL, but we must set the SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1 options by default. Similar to how we disable SSv2 and SSLv3. One may then reenable TLSv1 and TLSv1.1 if they really need to, for example to support IE10 and below.

Same for ciphers, the default acceptable list is likely outdated.

straight-shoota commented 4 years ago

Totally agree. My comment "not a security issue" was target towards calling tlsv1_method. I've updated the ciphers and methods in #9026

x448 commented 4 years ago

@straight-shoota thanks for starting PR #9026 and including SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1, and even updating the ciphersuite, etc. Also, holy shit that was fast!

I reported a similar issue about TLS with nim-lang/Nim this week so most of the issue text was reused. Nim was using tls_method but not yet disabling obsolete TLS versions.

straight-shoota commented 4 years ago

I discovered another issue with the OpenSSL API. We use SSL_CTX_set_cipher_list to configure the available ciphers. But this function only works for TLSv1.2 ciphers and below it does not apply to TLSv1.3 ciphersuites. So currently, it is not possible to configure TLSv1.3 ciphersuites, OpenSSL will always use the default (which seems to be mostly identical to the compatibility recommendations, so there are currently only minimal practical effects). The new function for TLSv1.3 is SSL_CTX_set_ciphersuites, but it's obviously only available on OpenSSL 1.1.1. The value format is almost identical to the cipher list, it's just separated by colons instead of white space.

Regarding the Crystal API it is probably necessary to copy that behaviour and have two separate methods ciphers= and ciphersuites=.

Also, I wonder why there's an explicit list of disabled ciphers in the default config (!RC4 !aNULL !eNULL !LOW !3DES !MD5 !EXP !PSK !SRP !DSS). As far as I understand, OpenSSL wouldn't use these ciphers anyway, unless they were explicitly enabled. So it seems quite unnecessary noise to specify them.

RX14 commented 4 years ago

Regarding the Crystal API it is probably necessary to copy that behaviour and have two separate methods ciphers= and ciphersuites=.

please no... let's just write TLS already and make a sane API...

crystal-lang / crystal