Bug 432353

Any update on this? To be clear: a network attacker can create new folders and tamper with local application state when STARTTLS is used.

Hi Damian,

I am going to work on it and to fix it. Pls give me some time.... :)

v/r
Andi

> Trojita accepts LIST, LSUB, STATUS, ... untagges responses before STARTTLS
> and incorporates them into local state.
> 
> I am not sure if this is already kind of a misbehavior even without
> STARTTLS, because the IMAP RFC does not really prohibit that. However, a
> meddler in the middle can use this to tamper with the state of Trojita.

Thanks for finding this, and sorry for not responding earlier. You're right, this can inject info about non-existing mailboxes (which will disappear after a reconnect), it can cause extra bandwidth consumption (due to syncing more than what is strictly needed to transfer), and it also has a Denial-of-Service potential (e.g., if the attacker pushes nonsense, the UID syncing code will ask the user to clear cache and refuse to proceed beforehand).

> This *could* also be escalated to a more severe issue. E.g. when an attacker
> injects a folder name with "\r\n<tag> <command>", it could trick Trojita to
> execute attacker-controlled commands on the IMAP server after login.
> 
> The only thing preventing this is sanitization of folder names, but I am not
> sure if we should count on that...

This cannot be exploited like that. Since IMAP is a text-based protocol, there are rules on how to "intervene" user-controlled (or even attacker-controlled) strings with protocol commands. However, this is not specific to a possible side-channel injection due to STARTTLS. The real user can just as well create a mailbox which has a newline in its name, and the IMAP code must handle this properly. Let's not call this "sanitization", please; it's a critical part of implementing a protocol. The STARTTLS vulnerability will only be relevant in this context if the attacker-controlled cache stored strings which are somehow escaped, and that is not the case.

A possibly relevant merge request was started @ https://invent.kde.org/pim/trojita/-/merge_requests/4

Hey Jan,  thank you for working on this issue!

> This cannot be exploited like that.

I know of at least one client where this is practically exploitable. However, I am not saying that it is possible in Trojita, though!

> Since IMAP is a text-based protocol, there are rules on how to "intervene" user-controlled (or even attacker-controlled) strings with protocol commands. However, this is not specific to a possible side-channel injection due to STARTTLS. The real user can just as well create a mailbox which has a newline in its name, and the IMAP code must handle this properly. Let's not call this "sanitization", please; it's a critical part of implementing a protocol.

I fully agree. Sanitization is not the correct term.

By the way, I know that you implemented the IMAP protocol very diligently in Trojita! ;-) Still, I also know IMAP very well and how complicated string handling is due to the many involved "string types" such as `tag`, `text`, `atom`, `astring`, `literal` ... (In fact, in my own IMAP implementation I was *so afraid* to forget to correctly encode some string in some place, that I wrapped all "string types" and use these wrappers throughout the whole library.¹)

> The STARTTLS vulnerability will only be relevant in this context if the attacker-controlled cache stored strings which are somehow escaped, and that is not the case.

Not sure if I understand that. But it doesn't matter. If Trojita implements the IMAP protocol correctly and properly escapes folder names, it should not matter.

¹ https://github.com/duesee/imap-codec/blob/6bf1e5d0da45d576bd9ed4ddc0b3640da8e2ba80/src/types/mailbox.rs#L142

Yeah, in Trojita, the code is similar, see src/Imap/Parser/Command.h and src/Imap/Parser/Command.cpp and handling of Commands::PartOfCommand.

Hehe, the code at

    https://github.com/KDE/trojita/blob/master/src/Imap/Parser/Command.cpp#L71

seems familiar :-)

`PartOfCommand` cannot contain a ", right?

Git commit 4953464742a65b401e5ea770a4bcb506c2f410d8 by Jan Kundrát.
Committed on 30/01/2022 at 23:24.
Pushed by jkt into branch 'bug-432353'.

IMAP: ignore unsolicited LIST and STATUS before we're authenticated

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.
Change-Id: I9292fcb20215ebe4dbc7a103fc9403dfa97b258b
Reported-by: Damian Poddebniak
Co-authored-by: Espen Sandøy Hustad <espen@ehustad.com>

M  +6    -1    src/Imap/Model/Model.cpp
M  +108  -1    tests/Imap/test_Imap_Tasks_OpenConnection.cpp
M  +5    -0    tests/Imap/test_Imap_Tasks_OpenConnection.h

https://invent.kde.org/pim/trojita/commit/4953464742a65b401e5ea770a4bcb506c2f410d8

Looks like a hook changed this before this hit master. Let's wait a few days for a review.

A possibly relevant merge request was started @ https://invent.kde.org/pim/trojita/-/merge_requests/6

Git commit 27573cf04802fc2fdeb7f7beace4612b22ea1942 by Jan Kundrát.
Committed on 31/01/2022 at 19:34.
Pushed by jkt into branch 'master'.

IMAP: ignore unsolicited LIST and STATUS before we're authenticated

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.
Change-Id: I9292fcb20215ebe4dbc7a103fc9403dfa97b258b
Reported-by: Damian Poddebniak
Co-authored-by: Espen Sandøy Hustad <espen@ehustad.com>
Reviewed-by: Caspar Schutijser <caspar@schutijser.com>

M  +6    -1    src/Imap/Model/Model.cpp
M  +121  -1    tests/Imap/test_Imap_Tasks_OpenConnection.cpp
M  +9    -0    tests/Imap/test_Imap_Tasks_OpenConnection.h

https://invent.kde.org/pim/trojita/commit/27573cf04802fc2fdeb7f7beace4612b22ea1942