1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
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
|
