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
