Closed Bug 635977 (CVE-2011-0075) Opened 11 years ago Closed 11 years ago

crash with bad iframe source


(Core :: Networking, defect, P1)




Tracking Status
blocking1.9.2 --- needed
status1.9.2 --- .17-fixed
blocking1.9.1 --- needed
status1.9.1 --- .19-fixed


(Reporter: aki.helin, Assigned: bzbarsky)


(Whiteboard: [sg:moderate])


(2 files)

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.
Attached file triggering file
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?
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.
Crash state hasn't opened here yet, but one should be at:
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.
Component: General → HTML: Parser
Ever confirmed: true
QA Contact: general → parser
Oh, I see this on Mac.
OS: Linux → All
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.
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.
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.
Component: HTML: Parser → Networking
QA Contact: parser → networking
Attached patch PatchSplinter Review
I have no idea how to write a testcase for this yet.
Attachment #514345 - Flags: review?(jduell.mcbugs)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
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;
Attachment #514345 - Flags: review?(jduell.mcbugs) → review+
Whiteboard: [need review]
> 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 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.
Attachment #514345 - Flags: approval2.0?
Comment on attachment 514345 [details] [diff] [review]

This applies to both branches too.
Attachment #514345 - Flags: approval1.9.2.15?
Attachment #514345 - Flags: approval1.9.1.18?
Whiteboard: [sg:moderate]
Attachment #514345 - Flags: approval2.0? → approval2.0+
Whiteboard: [sg:moderate] → [need landing][sg:moderate]
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [need landing][sg:moderate] → [sg:moderate]
Target Milestone: --- → mozilla2.0
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
Comment on attachment 514345 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Attachment #514345 - Flags: approval1.9.2.15?
Attachment #514345 - Flags: approval1.9.2.15+
Attachment #514345 - Flags: approval1.9.1.18?
Attachment #514345 - Flags: approval1.9.1.18+
FTP directory listings also use nsIndexedToHTML. Could this be remotely triggered on an FTP site? If so, might be worse than sg:moderate.
Hmm.  Yes, FTP may be able to cause the same issues.
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.
Alias: CVE-2011-0075
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.
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.