363219 - Redefined indexOf exposes scrollbar objects to script, can cause DOS or maybe worse

Status: RESOLVED WORKSFORME

(Core :: XBL, defect)

Description

[David Bloom]

User-Agent:       Opera/9.02 (Windows NT 5.2; U; en)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

Redefining indexOf allows scripts to get reference to viewport's scrollbar XUL DOM objects, which can allow them to hang the browser. It may also allow scripts to run priviliged code, but I am not familiar enough with the inner workings of Firefox's XUL UI to find a way to do that (if it is possible).

Reproducible: Always

Steps to Reproduce:
1. Declare a function that redefines String.prototype.indexOf
2. In the function: var foo = String.prototype.indexOf.caller.__parent__
3. foo.toString() should be "[object XULElement]". You can even set foo.width and foo.height to mess with the size of the viewport's scrollbars.
4. To make Firefox hang, add this to the redefined indexOf function to trigger an infinite loop: document.write("<div style=\"overflow:auto;\"><\/div>");



Actual Results:  
The XUL scrollbar objects are exposed to the script, and a hang can be caused by adding more scrollbars (by adding divs with overflow:auto or overflow:scroll) whenever indexOf is called.

Expected Results:  
Not exposed the XUL objects to script.

Comment 1

[David Bloom]

Attachment: Testcase

Comment 2

[Jesse Ruderman]

When I click the "crash" button in the testcase, I'm taken to https://bugzilla.mozilla.org/attachment.cgi?crash=crash.  I'm guessing that's not what you intended...

Comment 3

[Jesse Ruderman]

(I'm using Firefox trunk on Mac, fwiw)

Comment 4

[David Bloom]

Attachment: Testcase (revised)

Fixed testcase bug

Comment 5

[Jesse Ruderman]

I can reproduce the hang, but only on Windows (rather than Mac) and only with Firefox 2 (rather than trunk).

The infinite loop is just it going back and forth between your script and the script associated with scrollbars, right?

I guess the main concern here is that you're able to get a reference to a XUL scrollbar object that's supposed to be a hidden implementation detail.  I don't know how bad that is for security but it seems wrong/buggy even if it isn't a security hole.

-> Core: XBL, I guess.

Comment 6

[David Bloom]

(In reply to comment #5)
> The infinite loop is just it going back and forth between your script and the
> script associated with scrollbars, right?

Correct. Basically, it circumvents the "5-second rule" for nonresponsive scripts by tossing the script exucution between the page and the scrollbar.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the issue is that the page can mess with the script context of the scrollbar's binding, basically?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The scrollbar binding will be run in the page context since that's simply how xbl1 works, right?

Comment 9

[David Bloom]

I'm just going to throw this out there, even though I am not an expert on the XBL interface or the ECMAScript/Javascript standards and how this idea would comply with them...

This whole issue could be resolved by simply making String.prototype.indexOf read-only (so it cannot be redefined). I doubt there are many legitimate, non-malicious reasons to redefine indexOf.

Other functions, such as window.setTimeout are already read-only, so why not this?

Comment 10

[Boris Zbarsky [:bzbarsky]]

The Window object is a host object.  The String object is a native language object.  The behavior of the latter is controlled by the ECMA spec, and it's not clear to me that making indexOf readonly would be ok there.

Besides, what's special about indexOf?  Anything that a binding uses could be abused this way...

sicking: yeah, the XBL1 scope situation is as you describe.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> I'm just going to throw this out there, even though I am not an expert on the
> XBL interface or the ECMAScript/Javascript standards and how this idea would
> comply with them...

ECMA-262 is clear: see the first page of Chapter 15, which ends with "Every other property described in this section has the attribute { DontEnum } (and no others) unless otherwise specified."

> This whole issue could be resolved by simply making String.prototype.indexOf
> read-only (so it cannot be redefined). I doubt there are many legitimate,
> non-malicious reasons to redefine indexOf.

There are AOP use-cases that wrap standard methods with advice, and we are not going to break them. You're proposing to cripple an innocent bystander to try to fix a bug in a different module.

> Other functions, such as window.setTimeout are already read-only, so why not
> this?

Try |javascript:window.setTimeout = 42; window.setTimeout| in your location toolbar, it shows 42 for me.  All of these method bindings are mutable, and have been since I created them in 1995 in Netscape 2.

/be

Comment 12

[Brendan Eich [:brendan]]

Two issues here:

1.  The slow script dialog should catch ping-ponging metastable script execution, somehow.

2.  Should XBL2 (or does it already) run its code in the environment of the binding document, avoiding this class of errors/DOS-attacks?

Why is this bug security-sensitive?  If it's only a DOS, it should not be.

/be

Comment 13

[David Bloom]

(In reply to comment #12)
> Why is this bug security-sensitive?  If it's only a DOS, it should not be.
> 
> /be
> 

I agree. I only submitted it as security because I did not know if the scrollbars ran in any sort of privileged environment or not.

If anyone wants to remove the security label from this bug, feel free to.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Before we unmark this bug as security sensitive we should make sure you can't tweak the current code trivially to turn it into an exploitable crash.

I'm not sure I agree we should show the slow-script dialog here. That dialog is there to make sure that scripts don't lock up the UI, but that doesn't happen with this ping-ponging script, does it?

Comment 15

[Daniel Veditz [:dveditz]]

XBL widgets in pages no longer run with any privileges precisely because of this kind of trap, but maybe moz_bug_r_a4 has more tricks up his sleeve he can apply here.

Comment 16

[David Bloom]

Would it be possible to mutate some of the methods of the scrollbar object, potentially wrecking havoc in the priviliged code that accesses it?

Purely hypothetical example: The session restore feature seems to save the session as a Javascript file. Suppose it asks the scrollbar for its position to save. Maybe the scrollbar could return an invalid value that may allow execution of arbitrary code when the session is restored (and we can ensure that it will be by hanging firefox using the DOS, forcing the user to use session restore...).

I have not tried this myself and I doubt it's possible, but with these kinds of potential vulnerabilities mind, it may be a good idea to leave this bug as security-sensitive for the time being.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Would it be possible to mutate some of the methods of the scrollbar object,
> potentially wrecking havoc in the priviliged code that accesses it?

No.  See http://developer.mozilla.org/en/docs/XPCNativeWrapper.

> Purely hypothetical example: The session restore feature seems to save the
> session as a Javascript file. Suppose it asks the scrollbar for its position to
> save. Maybe the scrollbar could return an invalid value that may allow
> execution of arbitrary code when the session is restored (and we can ensure
> that it will be by hanging firefox using the DOS, forcing the user to use
> session restore...).

Good to think about, and session restore already sandboxes the evaluation of its JSON-encoded saved data (see http://lxr.mozilla.org/mozilla/source/browser/components/sessionstore/src/nsSessionStartup.js#134), but not useful to hypothesize broadly in this bug.

> I have not tried this myself and I doubt it's possible, but with these kinds of
> potential vulnerabilities mind, it may be a good idea to leave this bug as
> security-sensitive for the time being.

Wrong, it serves no purpose to limit the pool of people exploring generalized attack ideas to the people cc'd on this bug.  If this bug is truly a DOS (and I too welcome confirmation from moz_bug_r_a4 and shutdown), then it should not be security-sensitive.

/be

Comment 18

[moz_bug_r_a4]

I don't think I can abuse this bug to escalate privileges.

By the way, in cases where an unexposed scrollbar element could be abused, an
attacker can get it even without using this bug's trick.  e.g. by using
DOMAttrModified event listener.

Regarding a DOS issue, an attacker can make an infinite loop even without using
this bug's trick.  Note: this affects both trunk and branches.

  <binding id="a">
    <implementation>
      <constructor>
        document.body.appendChild(document.createElement("i"));
      </constructor>
    </implementation>
  </binding>

  <style>
  i { -moz-binding: url(#a); }
  </style>
  <i>i</i>

Comment 19

[moz_bug_r_a4]

Attachment: testcase - accessing scrollbar by using DOMAttrModified event listener

This works on fx2 and 1.5.0.x.

Comment 20

[moz_bug_r_a4]

Attachment: testcase - hang

This works on trunk, fx2 and 1.5.0.x.

Comment 21

[Jesse Ruderman]

The DOMAttrModified issue you're talking about is fixed or at least WFM on trunk, right?  See bug 345952.

Comment 22

[moz_bug_r_a4]

Yes, the DOMAttrModified issue does not affect trunk, though I cannot see the
bugs described in bug 345952.

Even on trunk, there are ways to access a scrollbar.

For example:
  BoxObject.prototype.getLookAndFeelMetric = function() {
    //xulobject is a scrollbar
    xulobject = arguments.callee.caller.caller.__parent__;
  }

Comment 23

[moz_bug_r_a4]

Attachment: testcase - works on trunk (hang)

Comment 24

[Daniel Veditz [:dveditz]]

None of these testcases seem to cause problems in recent builds (tried various builds from 3.6 through current).