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.
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?
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: http://crash-stats.mozilla.com/report/index/bp-bdb29bc1-4c3c-433c-bc91-47acc2110222
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.
Oh, I see this on Mac.
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.
Created attachment 514345 [details] [diff] [review] Patch I have no idea how to write a testcase for this yet.
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;
> 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] 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 on attachment 514345 [details] [diff] [review] Patch This applies to both branches too.
Comment on attachment 514345 [details] [diff] [review] Patch Approved for 126.96.36.199 and 188.8.131.52, a=dveditz for release-drivers
FTP directory listings also use nsIndexedToHTML. Could this be remotely triggered on an FTP site? If so, might be worse than sg:moderate.
Pushed: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4447edf333a9 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/14ebd9e548a2
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.
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.