717121 - crash nsMIMEHeaderParamImpl::DoParameterInternal

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Makoto Kato [:m_kato]]

Possible overrun.  When line 278, *str may be null since for-loop exits by *str == null.

272     // Skip forward to the end of this token. 
273     for (; *str && !nsCRT::IsAsciiSpace(*str) && *str != '=' && *str != ';'; str++)
274       ;
275     tokenEnd = str;
276
277     // Skip over whitespace, '=', and whitespace
278     while (nsCRT::IsAsciiSpace(*str)) ++str; 



This bug was filed from the Socorro interface and is 
report bp-3afb8381-e7af-49d4-ba83-3615f2120109 .
============================================================= 
0 	xul.dll 	nsMIMEHeaderParamImpl::DoParameterInternal 	netwerk/mime/nsMIMEHeaderParamImpl.cpp:278
1 	xul.dll 	nsMIMEHeaderParamImpl::GetParameterInternal 	netwerk/mime/nsMIMEHeaderParamImpl.cpp:187
2 	xul.dll 	MimeHeaders_get_parameter 	mailnews/mime/src/mimehdrs.cpp:527
3 	xul.dll 	MimeHeaders_get_name 	mailnews/mime/src/mimehdrs.cpp:738
4 	xul.dll 	MimeObject_write 	mailnews/mime/src/mimei.cpp:1791
5 	xul.dll 	MimeInlineTextPlain_parse_line 	mailnews/mime/src/mimetpla.cpp:477
6 	xul.dll 	MimeInlineText_convert_and_parse_line 	mailnews/mime/src/mimetext.cpp:442
7 	xul.dll 	MimeInlineText_rotate_convert_and_parse_line 	mailnews/mime/src/mimetext.cpp:570
8 	xul.dll 	convert_and_send_buffer 	mailnews/mime/src/mimebuf.cpp:184
9 	xul.dll 	mime_LineBuffer 	mailnews/mime/src/mimebuf.cpp:272
10 	xul.dll 	MimeInlineText_parse_decoded_buffer 	mailnews/mime/src/mimetext.cpp:358
11 	xul.dll 	MimeLeaf_parse_buffer 	mailnews/mime/src/mimeleaf.cpp:184
12 	xul.dll 	MimeMultipart_parse_child_line 	mailnews/mime/src/mimemult.cpp:686
13 	xul.dll 	MimeMultipart_parse_line 	mailnews/mime/src/mimemult.cpp:340
14 	xul.dll 	convert_and_send_buffer 	mailnews/mime/src/mimebuf.cpp:184
15 	xul.dll 	mime_LineBuffer 	mailnews/mime/src/mimebuf.cpp:272
16 	xul.dll 	MimeObject_parse_buffer 	mailnews/mime/src/mimeobj.cpp:275
17 	xul.dll 	MimeMessage_parse_line 	mailnews/mime/src/mimemsg.cpp:233
18 	xul.dll 	convert_and_send_buffer 	mailnews/mime/src/mimebuf.cpp:184
19 	xul.dll 	mime_LineBuffer 	mailnews/mime/src/mimebuf.cpp:272
20 	xul.dll 	MimeObject_parse_buffer 	mailnews/mime/src/mimeobj.cpp:275
21 	xul.dll 	mime_display_stream_write 	mailnews/mime/src/mimemoz2.cpp:1004
22 	xul.dll 	nsStreamConverter::OnDataAvailable 	mailnews/mime/src/nsStreamConverter.cpp:979
23 	xul.dll 	nsImapCacheStreamListener::OnDataAvailable 	mailnews/imap/src/nsImapProtocol.cpp:8633
24 	xul.dll 	nsInputStreamPump::OnStateTransfer 	netwerk/base/src/nsInputStreamPump.cpp:510
25 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:400
26 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:114
27 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
28 	xul.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:245
29 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
30 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:208
31 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:201
32 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:175
33 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
34 	xul.dll 	nsAppShell::Run 	widget/src/windows/nsAppShell.cpp:257
35 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:228
36 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3551
37 	thunderbird.exe 	do_main 	mail/app/nsMailApp.cpp:143
38 	thunderbird.exe 	NS_internal_main 	mail/app/nsMailApp.cpp:226
39 	thunderbird.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:107

Comment 1

[Makoto Kato [:m_kato]]

ah, comment #1 will be invalid.

MimeHeaders_get_name may return string that isn't null teminated.

Comment 2

[Wayne Mery (:wsmwk)]

all crashes are version 10 [1] 
=> regression?

[1] https://crash-stats.mozilla.com/report/list?product=Thunderbird&query_search=signature&query_type=exact&query=nsMIMEHeaderParamImpl%3A%3ADoParameterInternal%28char%20const*%2C%20char%20const*%2C%20nsMIMEHeaderParamImpl%3A%3AParamDecoding%2C%20char**%2C%20char**%2C%20char**%29&reason_type=contains&date=01%2F11%2F2012%2019%3A59%3A18&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=nsMIMEHeaderParamImpl%3A%3ADoParameterInternal%28char%20const*%2C%20char%20const*%2C%20nsMIMEHeaderParamImpl%3A%3AParamDecoding%2C%20char**%2C%20char**%2C%20char**%29

Comment 3

[Makoto Kato [:m_kato]]

(In reply to Wayne Mery (:wsmwk) from comment #2)
> all crashes are version 10 [1] 
> => regression?

I don't know...

Comment 4

[:aceman]

(In reply to Makoto Kato from comment #1)
> ah, comment #1 will be invalid.
> 
> MimeHeaders_get_name may return string that isn't null teminated.

Did you mean comment 0?

Comment 5

[Makoto Kato [:m_kato]]

(In reply to :aceman from comment #4)
> (In reply to Makoto Kato from comment #1)
> > ah, comment #1 will be invalid.
> > 
> > MimeHeaders_get_name may return string that isn't null teminated.
> 
> Did you mean comment 0?

Ah, yes.

This crash will occur if last character of parameter of Content-Disposition header is "'". if last is "'", nsMIMEHeaderParamImpl::DoParameterInternal will skip next null-terminator.

Comment 6

[Wayne Mery (:wsmwk)]

#2 crash for tbird v10.
earliest build with crash is 2011122210. does this help hit the regression?

firefox crash almost zero - only 2, and stack is different bp-dd4f4e30-5c2f-4835-846a-515d52120201

Comment 7

[Makoto Kato [:m_kato]]

Attachment: fix

test case is possible crash that depends on memory allocator.

Comment 8

[Makoto Kato [:m_kato]]

Attachment: fix v1.1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 594023 [details] [diff] [review]
fix v1.1

r=me

Comment 10

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 594070 [details] [diff] [review]
fix v2

bz, sorry.  previous fix has typo.  flag was invalid....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 594070 [details] [diff] [review]
fix v2

r=me

Comment 13

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f65800b074ac

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f65800b074ac

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 594070 [details] [diff] [review]
fix v2

[Approval Request Comment]
Regression caused by (bug #): Probably bug 692574 or bug 703015.
User impact if declined: This exhibits a top-crash for Thunderbird.
Testing completed (on m-c, etc.): Has landed on mozilla-central, will be shipped in TB 10.0.1.
Risk to taking this patch (and alternatives if risky): Adds a check for overruning the string length. Includes a test, so should be low risk.
String changes made by this patch: None

Note: I would also like this to be taken on mozilla-esr10 (no approval flags yet).

Comment 16

[Julian Reschke]

I'm not convinced the problem is fully understood.

I tested C-D header field values of both

  attachment; filename="

and

  attachment; filename='

and they do not cause a crash. I also notice that the test case tests a trailing double quote, while comment 5 talks about trailing single quotes.

Do we have a concrete test case that causes that crash?

Comment 17

[David :Bienvenu]

Reading off the end of a block of allocated memory isn't guaranteed to crash; you have to have a page fault, which makes it trickier to do. to get the test to fail, you can do tricks like memory mapping a file - we have tests that do this, but I can't find them at the moment.

Comment 18

[Makoto Kato [:m_kato]]

(In reply to Julian Reschke from comment #16)
> I'm not convinced the problem is fully understood.
> 
> I tested C-D header field values of both
> 
>   attachment; filename="
> 
> and
> 
>   attachment; filename='
> 
> and they do not cause a crash. I also notice that the test case tests a
> trailing double quote, while comment 5 talks about trailing single quotes.
> 
> Do we have a concrete test case that causes that crash?

This test case is possible crash.  Because this crash depends on memory allocator.  jemalloc allocates aligned size instead of requested size.

It is no way to crash easily.  But this fixes possible overrun.

Comment 19

[Alex Keybl [:akeybl]]

Comment on attachment 594070 [details] [diff] [review]
fix v2

[Triage Comment]
This has had time to bake on m-c, and is deemed low risk TB top crasher. I'm adding the tracking flag for ESR, although that'll be changing soon. See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Comment 20

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c05502ba4ac

Comment 21

[Makoto Kato [:m_kato]]

http://hg.mozilla.org/releases/mozilla-beta/rev/e5ec1b3295b7

Comment 22

[Alex Keybl [:akeybl]]

Please land on mozilla-esr10 as soon as possible. For more information on the landing process, please see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Comment 23

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/0adde0acc3f5

Comment 24

[u279076]

Does this only affect Thunderbird, or is there something QA can do to verify this fix in Firefox?

Comment 25

[Makoto Kato [:m_kato]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #24)
> Does this only affect Thunderbird, or is there something QA can do to verify
> this fix in Firefox?

bp-9aff268d-cd60-465d-a662-313802120218 is Firefox issue.  But this is no easy way to verify this.  crash test is possible case.

But there is no crash on Thunderbird 10.0.1 and 10.0.2.  so I think that this is fixed.

Comment 26

[u279076]

Calling this qa- based on comment 25.