Jesse Ruderman Reporter
	Description • 19 years ago

Steps to reproduce:
1. Go to a small web page, such as http://www.google.com/.
2. Use the bookmarklet in this bug's URL field.
3. Wait a while.

Result: more often than not, Firefox crashes with a minute or two.

Here are the crashes I've seen so far with the bookmarklet:

nsStyleContext::GetStyleData()
TB8922845W, TB8923024X, TB8923158K, TB8923275Q, TB8923389M, TB8923438Z

GetNifOrSpecialSibling()
TB8923432H

nsCSSFrameConstructor::GetFloatContainingBlock()
TB8922738K, TB8923005W, TB8923133W*

The starred crash report is particularly scary because it occurred at a "random
address" with SIGILL.  According to the libc manual, "SIGILL typically indicates
that the executable file is corrupted, or that you are trying to execute data."  

I'm filing this as a security-sensitive bug because I have already found a
potential security hole with it.  I will keep the bookmarklet secret for now as
well, because someone with the bookmarklet might be able to rediscover the
scarier crashes.

I can make a more deterministic version of the bookmarklet if that would make it
easier to make a testcase for each crash it finds.

	Brendan Eich [:brendan]
	Comment 1 • 19 years ago

Fun.  This needs attention, pronto.  Who will own it?

/be

	David Baron :dbaron:
	Comment 2 • 19 years ago

These additional assertions prevent one problem from showing up 20 frames down
the stack when we do a bogus reflow with a bogus 0 availableHeight -- they at
least cause the problem to show up as an assertion pretty close to where it
happens.  I think the underlying problem here is bug 203923.  But I don't know
whether it's related to any of the crashes, which I haven't hit yet.

	Jesse Ruderman Reporter
	Comment 3 • 19 years ago

The GetNifOrSpecialSibling crash might also be exploitable: TB8950615Y.

	Jesse Ruderman Reporter
	Comment 4 • 19 years ago

All of the crashes I've found so far happen on the Gecko 1.8 branch but not on
trunk.  I suspect they all have the same underlying cause, which was fixed on
the trunk after the branch point.

	Jesse Ruderman Reporter
	Comment 5 • 19 years ago

I've seen Opera(Win), IE(Win), and Safari(Mac) crash with this bookmarklet.  I
haven't seen Firefox trunk crash yet.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 19 years ago

Jesse, could you possibly try trunk builds to see when the crash disappeared on
trunk?

	Jesse Ruderman Reporter
	Comment 7 • 19 years ago

The crashes for the testcases on all four bugs (bug 306782, bug 306787, bug
306789, bug 306798) disappeared between Aug 16 and Aug 17 trunk nightlies.

2005-08-16-07-trunk - crash (all four testcases)
2005-08-17-09-trunk - no crash (all four testcases)

Bug 265367 was checked in between those builds.  Could it have fixed these crashes?

	Hixie (not reading bugmail)
	Comment 8 • 19 years ago

FWIW I eventually got a crash running that bookmarklet on this very page (the
bug report itself, I mean).

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 19 years ago

> Bug 265367 was checked in between those builds.  Could it have fixed these
> crashes?

Possibly, yes.  It would certainly help on any page that has an animated image
in a very direct way; for other pages I'm not sure how it could prevent crashes,
but I wouldn't be too surprised.

Also bug 289377 landed in the same range.  And so did bug 303484.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 19 years ago

So I found a general case where the patch for bug 265367 would prevent a crash.

Say we have nodes A, B, and C and that the frame for node A is a leaf (eg A is
an <img>).  Consider the following sequence of operations, starting with a state
when A is in the document and hence has a frame (called A').

1)  Insert B as a child of A.  This creates a frame B' for B, sets the parent of
    B' to be A', sets B' as the primary frame for B.  Then we try to append B'
    to the frame list of A'.  This throws, but the error return is ignored.  So
    B' is not in the child list of A' (since A' has no child list).
2)  Move A to a different place in the DOM.  This destroys A' and all its
    descendants.  Since B' is not a child of A', B' is NOT destroyed.
3)  Insert C as a child of B.  This tries to create a frame C' for C, with B' as
    its parent (since B' is what we got when we did a primary frame lookup on
    B).  We start looking up things like the containing blocks we should use,
    which means we walk the parent chain of B'.  But the mParent pointer of B'
    points to the now-deleted A' frame, or in other words to some random memory.
    We might not crash there right away, since the random memory comes from an
    arena and hence is probably still allocated to our process; if we've
    allocated over it again, it won't even be marked with 0xdddddddd.

The alternative to the patch for bug 265367 is to actually check rv for all
cases when we're inserting or appending frames and clean up if it fails (at the
very least clean up the primary frame map).  This is not completely trivial to
do right in full generality (i.e. with destruction of the not-appended frames),
I suspect, and the patch for bug 265367 is pretty safe given that there have
been no regressions reported yet, so maybe we should just take bug 265367 on the
branch and also add the rv checks on trunk just in case?

	Scott MacGregor
	Comment 11 • 19 years ago

Jesse, what did 1.0.x do with this test case? Is this crash a regression over
1.0? Fixing this crash may mean taking a pretty large change to gecko, Bug
265367 which scares me after a looking at it briefly. I'd like to see if this
crash is new or not and how many crash reports we are seeing in talkback before
making a decision on this and 265367.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 19 years ago

> what did 1.0.x do with this test case?

Crashed.

It also crashed on the testcases in bug 291022 and bug 291513, of course.  ;)

And I think the reason Jesse is worried is not just because it's a crash but a
crash likely to be exploitable.

For what it's worth, the change for bug 265367 is a good bit safer than a lot of
the other stuff we've been landing on branch for a while now...  At least in my
opinion.  The other approach (of checking rv on the inserts/appends) would
actually be less safe, imo (and would certainly be less tested; I was running
with the patch in bug 265367 for three months and didn't see any issues).

	Asa Dotzler [:asa]
	Comment 13 • 19 years ago

going to take 256367 asap.

	Jesse Ruderman Reporter
	Comment 14 • 19 years ago

I made a small change to the bookmarklet so it would work on XML pages:
- var myRoot = document.body;
+ var myRoot = document.body || document.documentElement;

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 19 years ago

Checked in bug 265367 on the 1.8 branch.

	Jesse Ruderman Reporter
	Comment 16 • 19 years ago

Aaron, tomorrow's branch build won't crash much when you use this bookmarklet on
HTML pages, so you can start testing accessiblity code and/or screen readers
with this bookmarklet.  See the testcase in e.g. bug 306915 for the version that
includes its own random number generator for reproducibility.

	Jesse Ruderman Reporter
	Comment 17 • 19 years ago

See also bug 306939, the metabug for another crash-finding bookmarklet.

	Jesse Ruderman Reporter
	Comment 18 • 19 years ago

I should do another round of testing on HTML now that I can avoid the crashes
caused by bug 265367.

Bug 306902 shows up often (with many different seeds) on mixed HTML-and-MathML.
 I'll do another round of testing on mixed HTML-and-MathML pages once it is fixed.

Bug 306911 or bug 306915 happens almost immediately when I run the bookmarklet
on a XUL page.  I'll do another round of testing on XUL pages once they are fixed.

	Jesse Ruderman Reporter
	Comment 19 • 19 years ago

How much do we care about assertion failures turned up by this kind of testing?
 Memory leaks?

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 19 years ago

> How much do we care about assertion failures turned up by this kind of testing?

I care a good bit.

> Memory leaks?

Same, esp. if we can get a non-randomized testcase... ;)

	Brendan Eich [:brendan]
	Comment 21 • 19 years ago

(In reply to comment #20)
> > How much do we care about assertion failures turned up by this kind of testing?
> 
> I care a good bit.

I proposed that we make assertions fatal for 1.9, see
http://weblogs.mozillazine.org/roadmap/archives/2005_01.html.  dbaron suggested
doing this first by setting the XPCOM_DEBUG_BREAK envariable for some tinderbox
machines, to build up cross-platform testing coverage.  That's a good idea, and
I think we will do this starting tomorrow, or as soon as we can.

/be

	Jesse Ruderman Reporter
	Comment 22 • 19 years ago

I'm seeing crashes at DoDeletingFrameSubtree (bug 307277) and/or
nsBlockFrame::Destroy (bug 307280) on multiple sites:
http://www.csszengarden.com/, http://www.43things.com/, http://www.w3.org/Style/.

Once those bugs are fixed, I'll retest those sites.

	Jesse Ruderman Reporter
	Comment 23 • 19 years ago

Putting new version of the bookmarklet in the URL field.  You can now control
the random number seed and several speed-related parameters.

	Jesse Ruderman Reporter
	Comment 24 • 19 years ago

I will retest http://www.xulplanet.com/tutorials/xulqa/treeimage.xul and other
tree examples after bug 307298 is fixed.

	Jesse Ruderman Reporter
	Comment 25 • 19 years ago

I will retest http://www.carto.net/papers/svg/samples/text.svg after bug 307314
is fixed.

	Jesse Ruderman Reporter
	Comment 26 • 19 years ago

I will retest http://www.carto.net/papers/svg/samples/symbol.svg after bug
307322 is fixed.

	Jesse Ruderman Reporter
	Comment 27 • 19 years ago

I retested http://www.carto.net/papers/svg/samples/text.svg on trunk (now that
bug 307314 is fixed) and didn't hit any more crashes :)

	Jesse Ruderman Reporter
	Comment 28 • 19 years ago

I will retest everything under
http://www.w3.org/Graphics/SVG/Test/20030813/htmlframe/full-index.html once bug
307444 is fixed.

	Jesse Ruderman Reporter
	Comment 29 • 19 years ago

... and http://www.svgbasics.com/.

	Jesse Ruderman Reporter
	Comment 30 • 19 years ago

I will retest http://www.svgbasics.com/examples/marker_ex1.svg after bug 307470
is fixed.

	Jesse Ruderman Reporter
	Comment 31 • 19 years ago

I will retest http://www.w3.org/TR/SVG/images/filters/filters01.svg on Windows
after bug 307565 is fixed with seeds 0 and 1, at least.

	Jesse Ruderman Reporter
	Comment 34 • 19 years ago

* Added discovery of nodes in frames, iframes, and <embed>s containing SVG.
* Added discovery of XBL anonymous children.
* Made it use chained setTimeout instead of setInterval to improve
responsiveness when slow.
* Increased the default speed.
* Commented out the "javascript:" at the top of the non-bookmarklet version
because it caused a parse error in Safari.

	Jesse Ruderman Reporter
	Comment 35 • 19 years ago

List includes URLs I found crashes on and URLs that use rarely-used Gecko
features.

	Mats Palmgren (inactive)
	Comment 36 • 19 years ago

FYI, I'm not authorized to access the majority of the bugs you're referring,
in case you want any help on those...

	Jesse Ruderman Reporter
	Comment 37 • 19 years ago

There are five remaining StirDOM crashes that look exploitable to me:
  * Tables:     bug 308752
  * DOM+UI:     bug 307992, bug 307989
  * MathML:     bug 309120, bug 307826

There are several types of content for which Stir DOM runs into the same crash
often enough that it's hard to tell whether there are other crashes:
  * XBL:        bug 308120
  * XUL grids:  bug 306911
  * XUL trees:  bug 291531, bug 309732
  * SVG:        bug 307444

Based on what bz said in a security-group@ email, it sounds like we can't fix
the XUL crashes for Firefox 1.5 without making Firefox 1.5 slip.  I'm ok with
postponing the release of Stir DOM until the next version of Firefox and hoping
nobody else thinks of doing the same kind of fuzz testing on web browsers.  It
would be nice if the already-known, exploitable-looking crashes were fixed for
Firefox 1.5, though.

Gecko has a hidden pref for disabling SVG for use during security firedrills. 
It might be worthwhile to add similar prefs for disabling:
  * XUL in non-chrome
  * MathML
  * XBL in non-chrome (including <marquee> and <xul:button>)

	Asa Dotzler [:asa]
	Comment 38 • 19 years ago

We'll evaluate each crash fix as they come in. No longer blocking on tracking bugs.

	Jesse Ruderman Reporter
	Comment 39 • 19 years ago

This is one of the tools I use to reduce Stir DOM testcases.  Its output is
intended to be pasted back in, replacing the first two lines of the script.

	Bob Clary [:bc] (inactive)
	Comment 40 • 19 years ago

crashes or timeouts found in this run appear as 

stirdom: url?fuzz=parms...

The test loads the page (with the querystring), then runs the stirdom
bookmarklet with the specified parameters. You can copy/paste the parameters
from the query string directly into the stirdom input prompt.

The end of each line identifies the machine, the date the test run began and
the build which was tested. For example, 

prunessh/2005-12-17-02-34-33-firefox-1.5-build-dbg-1.8_2005121411.log

was run on prune (a windows machine), on Dec 17, using a 1.8 debug build built
on 2005-12-14-11. 

You can reproduce each test case by loading the url including the query string,
then running stirdom with the appropriate parameter.

	Jesse Ruderman Reporter
	Comment 41 • 19 years ago

Changes made to both Stir DOM and Random Styles recorders:
1. Make it record information about chunks/intervals so that
  (a) it can record the equivalent of a nonzero "number of changes to do immediately" in the bookmarklet.
  (b) while reducing, the chunk boundaries don't move.
2. Make it work with both XML and HTML without requiring separate versions.
3. Improve the instructions.

	Jesse Ruderman Reporter
	Comment 42 • 19 years ago

Stupid bug:

  if(n.getSVGDocument){try{addNodes(n.getSVGDocument());}catch(ex){}}

should check that n.getSVGDocument() is non-null before recursing.  I'm not fixing it (yet) because I'm lazy, and because it would change the behavior of the script on any page that uses Flash and I want to avoid changing its behavior too much.

	Jesse Ruderman Reporter
	Comment 43 • 18 years ago

Stir DOM and some other fuzzers will need changes once bug 47903, "WRONG_DOCUMENT_ERR not being thrown", is fixed.  I'm not fixing Stir DOM now because I'm hoping to use adoptNode (bug 330677) rather than importNode.

	Jesse Ruderman Reporter
	Comment 44 • 18 years ago

* Converted it to use fuzz.js (see bug 339948).
* Turned off recursing into XBL for now.
* No longer uses separate versions for bookmarklet-source and recording :)
* Various small changes.

	Jesse Ruderman Reporter
	Comment 45 • 18 years ago

Updated for fuzz.js 2.0.

	Daniel Veditz [:dveditz]
	Comment 47 • 18 years ago

Shouldn't have security bugs assigned to nobody. Jesse can own his test bugs

	Jesse Ruderman Reporter
	Comment 48 • 18 years ago

Comment on attachment 242967 [details]
Stir DOM 3.0

New version in bug 339948.