summaryrefslogtreecommitdiff
path: root/user/trojita/plaintext-upgrade-attack.patch
diff options
context:
space:
mode:
Diffstat (limited to 'user/trojita/plaintext-upgrade-attack.patch')
-rw-r--r--user/trojita/plaintext-upgrade-attack.patch221
1 files changed, 221 insertions, 0 deletions
diff --git a/user/trojita/plaintext-upgrade-attack.patch b/user/trojita/plaintext-upgrade-attack.patch
new file mode 100644
index 000000000..f3c30b9aa
--- /dev/null
+++ b/user/trojita/plaintext-upgrade-attack.patch
@@ -0,0 +1,221 @@
+From 27573cf04802fc2fdeb7f7beace4612b22ea1942 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Jan=20Kundr=C3=A1t?= <jkt@kde.org>
+Date: Sun, 30 Jan 2022 23:56:31 +0100
+Subject: [PATCH] IMAP: ignore unsolicited LIST and STATUS before we're
+ authenticated
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+These responses were combined with legit responses which arrive from the
+real server later on, after having authenticated, and as a result, a
+malicious active attacker on a network was able to inject extra fake
+mailboxes to the GUI even when upgrading connection from plaintext to a
+secure one via STARTTLS.
+
+These two responses are the only ones which are handled by our pre-task
+code directly in Model.cpp, which means that the state machine has to be
+checked and messages which are received prior to authentication must be
+rejected. Since this is really a pretty serious violation of the IMAP
+state machine, let's not ignore them silently, and simply treat them as
+any other unexpected response.
+
+BUG: 432353
+Change-Id: I9292fcb20215ebe4dbc7a103fc9403dfa97b258b
+Reported-by: Damian Poddebniak
+Co-authored-by: Espen Sandøy Hustad <espen@ehustad.com>
+Reviewed-by: Caspar Schutijser <caspar@schutijser.com>
+---
+ src/Imap/Model/Model.cpp | 7 +-
+ tests/Imap/test_Imap_Tasks_OpenConnection.cpp | 122 +++++++++++++++++-
+ tests/Imap/test_Imap_Tasks_OpenConnection.h | 9 ++
+ 3 files changed, 136 insertions(+), 2 deletions(-)
+
+diff --git a/src/Imap/Model/Model.cpp b/src/Imap/Model/Model.cpp
+index 7a393c7a..79e34658 100644
+--- a/src/Imap/Model/Model.cpp
++++ b/src/Imap/Model/Model.cpp
+@@ -1,4 +1,4 @@
+-/* Copyright (C) 2006 - 2014 Jan Kundrát <jkt@flaska.net>
++/* Copyright (C) 2006 - 2021 Jan Kundrát <jkt@flaska.net>
+
+ This file is part of the Trojita Qt IMAP e-mail client,
+ http://trojita.flaska.net/
+@@ -519,6 +519,8 @@ void Model::handleList(Imap::Parser *ptr, const Imap::Responses::List *const res
+ {
+ if (accessParser(ptr).connState == CONN_STATE_LOGOUT)
+ return;
++ if (accessParser(ptr).connState < CONN_STATE_AUTHENTICATED)
++ throw UnexpectedResponseReceived("Unexpected LIST response before authentication succeeded", *resp);
+ accessParser(ptr).listResponses << *resp;
+ }
+
+@@ -547,6 +549,9 @@ void Model::handleStatus(Imap::Parser *ptr, const Imap::Responses::Status *const
+ {
+ if (accessParser(ptr).connState == CONN_STATE_LOGOUT)
+ return;
++ if (accessParser(ptr).connState < CONN_STATE_AUTHENTICATED)
++ throw UnexpectedResponseReceived("Unexpected STATUS response before authentication succeeded", *resp);
++
+ Q_UNUSED(ptr);
+ TreeItemMailbox *mailbox = findMailboxByName(resp->mailbox);
+ if (! mailbox) {
+diff --git a/tests/Imap/test_Imap_Tasks_OpenConnection.cpp b/tests/Imap/test_Imap_Tasks_OpenConnection.cpp
+index b72898c4..953fe6f9 100644
+--- a/tests/Imap/test_Imap_Tasks_OpenConnection.cpp
++++ b/tests/Imap/test_Imap_Tasks_OpenConnection.cpp
+@@ -1,4 +1,4 @@
+-/* Copyright (C) 2006 - 2014 Jan Kundrát <jkt@flaska.net>
++/* Copyright (C) 2006 - 2021 Jan Kundrát <jkt@flaska.net>
+
+ This file is part of the Trojita Qt IMAP e-mail client,
+ http://trojita.flaska.net/
+@@ -611,5 +611,125 @@ void ImapModelOpenConnectionTest::provideAuthDetails()
+ }
+ }
+
++void ImapModelOpenConnectionTest::dataBug432353()
++{
++ QTest::addColumn<QByteArray>("garbage");
++ QTest::newRow("LIST") << QByteArray("* LIST (\\HasNoChildren) \".\" x\r\n");
++ QTest::newRow("STATUS") << QByteArray("* STATUS INBOX (MESSAGES 123)\r\n");
++}
++
++void ImapModelOpenConnectionTest::testNoListStatusUponConnect_data()
++{
++ dataBug432353();
++}
++
++void ImapModelOpenConnectionTest::testNoListStatusUponConnect()
++{
++ QFETCH(QByteArray, garbage);
++ reinit(TlsRequired::Yes);
++ cEmpty();
++ QCOMPARE(model->rowCount(QModelIndex()), 1);
++ ExpectSingleErrorHere x(this);
++ cServer("* OK foo\r\n" + garbage);
++}
++
++void ImapModelOpenConnectionTest::testNoListStatusStartTls_data()
++{
++ dataBug432353();
++}
++
++void ImapModelOpenConnectionTest::testNoListStatusStartTls()
++{
++ QFETCH(QByteArray, garbage);
++ reinit(TlsRequired::Yes);
++ cEmpty();
++ cServer("* OK [Capability imap4rev1 starttls] foo\r\n");
++ QVERIFY(completedSpy->isEmpty());
++ QVERIFY(authSpy->isEmpty());
++ cClient(t.mk("STARTTLS\r\n"));
++ cServer(t.last("OK will establish secure layer immediately\r\n"));
++ QVERIFY(authSpy->isEmpty());
++ cClient("[*** STARTTLS ***]" + t.mk("CAPABILITY\r\n"));
++ QVERIFY(completedSpy->isEmpty());
++ QVERIFY(authSpy->isEmpty());
++ cServer("* CAPABILITY IMAP4rev1\r\n" + t.last("OK capability completed\r\n"));
++ cClient(t.mk("LOGIN luzr sikrit\r\n"));
++ QCOMPARE(authSpy->size(), 1);
++ ExpectSingleErrorHere x(this);
++ cServer(garbage + t.last("OK [CAPABILITY IMAP4rev1] logged in\r\n"));
++}
++
++void ImapModelOpenConnectionTest::testNoListStatusBeforeAuthenticated_data()
++{
++ dataBug432353();
++}
++
++void ImapModelOpenConnectionTest::testNoListStatusBeforeAuthenticated()
++{
++ QFETCH(QByteArray, garbage);
++ reinit(TlsRequired::Yes);
++ cEmpty();
++ cServer("* OK [Capability imap4rev1 starttls] foo\r\n");
++ QVERIFY(completedSpy->isEmpty());
++ QVERIFY(authSpy->isEmpty());
++ cClient(t.mk("STARTTLS\r\n"));
++ cServer(t.last("OK will establish secure layer immediately\r\n"));
++ QVERIFY(authSpy->isEmpty());
++ cClient("[*** STARTTLS ***]" + t.mk("CAPABILITY\r\n"));
++ QVERIFY(completedSpy->isEmpty());
++ QVERIFY(authSpy->isEmpty());
++ cServer("* CAPABILITY IMAP4rev1\r\n" + t.last("OK capability completed\r\n"));
++ cClient(t.mk("LOGIN luzr sikrit\r\n"));
++ QCOMPARE(authSpy->size(), 1);
++ ExpectSingleErrorHere x(this);
++ cServer(garbage + t.last("OK [CAPABILITY IMAP4rev1] logged in\r\n"));
++}
++
++void ImapModelOpenConnectionTest::testListStatusUnsolicited()
++{
++ model->cache()->setChildMailboxes(QString(),
++ QList<Imap::Mailbox::MailboxMetadata>()
++ << Imap::Mailbox::MailboxMetadata(QLatin1String("INBOX"), QString(), QStringList())
++ );
++
++ reinit(TlsRequired::Yes);
++ cEmpty();
++ cServer("* OK [Capability imap4rev1 starttls] foo\r\n");
++ QVERIFY(completedSpy->isEmpty());
++ QVERIFY(authSpy->isEmpty());
++ cClient(t.mk("STARTTLS\r\n"));
++ cServer(t.last("OK will establish secure layer immediately\r\n"));
++ QVERIFY(authSpy->isEmpty());
++ cClient("[*** STARTTLS ***]" + t.mk("CAPABILITY\r\n"));
++ QVERIFY(completedSpy->isEmpty());
++ QVERIFY(authSpy->isEmpty());
++ cServer("* CAPABILITY IMAP4rev1\r\n" + t.last("OK capability completed\r\n"));
++ cClient(t.mk("LOGIN luzr sikrit\r\n"));
++ QCOMPARE(authSpy->size(), 1);
++ cServer(t.last("OK [CAPABILITY IMAP4rev1] logged in\r\n") +
++ "* LIST (\\HasNoChildren) \".\" abc\r\n"
++ "* LIST (\\HasNoChildren) \".\" def\r\n"
++ "* STATUS INBOX (MESSAGES 123)\r\n"
++ );
++ QCOMPARE(model->rowCount(QModelIndex()), 1);
++ cClient(t.mk("LIST \"\" \"%\"\r\n"));
++ cEmpty();
++ QCOMPARE(completedSpy->size(), 1);
++ QVERIFY(failedSpy->isEmpty());
++ cServer("* LIST (\\Noselect \\HasChildren) \".\" \"xyz\"\r\n"
++ "* LIST (\\HasNoChildren) \".\" \"INBOX\"\r\n"
++ + t.last("OK list done\r\n")
++ + "* STATUS INBOX (MESSAGES 789)\r\n");
++ cEmpty();
++
++ // Mailboxes "abc" and "def" were reported by the server as an async replay after having authenticated,
++ // so there's no reason not to trust them. However, they were received before a LIST was issues,
++ // so they should be probably ignored -- but that is tricky to do properly with command pipelining, etc.
++ QCOMPARE(model->rowCount( QModelIndex() ), 5 /* empty root, INBOX, abc, def, xyz */);
++ QCOMPARE(model->index(1, 0, QModelIndex()).data().toString(), QLatin1String("INBOX"));
++ QCOMPARE(model->index(2, 0, QModelIndex()).data().toString(), QLatin1String("abc"));
++ QCOMPARE(model->index(3, 0, QModelIndex()).data().toString(), QLatin1String("def"));
++ QCOMPARE(model->index(4, 0, QModelIndex()).data().toString(), QLatin1String("xyz"));
++}
+
+ QTEST_GUILESS_MAIN( ImapModelOpenConnectionTest )
+diff --git a/tests/Imap/test_Imap_Tasks_OpenConnection.h b/tests/Imap/test_Imap_Tasks_OpenConnection.h
+index 4df65c05..6a0e2704 100644
+--- a/tests/Imap/test_Imap_Tasks_OpenConnection.h
++++ b/tests/Imap/test_Imap_Tasks_OpenConnection.h
+@@ -70,6 +70,15 @@ private slots:
+
+ void testExcessivePasswordPrompts();
+
++ void dataBug432353();
++ void testNoListStatusUponConnect();
++ void testNoListStatusUponConnect_data();
++ void testNoListStatusStartTls();
++ void testNoListStatusStartTls_data();
++ void testNoListStatusBeforeAuthenticated();
++ void testNoListStatusBeforeAuthenticated_data();
++ void testListStatusUnsolicited();
++
+ void provideAuthDetails();
+ void acceptSsl(const QList<QSslCertificate> &certificateChain, const QList<QSslError> &sslErrors);
+
+--
+GitLab
+
generated by cgit v1.2.3-70-g09d2 (git 2.46.0) at 2024-11-22 04:05:59 +0000