Closed Bug 1177702 Opened 5 years ago Closed 5 years ago

seamonkey crashes via mozilla::mailnews::EncodedHeader when reading poorly encoded UTF8 from field

Categories

(MailNews Core :: Backend, defect, critical)

Other Branch
defect
Not set
critical

Tracking

(thunderbird39 wontfix, thunderbird40 fixed, thunderbird41 fixed, thunderbird42 fixed, thunderbird_esr3839+ fixed, seamonkey2.35 affected, seamonkey2.36 affected, seamonkey2.37 fixed, seamonkey2.38 fixed, seamonkey2.39 fixed)

RESOLVED FIXED
Thunderbird 42.0
Tracking Status
thunderbird39 --- wontfix
thunderbird40 --- fixed
thunderbird41 --- fixed
thunderbird42 --- fixed
thunderbird_esr38 39+ fixed
seamonkey2.35 --- affected
seamonkey2.36 --- affected
seamonkey2.37 --- fixed
seamonkey2.38 --- fixed
seamonkey2.39 --- fixed

People

(Reporter: tahir, Assigned: rkent)

References

Details

(4 keywords)

Crash Data

Attachments

(2 files)

Attached file msgSrc.txt
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 SeaMonkey/2.33.1
Build ID: 20150321194901

Steps to reproduce:

Receive an e-mail that contains poorly encoded UTF-8 characters. (A message source is attached. Some non-related parts are modified for privacy reasons.)


Actual results:

Seamonkey crashed.

(Thunderbird 38.0.1 also crashes.)


Expected results:

It should display the message.
Do you have a crash report ID?
Flags: needinfo?(tahir)
Several crash reports have been submitted by the crash reporter, but I'm unaware of the crash report ID. Where can I find it?
Thunderbird is trickier without an addon like "View About", but Seamonkey should support the about:crashes URL which will have links to your submissions.
Component: MailNews: Backend → Backend
Product: SeaMonkey → MailNews Core
Version: SeaMonkey 2.33 Branch → Other Branch
Need someone from mailnews to confirm.
Flags: needinfo?(vseerror)
I save the attachment as a .eml file and opened it in Thunderbird 38.0.1 without difficulties (including no crash).
Probably the same as fixed Bug 1175190 - Thunderbird 38 crashes in mozilla::mailnews::EncodedHeader
Crash Signature: [@ memcpy | nsTArray_Impl<T>::AppendElements<T>(mozilla::dom::PBlobStreamChild* const*, unsigned int) ]
Flags: needinfo?(vseerror)
Hi Kent James,

eml file should not be a problem as the content is valid html. The problematic part is the from field. "Cem Yusuf Aydoğdu" in UTF-8 must be encoded as "Q2VtIFl1c3VmIEF5ZG/En2R1" in Base64. Somehow, the "/" character is HTTP encoded an ended up as "=2F" which is still valid in Base64 alphabet.

However, when the BAse64 decoder tries to decode the whole from field to UTF-8 the msf file gets broken. So, displaying the file on its own is not a problem, but receiving the file from an IMAP folder and displaying in is a problem.
Crash Signature: [@ memcpy | nsTArray_Impl<T>::AppendElements<T>(mozilla::dom::PBlobStreamChild* const*, unsigned int) ]
Wayne is right in comment 7. You should try this in a build done after the patch for that landed. This landed in central, aurora, and beta on 2015-06-22. The SM people will have to tell you how to find a recent SM build that would include that patch.
From the instructions in comment 8 I was able to reproduce this crash in Thunderbird, and the patch for bug 1175190 does not solve it. With a test case should hopefully be an easy fix though.
The test case here is really a test case for bug 1175190, and demonstrates that the fix there did not work. Rather than open that bug, I'll just continue the work here.

The crash occurs in the .Adopt here:

  DebugOnly<nsresult> rv = headerParser->ParseEncodedHeader(aHeader, aCharset,
    false, &length, &addresses);
  MOZ_ASSERT(NS_SUCCEEDED(rv), "This should never fail!");
  if (length > 0) {
    retval.Adopt(addresses, length);
  }

The debugger shows that length = 1 and addresses is null.

I don't really understand that value of only doing the error checking and assert on debug builds. The "this should never fail" is false, the code does fail, and then the undefined values of addresses and length after the failure are passed to Adopt.

The failure by the way occurs in decode_base64(buffer, more) of jsmime.js on this line:

return [atob(sanitize), buffer];

because sanitize has an invalid "=" inside that is not caught by the sanitize function.

I think that the solution is to abandon the hubris of "this should never fail", check the return value, and not call the Adopt if there is a failure. We've already been surprised once by the values after failure!
Assignee: nobody → rkent
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
See Also: → 1175190
Attached patch 1177702.patchSplinter Review
Attachment #8628139 - Flags: review?(mkmelin+mozilla)
The crashes here are benign from a security POV.
Daniel: Is there any reason we need to keep this as a secure bug? It makes it more difficult to work on.
Comment on attachment 8628139 [details] [diff] [review]
1177702.patch

Review of attachment 8628139 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/mime/src/MimeHeaderParser.cpp
@@ +82,2 @@
>      &length, &addresses);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv), "Javascript jsmime returned an error!");

an assertion doesn't sound appropriate, since there's a known situation where it would happen.  a warning maybe?
same below of course
Attachment #8628139 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin from comment #15)
> Comment on attachment 8628139 [details] [diff] [review]
> 1177702.patch
> 
> Review of attachment 8628139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/mime/src/MimeHeaderParser.cpp
> @@ +82,2 @@
> >      &length, &addresses);
> > +  MOZ_ASSERT(NS_SUCCEEDED(rv), "Javascript jsmime returned an error!");
> 
> an assertion doesn't sound appropriate, since there's a known situation
> where it would happen.  a warning maybe?
> same below of course

Here's how I interpret this. The underlying javascript code is not supposed to be generating errors, in fact the error in this case is due to an inadequate regex used to sanitize base64 values. The MOZ_ASSERT is saying "hey this is not supposed to happen, put me into a debugger so that I can figure out what is going on". I think that is still appropriate, that is the underlying bug is not actually fixed here. I pondered trying to fix it, but instead I am mostly addressing here the crash, which could be caused by other errors. So I think that the crash fix is correct, while the underlying issue remains.

I'll compromise and file a followup bug for the underlying issue.
Comment on attachment 8628139 [details] [diff] [review]
1177702.patch

http://hg.mozilla.org/comm-central/rev/2da3cfd5d97b

We'll also uplift this.
Attachment #8628139 - Flags: approval-comm-esr38+
Attachment #8628139 - Flags: approval-comm-beta+
Attachment #8628139 - Flags: approval-comm-aurora+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 42.0
In reply to Comment 13 and Comment 14: I was the one who marked this as a secure bug at first. There is no data loss risk, I know. But there is a serious DoS attack vector here. A spammer can crash all Thunderbird/Seamonkey users with a tailor made from field. I assume we don't want it as the Mozilla community. Do we?
tahir, can you test a nightly seamonkey 2.39a1 build to confirm the crash is gone for you?
at the bottom of https://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk/
Blocks: 1175190
Crash Signature: [@ memcpy | nsTArray_Impl<T>::AppendElements<T>(mozilla::dom::PBlobStreamChild* const*, unsigned int) ]
Flags: needinfo?(tahir)
Summary: seamonkey crashes when reading poorly encoded UTF8 from field → seamonkey crashes via mozilla::mailnews::EncodedHeader when reading poorly encoded UTF8 from field
Yes, I'm using 2.39a1 nightly for a few days. Crash is gone for a few days. However, the from field is completely empty now. I am not sure if we should consider this as normal or as another bug? as soon as the original Base64 string was not normal.

In the webmail client for example, the from field was nearly readable. Still, it is rendered from a HTTP encoded string so it is a different story. I'm not sure if such a base64 string is recoverable at that point.
Flags: needinfo?(tahir)
(In reply to tahir from comment #21)
> Yes, I'm using 2.39a1 nightly for a few days. Crash is gone for a few days.
> However, the from field is completely empty now. I am not sure if we should
> consider this as normal or as another bug? as soon as the original Base64
> string was not normal.

Is this from the same type of testcase as was used to fix this bug?

In any even, please file a new bug report with the testcase
Severity: normal → critical
Sure, the same testcase.
Sure, I will file a new bug.

Thanks.
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.