From 27573cf04802fc2fdeb7f7beace4612b22ea1942 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kundr=C3=A1t?= 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 Reviewed-by: Caspar Schutijser --- 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 +/* Copyright (C) 2006 - 2021 Jan Kundrát 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 +/* Copyright (C) 2006 - 2021 Jan Kundrát 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("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(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 &certificateChain, const QList &sslErrors); -- GitLab