409500 - Crash when serializing security info to disk cache [@ memcpy - nsStringInputStream::SetData]

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Radek 'sysKin' Czyz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122105 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007122105 Minefield/3.0b3pre

I've been hitting this new crasher when using my bank website.

Reproducible: Sometimes

Steps to Reproduce:
1. Set browser.cache.disk_cache_ssl = true
2. Visit https://www3.netbank.commbank.com.au/netbank/bankmain
3. Keep highlighting the url and pressing enter to reaload the page
Actual Results:  
bp-c6c0970f-aeb7-11dc-9747-001a4bd43e5c
bp-6bf0588a-adda-11dc-adfb-001a4bd46e84
..and friends

Expected Results:  
No crash :)

I could not crash under debugger. I tried other https websites and they don't crash - luckily the login page of my bank is enough (although navigation later on crashes more easily, I think).

Comment 1

[timeless]

0  	msvcr80.dll@0x151ac  	
1 	nsStringInputStream::SetData(char const*, int) 	mozilla/xpcom/io/nsStringStream.cpp:189
2 	NS_NewByteInputStream(nsIInputStream**, char const*, int, nsAssignmentType) 	mozilla/xpcom/io/nsStringStream.cpp:366
3 	NS_NewCStringInputStream(nsIInputStream**, nsACString_internal const&) 	mozilla/xpcom/io/nsStringStream.cpp:412
4 	NS_DeserializeObject(nsACString_internal const&, nsISupports**) 	mozilla/netwerk/base/src/nsSerializationHelper.cpp:82
5 	nsDiskCacheEntry::CreateCacheEntry(nsCacheDevice*) 	mozilla/netwerk/cache/src/nsDiskCacheEntry.cpp:90

Comment 2

[Jesse Ruderman]

Bug 458340 may be related.

Comment 3

[Colin Blake]

I am seeing a similar crash too at one of our customer sites:

00 msvcr90!TrailUpVec+0x18
01 xpcom_core!nsStringInputStream::SetData+0x42
02 xpcom_core!NS_NewByteInputStream+0x6c
03 xpcom_core!NS_NewCStringInputStream+0x1a
04 necko!NS_DeserializeObject+0x54
05 necko!nsDiskCacheEntry::CreateCacheEntry+0xce
06 necko!nsDiskCacheDevice::FindEntry+0x83
07 necko!nsCacheService::SearchCacheDevices+0x61
08 necko!nsCacheService::ActivateEntry+0x84
09 necko!nsCacheService::ProcessRequest+0x2d
0a necko!nsCacheService::OpenCacheEntry+0x60
0b necko!nsCacheSession::OpenCacheEntry+0x19
0c necko!nsHttpChannel::OpenCacheEntry+0x1ce
0d necko!nsHttpChannel::Connect+0x106
0e necko!nsHttpChannel::AsyncOpen+0x11e
0f gklayout!nsScriptLoader::ProcessScriptElement+0x8fe
10 gklayout!nsScriptElement::MaybeProcessScript+0x6d
11 gklayout!nsHTMLScriptElement::MaybeProcessScript+0x1e
12 gklayout!nsHTMLScriptElement::DoneAddingChildren+0xc
13 gklayout!HTMLContentSink::ProcessSCRIPTEndTag+0x59
14 gklayout!SinkContext::CloseContainer+0x134
15 gklayout!HTMLContentSink::CloseContainer+0x2f
16 gkparser!CNavDTD::CloseContainer+0xa1
17 gkparser!CNavDTD::HandleEndToken+0x27c
18 gkparser!CNavDTD::HandleToken+0x2df
19 gkparser!CNavDTD::BuildModel+0x177
1a gkparser!nsParser::BuildModel+0x6c
1b gkparser!nsParser::ResumeParse+0xbe
1c gkparser!nsParser::ContinueInterruptedParsing+0x31
1d gklayout!nsContentSink::ContinueInterruptedParsingIfEnabled+0x1d
1e gklayout!nsRunnableMethod<nsJSChannel>::Run+0xd
1f xpcom_core!nsThread::ProcessNextEvent+0xc3
20 xpcom_core!NS_ProcessNextEvent_P+0x20
21 gkwidget!nsBaseAppShell::Run+0x2a
22 tkitcmps!nsAppStartup::Run+0x1e
23 xul!XRE_main+0x1360
24 HsEngine!NS_internal_main+0xc1
25 HsEngine!wmain+0xf3
26 HsEngine!__tmainCRTStartup+0x10f
27 kernel32!BaseProcessStart+0x23

Comment 4

[Boris Zbarsky [:bzbarsky]]

So... NS_DeserializeObject (introduced in bug 262116) is just broken.  In particular, it computes |size| to be _sufficient_ to hold the base64-decoded data, then decodes the data, then uses |size| as the correct datasize.  It's only the correct datasize if the original data had a size that's 0 mod 3.  Otherwise, it's too big.  The correct way to compute the datasize can be found in nsDataChannel::OpenContentStream, say (or various other consumers of PL_Base64Decode).  It's too bad NSPR's API here sucks so much.

In any case, we end up passing a length that's bigger than the actual allocated size of the buffer (but only when the original data size was 1 mod 3, due to the null that NSPR allocates for us).  Then the string input stream tries to make a copy and memcpys that one extra byte that's outside the buffer.  That's presumably when the crash (or UMR in bug 458340) would hit.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Actally looking for the right char too

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/d1f1ad39e10e

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

We should fix this on branches...

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

>+  size = (size * 3) / 4;

If the string length is > 0x55555555 then this overflows to a small number. Does something upstream from here ensure that the object won't be that big?

Comment 11

[Boris Zbarsky [:bzbarsky]]

I don't know for sure, but if |size| ends up too small (possibly only if some of the objects are serializing data under web page control like URIs), then we'll just fail to deserialize the object (since the stream will end before we have all the data we expect to get).  Presumably the upstream code can already deal with that, since deserialization failure can happen for various reasons...

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz

code-freeze is Sept 22. If this misses we'll push it to the next cycle.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/14cd5d57b639

Checking in netwerk/base/src/nsSerializationHelper.cpp;
/cvsroot/mozilla/netwerk/base/src/nsSerializationHelper.cpp,v  <--  nsSerializationHelper.cpp
new revision: 1.2; previous revision: 1.1
done

Comment 14

[Al Billings [:abillings - ex-MoCo]]

I can't get this to crash in 1.9.0.14 at all.

Comment 15

[Jesse Ruderman on Windows]

Al, this bug never had much of a reproducible testcase.  You might have better luck under Valgrind (see bug 458340, a dup of this bug).

Comment 16

[Benjamin Smedberg]

Comment on attachment 400946 [details] [diff] [review]
Actally looking for the right char too

blocking doesn't need approval

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/297281de34e1

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #15)
> Al, this bug never had much of a reproducible testcase.  You might have better
> luck under Valgrind (see bug 458340, a dup of this bug).

Jesse, I'm not familiar with Valgrind. So it's kinda hard for me to verify. Would it be easy a fast for you to run a check? You have reported bug 458340 which has been duped. Thanks.

Comment 19

[Jesse Ruderman]

Bug 458340 was a one-off occurrence, so I can't really retest that.  I tested the steps in comment 0 and didn't see any cache-related warnings from Valgrind.  V.