user/trojita: CVE patches (#137)

3 files changed, 207 insertions, 4 deletions

diff --git a/user/trojita/APKBUILD b/user/trojita/APKBUILD
index c55e5453c..3dd6a5ab6 100644
--- a/user/trojita/APKBUILD
+++ b/user/trojita/APKBUILD
@@ -2,7 +2,7 @@
 # Maintainer: Kiyoshi Aman <adelie@aerdan.vulpine.house>
 pkgname=trojita
 pkgver=0.7
-pkgrel=0
+pkgrel=1
 pkgdesc="Qt-based IMAP email client"
 url="http://trojita.flaska.net/"
 arch="all"
@@ -15,7 +15,15 @@ makedepends="cmake extra-cmake-modules zlib-dev qt5-qtbase-dev qt5-qtwebkit-dev
 	qtkeychain-dev"
 source="https://sourceforge.net/projects/trojita/files/src/trojita-$pkgver.tar.xz
 	use-qgpgme.patch
-	fix-gpg.patch"
+	fix-gpg.patch
+	CVE-2019-10734.patch
+	CVE-2020-15047.patch
+	"
+
+# secfixes:
+#   0.7-r1:
+#    - CVE-2019-10734
+#    - CVE-2020-15047
 
 build() {
 	if [ "$CBUILD" != "$CHOST" ]; then
@@ -34,7 +42,8 @@ build() {
 }
 
 check() {
-	CTEST_OUTPUT_ON_FAILURE=TRUE ctest
+	# test_Html_formatting: requires X11
+	CTEST_OUTPUT_ON_FAILURE=TRUE ctest -E test_Html_formatting
 }
 
 package() {
@@ -43,4 +52,6 @@ package() {
 
 sha512sums="fe4d9316f97d913619f27d24a5023c3d8dd4a6b9fb058651be12c67188f394aa8cbb60c7593e5eb28fc12fc883b76deeeb5f4f631edd255fdec4c5862c9a91c8  trojita-0.7.tar.xz
 740c2410d7236d722482f05dd1d2c681e35543620823cb7c1396710081f9de4f6ae530c5b5442ecf5d08a8e552f0697f3a35bf51e07a3b4336dec7021b665706  use-qgpgme.patch
-9d0fbf7c0b0f6975990a7705f9d43043e5807401cee179d7a07f9514856332d6bb1aa8448e84d0083003c34a3bb181080b973e8c1f77d1b5a8930d07d57702da  fix-gpg.patch"
+9d0fbf7c0b0f6975990a7705f9d43043e5807401cee179d7a07f9514856332d6bb1aa8448e84d0083003c34a3bb181080b973e8c1f77d1b5a8930d07d57702da  fix-gpg.patch
+db96a566924b5d7b80787ab624af3726d5dd3459653192436a377d6482ab73801a7dcca1df1b1d937cf0d0798b827e04f8ef2c1124f91dc9da3e8036ef61e28a  CVE-2019-10734.patch
+2477612aca1e558fa3ba2b434a701cc255c573ac7e2001e7b5921c9b991f7c95720f53b70b49824e36bafb53ab53477950cb8d436e637fda4d59c7ec5883ce5f  CVE-2020-15047.patch"
diff --git a/user/trojita/CVE-2019-10734.patch b/user/trojita/CVE-2019-10734.patch
new file mode 100644
index 000000000..d52edb042
--- /dev/null
+++ b/user/trojita/CVE-2019-10734.patch
@@ -0,0 +1,104 @@
+From 8db7f450d52539b4c72ee968384911b6813ad1e7 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Jan=20Kundr=C3=A1t?= <jkt@kde.org>
+Date: Thu, 25 Jun 2020 21:39:34 +0200
+Subject: [PATCH] Prevent a possible decryption oracle attack
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Thanks to Jens Mueller (Ruhr-Uni Bochum and FH Münster) for reporting
+this. The gist is that an attacker can embed arbitrary ciphertext into
+their messages. Trojita decrypts that, and when we hit reply, the
+original *cleartext* gets quoted and put into a reply for the attacker
+to see.
+
+Fix this by not quoting any plaintext which originated in an encrypted
+message. That's pretty draconian, but hey, it works and we never came up
+with any better patch. Also, given that Trojita does not encrypt
+outgoing messages yet, this is probably also a conservative thing to do.
+
+Change-Id: I84c45b9e707eb7c99eb7183c6ef59ef41cd62c43
+CVE: CVE-2019-10734
+BUG: 404697
+---
+ src/Cryptography/GpgMe++.cpp | 2 ++
+ src/Gui/MessageView.cpp      | 9 ++++++++-
+ src/Gui/PartWidget.cpp       | 8 ++++++++
+ src/Imap/Model/ItemRoles.h   | 2 +-
+ 4 files changed, 19 insertions(+), 2 deletions(-)
+
+diff --git a/src/Cryptography/GpgMe++.cpp b/src/Cryptography/GpgMe++.cpp
+index e012f603..716b8aff 100644
+--- a/src/Cryptography/GpgMe++.cpp
++++ b/src/Cryptography/GpgMe++.cpp
+@@ -267,6 +267,8 @@ QVariant GpgMePart::data(int role) const
+     switch (role) {
+     case Imap::Mailbox::RolePartSignatureVerifySupported:
+         return m_wasSigned;
++    case RolePartDecryptionSupported:
++        return m_isAllegedlyEncrypted;
+     case RolePartCryptoNotFinishedYet:
+         return m_waitingForData ||
+                 (m_crypto.valid() &&
+diff --git a/src/Gui/MessageView.cpp b/src/Gui/MessageView.cpp
+index 7d649308..c95e0878 100644
+--- a/src/Gui/MessageView.cpp
++++ b/src/Gui/MessageView.cpp
+@@ -354,7 +354,6 @@ bool MessageView::eventFilter(QObject *object, QEvent *event)
+ QString MessageView::quoteText() const
+ {
+     if (auto w = bodyWidget()) {
+-        QStringList quote = Composer::quoteText(w->quoteMe().split(QLatin1Char('\n')));
+         const Imap::Message::Envelope &e = message.data(Imap::Mailbox::RoleMessageEnvelope).value<Imap::Message::Envelope>();
+         QString sender;
+         if (!e.from.isEmpty())
+@@ -362,6 +361,14 @@ QString MessageView::quoteText() const
+         if (e.from.isEmpty())
+             sender = tr("you");
+ 
++        if (messageModel->index(0, 0) /* fake message root */.child(0, 0) /* first MIME part */.data(Imap::Mailbox::RolePartDecryptionSupported).toBool()) {
++            // This is just an UX improvement shortcut: real filtering for CVE-2019-10734 is in
++            // MultipartSignedEncryptedWidget::quoteMe().
++            // That is required because the encrypted part might not be the root part of the message.
++            return tr("On %1, %2 sent an encrypted message:\n> ...\n\n").arg(e.date.toLocalTime().toString(Qt::SystemLocaleLongDate), sender);
++        }
++
++        QStringList quote = Composer::quoteText(w->quoteMe().split(QLatin1Char('\n')));
+         // One extra newline at the end of the quoted text to separate the response
+         quote << QString();
+ 
+diff --git a/src/Gui/PartWidget.cpp b/src/Gui/PartWidget.cpp
+index bb27604d..96eff338 100644
+--- a/src/Gui/PartWidget.cpp
++++ b/src/Gui/PartWidget.cpp
+@@ -378,6 +378,14 @@ void MultipartSignedEncryptedWidget::updateStatusIndicator()
+ 
+ QString MultipartSignedEncryptedWidget::quoteMe() const
+ {
++    if (m_partIndex.data(Imap::Mailbox::RolePartDecryptionSupported).toBool()) {
++        // See CVE-2019-10734, the point is not to leak cleartext from encrypted content. Even when Trojita starts supporting
++        // encryption of outgoing mail, we will have to check whether the encrypted cleartext is from the same sender, whether
++        // it matches the list of recipients (which is dynamic and can be set later on), etc etc.
++        // TL;DR, this is a can of worms.
++        return tr("[Encrypted message]");
++    }
++
+     return quoteMeHelper(children());
+ }
+ 
+diff --git a/src/Imap/Model/ItemRoles.h b/src/Imap/Model/ItemRoles.h
+index 4588d4d0..00adb3bb 100644
+--- a/src/Imap/Model/ItemRoles.h
++++ b/src/Imap/Model/ItemRoles.h
+@@ -193,7 +193,7 @@ enum {
+     RolePartSignatureVerifySupported,
+     /** @short Is the format of this particular multipart/encrypted supported and recognized?
+ 
+-    See RolePartSignatureVerifySupported, this is an equivalent.
++    If true, this message part represents content of an encrypted message that Trojita can attempt to decrypt.
+     */
+     RolePartDecryptionSupported,
+     /** @short Is there any point in waiting longer?
+-- 
+GitLab
+
diff --git a/user/trojita/CVE-2020-15047.patch b/user/trojita/CVE-2020-15047.patch
new file mode 100644
index 000000000..6199c01fd
--- /dev/null
+++ b/user/trojita/CVE-2020-15047.patch
@@ -0,0 +1,88 @@
+From 77ddd5d44f2bf4155d0c9b6f7d05f01713b32d5d Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Jan=20Kundr=C3=A1t?= <jkt@kde.org>
+Date: Thu, 25 Jun 2020 11:30:51 +0200
+Subject: [PATCH] SMTP: Do not ignore TLS errors
+
+This fixes a CVE-2020-15047 (category: CWE-295). Since commit 0083eea5ed
+which added initial, experimental support for SMTP message submission,
+we have apparently never implemented proper SSL/TLS error handling, and
+the code has ever since just kept silently ignoring any certificate
+verification errors.  As a result, Trojita was susceptible to a MITM
+attack when sending e-mails. The information leaked include user's
+authentication details, including the password, and the content of sent
+messages.
+
+Sorry for this :(.
+
+Now, this patch re-enabes proper TLS error handling. It was not possible
+to directly re-use our code for TLS key pinning which we are using for
+IMAP connections. In the Qt TLS code, the decision to accept or not
+accept a TLS connection is a blocking one, so the IMAP code relies upon
+the protocol state machine (i.e., another layer) for deciding whether to
+use or not to use the just-established TLS connection. Implementing an
+equivalent code in the SMTP library would be nice, but this hot-fix has
+a priority. As a result, SMTP connections to hosts with, e.g.,
+self-signed TLS certs, are no longer possible. Let's hope that this is
+not a practical problem with Lets Encrypt anymore.
+
+Thanks to Damian Poddebniak for reporting this bug.
+
+Change-Id: Icd6bbb2b0fb3e45159fc9699ebd07ab84262fe37
+CVE: CVE-2020-15047
+BUG: 423453
+---
+ src/MSA/SMTP.cpp | 11 +++++++++--
+ src/MSA/SMTP.h   |  1 +
+ 2 files changed, 10 insertions(+), 2 deletions(-)
+
+diff --git a/src/MSA/SMTP.cpp b/src/MSA/SMTP.cpp
+index 3a054516..ac1eefc5 100644
+--- a/src/MSA/SMTP.cpp
++++ b/src/MSA/SMTP.cpp
+@@ -21,6 +21,7 @@
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+ #include "SMTP.h"
++#include "UiUtils/Formatting.h"
+ 
+ namespace MSA
+ {
+@@ -32,8 +33,8 @@ SMTP::SMTP(QObject *parent, const QString &host, quint16 port, bool encryptedCon
+     user(user), failed(false), isWaitingForPassword(false), sendingMode(MODE_SMTP_INVALID)
+ {
+     qwwSmtp = new QwwSmtpClient(this);
+-    // FIXME: handle SSL errors properly
+-    connect(qwwSmtp, &QwwSmtpClient::sslErrors, qwwSmtp, &QwwSmtpClient::ignoreSslErrors);
++    // FIXME: handle SSL errors in the same way as we handle IMAP TLS errors, with key pinning, etc.
++    connect(qwwSmtp, &QwwSmtpClient::sslErrors, this, &SMTP::handleSslErrors);
+     connect(qwwSmtp, &QwwSmtpClient::connected, this, &AbstractMSA::sending);
+     connect(qwwSmtp, &QwwSmtpClient::done, this, &SMTP::handleDone);
+     connect(qwwSmtp, &QwwSmtpClient::socketError, this, &SMTP::handleError);
+@@ -78,6 +79,12 @@ void SMTP::handleError(QAbstractSocket::SocketError err, const QString &msg)
+     emit error(msg);
+ }
+ 
++void SMTP::handleSslErrors(const QList<QSslError>& errors)
++{
++    auto msg = UiUtils::Formatting::sslErrorsToHtml(errors);
++    emit error(tr("<p>Cannot send message due to an SSL/TLS error</p>\n%1").arg(msg));
++}
++
+ void SMTP::setPassword(const QString &password)
+ {
+     pass = password;
+diff --git a/src/MSA/SMTP.h b/src/MSA/SMTP.h
+index 453407d3..913bb873 100644
+--- a/src/MSA/SMTP.h
++++ b/src/MSA/SMTP.h
+@@ -43,6 +43,7 @@ public slots:
+     virtual void setPassword(const QString &password);
+     void handleDone(bool ok);
+     void handleError(QAbstractSocket::SocketError err, const QString &msg);
++    void handleSslErrors(const QList<QSslError>& errors);
+ private:
+     QwwSmtpClient *qwwSmtp;
+     QString host;
+-- 
+GitLab
+