55070 - XBL needs simple defense against D.O.S./infinite recursion

Status: RESOLVED FIXED

(Core :: XBL, defect, P3)

Description

[Brendan Eich [:brendan]]

Denial of service, or more likely, XBL author error, can lead to infinite 
recursion and other runaway conditions.  A few obvious ones: two bindings that 
extend each other; a bindingattached handler that clones a node and inserts it 
in the document, causing the same handler to run again, cloning another node, 
etc.

/be

Comment 1

[Brendan Eich [:brendan]]

From hyatt's posting to m.xpcom:

div {
  -moz-binding: url(myfile.xml#mybinding);
}

<binding id="mybinding">
  <content>
    <html:div/>
  </content>
</binding>

/be

Comment 2

[hand]

Attachment: Zipped test case for recursive cloneNode

Comment 3

[John Morrison]

Ick. Cancel out of that cloneNode testcase after letting it go ~6
recursions deep. When you subsequently do 'File->Quit', on Windows (but
not Linux), mozilla-the-process never fully dies. It seems to be exercising
a bug in Win2k (both trunk and PR3-opt builds). 

When you break on the zombie process in the debugger, it says "DBG: Break 
command failed within 3 seconds. DBG: Potential deadlock. Soft broken.", and 
the 'stack' is pointing at OLE32.dll, USER32.DLL, and NTDLL.DLL.

Comment 4

[Peter Trudelle]

nominate for rtm, cc mstoltz

Comment 5

[Peter Trudelle]

rtm-/future

Comment 6

[Asa Dotzler [:asa]]

*** Bug 167148 has been marked as a duplicate of this bug. ***

Comment 7

[:Gavin Sharp [email: [e-mail]]]

*** Bug 271483 has been marked as a duplicate of this bug. ***

Comment 8

[Jesse Ruderman]

*** Bug 287303 has been marked as a duplicate of this bug. ***

Comment 9

[Martijn Wargers (dead)]

Attachment: testcase, using extends, crashes on load

I accidentally filed bug 350754, which is a duplicate of this one, but someone made a fix for that, for a certain type of xbl recursion.
This is another case of recursion in xbl, which isn't fixed by that patch.

Talkback ID: TB23024067K
NS_EscapeURL  [mozilla\xpcom\io\nsescape.cpp, line 440]

Comment 10

[Martijn Wargers (dead)]

Attachment: recursive cloneNode case

This is the recursive cloneNode case, just one page for easy viewing.
It doesn't crash Mozilla, it just freezes and slowly eats all available memory.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So the "recursive cloneNode" thing is not something we can do much about, really... that could be done without XBL too -- just keep cloning nodes.

The other we should fix; probably with a check similar to bug 350754.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 13

[Mats Palmgren (inactive)]

Attachment: Testcase #5

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #6

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Something like this perhaps?

Comment 16

[Mats Palmgren (inactive)]

Attachment: Screenshot of error messages

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 241729 [details] [diff] [review]
Patch rev. 1

>Index: dom/locales/en-US/chrome/layout/xbl.properties
>+CircularExtendsBinding=Extending the XBL binding "%S" with "%S" would make it circular

Maybe more like:

  Extending the XBL binding "%S" with "%S" would lead to it extending iteself.

or something?

>Index: content/xbl/src/nsXBLService.cpp

>+  aDontExtendURIs.AppendElement(aURI);

Need to return error if this fails.

>@@ -1008,27 +1022,48 @@ >+          PRBool equal;
>+          if (NS_FAILED(uri->Equals(bindingURI, &equal)) || equal) {

I think if the Equals() call fails we should just return that error instead of reporting the recursion error message to the console.

>Index: content/xbl/src/nsXBLService.h
>+  nsresult GetBinding(nsIContent* aBoundElement, nsIURI* aURI,
>+                      PRBool aPeekFlag, PRBool* aIsReady,
>+                      nsXBLBinding** aResult,
>+                      nsVoidArray& aDontExtendURIs);

Document, please.  And is there a reason not to be using nsTArray<nsIURI>& for the array?  And probably pre-initi it to a size of 8 (or whatever measurements show?) since we only use it on the stack and generally don't have too deep a extends tree.

Comment 18

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

(In reply to comment #17)
> Maybe more like:
>   ... would lead to it extending iteself.

Fixed.

> >+  aDontExtendURIs.AppendElement(aURI);
> Need to return error if this fails.

Fixed.

> I think if the Equals() call fails we should just return that error
> instead of reporting the recursion error message to the console.

Fixed. Also fixed IsAncestorBinding() in the same way (bug 350754).

> ... is there a reason not to be using nsTArray<nsIURI>& for the array?

Yes, nsTArray<E> requires that E has a copy ctor and nsIURI is abstract.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/glue/nsTArray.h&rev=1.22&root=/cvsroot&mark=185#178
I changed nsVoidArray& to nsTArray<nsIURI*>& instead.

> ... pre-init it to a size of 8 (or whatever measurements show?)

Measuring: starting Firefox, open Preferences, open DOMI.
Distribution of 1361 GetBinding() calls gave these array sizes:
1: 514
2: 160
3: 360
4: 142
5: 136
6: 49
none had 7 or more. (Just starting Fx also gave a 1..6 distribution.)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I changed nsVoidArray& to nsTArray<nsIURI*>& instead.

Er, that's what I meant.  nsTArray<nsIURI> was a typo.  ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 242320 [details] [diff] [review]
Patch rev. 2

>Index: content/xbl/src/nsXBLService.cpp
>+nsXBLService::GetBinding(nsIContent* aBoundElement, nsIURI* aURI, 
>+  NS_ENSURE_TRUE(aDontExtendURIs.AppendElement(aURI) != nsnull,

Shouldn't need the != nsnull thing there, right?

With that nit, looks great!

r+sr=bzbarsky

Comment 21

[Mats Palmgren (inactive)]

(In reply to comment #20)
> >+  NS_ENSURE_TRUE(aDontExtendURIs.AppendElement(aURI) != nsnull,
> 
> Shouldn't need the != nsnull thing there, right?

Right, I was thinking of bug 340244 but NS_ENSURE_* doesn't suffer from that.

Checked in to trunk (without the "!= nsnull") at 2006-11-08 17:58 PDT.

-> FIXED