Murmur default TLS configuration potentially vulnerable to TLS issues

[nta]

Apparently Murmur (both on Linux as well as the binary shipped on Windows -- including the latest published 1.3.0 snapshot) only accepts connections over the TCP control channel using TLS v1.0, and while doing so does not default to the (weak, and therefore not even recommended as a workaround) RC4 cipher.

This means it might (if the application protocol doesn't prevent the attack somehow - I'm unsure of exact details) be vulnerable to the fairly old BEAST attack using an issue in TLSv1.0.

[mkrautz]

We can do TLS 1.2 now that we're on Qt 5. This hasn't been done yet, because we're still in our hybrid Qt 4 / Qt 5 build state.

Ideally, we should allow TLS 1.2 when using Qt 5.

We should probably just do it in an #if QT_VERSION >= 0x050000 block for now.

[hacst]

To be pedantic the BEAST attack doesn't effect us because we don't speak HTTP ;) On a more serious note I also don't think the underpinning crypto issue would create a vulnerability for Mumble. From what I understand it relies on extracting a secret using a chosen plaintext attack by making you repeatedly send the secret together with an appended plaintext the attacker chooses. While getting chosen plaintext sent to a client in Mumble is trivial once you are connected to a server, we simply don't have anything in there you could trigger to be sent repeatedly with the chosen plaintext you'd be interested in compromising. AFAIK this issue does not expose the actual encryption key in any way so other data sent over the session remains safe. I'm not sure if the versions of OpenSSL we are working with under the hood are vulnerable even with TLSv1.0 .Unless they have SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS (protects against this specific issue) enabled from somewhere. Nonetheless we should strive to keep up to date with the crypto protocols. I'm not exacttly a cryptographer...

@mkrautz If you specify differing versions for that won't we loose compatibility? I don't think we can give it a custom protocol selection it can use.

[mkrautz]

@hacst, I agree with your analysis.

I took a look at this yesterday, and it's worth noting that Qt seems to set the SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS flag. But the Qt documentation is, in my opinion, very unclear on what's actually happening. It's quite confusing:

QSsl::SslOptionDisableEmptyFragments -- "Disables the insertion of empty fragments into the data when using block ciphers. When enabled, this prevents some attacks (such as the BEAST attack), however it is incompatible with some servers."

And below the table: "By default, SslOptionDisableEmptyFragments is turned on since this causes problems with a large number of servers. [...]"

I think the SslOptionDisableEmptyFragments text tries to say that when the SslOptionDisableEmptyFragments flag IS NOT SET, empty fragments will be present (BEAST mitigation). However, the text says "when enabled, this prevents some attacks (such as the BEAST attack)", and goes on to say, below the table that SslOptionDisableEmptyFragments is "turned on". I think that means that the SslOptionDisableEmptyFragments is set, causing the empty fragment hack (BEAST mitigation) to be DISABLED.

...But I believe that the wording used can very easily be misunderstood.

Unrelated, but also confusing. The text below the table goes on to say: "SslOptionDisableLegacyRenegotiation is also turned on, since it introduces a security risk.". I think it tries to say that legacy renegotiation is disabled because it is a security risk. But I think it too can easily be misunderstood. :-)

[hacst]

I think with #1677 etc. we are now as good as we can reasonably get with our cipher selection. Anything more we want to do here? Anything more on SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS? Otherwise we should close this.

[Avamander]

Given that TLSv1 and TLSv1.1 are both discouraged to use it would be really good if they were disabled by-default.

[botanegg]

Bump https://arstechnica.com/gadgets/2018/10/browser-vendors-unite-to-end-support-for-20-year-old-tls-1-0/

https://github.com/mumble-voip/mumble/blob/e90eb8c4cd7edc0cd3eddafac622edc45efff794/src/murmur/Server.cpp#L1376-L1384

[Krzmbrzl]

To me it seems as if we should simply use QSsl::SecureProtocols. Quoting from https://doc.qt.io/qt-5/qssl.html#SslProtocol-enum:

The default option, using protocols known to be secure.

I guess this should be good for this case and also shifts the burden of checking which currently is secure to Qt reducing the work for us. (And it would also allow us to remove this preprocessor-construct entirely)

What do you think?

[botanegg]

Currently QSsl::SecureProtocols still TLS 1.0 or later for all ssl backends, but winrt with like 1.1

https://github.com/qt/qtbase/blob/bd793d3c46ec08ff84dd67db467def8578922697/dist/changes-5.11.0#L199-L204

Anyone knows, may openssl possible to disable old tls version by self?

[Avamander]

Currently QSsl::SecureProtocols still TLS 1.0 or later for all ssl backends, but winrt with like 1.1 Anyone knows, may openssl possible to disable old tls version by self?

Right now it should be minimum TLSv1.2 IMHO.

It could be achieved by disallowing bad ciphersuites. HIGH:!MEDIUM:!LOW on any reasonable OpenSSL build gives a fine list of ciphers and should remain more up-to-date without manual intervention.

[Krzmbrzl]

@Avamander could you elaborate on this, please? How can this be integrated in the code?

[streaps]

Anyone knows, may openssl possible to disable old tls version by self?

You can set some defaults in openssl.cnf, but this doesn't seem to have any influence on QtSsl.

QSsl::TlsV1_2OrLater does prevent <TLS 1.2 connections.

To me it seems as if we should simply use QSsl::SecureProtocols

I'm not sure how accurate the Qt documentation is, but the description of QSsl::SecureProtocols changed only recently in Qt 5.14 (which also removed support for SSLv3):

The default option, using protocols known to be secure. https://doc.qt.io/qt-5.14/qssl.html

The default option, using protocols known to be secure; currently behaves similar to TlsV1Ssl3 except denying SSLv3 connections that does not upgrade to TLS. https://doc.qt.io/qt-5.12/qssl.html

Are there clients that use Qt 4.8 out there? According to the documentation it seems QtSsl in 4.8 supports TLS 1.0 and lower only. https://doc.qt.io/archives/qt-4.8/qssl.html

[streaps]

The whole #if block looks weird, but here are some explanations:

https://github.com/mumble-voip/mumble/commit/71e522f4c4130c726251f54e404b500e1ebe46b4

If that is correct, the Qt documentation is also wrong.

[streaps]

This idea is also interesting (quote from the same commit https://github.com/mumble-voip/mumble/commit/71e522f4c4130c726251f54e404b500e1ebe46b4):

If something comes up in the future that makes us abandon TLSv1.0, we'll still handshake it. Rejection can happen at the Mumble protocol level instead, providing a better user experience.

If there were a tlsversion config parameter in murmur.ini, an admin could set the minimal TLS version and users would get a message that explains why they are not able to connect.

[streaps]

Since Qt 5.11:

Make QSsl::SecureProtocols also enable use of TLS1.{1,2}

Previously it was only enabling use of TLS1.0, unlike our openssl backend, which understandably caused some confusion among some of our users. https://github.com/qt/qtbase/commit/4a56d86ee43bd6be23f70ef2d29ba4b4ebb17029

Since Qt 5.4:

Update QSsl::SecureProtocols to not include Sslv3

After the poodle vulnerability SSLv3 should like SSLv2 no longer be considered safe, so when a user request a safe protocol we should only allow TLS versions. https://github.com/qt/qtbase/commit/3fd2d9eff8c1f948306ee5fbfe364ccded1c4b84

[Krzmbrzl]

Furthermore the linked commit states

The current Mumble protocol requires TLS 1.0.

so dropping it would also require a protocol change.

Thus I think adding a server-config option to set the minimum TLS version is actually a good idea :point_up:

[streaps]

The current Mumble protocol requires TLS 1.0.

The Mumble protocol itself doesn't require TLS 1.0, does it? It seems to work fine with every TLS version the client and server agree on.

 $ openssl s_client -tls1_3 localhost:64738
CONNECTED(00000003)
depth=0 CN = Murmur Autogenerated Certificate v2
[...]
SSL-Session:
    Protocol  : TLSv1.3
    Cipher    : TLS_AES_256_GCM_SHA384
[...]
---
read R BLOCK
��1.3.1�X11"Linux 5.4.43-1-lts

Of course, if you take the documentation literaly, then the protocol requires it, ...

"The TCP control channel uses TLSv1 AES256-SHA"

... but then every connection that uses TLS 1.1, 1.2 or 1.3 would violate the protocol.

[Avamander]

@Krzmbrzl

could you elaborate on this, please? How can this be integrated in the code?

https://bugreports.qt.io/browse/QTBUG-36272?page=com.atlassian.streams.streams-jira-plugin%3Aactivity-stream-issue-tab

There's an QT issue open about this and there's also one possible solution in the comments. Only allowing PFS ciphers using the solution mentioned will probably effectively require newer TLS versions.

[Krzmbrzl]

Are there clients that use Qt 4.8 out there? According to the documentation it seems QtSsl in 4.8 supports TLS 1.0 and lower only.

I just re-read the conversation here and stumbled upon your comment. We have dropped support for Qt 4 for the 1.4.0 release anyways so that doesn't restrict us.

[davidebeatrici]

This issue has been fixed for a while now.