Last Comment Bug 619021 - (CVE-2011-0074) crash when opening a page with several marquee elements [@ nsStyleSet::GetContext ]
(CVE-2011-0074)
: crash when opening a page with several marquee elements [@ nsStyleSet::GetCon...
Status: RESOLVED FIXED
[sg:critical?][hardblocker](?) [qa-ex...
: crash, regression, testcase, verified1.9.2
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: P2 critical (vote)
: mozilla2.0b10
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-14 00:11 PST by Aki Helin
Modified: 2014-07-22 13:04 PDT (History)
13 users (show)
rforbes: sec‑bounty+
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.17+
.17-fixed
needed
.19-fixed


Attachments
page triggering the error (269 bytes, text/html)
2010-12-14 00:17 PST, Aki Helin
no flags Details
page triggering the error (304 bytes, text/html)
2010-12-14 00:29 PST, Aki Helin
no flags Details
crash report (mac debug + minidump_stackwalk) (67.78 KB, text/plain)
2010-12-19 13:30 PST, Jesse Ruderman
no flags Details
valgrind output (82.08 KB, text/plain)
2011-01-09 17:33 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details
patch 1: add assertions (4.03 KB, patch)
2011-01-10 16:47 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 2: warn when frame tree is too deep (2.99 KB, patch)
2011-01-10 16:47 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
patch 3: fix (2.02 KB, patch)
2011-01-10 16:48 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch 1: add assertions (4.26 KB, patch)
2011-01-10 22:36 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
patch 3: fix (5.17 KB, patch)
2011-01-10 22:36 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
patch 1: add assertions (5.00 KB, patch)
2011-01-11 12:25 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
patch 2 for 1.9.2 (3.05 KB, patch)
2011-01-28 14:26 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
dveditz: approval1.9.2.17+
Details | Diff | Splinter Review
patch 3 for 1.9.2 (5.29 KB, patch)
2011-01-28 14:26 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
dveditz: approval1.9.2.17+
Details | Diff | Splinter Review
patch 2 for 1.9.1 (3.05 KB, patch)
2011-02-01 11:54 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review
patch 3 for 1.9.1 (5.30 KB, patch)
2011-02-01 11:54 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description Aki Helin 2010-12-14 00:11:19 PST
User-Agent:       Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Firefox 4.0b7

Opening a specific HTML document consisting mainly of marquee elements causes current Firefox 4.0b7 to crash with an invalid read of size 4 with a high address (0xf0dea813) in libxul.so.

Reproducible: Always

Steps to Reproduce:
1.$ echo "<foo> <marquee> <marquee> <marquee> <marquee> <marquee> 
<marquee> <foo> <marquee> <foo> <marquee> <foo> <object> <marquee> 
<foo> <marquee> <marquee> <foo> <foo> <marquee> <marquee> <foo> 
<marquee> <marquee> <marquee> <foo> <marquee> <foo> <foo> <marquee> 
<marquee>" > marquee.html
2. 
3. firefox marquee.html
Actual Results:  
Firefox crashes with a segfault.

Expected Results:  
Firefox remains running.

This looks like a potentially dangerous crash to me. 4.0 stable release is probably not too far off, and many people are already using 4.0 beta series, so reporting this as a security issue to be on the safe side.
Comment 1 Aki Helin 2010-12-14 00:17:07 PST
Created attachment 497459 [details]
page triggering the error
Comment 2 Aki Helin 2010-12-14 00:29:03 PST
Created attachment 497461 [details]
page triggering the error

More correct file for triggering the error. Also the reproduction instructions above seem to be using the wrong one.

Crash state is at http://crash-stats.mozilla.com/report/index/bp-bf85ff2b-8b14-4041-b84e-7d1bb2101214
Comment 3 Jesse Ruderman 2010-12-19 13:30:06 PST
Created attachment 498640 [details]
crash report (mac debug + minidump_stackwalk)
Comment 4 Jesse Ruderman 2010-12-19 13:35:39 PST
Comment 0 and comment 3 have the crash address as 0xf0dea813 (frame poison address). But comment 2 has the crash address as 0xf363a813, so this might be exploitable.
Comment 5 Timothy Nikkel (:tnikkel) 2010-12-20 00:55:43 PST
regression range
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2f1347cd7c44&tochange=b14428284d51

And I can't narrow that down any further by building myself because it seems like the tree from that time doesn't build with whatever version of pango I have on my Linux system now.
Comment 6 Boris Zbarsky [:bz] (TPAC) 2010-12-20 13:52:27 PST
Fwiw, on 10.6 builds that old don't build either.  But nothing in that range looks obviously relevant...
Comment 7 Timothy Nikkel (:tnikkel) 2010-12-20 13:53:46 PST
Comment 5 was on Windows. Linux seems to have a different regression range...
Comment 8 Timothy Nikkel (:tnikkel) 2010-12-20 14:08:53 PST
regression range on linux
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d9d4bf676f65&tochange=35d78a340566
Comment 9 Timothy Nikkel (:tnikkel) 2010-12-21 00:23:56 PST
And linux debug builds before that range crash too...
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2011-01-05 16:05:20 PST
This needs an owner, over to tn for now. Please speak up if this needs to be owned by someone else.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-06 14:48:59 PST
I should probably take this instead.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-09 17:33:03 PST
Created attachment 502404 [details]
valgrind output
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 14:10:42 PST
It looks like the poisoned frame pointer is coming from nsFrame::DoGetParentStyleContextFrame (on an nsBlockFrame with a :-moz-anonymous-block style context), where we should have gone (based on examining the frame we were calling it for) through the not-out-of-flow but special-frame codepath:

      *aProviderFrame = GetIBSpecialSiblingForAnonymousBlock(this);

      if (*aProviderFrame) {
        return NS_OK;
      }

So that implies that we have a dangling pointer in the nsIFrame::IBSplitSpecialPrevSibling frame property.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 14:14:20 PST
Based on the valgrind data, I'm guessing that means we somehow got a next-in-flow as an IBSplitSpecialPrevSibling, which really shouldn't happen; that should always point to the first-in-flow, I thought.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 14:17:37 PST
Or, perhaps more likely, we called DeleteNextInFlowChild on something that was supposed to be empty, but it wasn't empty because it erroneously reported NS_FRAME_IS_COMPLETE.
Comment 17 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 15:00:06 PST
(In reply to comment #16)
> Or, perhaps more likely, we called DeleteNextInFlowChild on something that was
> supposed to be empty, but it wasn't empty because it erroneously reported
> NS_FRAME_IS_COMPLETE.

Yes.  I added assertions for this, and they fire on this testcase (but not in normal browsing of a few sites).
Comment 18 Boris Zbarsky [:bz] (TPAC) 2011-01-10 15:03:15 PST
We should have those assertions permanently in the tree....
Comment 19 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 16:24:48 PST
Yes, it's in my patch queue.

We're returning NS_FRAME_IS_COMPLETE incorrectly because of nsFrame::IsFrameTreeTooDeep.  Joy.

Maybe if I hack initialization of nsHTMLReflowState::mReflowDepth for the reflow root case to match what we do in the reflow-from-root case, it will fix this...
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 16:36:29 PST
(In reply to comment #19)
> Maybe if I hack initialization of nsHTMLReflowState::mReflowDepth for the
> reflow root case to match what we do in the reflow-from-root case, it will fix
> this...

It doesn't.  I guess I'll have to investigate how we end up with the frame being split.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 16:45:58 PST
Reflow depth for a given frame seems to vary quite a bit.

Given that, I think the better solution is to just report incomplete status when appropriate.
Comment 22 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 16:47:05 PST
Created attachment 502663 [details] [diff] [review]
patch 1: add assertions
Comment 23 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 16:47:58 PST
Created attachment 502665 [details] [diff] [review]
patch 2: warn when frame tree is too deep

I think a console warning in this case is appropriate; it would have helped me to know that we were entering this code.

But we don't need the old DEBUG_kipp blocks that don't compile anymore.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 16:48:33 PST
Created attachment 502666 [details] [diff] [review]
patch 3: fix
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-10 16:56:54 PST
+  /**
+   * Assert that the frame tree rooted at |aSubtreeRoot| is empty, i.e.,
+   * that it contains no first-in-flows.
+   */

I don't think that AssertFrameTreeIsEmpty is the best name for this. Apart from text frames, we also support split image frames, so this can return true even when there's real content in the frame tree.

+  // Should we also assert about text frames not mapping any text, or is
+  // that necessarily fixed up in the next continuation when the
+  // previous continuation maps all the text?

It should be fixed in the next continuation, but there's no harm in asserting it.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-10 16:59:51 PST
In part 3, maybe we should pass &aReflowStatus as a parameter to IsFrameTreeTooDeep and have it set the status?
Comment 27 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 22:36:22 PST
Created attachment 502733 [details] [diff] [review]
patch 1: add assertions

renames the assertion function, and adds assertions based on nsIFrame::GetOffsets for text frames
Comment 28 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-10 22:36:47 PST
Created attachment 502734 [details] [diff] [review]
patch 3: fix

good idea
Comment 29 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-11 11:34:54 PST
It turns out the start == end assertion (added in the second version of patch 1) fires on five tests:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/generic/crashtests/472774-1.html | assertion count 3 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/generic/crashtests/478185-1.html | assertion count 4917 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/bugs/23604-1.html | assertion count 2 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/bugs/372553-1.html | assertion count 3 is more than expected 0 assertions
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/bugs/457398-1.html | assertion count 2 is more than expected 0 assertions
Comment 30 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-11 11:38:25 PST
...which all involve :first-letter.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-11 12:25:28 PST
Created attachment 502899 [details] [diff] [review]
patch 1: add assertions

work around :first-letter issues.

I tried some things to get good values, but I couldn't find a way that worked.
Comment 32 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-11 17:13:23 PST
http://hg.mozilla.org/mozilla-central/rev/dfa73f7b1acf
http://hg.mozilla.org/mozilla-central/rev/67cfc95b4b90
http://hg.mozilla.org/mozilla-central/rev/e2f7319148ce

Need to remember to add the test later (though I think Jesse goes through and does those occasionally).
Comment 33 Daniel Veditz [:dveditz] 2011-01-28 13:02:54 PST
Takes down 3.6.x too.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-28 14:25:30 PST
I merged patches 2 and 3 to 1.9.2 and tested that they do in fact fix attachment 497461 [details] (I got a crash without them and it went away with them).  I merged patch 2 simply because it made merging (and reading) patch 3 easier.  Patch 2 is debug-only.
Comment 35 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-28 14:26:26 PST
Created attachment 508002 [details] [diff] [review]
patch 2 for 1.9.2
Comment 36 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-28 14:26:55 PST
Created attachment 508003 [details] [diff] [review]
patch 3 for 1.9.2
Comment 37 Daniel Veditz [:dveditz] 2011-01-31 10:22:51 PST
Some of the bugs in the regression range appear to have been fixed in 1.9.1 as well. Does this affect that branch? Or can we narrow the regression range to a specific patch so we can see if that's one of the ones that was backported?
Comment 38 Daniel Veditz [:dveditz] 2011-01-31 10:24:27 PST
Comment on attachment 508003 [details] [diff] [review]
patch 3 for 1.9.2

Approved for 1.9.2.15, a=dveditz for release-drivers
Comment 39 Daniel Veditz [:dveditz] 2011-01-31 10:26:51 PST
fwiw 3.5.18pre doesn't crash on the testcase.
Comment 41 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-31 11:53:48 PST
I don't know why it doesn't crash on 1.9.1; I'd have thought the underlying problem would be present going back pretty much forever.  It's possible it's something specific to the testcase.

The patch should be safe to take on 1.9.1 if we want to take it there.
Comment 42 Timothy Nikkel (:tnikkel) 2011-01-31 14:44:48 PST
The fact that the regression ranges are all different on win opt, linux opt, and linux debug and none of them contains anything suspicious probably means we are just getting lucky when it doesn't crash.
Comment 43 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-02-01 11:54:13 PST
Created attachment 508839 [details] [diff] [review]
patch 2 for 1.9.1
Comment 44 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-02-01 11:54:32 PST
Created attachment 508840 [details] [diff] [review]
patch 3 for 1.9.1
Comment 45 Daniel Veditz [:dveditz] 2011-02-02 10:50:39 PST
Comment on attachment 508839 [details] [diff] [review]
patch 2 for 1.9.1

Approved for 1.9.1.18, a=dveditz for release-drivers
Comment 46 Daniel Veditz [:dveditz] 2011-02-02 10:50:52 PST
Comment on attachment 508840 [details] [diff] [review]
patch 3 for 1.9.1

Approved for 1.9.1.18, a=dveditz for release-drivers
Comment 48 Al Billings [:abillings] 2011-02-04 15:25:27 PST
I cannot reproduce the crash using the testcase in comment 1 with the Firefox 4 beta 10 build (pre-fix) on Windows 7 or OS X 10.6.

Is there a better STR?
Comment 49 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-02-04 15:39:36 PST
The testcase in comment 2 worked pretty reliably for me, if my memory is correct.  (Were you really using the one in comment 1 instead?)
Comment 50 Al Billings [:abillings] 2011-02-04 17:00:28 PST
I was using comment 2. What OS have you been using?
Comment 51 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-02-04 19:14:17 PST
I was on 64-bit Linux.  tn also saw it on Windows (comment 5 and comment 7), and Jesse did on Mac (comment 3).
Comment 52 Daniel Veditz [:dveditz] 2011-03-04 10:14:55 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 53 Al Billings [:abillings] 2011-03-22 17:26:06 PDT
Verified fixed using attached test page in 1.9.2 nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.16pre) Gecko/20110317 Namoroka/3.6.16pre (.NET CLR 3.5.30729)) and crash in 1.9.2.15. 

Test page does NOT crash in 1.9.1.17. Have we actually seen this crash in 1.9.1 or do we have STR there?
Comment 54 Mats Palmgren (:mats) 2013-05-18 09:41:34 PDT
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ad36a84c5d
Comment 55 Phil Ringnalda (:philor) 2013-05-18 18:14:36 PDT
https://hg.mozilla.org/mozilla-central/rev/c0ad36a84c5d
Comment 56 Raymond Forbes[:rforbes] 2013-07-19 18:44:24 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

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