MX injection and type juggling vulnerabilities

[r0xen]

Hello,

I'm here to report two vulnerabilities I have found while doing research on Roundcube 1.3.4, which are also present in your last release 1.3.5.

This two bugs are not exploitable in the wild, at least to my current knowledge; nonetheless fixing them should be a priority of yours because they could be chained with other minor stuff and then become exploitable in a realistic, attacker-pov efficient way. Plus, with the ongoing grow of this project you may introduce features that could be used to leverage this stuff.

Since the bugs are not so easy to spot, especially the mx injection, I'll now try to explain myself in the clearest way possible, the code I'll refer to it's the 1.3.5. I'll conclude with a brief summary.

MX Injection

On function archive.php:move_messages() we have:

A little bit of context:

• rcmail::get_uids inside the foreach cycle it's responsible to get $mbox from $uids which is passed via POST (line 132) (but anyway passing them by GET will work too); if provided with a format like ID-MBOX it will split the thing and have $uids =array(ID) and $mbox ="MBOX"; Fine.

• The first IF and ELSE IF (line 153 and 157) set our prerequisite to exploit the bug: the archive folder has to be set, and the archive_type must be set and be different from "folder" that's because the function move_messages_worker() (line 168) do his job right: will call archive.php:move_messages_worker() which will call rcube_imap.php:move_message() which will call rcube_storage.php:parse_uids() which sanitize $uids.

The problem lies in that else branch (archive.php line 170):

• line 176 archive.php:move_messages() calls fetch_headers($mbox, $uids);
• line 1235 rcube_imap.php:fetch_headers() calls fetchHeaders($folder,$msgs) where $folder is $mbox and $msgs is $uids
• line 2600 rcube_imap_generic.php:fetchHeaders() calls fetch($mailbox, $message_set, $is_uid, $query_items);
• rcube_imap_generic.php:fetch() it's a core function used everywhere for doing is job: fetching things.

On line 2360 $mailbox it's checked and the function returns false, so the attacker can't exploit that but, no check are done on $message_set which, still, is our user-controlled input which will end in - line 2369 - the command to the MX server causing an MX injection.

PHP Type Juggling

This is far more easy to spot and straightforward, few words: on rcube.php:check_request() we have

as you can see every check it's performed just with the == operator which is a loose not strict operator.
This is not exploitable right now, and it's just a theorical bug, because you just use HTTP Paramaters which are strings, not typed but if you'll introduce JSON then this will become easily exploitable and will cause a CSRF bypass.

php > var_dump("84829randomstring-csrfs9499" == TRUE); bool(true) php > var_dump("84829randomstring-csrfs9499" === TRUE); bool(false)

Nonetheless as I said in my introduction you should fix this: what if I opened a "JSON for post parameters" request as a feature request?

I hope I made myself enough clear, if you need more explanation: I am willing to help. When you fix this I'd like to write and publish a technical blog post about my findings ( the mx injection it's quite hided and nice, I think) - if that's okay with you.

PS: I think this issue should be private, not familiar with github if that's possible maybe we should do that.

[alecpl]

1. I don't know how could you exploit FETCH with evil $message_set, but I guess we could validate it in compressMessageSet().
2. Not really an issue, but I guess we can use strict operators in line 959 and 974.

[r0xen]

You can execute any command on that mx injection, you have

        $cmd      = ($is_uid ? 'UID ' : '') . 'FETCH';
        $request  = "$key $cmd $message_set (" . implode(' ', $query_items) . ")";

a simple 11%20BODY[]%0aA0003%20your_command should do the job.

Yeah the type juggling it's not a real issue, but could be in the future, seemed worthy to report to me, the bug it's there even if it's not exploitable

[alecpl]

Fixed.

[r0xen]

Good, PS: I checked the exploitability of the mx injection more in depth and it's completely exploitable.
Thing is that in archive.php:135 "_uids" it's taken via POST so it seems that you cannot exploit this since you'll end with check_request() checking for a token. But it's not like this. In archive.php:156 there's a call to rcmail::get_uids() which get "_uids" again BUT with INPUT_GPC. So after line 156 our _uids passed from GET it's injected. This by passes check_request: cause a request to ?_task=mail&_mbox=INBOX&_action=plugin.move2archive&_uid=exploit it's considered a post, with empty $_POST. Which means that in versions previous to the archive.php:move_messages() first check for ajax requests this it's exploitable by just tricking the victim with clicking and/or a simple html page. Posterior version may be more difficult to exploit due to same origin policy.

[alecpl]

That's a different issue. Please, create a new ticket.