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

RESOLVED FIXED in Thunderbird 42.0

Status

MailNews Core
Backend
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: tahir, Assigned: rkent)

Tracking

(4 keywords)

Other Branch
Thunderbird 42.0
crash, csectype-dos, sec-low, testcase

Thunderbird Tracking Flags

(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)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8626518 [details]
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.

Comment 1

2 years ago
Do you have a crash report ID?
Flags: needinfo?(tahir)
(Reporter)

Comment 2

2 years ago
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)
(Assignee)

Comment 5

2 years ago
I save the attachment as a .eml file and opened it in Thunderbird 38.0.1 without difficulties (including no crash).
(Reporter)

Comment 6

2 years ago
Several crash report ID's below.

bp-8af6930e-720b-4fd8-a9f0-510062150626	2015-06-26	10:51
bp-75a23ac8-4ec5-46d4-a4cc-912b72150626	2015-06-26	10:50
bp-0c2f8218-3570-42da-a3c8-898012150626	2015-06-26	10:49
bp-7b1a931b-bb61-44d9-baa3-10c4f2150626	2015-06-26	10:31
bp-7cc5814e-9603-4740-b442-a90de2150626	2015-06-26	10:28
bp-b29b8293-c26c-40a3-a001-395932150625	2015-06-26	00:41
bp-2d7a41ae-490a-4ef2-8641-277742150625	2015-06-25	23:44
bp-e2d03b8b-756f-4899-a3d0-95fb72150625	2015-06-25	16:59
bp-c86141bd-0e3e-4963-bf72-c9ea82150625	2015-06-25	14:52
bp-404385fe-80f5-4bba-8006-3f3192150625	2015-06-25	12:18
bp-ef81a55b-8161-41a1-ab84-4ea9b2150624	2015-06-24	21:20
bp-66a83f59-14ad-44b4-a15d-2ae532150624	2015-06-24	21:15
bp-fa0089e7-f7b6-42d0-bdfc-3b7bb2150624	2015-06-24	21:10
bp-0d946714-f84e-4514-a0fc-e14e92150624	2015-06-24	21:01
bp-c75cd4f4-2419-4a72-8e7e-995212150624	2015-06-24	20:59
bp-f69a7127-5995-43f8-bf03-04afe2150624	2015-06-24	20:47
bp-b143f6c9-1030-4796-981b-592862150624	2015-06-24	20:19
Flags: needinfo?(tahir)

Comment 7

2 years ago
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)
(Reporter)

Comment 8

2 years ago
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) ]
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 10

2 years ago
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.
status-thunderbird_esr38: --- → affected
tracking-thunderbird_esr38: --- → 39+
(Assignee)

Comment 11

2 years ago
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)

Updated

2 years ago
Assignee: nobody → rkent
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

2 years ago
See Also: → bug 1175190
(Assignee)

Comment 12

2 years ago
Created attachment 8628139 [details] [diff] [review]
1177702.patch
(Assignee)

Updated

2 years ago
Attachment #8628139 - Flags: review?(mkmelin+mozilla)
The crashes here are benign from a security POV.
Keywords: crash, csectype-dos, sec-low, testcase
(Assignee)

Comment 14

2 years ago
Daniel: Is there any reason we need to keep this as a secure bug? It makes it more difficult to work on.

Comment 15

2 years ago
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+
(Assignee)

Comment 16

2 years ago
(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.
(Assignee)

Comment 17

2 years ago
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+
(Assignee)

Updated

2 years ago
status-thunderbird39: --- → wontfix
status-thunderbird40: --- → affected
status-thunderbird41: --- → affected
status-thunderbird42: --- → fixed
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 42.0
(Assignee)

Comment 18

2 years ago
http://hg.mozilla.org/releases/comm-aurora/rev/6cd8e8a55d5f
http://hg.mozilla.org/releases/comm-beta/rev/253dda854734
http://hg.mozilla.org/releases/comm-esr38/rev/1b47ff562afc
status-seamonkey2.35: --- → affected
status-seamonkey2.36: --- → affected
status-seamonkey2.37: --- → fixed
status-seamonkey2.38: --- → fixed
status-seamonkey2.39: --- → fixed
status-thunderbird40: affected → fixed
status-thunderbird41: affected → fixed
status-thunderbird_esr38: affected → fixed
(Reporter)

Comment 19

2 years ago
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?

Comment 20

2 years ago
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
(Reporter)

Comment 21

2 years ago
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)

Comment 22

2 years ago
(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

Updated

2 years ago
Severity: normal → critical
(Reporter)

Comment 23

2 years ago
Sure, the same testcase.
Sure, I will file a new bug.

Thanks.

Updated

2 years ago
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.