Closed Bug 901926 Opened 8 years ago Closed 8 years ago
[Email] Email app stops working if attachment contains an IMAP literal
832 bytes, text/plain
746 bytes, text/plain
936 bytes, text/plain
302 bytes, text/html
412 bytes, text/html
46 bytes, text/x-github-pull-request
|Details | Review|
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.
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().
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 ;)
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
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.
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)
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.)
triage: working on this for 1.2.
Assignee: nobody → mcavanaugh
blocking-b2g: koi? → koi+
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.
Status: NEW → ASSIGNED
Summary: Email app stops working if attachment filename contains quote character → [Email] Email app stops working if attachment contains an IMAP literal
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. :-)
[one of these days i'll get the pull request attachment right on the first try]
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.
Pointer to Github pull-request
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 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).
Attachment #794347 - Flags: review- → review?(bugmail)
Actually, on further review, it looks like this patch doesn't process IDLE correctly. Looking into that now.
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 on attachment 794347 [details] pull-request.html Ah... I remember being worried about that as I made that change. r=asuth
Attachment #794347 - Flags: review?(bugmail) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
Fixed landing in https://github.com/mozilla-b2g/gaia/commit/b9eba46ad47fd55985670dd80ee91362385ce4e5
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.
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.
The dupe indicates this reproduces on 1.1 as well.
Paul, is this something you'd want to see on v1.1 still?
Yes if possible, but its only a DoS (sec-moderate), so I won't complain if there are issues with this.
(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?
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:
Attachment #8365127 - Flags: approval-mozilla-b2g18?
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 on attachment 8365127 [details] [review] Gaia Taking this for sec-moderate
Attachment #8365127 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
You need to log in before you can comment on or make changes to this bug.