qt: Force TLS1.0+ for SSL connections

laanwj commented at 6:11 pm on July 6, 2015: member

I recently got a notification that my suggestion for a flag to force TLS has been implemented and will be in Qt 5.5.0: https://bugreports.qt.io/browse/QTBUG-43168

Because of the POODLE attack it is recommended to disable SSLv3 (https://disablessl3.com/).

So on Qt 5.5.0+ set the flag in the default QSslConfiguration (used for payment requests) to require SSL protocols TLS1.0+.

qt: Force TLS1.0+ for SSL connections 15e26a6a9a

laanwj added the label GUI on Jul 6, 2015

jonasschnelli commented at 6:54 pm on July 6, 2015: contributor

Concept ACK.

Diapolo commented at 5:53 am on July 7, 2015: none

Compiles fine, concept ACK and tested ACK. Feels saver than to rely on default values!

I also verified via Wireshark by connecting with https://bitcoincore.org/~gavin/createpaymentrequest.php and these are the results:

Forcing SSL3: ssl3

Using your patch (but also default on Win81 x64 with Qt5.5 64-bit): tls1_or_later_and_default

Diapolo commented at 5:53 am on July 7, 2015: none

While you are on this, IMHO we still have SSL support for RPC and the cipher string lists SSL3 AFAIK.

laanwj commented at 7:40 am on July 7, 2015: member

Thanks for the extensive testing, @Diapolo!

With regard to SSL in RPC, this was already solved in #5434:

0rpc_ssl_context->set_options(ssl::context::no_sslv2 | ssl::context::no_sslv3);

Diapolo commented at 7:48 am on July 7, 2015: none

No problem :), was fun trying it.

I was talking about that line string strCiphers = GetArg("-rpcsslciphers", "TLSv1.2+HIGH:TLSv1+HIGH:!SSLv2:!aNULL:!eNULL:!3DES:@STRENGTH"); is this missing a !SSLv3?

laanwj commented at 7:51 am on July 7, 2015: member

@Diapolo As far as I know it’s not necessary to change anything to the cipher list, and may be even harmful. The vulnerability is in the protocol itself, not in any of the ciphers introduced with SSLv3.

jwilkins commented at 11:32 am on July 8, 2015: none

@laanwj any reason not to use just TLSv1.2? To quote AGL: “This seems like a good moment to reiterate that everything less than TLS 1.2 with an AEAD cipher suite is cryptographically broken. " - https://www.imperialviolet.org/2014/12/08/poodleagain.html

Something like TLSv1.2 with ‘AES256+EECDH:AES256+EDH:AES128+EECDH:AES128+EDH:!SHA:!RC4:!DSS:@STRENGTH’ (which expands to ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES256-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES128-SHA256)

laanwj commented at 11:44 am on July 8, 2015: member

@jwilkins Sounds sensible. Although we also need to be careful not to put the bar too high, otherwise payments requests don’t work anymore with some servers. At least “drop SSLv3” has been embraced by everyone.

Diapolo commented at 1:57 pm on July 8, 2015: none

I’d also prefer TLS1.2 and would argue, that all BTC sites using payment requests should use strong or best encryption. But as @laanwj sais this breaks some “legacy” clients or perhaps some badly configured payment request service providers. Another problem could be a bad or too technical warning that Bitcoin Core is showing to a user, if the TLS1.2 connection fails.

Perhaps let’s bump the requirement every major new version until we reach TLS1.2. Stick with 1.0 for know, use 1.1 with the next release and perhaps 1.2 with the next major release or something like that? It should of course be mentioned in the release notes and it should be looked at BIP70, if there is a definition that prevents using highest encryption levels.

Also I remember a discussion about removing SSL support for rpc, what was the reason to not do that or is it just not removed?

Edit: I’d also like to ping you as I know I’ve got 2 open payment request related pulls ^^. Sorry I know your time is rare…

sipa commented at 4:24 pm on July 9, 2015: member

utACK

laanwj merged this on Jul 10, 2015

laanwj closed this on Jul 10, 2015

laanwj referenced this in commit 708037fcc7 on Jul 10, 2015

laanwj commented at 6:21 am on July 24, 2015: member

Backported to 0.11 branch as 8e5a96908a91131c35fcb119fce8831ec80c61c1

laanwj referenced this in commit 8e5a96908a on Jul 24, 2015

MarcoFalke locked this on Sep 8, 2021

qt: Force TLS1.0+ for SSL connections #6384