901926 - [Email] Email app stops working if attachment contains an IMAP literal

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Frederik Braun [:freddy]]

Attachment: exception stack

If the filename of an attachment contains a quote, the IMAP parser goes throws an uncaught exception and stops processing any emails newer than this.

The stack (from b2g-desktop, so line numbers are a few lines off) and PoC email are attached.

Comment 1

[Frederik Braun [:freddy]]

Attachment: email source code

This is the source code of the email as you would put it into the SMTP session (after DATA). To reproduce you may also use my smtp python script as attached in bug 783958 and use this as the input for server.sendmail().

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Fortuitous timing.  In bug 900676 Gregor has been experiencing a nearly identical failure and I just had him send me some e-mails, but I think he sent the wrong ones, so your exact repro case is handy.

Right now, I tend to use https://addons.mozilla.org/en-US/thunderbird/addon/importexporttools/ for Thunderbird to APPEND test e-mails into an IMAP account since that way I can put any message in exactly as I want it without getting extra transport headers munged in or running into spam filters, etc.

parseExpr in imap.js is trying to do the right thing with escaped quotes, but it clearly must not be.  We are missing a (probably unrelated?) change in https://github.com/mscdex/node-imap/commit/e2c7cdd32f1067c850ee985d36b98db0f34a0e54 but I don't have time to look into this further right now.

:lightsofapollo, I know what you're thinking, but andris9' inbox does not parse BODYSTRUCTURE yet ;)

Comment 4

[Marcus Cavanaugh [:mcav]]

Trying to reproduce this today; after sending messages with attachments containing quote characters, the device still receives new messages as usual. Using the testcase provided by :freddyb, the app displays the attachment as "unnamed-1" and the file is viewable.

Sending an attachment named foo'"quoted.txt via mail.app, the app displays the filename as foo'\"quoted.txt, and rather than loading the attachment, it displays an infinitely-spinning spinner.

It looks like there's some incorrect parsing going on, but I'm unable to reproduce it causing the app to completely stop downloading new mail. Any thoughts on how to reproduce this more precisely? I'll keep investigating in the meantime.

Gaia version was from 2 days ago: https://github.com/mozilla-b2g/gaia/commit/18d8efc064cdd493f4f369e65e5abb761ac07d60

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Ooops; on IRC I didn't realize you were referring to this bug when talking about servers to use.  This is very potentially IMAP server dependent.  On the duped-to-this-bug of bug 900676 gwagner indicated his problem was occurring using his mozilla.com e-mail address which is backed by zimbra.  :freddyb would appear to have been using lavabit which would appear to have been a dovecot server before it ceased to exist.

In general, it's worth setting up a dovecot server on localhost for your development machine.  The testing infrastructure used to only work against real servers, for which a localhost dovecot was essential.  Now we currently only use fake-servers, but we want to also run against real-servers again as part of validation, so you'll need a localhost dovecot again soon anyways.  I find it essential on airplanes and at work weeks where you can't connect to 'real' servers in the world.  It's harder to use with a real device depending on your network setup, but I do a lot of my more exciting testing in firefox nightly using the DEBUG=1 profile anyways.

Comment 6

[Frederik Braun [:freddy]]

Yes, I got this while downloading emails. The email before the one attached here went through and then the app got stuck and refused to receive any further emails (including the broken one)

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Oh, and for getting IMAP protocol traces, you'll probably want to enable https://wiki.mozilla.org/Gaia/Email/SecretDebugMode and then view the log with ArbPL.  Feel free to ping me on IRC if you need assistance getting that working or getting the logs going.  (Alternatively, if you are just testing against dovecot on localhost, we do allow non-SSL connections via the 'localhost' alias, so you could use that and tcpdump/etc.  or have the server generate a protocol trace if it supports that.)

Comment 8

[Dylan Oliver [:doliver]]

triage: working on this for 1.2.

Comment 9

[Marcus Cavanaugh [:mcav]]

Attachment: IMAP response

While I haven't been able to reproduce this specific crash-case (attachment names having a quote character), I think both this and bug 900676 are manifestations of the same bug, which is caused by the IMAP parser (from node-imap) misinterpreting a FETCH response.

Put simply, our parser does not interpret string literals correctly. In the attached message, the server split the FETCH response header line into two using a literal; our parser expects that literal to be followed by the body. Instead, it finds an incomplete header line, causing the parser to barf looking for a "BODY[" regex.

In discussion on IRC, :asuth mentioned that this would be addressed by replacing the IMAP parser with a better one (bug 885105), but that might be more involved than this warrants. The upstream of node-imap appears to add better support for literals but it has diverged quite a bit from our implementation.

I'm looking into this now to see what a good fix might look like.

Comment 10

[Marcus Cavanaugh [:mcav]]

Attachment: pull-request.html

This patch does two things:

- It modifies the way node-imap parses the FETCH response. Instead of terminating its search for the header at the first CRLF, it continues until it actually receives the full line, all the way through any embedded literals.

- It updates the parseExpr() function to parse literals properly.

These changes seem sane to me, and I believe they address the root cause of this sync error, but it definitely feels like putting duct tape on duct tape. An IMAP server could send literals in place of quoted strings anywhere it wanted, and the node-imap parser doesn't handle that.

I believe upstream does a better job of parsing literals, but it has diverged enough from our implementation that it would probably involve many more changes than I've done here; additionally we have changes that upstream does not, which I'm not entirely familiar with (something to do with socket and buffer shims?).

Andrew/James, if you think this should be addressed with a more comprehensive/correct solution (i.e. effectively updating to node-imap HEAD), I'm happy to go back and do more work on this, but I may need guidance on what was changed from node-imap and if those changes are still relevant. That said, this patch should improve the state of things.

Having seen the current state of the parser, I'm full on board with the sentiment about bug 885105. Someday. :-)

Comment 11

[Marcus Cavanaugh [:mcav]]

Attachment: pull-request.html

[one of these days i'll get the pull request attachment right on the first try]

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 794347 [details]
pull-request.html

First, great work on this, especially not going insane!  I had forgotten just how painful up (our fork of) imap.js is to hack on.

During my review I realized there might be some issues if the literal contained an \r\n.  I don't think our exact triggering scenario should result in this because of rfc2822 parsing rules, but it concerned me enough that I dug into it.

So I added a test case for that, and then I started poking around to figure out how we would actually deal with that case so I could provide useful feedback.  In the end, I ended up hacking some changes to your fix in the line-parsing layer that addressed the problem and I think make the code a little less horrible.  (Not that it does us much good since I'm confident we don't want to ever touch this code further.)

Anywho, I'm creating a separate pull request for you to look at that consists of your commit plus my commit on top.  I'm doing this instead of a pull request against your branch because when I've done that before the whole process seemed super-weird.  Far better for you to just grab the commit from my branch and pull it into yours.

Other than what I mentioned above, I also had the test add output constraints since I noticed when I was doing my hacking that the tests claimed to pass when I had instead totally broken parsing.

If you're fine with my changes, feel free to land them with r=asuth for your part and your review on my part.  If you want to iterate based off what I've done, that's great too and then I can just review your incremental changes.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/244

Pointer to Github pull-request

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 797167 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/244

(the aforementioned pull request with the extra commit)

Comment 15

[Marcus Cavanaugh [:mcav]]

Comment on attachment 797167 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/244

Nice! That seems cleaner. Too bad imap.js's parsing is so gross in general. Your changes look good to me. Since there were just a couple lingering log statements and an outdated comment, I cherry-picked your commit and touched those up:

https://github.com/mcav/gaia-email-libs-and-more/commit/0500e2674f7605d219db0d861796e93b393d049c

If that looks good to you, I can land it (or you can, if you'd rather).

Comment 16

[Marcus Cavanaugh [:mcav]]

Actually, on further review, it looks like this patch doesn't process IDLE correctly. Looking into that now.

Comment 17

[Marcus Cavanaugh [:mcav]]

Comment on attachment 794347 [details]
pull-request.html

Okay, the issue was just that curReq was being referenced when it was undefined (in the case of commands like IDLE), whereas it wasn't before this patch. Whitespace-ignoring-diff here: https://github.com/mcav/gaia-email-libs-and-more/commit/78312dd1b5033d70f69813fe23769777042e695f?w=1

Looks to work now.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 794347 [details]
pull-request.html

Ah... I remember being worried about that as I made that change.

r=asuth

Comment 19

[Marcus Cavanaugh [:mcav]]

Landed in GELAM: https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/73a1ff336353f9537e9022a2842e8dd7771e47ef

Landed in gaia: https://github.com/mozilla-b2g/gaia/commit/a7941018788645f658011b623068ef9904d7f1b4

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

The gaia landing appears to have accidentally backed out the fix for bug 899070.

We probably want to back out the landing of https://github.com/mozilla-b2g/gaia/commit/a7941018788645f658011b623068ef9904d7f1b4 then create a new pull request from gaia-email-libs-and-more in its proper state to re-land this, although since we're not going to uplift this one or that one, it doesn't tremendously matter.  :lightsofapollo probably has a better idea of how to best handle it.

Comment 21

[Marcus Cavanaugh [:mcav]]

Fixed landing in https://github.com/mozilla-b2g/gaia/commit/b9eba46ad47fd55985670dd80ee91362385ce4e5

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

Can we clear the security bit on this bug?  The fix's unit test already clues a mean/bad person into how to send an e-mail that would cause our sync process to fail.

Comment 23

[Frederik Braun [:freddy]]

It's a DoS and it's fixed. I don't know how the update story on Firefox OS is right now. If we have a substantial amount of users on an unpatched version, I would prefer not putting them at risk (Mischievous people reading a clear text description of the bug and how to reproduce is still something else than a patch).

I'm happy with opening if a significant majority of our users is not using this version, though.

Comment 24

[Jason Smith [:jsmith]]

The dupe indicates this reproduces on 1.1 as well.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Paul, is this something you'd want to see on v1.1 still?

Comment 26

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Yes if possible, but its only a DoS (sec-moderate), so I won't complain if there are issues with this.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

(In reply to Paul Theriault [:pauljt] from comment #26)
> Yes if possible, but its only a DoS (sec-moderate), so I won't complain if
> there are issues with this.

Looks like it should uplift without much difficulty. Marcus, can you please request approval‑mozilla‑b2g18 on this?

Comment 28

[Marcus Cavanaugh [:mcav]]

Attachment: Gaia

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 29

[Marcus Cavanaugh [:mcav]]

Ah sorry Ryan, I sent that from my phone last night and was fairly distracted at the time, thought it might be some sort of permissions issue. Wasn't trying to be snarky, I promise. :)

[Approval Request Comment]

Bug caused by (feature/regressing bug #):
The IMAP support library we pulled from NodeJS was not advanced enough to handle certain IMAP protocol features, causing the e-mail app to break in unexpected ways when some servers send certain formed requests. This particular instance was caused by attachments with a quote in the filename, but the nature of the bug means that this problem could be triggered by other conditions also.

User impact if declined:
Users may be unable to sync their e-mail if their IMAP server sends certain types of responses; this can be triggered by a variety of conditions but the one identified here was an attachment with a quote character in the name. Some servers are affected, some aren't; depending on what the server does, users may be unable to sync new mail.

Testing completed:
Unit tests, manual verification, plus additional testing as this feature landed in 1.2 and 1.3.

Risk to taking this patch (and alternatives if risky): 
Minimal; this has caused no problems in 1.2/1.3 so far.

String or UUID changes made by this patch: None.

Comment 30

[Preeti Raghunath(:Preeti)]

Comment on attachment 8365127 [details] [review]
Gaia

Taking this for sec-moderate

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://github.com/mozilla-b2g/gaia/commit/c5cb252e5d1aa45d14f3a2ea182e73e2271e4496