Last Comment Bug 635977 - (CVE-2011-0075) crash with bad iframe source
: crash with bad iframe source
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: x86_64 All
: P1 critical (vote)
: mozilla2.0
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Patrick McManus [:mcmanus]
Depends on:
  Show dependency treegraph
Reported: 2011-02-22 13:08 PST by Aki Helin
Modified: 2016-01-19 16:51 PST (History)
7 users (show)
rforbes: sec‑bounty+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

triggering file (20 bytes, text/html)
2011-02-22 13:11 PST, Aki Helin
no flags Details
Patch (3.96 KB, patch)
2011-02-22 15:55 PST, Boris Zbarsky [:bz] (still a bit busy)
jduell.mcbugs: review+
benjamin: approval2.0+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description Aki Helin 2011-02-22 13:08:04 PST
User-Agent:       Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
Build Identifier: 4.0b11

Firefox 4.0b11 crashes in libxul with a segfault on a bad and possibly controllable read when given an iframe with source address ";%80".

Reproducible: Always

Steps to Reproduce:
1. echo '<iframe src=";%80">' > page.html
2. ...
3. firefox page.html
Actual Results:  
Firefox is sorry it followed a bad pointer.

Expected Results:  
Firefox should only follow good pointers.

The above minimal case crashes with:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xeffffb70 (LWP 31736)]
0xf71dafaf in ?? () from ./
=> 0xf71dafaf:	mov    (%edx,%eax,4),%edi
with eax = -1, edx = 0

Different files causing this seem to have edx=0, but eax as different values like -256975184, which make this look less like a harmless null deref. Filing conservatively as a security issue.
Comment 1 Aki Helin 2011-02-22 13:11:37 PST
Created attachment 514285 [details]
triggering file
Comment 2 Benjamin Smedberg [:bsmedberg] 2011-02-22 13:13:26 PST
I tried to reproduce this with and your attached testcase and I did not crash on any OS with nightly builds or b11. Do you have any extensions installed? Can you reproduce this in a new profile?
Comment 3 Aki Helin 2011-02-22 13:20:43 PST
No crash here either when following the link, but if the file is saved and opened locally, Firefox crashes when opening it. Curious.

No extensions installed here. The computer has 64-bit Ubuntu 10.10. After removing $HOME/.mozilla, first load did not crash, but all the subsequent ones did.
Comment 4 Aki Helin 2011-02-22 13:23:50 PST
Crash state hasn't opened here yet, but one should be at:
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-02-22 13:52:41 PST
I do see a crash here, from local file.  It's preceded by two assertions:

###!!! ASSERTION: Got Stop on wrong stream.: 'mRequest == aRequest', file /Users/bzbarsky/mozilla/vanilla/mozilla/parser/html/nsHtml5StreamParser.cpp, line 751
###!!! ASSERTION: Stream ended without being open.: 'STREAM_BEING_READ == mStreamState', file /Users/bzbarsky/mozilla/vanilla/mozilla/parser/html/nsHtml5StreamParser.cpp, line 706

The actual crash has happened in three different places so far, but I bet if we fix those asserts we'll fix the crash too.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-02-22 13:53:09 PST
Oh, I see this on Mac.
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-02-22 13:57:32 PST
It's quite interesting that I can reliably crash with breakpad saying things like "No proper signature could be created because no good data for the crashing thread (7) was found". Ted might be interested in that.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-02-22 14:01:01 PST
In this case what seems to be happening is that nsHtml5StreamParser::OnStopRequest fires for the subframe URI when mRequest is null.  At this point mStreamState is STREAM_NOT_STARTED.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-02-22 15:22:50 PST
OK, this is a bug in nsIndexedToHTML (hence the local-only aspect).  In particular, there are all sorts of cases in which nsIndexedToHTML::OnStartRequest has all sorts of cases in which it takes early returns without calling OnStartRequest on mListener.  One of these is the "unescape the filename failed" case, which is what we're hitting here, depending on the filesystem charset (which is why I think Benjamin can't reproduce).  But nsIndexedToHTML::OnStopRequest always calls OnStopRequest on the listener.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-02-22 15:55:06 PST
Created attachment 514345 [details] [diff] [review]

I have no idea how to write a testcase for this yet.
Comment 11 Jason Duell [:jduell] (needinfo me) 2011-02-23 05:02:16 PST
Comment on attachment 514345 [details] [diff] [review]

Looks good.  If this passes manual testing, I'd be fine not having an automated test.  This is a simple patch, and it clears up a large set of obviously incorrect return paths.

>+    // Push buffer to the listener now, so the initial HTML will not
>+    // be parsed in OnDataAvailable().

Is it just me or does this comment belong above the call to FormatInputStream, not here?  We won't be pushing to the listener if OnStartRequest fails.  I'm also not clear on how "the initial HTML would get parsed" if we didn't call FormatInputStream, but whatever.  Seems like "Parse initial data and call OnDataAvailable" is a more accurate summary.  But I don't know this code that well.

>+    rv = mListener->OnStartRequest(request, aContext);
>+    if (NS_FAILED(rv)) return rv;
>+    // The request may have been canceled, and if that happens, we want to
>+    // suppress calls to OnDataAvailable.
>+    request->GetStatus(&rv);
>+    if (NS_FAILED(rv)) return rv;
>+    rv = FormatInputStream(request, aContext, buffer);
>+    return rv;
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-02-23 09:11:30 PST
> Is it just me or does this comment belong above the call to FormatInputStream,

Yes.  I've moved it there and changed it to just say "Push our buffer to the listener".

I have no idea what that comment was trying to say; it's part of the initial checkin of the file in 2001.  Ccing dougt in case he happens to recall what he meant.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-02-23 09:13:19 PST
Comment on attachment 514345 [details] [diff] [review]

Requesting 2.0 approval.   This should be a very safe change and it fixes what looks like a possibly exploitable crash that can happen when loading local HTML files.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-02-23 09:14:14 PST
Comment on attachment 514345 [details] [diff] [review]

This applies to both branches too.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-02-24 10:47:01 PST
Comment 16 Daniel Veditz [:dveditz] 2011-02-25 10:37:42 PST
Comment on attachment 514345 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Comment 17 Daniel Veditz [:dveditz] 2011-02-25 10:48:57 PST
FTP directory listings also use nsIndexedToHTML. Could this be remotely triggered on an FTP site? If so, might be worse than sg:moderate.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2011-02-25 11:01:22 PST
Hmm.  Yes, FTP may be able to cause the same issues.
Comment 20 Daniel Veditz [:dveditz] 2011-03-04 10:15:09 PST
The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.
Comment 23 chris hofmann 2011-05-20 11:36:08 PDT
This is moderate so usually wounldn't qualify for a bounty, but we will award $500 based on the possibilty that the ftp idea could be turned into an exploit PoC.  Aki, if you wanted to try that we could up the bounty on this.
Comment 24 Raymond Forbes[:rforbes] 2013-07-19 18:43:31 PDT

Note You need to log in before you can comment on or make changes to this bug.