717242 - manifest.appcache with an invalid domain in it will reliably crash firefox (once the URL is approved for offline)

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[arno renevier]

Hi,
I am on a site with a manifest cache file containing many urls.
If I close firefox while the manifest is updating, firefox sometimes crashes.
Here is a crash stack:

InitOfflineCacheEntry for 0x7fffd7cab800 ((nil))

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff6121212 in nsTArray<nsHttpHeaderArray::nsEntry, nsTArrayDefaultAllocator>::IndexOf<nsHttpAtom, nsHttpHeaderArray::nsEntry::MatchHeader> ()
   from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
(gdb) where 
#0  0x00007ffff6121212 in nsTArray<nsHttpHeaderArray::nsEntry, nsTArrayDefaultAllocator>::IndexOf<nsHttpAtom, nsHttpHeaderArray::nsEntry::MatchHeader> ()
   from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#1  0x00007ffff612125f in nsHttpHeaderArray::LookupEntry(nsHttpAtom, nsHttpHeaderArray::nsEntry**) ()
   from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#2  0x00007ffff61212c1 in nsHttpHeaderArray::GetHeader(nsHttpAtom, nsACString_internal&) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#3  0x00007ffff6138200 in nsHttpChannel::AddCacheEntryHeaders(nsICacheEntryDescriptor*) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#4  0x00007ffff613863b in nsHttpChannel::InitOfflineCacheEntry() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#5  0x00007ffff613b91b in nsHttpChannel::CallOnStartRequest() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#6  0x00007ffff613ba18 in nsHttpChannel::ContinueOnStartRequest2(unsigned int) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#7  0x00007ffff613e7c5 in nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#8  0x00007ffff60d09d9 in nsInputStreamPump::OnStateStart() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#9  0x00007ffff60d0c3b in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#10 0x00007ffff6a40126 in nsInputStreamReadyEvent::Run() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#11 0x00007ffff6a4e359 in nsThread::ProcessNextEvent(bool, bool*) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#12 0x00007ffff6a237cd in NS_ProcessNextEvent_P(nsIThread*, bool) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#13 0x00007ffff6a4e810 in nsThread::Shutdown() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#14 0x00007ffff60e3f7e in nsSocketTransportService::Shutdown() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#15 0x00007ffff60d27fb in nsIOService::SetOffline(bool) () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#16 0x00007ffff60d3924 in nsIOService::Observe(nsISupports*, char const*, unsigned short const*) ()
   from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#17 0x00007ffff6a31522 in nsObserverList::NotifyObservers(nsISupports*, char const*, unsigned short const*) ()
   from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#18 0x00007ffff6a317d4 in nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) ()
   from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#19 0x00007ffff60bd99b in nsXREDirProvider::DoShutdown() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#20 0x00007ffff60b89de in ScopedXPCOMStartup::~ScopedXPCOMStartup() () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#21 0x00007ffff60bd0e4 in XRE_main () from /home/arno/mozilla/objdir/xulrunner/dist/bin/libxul.so
#22 0x0000000000402844 in main ()


Actually, mResponseHead is null when entering nsHttpChannel::InitOfflineCacheEntry, thenmResponseHead->GetHeader(nsHttp::Vary, buf); will crash (in  AddCacheEntryHeaders).

At first look, it looks similar to 551990, so I'm ccing dholbert.

Comment 1

[Daniel Holbert [:dholbert]]

(I don't really know anything about bug 551990; I just happened to be the one to find it in a tinderbox log & file it. :))

Comment 2

[arno renevier]

oh, sorry

Comment 3

[Scoobidiver (away)]

Dupe of bug 609895 which is currently about Fennec?

Comment 4

[Josh Matthews [:jdm]]

I'd like to keep this bug separate, as it appears to have really good STR.

Comment 5

[Honza Bambas (:mayhemer)]

We may want to change 'if (mResponseHead && mResponseHead->NoStore())' to 'if (!mResponseHead || mResponseHead->NoStore())' in nsHttpChannel::InitOfflineCacheEntry().  

I think it is quit OK, based on:
- InitOfflineCacheEntry gets called only when we are downloading the manifest or its items, thus when there is no response (server connect failed) there is also nothing to write and we are OK to close (actually it will doom in this case) w/o update of the entry
- remaining calls are made only when there is a response head

Comment 6

[Honza Bambas (:mayhemer)]

-> me

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

I am seeing a similar crash in b2g on startup that I think may have the same root cause.  Namely, inside, nsHttpChannel::AddCacheEntryHeaders on line 3220:
  mResponseHead->GetHeader(nsHttp::Vary, buf);
is being called where mResponseHead is null.

(gdb) p mResponseHead
$6 = {
  mRawPtr = 0x0
}

I am serving B2G via apache2 on localhost, and the http_trace looks like so:

15:55:30.785 browser:34067 -> browser:80 #1 HTTP 1.1 request: GET /manifest.appcache
    Host: homescreen.gaia.localhost
    User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120418 Firefox/14.0a1
    Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
    Accept-Language: en-us,en;q=0.5
    Accept-Encoding: gzip, deflate
    Connection: keep-alive
    Referer: http://homescreen.gaia.localhost/
    X-Moz: offline-resource
15:55:30.785 browser:80 -> browser:34067 #1 HTTP 1.1 response: 200 OK
    Date: Wed, 18 Apr 2012 22:55:30 GMT
    Server: Apache/2.2.22 (Ubuntu)
    Last-Modified: Wed, 18 Apr 2012 22:23:52 GMT
    ETag: "8e7450-c38-4bdfb829ab9f7"
    Accept-Ranges: bytes
    Content-Length: 3128
    Cache-Control: max-age=3
    Expires: Wed, 18 Apr 2012 22:55:33 GMT
    Keep-Alive: timeout=5, max=100
    Connection: Keep-Alive
    Content-Type: text/cache-manifest

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> We may want to change 'if (mResponseHead && mResponseHead->NoStore())' to
> 'if (!mResponseHead || mResponseHead->NoStore())' in
> nsHttpChannel::InitOfflineCacheEntry().  

I made this change locally and this fixes the crashing situation for me!  Do we need a unit test for this fix?

Comment 9

[Honza Bambas (:mayhemer)]

Andrew, please convert your change to a patch and take this bug.  We may create a test later.  I'm not sure how a test can that easily be done atm..

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Attachment: do not try to add headers when there is no response, avoiding a crash

Here's the patch that works for me which coincidentally is also exactly what you proposed... :)

I'm asking you for review as you are a valid peer, but if you think someone else with deeper knowledge of the code who didn't also propose the solution might be a better reviewer, please do that.

It appears that in my specific situation, what is happening is that I messed up my /etc/hosts file and so an NS_ERROR_UNKNOWN_HOST is received when the fetch gets down to http://gaia.localhost/webapi.js in manifest.appcache.  Every entry preceding it in the list was domain-relative and so successful.  This sounds consistent with your analysis.

https://tbpl.mozilla.org/?tree=Try&rev=88ef4b8e9bc3

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

So, this is now a highly reproducible crash, so I'm marking it private.  This may be the wrong checkbox.

There is a mitigating factor, in that I believe you have to actually say "yes" to an infobar before it will crash your computer, but since that gets persisted so that any future attempt to hit the URL will crash you, I say it's fairly annoying.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

I e-mailed [e-mail] requesting that they triage this properly to a security bug or clear the project bug if crashers aren't all that bad.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 616683 [details] [diff] [review]
do not try to add headers when there is no response, avoiding a crash

Forwarding to Patrick.  It's not allowed to review my own patch :)

See comment 5 for reasoning.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

I'm not sure what to do about landing this since it did get marked as a security bug...  Do we request approval for actively supported branches before landing and synchronize landing?  Honza, will you take it from here since it's your patch? :) :)

Since it's a null-pointer-subscripted-dereference (if we don't abort ourself on mRawPtr being 0, we will take some subset on 0), I don't think this qualifies as exploitable and it's mainly a question of griefers having a way to crash firefox if they can induce a bad (or perhaps unresponsive) domain in an authorized manifest.appcache.  So a bogus manifest or I suppose attacking the DNS or server infrastructure referenced by a valid manifest.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

If it's not exploitable, we don't need to keep this hidden or worry about special landing procedures. There are plenty of ways to crash Firefox for DoS purposes, one more isn't anything special :)

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 616683 [details] [diff] [review]
do not try to add headers when there is no response, avoiding a crash

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any): none

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 616683 [details] [diff] [review]
do not try to add headers when there is no response, avoiding a crash

mozilla-central is open again, you don't need approval.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 616683 [details] [diff] [review]
do not try to add headers when there is no response, avoiding a crash

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a83239d5572

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/9a83239d5572