From 4a4ac4b35fd5faf14e7143c0c8babe65c9c95e92 Mon Sep 17 00:00:00 2001 From: "A. Wilcox" Date: Fri, 18 Nov 2022 03:11:50 -0600 Subject: user/trojita: Fix build and add patches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Ensures Trojitá does not crash using attachments. * Port to Qt 5.15. * Fix a security issue with STARTTLS. * Ensure passwords are stored correctly. * Fix signedness on unsigned char platforms (ARM, Power). Closes: #856 --- user/trojita/plaintext-upgrade-attack.patch | 221 ++++++++++++++++++++++++++++ 1 file changed, 221 insertions(+) create mode 100644 user/trojita/plaintext-upgrade-attack.patch (limited to 'user/trojita/plaintext-upgrade-attack.patch') 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?= +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 + -- cgit v1.2.3-60-g2f50