qt5-network openssl3 armhf does not support tls1.3

[msaxl]

[Impact]

Qt 5 Network library does not use TLS 1.3 on armhf, and falls back to less secure protocols.

[Test Plan]

1. Create test.cpp with the following contents:

#include <QtCore/QCoreApplication>
#include <QtCore/QDebug>
#include <QtNetwork/QSslConfiguration>
#include <QtNetwork/QSslSocket>

int main(int argc, char **argv) {
    QCoreApplication app(argc, argv);
    QSslSocket s;
    QSslConfiguration cfg = s.sslConfiguration();
    cfg.setProtocol(QSsl::TlsV1_3OrLater);
    s.setSslConfiguration(cfg);
    s.connectToHostEncrypted("www.ubuntu.com", 443);
    s.waitForConnected();
    qDebug() << s.sessionProtocol();
    return 0;
}

2. Create test.pro with the following contents:

CONFIG += debug warn_all
QT = core network
SOURCES = test.cpp

3. Install qtbase5-dev package.

4. Compile using `qmake && make`.

5. Run the generated ./test executable. It should print 15, not -1.

[Where problems could occur]

It is unlikely to cause issues on 64-bit platforms because long and uint64_t are both 64 bits long. On armhf potential problems may be related to availability of other protocols.

[Original Description]

lsb_release
Description: Ubuntu 22.04 LTS
Release: 22.04

libqt5network5/jammy,now 5.15.3+dfsg-2 armhf
libssl3/jammy-updates,jammy-security,now 3.0.2-0ubuntu1.6 armhf

the qt5 armhf version shipped with ubuntu jammy has a regression in tls1.3 support (simply missing in runtime).

openssl supports tls1.3, so the underlying library works.
x86_64 is obviously not affected
the short sample applications writes -1 on armhf, 15 on x86_64 (unknown protocol vs tls1.3)

        QSslSocket* s = new QSslSocket();
        QSslConfiguration cfg = s->sslConfiguration();
        cfg.setProtocol(QSsl::TlsV1_3OrLater);
        s->setSslConfiguration(cfg);
        s->connectToHostEncrypted("tls13-enabled.server",443);
        s->waitForConnected();
        printf("%d\n",s->sessionProtocol());

marking it as security since the most secure tls protocol is not used on some platforms

[msaxl (saxl)]

i think I have a trace where the issue is:
openssl3 openssl's options is a uint64_t, but in qsslsocket_openssl.cpp the method is defined as
long QSslSocketBackendPrivate::setupOpenSslOptions(QSsl::SslProtocol protocol, QSsl::SslOptions sslOptions)

long on 64bit platforms is 64 bit long, but on armhf (32bit) it is 32bit.

see
https://www.openssl.org/docs/man3.0/man3/SSL_CTX_set_options.html
vs
https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_options.html

is this already fixed in qt6? the qt5.15 openssl3 is a ubuntu backport, right?

[msaxl (saxl)]

this should fix the issue

this however requires openssl3.0, but that should be ok for ubuntu going forward

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "openssl3_set_options.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

[msaxl (saxl)]

actually the first patch was missing something and did not compile

[Dmitry Shachnev (mitya57)]

Thank you for the patch!

Qt 6 still uses unsigned long:
https://code.qt.io/cgit/qt/qtbase.git/tree/src/plugins/tls/openssl/qsslsocket_openssl_symbols.cpp#n126

Can you please submit your patch to codereview.qt-project.org, or at least file a bug at bugreports.qt.io?

We usually don't add patches that were not merged upstream.

[msaxl (saxl)]

https://bugreports.qt.io/browse/QTBUG-105041

this however has priority low.
additionally openssl1.1 and openssl3 are not compatible in this case if libssl is loaded in runtime

for 32bit this is only solvable if compiletime forces openssl version to 3 OR 1.1, but then the corresponding version MUST be loaded or someone implements a version check in runtime. Using the q_SSL_CTX_set_options funcion will not work in this case since the symbol is not unique

[msaxl (saxl)]

just a side node on the findings while hunting down this issue in gdb:

on armhf I think the calling convention is that integers are passed on registers. uint64 is not a (32bit) integer and since the value passed to SSL_CTX_set_options was not related in any way to the value passed in q_SSL_CTX_set_options I think uint64_t are expected to be on the stack. I cannot tell what value is in that place/where it came from, but it ALWAYS had bit29 set. Bit29 means disable tls1.3.

I don't know if i686 has a similar calling convention, but if not and i686 being a little endian architecture, that systems are not affected by this (probably the most important platform being 32bit windows)

[msaxl (saxl)]

@mitya57 the patch is now submitted to codereview. I am however only able to submit to the dev branch (took me a while to get this, never used gerrit before). This also means that the patch I submitted is for qt6. There is no way i send a codereview for qt5 anymore, so I don't know who will do the backport if the qt6 patch gets merged.

[Dmitry Shachnev (mitya57)]

Thank you. Once the patch is accepted upstream, I will backport it to Debian/Ubuntu packaging.

In Ubuntu we don't care about older OpenSSL versions, but upstream Qt does care.

[msaxl (saxl)]

This is my suggested backport of the upstream patch.

since, as you might know, the file locations changed a bit, lso the file defining the new datatype moved from qsslsocket_openssl_symbols_p.h to qsslsocket_openssl_p.h since it is required there (setupOpenSslOptions is defined there, but qsslsocket_openssl.cpp, which includes qsslsocket_openssl_p.h includes qsslsocket_openssl_symbols_p.h too late; this is done differently in qt6 where setupOpenSslOptions is in qsslcontext_openssl.cpp)

[Dmitry Shachnev (mitya57)]

Thank you. I will be offline for a few days, so I will upload this next week.

[Launchpad Janitor (janitor)]

This bug was fixed in the package qtbase-opensource-src - 5.15.4+dfsg-5

---------------
qtbase-opensource-src (5.15.4+dfsg-5) unstable; urgency=medium

  * Add a patch to update signature of SSL_CTX_set_options for OpenSSL 3
    (LP: #1981807). Thanks Michael Saxl!

 -- Dmitry Shachnev <email address hidden> Sun, 07 Aug 2022 16:56:40 +0300

[Dmitry Shachnev (mitya57)]

I used your patch from comment #10 with only one minor change: for old OpenSSL versions I replaced long with unsigned long to match the latest version of upstream patch. But it doesn't matter for Ubuntu anyway.

I am attaching a debdiff for jammy-security and subscribing ~ubuntu-security-sponsors.

[Dmitry Shachnev (mitya57)]

I was not able to test it on Ubuntu because I don't have armhf hardware, but I have just tested it on a Debian porterbox. The only difference between Debian bookworm and sid versions of Qt is presence of this patch.

I used this test code and complied it with qmake && make.

mitya57@harris:~/test$ cat test.pro
CONFIG += debug warn_all
QT = core network
SOURCES = test.cpp

mitya57@harris:~/test$ cat test.cpp
#include <QtCore/QCoreApplication>
#include <QtCore/QDebug>
#include <QtNetwork/QSslConfiguration>
#include <QtNetwork/QSslSocket>

int main(int argc, char **argv) {
    QCoreApplication app(argc, argv);
    QSslSocket s;
    QSslConfiguration cfg = s.sslConfiguration();
    cfg.setProtocol(QSsl::TlsV1_3OrLater);
    s.setSslConfiguration(cfg);
    s.connectToHostEncrypted("www.ubuntu.com", 443);
    s.waitForConnected();
    qDebug() << s.sessionProtocol();
    return 0;
}

Without patch:

(bookworm_armhf-dchroot)mitya57@harris:~/test$ ./test
-1

With patch:

(sid_armhf-dchroot)mitya57@harris:~/test$ ./test
15

[msaxl (saxl)]

I have a version with the last attached patch in my ppa. This version works for me.
Is there a change we get a SRU for this? Who would make that request?

[Dmitry Shachnev (mitya57)]

A few days ago I asked on #ubuntu-security about it and was told that it's better to make a non-security SRU upload for it:
https://irclogs.ubuntu.com/2022/08/22/%23ubuntu-security.html#t12:01

So I uploaded it, and now it's waiting in unapproved queue for a release team member review:
https://launchpad.net/ubuntu/jammy/+queue?queue_state=1

[Chris Halse Rogers (raof)]

The patch looks reasonable (assuming that it doesn't change ABI, which seems to be the case). Could you be able to update the bug with the necessary SRU information (the https://wiki.ubuntu.com/StableReleaseUpdates#SRU_Bug_Template can help here)?

Particularly, the [Test Plan] and [Where problems might occur] parts are important. You've got most of a test-plan there already (but we should have some tests of existing packages to check we don't have a regression and make sure ABI hasn't been broken). I can help there, but you probably have better insight into this code and where it might go wrong than me :). If you'd like help, give me a ping (RAOF in #ubuntu-release:libera.chat).

[Dmitry Shachnev (mitya57)]

Oops, forgot about that. Done.

Also, ABI is not affected. We have symbols to track ABI, and there are no symbols changes for libqt5network5.

[Robie Basak (racb)]

In #ubuntu-security just now:

14:44 <rbasak> sarnold: please could we have a definitive nack if you don't want bug 1981807 in the security pocket? Looking at the previous IRC conversation, it looks like it was a "decision pending review".

[Steve Langasek (vorlon)]

Hello msaxl, or anyone else affected,

Accepted qtbase-opensource-src into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/qtbase-opensource-src/5.15.3+dfsg-2ubuntu0.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Ubuntu SRU Bot (ubuntu-sru-bot)]

All autopkgtests for the newly accepted qtbase-opensource-src (5.15.3+dfsg-2ubuntu0.2) for jammy have finished running.
The following regressions have been reported in tests triggered by the package:

drumkv1/0.9.24-1 (armhf)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/jammy/update_excuses.html#qtbase-opensource-src

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

[msaxl (saxl)]

looking at the regression log, I see that it fails to launch jackd (exec of JACK server (command = "/usr/bin/jackd") failed: No such file or directory). Other platforms (amd64) do not have that log output.
I suspect this is because drumkv1_jack was not started yet (and so the test is flaky). Essentially I do not see a connection between this change and this package failing. /usr/bin/drumkv1_jack does not even link to libQt5Network.so.5

[msaxl (saxl)]

Just tested the proposed version on two armhf systems. Both server and client mode now negotiate to tls1.3 if applicable. The other qt applications do still work. Of corse the test application in this thread also works (outputs 15)

Package: libqt5network5
Version: 5.15.3+dfsg-2ubuntu0.2
Package: libssl3
Version: 3.0.2-0ubuntu1.6

So far I don't have any issues (also on amd64 I saw no regression, but as already noted in the binary there should be no difference on amd64 since sizeof(long) == sizeof(unint64_t) == sizeof(qossloptions))

[Dmitry Shachnev (mitya57)]

Marking as verified per comment #23. I also restarted the failed autopkgtest.

[Launchpad Janitor (janitor)]

This bug was fixed in the package qtbase-opensource-src - 5.15.3+dfsg-2ubuntu0.2

---------------
qtbase-opensource-src (5.15.3+dfsg-2ubuntu0.2) jammy; urgency=medium

  * Add a patch to update signature of SSL_CTX_set_options for OpenSSL 3
    (LP: #1981807). Thanks Michael Saxl!

 -- Dmitry Shachnev <email address hidden> Wed, 10 Aug 2022 11:37:53 +0300

[Łukasz Zemczak (sil2100)]

The verification of the Stable Release Update for qtbase-opensource-src has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.