qt: Revert "Force TLS1.0+ for SSL connections" #14403

pull real-or-random wants to merge 1 commits into bitcoin:master from real-or-random:sslv3 changing 1 files +0 −8

real-or-random commented at 8:52 AM on October 5, 2018: member

This reverts commit 15e26a6a9afe299b9ca6fced73b876644365879b, whose purpose was to tweak the Qt configuration to force TLS, i.e., to disable SSLv3, in Qt versions >= 5.5. However, the default behavior of Qt >= 5.4 is to disable SSLv3 anyway [1], so the configuration tweak is redundant.

With Qt 5.11.2, the configuration tweak is not only redundant but in fact provokes a deadlock due to a bug in Qt 5.11.2. Since the deadlock occurs at the early startup stage of bitcoin-qt, it renders bitcoin-qt entirely non-functional when compiled against Qt 5.11.2 (and maybe other Qt versions).

Fixes #14359.

[1] https://code.qt.io/cgit/qt/qtbase.git/commit/?id=3fd2d9eff8c1f948306ee5fbfe364ccded1c4b84
fanquake added the label GUI on Oct 5, 2018
fanquake requested review from laanwj on Oct 5, 2018
ken2812221 commented at 4:07 AM on October 6, 2018: contributor

I am not sure it's a good idea to rely on the default behavior. Maybe you could disable these codes on bugged qt versions instead of deleting them all.
fanquake commented at 6:46 AM on October 8, 2018: member

Had a quick discussion with @MarcoFalke, we'll wait for some follow-up on the upstream ticket, now that a minimal example has been made available.
real-or-random commented at 9:14 AM on October 8, 2018: member

It certainly makes sense to wait for upstream. But even if this is fixed upstream, I don't think that this PR should be discarded. At the moment, there is no real difference between the default SecureProtocols and our settings. If a difference appears in the future, it will be due to Qt disabling insecure TLS versions/features in SecureProtocols, so we'll get that change "for free" without any maintenance. Everything else equal, this PR removes 8 lines of code.

Whatever, let's wait for upstream first.
fanquake commented at 4:22 AM on October 9, 2018: member

utACK 592f3e4

Upstream has closed the ticket. Given that any effects should be limited to qt 5.5+, and what we were doing (QSsl::TlsV1_0OrLater) essentially matches the default qt 5.5 behaviour (QSsl::TlsV1SslV3, except denying connections that do not upgrade to TLS), lets merge and fix the GUI on Fedora, Debian Sid and Arch Linux.
MarcoFalke commented at 4:29 AM on October 9, 2018: member

I'd prefer to remove all ssl code (i.e. the payment protocol as well), but let's leave that for later.
laanwj commented at 8:24 AM on October 9, 2018: member

I think relying on upstream is dangerous as long as there is no minimum required and enforced Qt5 version (#13478).

As a requirement for a change like this it shouldn't only be fixed for the Qt version that gitian builds against, but for all supported Qt versions.

I'd prefer to remove all ssl code (i.e. the payment protocol as well), but let's leave that for later.

I tried that once: #11622

in src/qt/bitcoin.cpp:576 in 592f3e4caf outdated

 571 | @@ -573,13 +572,6 @@ int main(int argc, char *argv[])
 572 |  #ifdef Q_OS_MAC
 573 |      QApplication::setAttribute(Qt::AA_DontShowIconsInMenus);
 574 |  #endif
 575 | -#if QT_VERSION >= 0x050500

laanwj commented at 8:25 AM on October 9, 2018:

so concretely: please make this into a range instead of deleting it

real-or-random commented at 9:08 AM on October 9, 2018: member

I think relying on upstream is dangerous as long as there is no minimum required and enforced Qt5 version (#13478).

As a requirement for a change like this it shouldn't only be fixed for the Qt version that gitian builds against, but for all supported Qt versions.

To be honest, I don't understand what you're proposing.

Qt version	<= 5.3	5.4	>= 5.5
Without this PR	unsafe?	safe due to Qt defaults	safe due to our config
With this PR	unsafe?	safe due to Qt defaults	safe due to Qt defaults

So this PR does not ignore any unsupported versions except <= 5.3 but nothing is changed for those old versions. (Maybe we should at least force a specific TLS version for those old Qt versions because that's the only possible thing in the Qt config).

Now I don't understand how a check for a version range (which range?) will help, and also I don't see how a minimum version is related here (except that the minimum version should possibly be 5.4, because we can't fix older versions properly).

Ignoring that discussion, please don't merge yet because I want to update the commit message to include our new knowledge about the deadlock. I'll do that when we know what the code should look like exactly.

laanwj commented at 9:48 AM on October 9, 2018: member

ok…yes in that case the whole thing is redundant for every Qt version, that was unclear to me, thanks

utACK

qt: Revert "Force TLS1.0+ for SSL connections"

This reverts commit 15e26a6a9afe299b9ca6fced73b876644365879b, whose
purpose was to tweak the Qt configuration to force TLS, i.e., to
disable SSLv3, in Qt versions >= 5.5. However, the default behavior
of Qt >= 5.4 is to disable SSLv3 anyway [1], so the configuration
tweak is redundant.

With Qt 5.11.2, the configuration tweak is not only redundant but in
fact provokes a deadlock (#14359) due to Qt 5.11.2 being incompatible
with OpenSSL 1.1.1 [2]. Since the deadlock occurs at the early startup
stage of bitcoin-qt, it renders bitcoin-qt entirely non-functional
when compiled against OpenSSL 1.1.1 and Qt 5.11.2 (and possible future
combinations of OpenSSL and Qt versions).

This commit fixes #14359 by removing the redundant code.

[1] https://code.qt.io/cgit/qt/qtbase.git/commit/?id=3fd2d9eff8c1f948306ee5fbfe364ccded1c4b84
[2] https://bugreports.qt.io/browse/QTBUG-70956

7d173c4cd1

real-or-random force-pushed on Oct 9, 2018
real-or-random commented at 10:05 AM on October 9, 2018: member

Okay, I updated the commit message (and didn't touch the code), ready to merge from my side.
fanquake commented at 12:39 AM on October 10, 2018: member

re-utACK https://github.com/bitcoin/bitcoin/pull/14403/commits/7d173c4cd1885ff5bcf9e5f8f7f712138dd8a445
fanquake added this to the "Mergeable" column in a project

laanwj merged this on Oct 16, 2018
laanwj closed this on Oct 16, 2018
laanwj referenced this in commit 2468471e13 on Oct 16, 2018
meshcollider removed this from the "Mergeable" column in a project

laanwj referenced this in commit 0242b5afa4 on Nov 6, 2018
laanwj referenced this in commit b0e88b8914 on Nov 6, 2018
lynxcoins referenced this in commit 322b61e033 on Nov 7, 2018
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#14359 bitcoin-qt hangs in futex() / __cxa_guard_acquire()#14666 qt: Revert "Force TLS1.0+ for SSL connections" (0.17)