635977 - (CVE-2011-0075) crash with bad iframe source

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Aki Helin]

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 ./libxul.so
=> 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]

Attachment: triggering file

Comment 2

[Benjamin Smedberg]

I tried to reproduce this with http://office.smedbergs.us/test.html 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]

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]

Crash state hasn't opened here yet, but one should be at:
  http://crash-stats.mozilla.com/report/index/bp-bdb29bc1-4c3c-433c-bc91-47acc2110222

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Oh, I see this on Mac.

Comment 7

[Benjamin Smedberg]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: Patch

I have no idea how to write a testcase for this yet.

Comment 11

[Jason Duell]

Comment on attachment 514345 [details] [diff] [review]
Patch

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 [:bzbarsky]]

> 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 [:bzbarsky]]

Comment on attachment 514345 [details] [diff] [review]
Patch

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 [:bzbarsky]]

Comment on attachment 514345 [details] [diff] [review]
Patch

This applies to both branches too.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 514345 [details] [diff] [review]
Patch

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers

Comment 17

[Daniel Veditz [:dveditz]]

FTP directory listings also use nsIndexedToHTML. Could this be remotely triggered on an FTP site? If so, might be worse than sg:moderate.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4447edf333a9
  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/14ebd9e548a2

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yes, FTP may be able to cause the same issues.

Comment 20

[Daniel Veditz [:dveditz]]

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]

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]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719