Closed Bug 423947 Opened 16 years ago Closed 16 years ago

Content can exploit calls to addEventListener from FireBug

Categories

(Core :: Security, defect, P2)

1.8 Branch
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Unassigned)

References

()

Details

From bug 344751 comment 40

* When Firebug does |win.addEventListener("foo", y, false);| (win is SJOW),
content can access y and abuse it.

Since win is a SJOW and not a NativeWrapper if content has overridden addEventListener it will be called and the |y| function will be given to the content code, which can then use that to get elevated privileges.

I believe the correct fix here is for FireBug not to opt out of NativeWrappers. Opting out means that any DOM calls that are given a callback can be exploited, and any other DOM call can be overridden by the web page.
Flags: blocking1.9?
As I understand things, this should be trivial to fix in Firebug -- simply call |XPCNativeWrapper(win).addEventListener(...)| instead of what it does now.
Yeah, but solving it that way would be a never ending game of whack-a-mole to ensure correct behavior. Sure, you can probably cover the places where a callback is passed to the function and thereby avoid this exact exploit.

But really, you want any DOM access to happen through XPCNativeWrappers. Otherwise you can't rely on any return value you're getting back being honest. This is why XPCNativeWrapper works the way it does.

So the fix really here is for FireBug to not opt out of XPCNativeWrappers.
I had that in my comment, then backed off :-).

Dolske and I briefly discussed this a week ago. I argued that FireBug should not be opting out of XPCNativeWrappers and using .wrappedJSObject in the cases that it *really* (no, *really, really*) wanted access to the original object: e.g. for enumeration in the object view. The problem is that we need someone to step up and do that work. I think sdwilsh looked into it last summer, but I don't know how far he got.
I didn't get far at all, and I won't have cycles for this until May.
I don't really understand any of this so I don't know how I can help.  From what little I know it seems to me the real problem here is the ability to change 'get' on properties in the first place. If that were disabled accessing object properties would no longer be a problem.
(In reply to comment #0) 
> Since win is a SJOW and not a NativeWrapper if content has overridden
> addEventListener it will be called and the |y| function will be given to the
> content code, which can then use that to get elevated privileges.

The only circumstance where Firebug wants to call content-provided methods is in command-line; let's address that special case in Bug 423796.

> I believe the correct fix here is for FireBug not to opt out of NativeWrappers.
> Opting out means that any DOM calls that are given a callback can be exploited,
> and any other DOM call can be overridden by the web page.

Firebug wants window.foo() to mean the foo() absent the page content javascript. I gather from http://developer.mozilla.org/en/docs/XPCNativeWrapper#What_XPCNativeWrapper_does that removing 'xpcnativewrappers=no' in the firebug chrome.manifest would cause the wanted behavior for Firebug originated function calls.

However as I understand it, this change would also mean that "for(p in window)" would no longer enumerate the contents of 'window'.

The two fundamental problems are 1) "window" has two semantics, the object to hold and manipulate a web page plus the object to contain user-created properties and methods, and 2) enumeration of properties can be redefined, so that an attempt to manipulate a web page (the first semantic) becomes corrupted by user-defined functions (the second semantic).

Is there more to it than this?
But you can use .wrappedJSObject() to get an enumerable window, can't you?
(In reply to comment #8)
> But you can use .wrappedJSObject() to get an enumerable window, can't you?
> 
I don't know, can I?  

If in a scope with xpcnativewrappers=yes:
  window -- means the first semantic, no user-defined functions
  window.wrappedJSObject() -- means the second semantic, user-defined functions.
Then whenever window.wrappedJSObject is used we are just back to where we are today.
  If (window in xpcnativewrappers=no) == 
     (window.wrappedJSObject() in xpcnativewrappers=yes )
  then we gain nothing.

So there must be something else involved.
Not accessible to reporter
(In reply to comment #9)
> I don't know, can I?  

Yes, see comment 4.

> If in a scope with xpcnativewrappers=yes:
>   window -- means the first semantic, no user-defined functions
>   window.wrappedJSObject() -- means the second semantic, user-defined

Note, |window.wrappedJSObject|, no parens.

> functions.
> Then whenever window.wrappedJSObject is used we are just back to where we are
> today.

Not quite. The general idea for writing secure JavaScript extensions is that, by default, you don't want content to be able to really muck with the window (change addEventListener, etc). The tradeoff is that you won't be able to see content-defined functions/set properties on the window, etc. By using XPCNativeWrapper, you are guaranteed that everything you do with the window (and other objects) has the expected behavior and won't even run content-defined code.

If you do need to mess with the actual object (and people do do this, e.g. if they have knowledge that their extension runs on a page that defines some useful function) we added |XPCNativeWrapper.wrappedJSObject|. This allows you to access such functions, but you have to be very careful about what you do with the resulting object. Getting properties and enumerating the object won't expose you to privilege escalation attacks ala bug 344494, but if you set a property to a privileged object, then you're hosed.

See also http://developer.mozilla.org/en/docs/XPConnect_wrappers#Wrapper_Wrappers .

The idea proposed in comment 3 and comment 4 is that you turn of xpcnativewrappers=no and carefully add .wrappedJSObject to the places that you really truly need to modify the underlying objects (e.g. for the JS Object inspector view). Unfortunately, it isn't a panacea; I worry that we'll need to add some additional mechanism for you to safely be able to add window.console. I'll comment in the relevant bug about that, though.

Does that make sense?
(In reply to comment #10)
> (In reply to comment #9)
> > I don't know, can I?  
> 
> Yes, see comment 4.

I don't understand comment 4.  What does "*really*" mean? Or "original object"?

However I interpret your answer to mean: " in a scope where xpcnativewrapers=yes, window.wrappedjsobject refers to an object whose properties can be enumerated and they will include user-defined properties".

> 
> > If in a scope with xpcnativewrappers=yes:
> >   window -- means the first semantic, no user-defined functions
> >   window.wrappedJSObject() -- means the second semantic, user-defined
> 
> Note, |window.wrappedJSObject|, no parens.

And no vertical bars I assume.

> 
> > functions.
> > Then whenever window.wrappedJSObject is used we are just back to where we are
> > today.
> 
> Not quite. The general idea for writing secure JavaScript extensions is that,
> by default, you don't want content to be able to really muck with the window
> (change addEventListener, etc). The tradeoff is that you won't be able to see
> content-defined functions/set properties on the window, etc. By using
> XPCNativeWrapper, you are guaranteed that everything you do with the window
> (and other objects) has the expected behavior and won't even run
> content-defined code.
> 
> If you do need to mess with the actual object (and people do do this, e.g. if
> they have knowledge that their extension runs on a page that defines some
> useful function) we added |XPCNativeWrapper.wrappedJSObject|. This allows you
> to access such functions, but you have to be very careful about what you do
> with the resulting object. Getting properties and enumerating the object won't
> expose you to privilege escalation attacks ala bug 344494, but if you set a
> property to a privileged object, then you're hosed.

So why not make such 'set' operations errors?  "very careful" is not very useful.

> 
> See also
> http://developer.mozilla.org/en/docs/XPConnect_wrappers#Wrapper_Wrappers .

What is the appropriate way to comment on that content? Some parts don't make sense.

> 
> The idea proposed in comment 3 and comment 4 is that you turn of
> xpcnativewrappers=no and carefully add .wrappedJSObject to the places that you
> really truly need to modify the underlying objects (e.g. for the JS Object
> inspector view). Unfortunately, it isn't a panacea; I worry that we'll need to
> add some additional mechanism for you to safely be able to add window.console.
> I'll comment in the relevant bug about that, though.
> 
> Does that make sense?

Except for the bits where you say "carefully" and "really truly".   The whole point of these wrapper things is to help developers avoid security problems. So we can't fall back on "careful". 
So here's the deal:

There are several objects involved here:

A) The "real" javascript object.
   This is what the webpage usually uses.

B) The XPCNativeWrapper wrapping the "real" object in A.
   This is the object FireBug gets handed to it (at least for all DOM objects)
   when xpcnativewrappers=yes is used.
   This is also the object you get if you use XPCNativeWrapper(foo) if foo is a
   "real" object or a SJOW (see below).
   This object exposes only those methods and properties that are normally 
   available on the "real" object through the normal DOM. But not those defined
   by the web page. I.e. .firstChild is there on a document, but
   .myHilariousCounter isn't. Any DOM property that is overridden by content
   will be ignored and the real DOM property is exposed instead.

C) The SJOW (safe js object wrapper) wrapping ther "real" object in A.
   This is the object FireBug gets handed to it (at least for all DOM objects)
   when xpcnativewrappers=no is used.
   This is also the object you'll get when getting .wrappedJSObject from an
   XPCNativeWrapper.
   This object exposes all methods and properties defined on the "real". But
   lets you access them in such a way that getters and methods defined on the
   object won't be able to get elevated privileges.

Also note that there is no difference between enumerating and getting properties. If you can get a property then it'll be there when you enumerate the object. And the other way around.


So in general you obviously want XPCNativeWrappers. They will act as if the webpage hadn't done anything funky on the object.

However, if you somewhere really do want to see anything weird that the page has modified you should get an SJOW. Preferably by getting .wrappedJSObject off of a XPCNativeWrapper. This is useful if you want to display to the user the properties that the webpage sees on the "real" object.


There are further differences between the XPCNativeWrapper and the SJOW wrappers. If you set a property on the XPCNativeWrapper it'll only be set on the XPCNativeWrapper. If you set something on a SJOW the set will be forwarded to the "real" object.


So now to some harder parts. In general accessing a SJOW or XPCNativeWrapper is safe. However, if you use it such a way that you are handing chrome functions to content you will no longer be safe. For example the following chrome code:

mySJOWobject.addEventListener("load", myChromeFunction, false)

is unsafe since you are calling something that is potentially a content function (if content overrides addEventListener) and passing in a chrome function. However, the following chrome code:

myXPCNativeWrapper.addEventListener("load", myChromeFunction, false)

is safe since you'll only pass myChromeFunction to the DOM code which does not provide a way for content to get to it. Another scenario is:

mySJOWobject.setUserData("hello", myChromeFunction, null)
myXPCNativeWrapper.setUserData("hello", myChromeFunction, null)

Both of these are unsafe since content can in both cases use getUserData to get to the stored myChromeFunction.

Yet another example:
mySJOWobject.myOwnProperty = myChromeFunction;
myXPCNativeWrapper.myOwnProperty = myChromeFunction;

The first one isn't safe as the property will be set on the "real" object and content can get to myChromeFunction. The second one is safe, but does not make it possible for content to use myOwnProperty.


So as you can see, XPCNativeWrappers are more safe than SJOW. Especially when all you want to do is access normal DOM functions. And both with xpcnativewrappers=yes and with xpcnativewrappers=no you can get to either the XPCNativeWrapper and the SJOW, using XPCNativeWrapper(foo) and foo.wrappedJSObject.

But using xpcnativewrappers=yes is clearly safer, as you have to opt in to the less secure wrapper, rather than with xpcnativewrappers=no where you will by default get the less secure wrapper and have to opt in to the more secure one.



Does that make sense? :)
(In reply to comment #12)

I have a sense of the terminology and issues, thanks.  I have also been thinking of what would be essential for Firebug to succeed and be secure, but I'll come back to that later.

> So here's the deal:
> 
> There are several objects involved here:
> 
> A) The "real" javascript object.
>    This is what the webpage usually uses.

Could you have said: 

A) A javascript object created in the scope of a web page.

? "real" is ok, but "usually" just confuses me. I am thinking "ok, so about those other cases..."

> 
> B) The XPCNativeWrapper wrapping the "real" object in A.
>    This is the object FireBug gets handed to it (at least for all DOM objects)
>    when xpcnativewrappers=yes is used.
>    This is also the object you get if you use XPCNativeWrapper(foo) if foo is a
>    "real" object or a SJOW (see below).
>    This object exposes only those methods and properties that are normally 
>    available on the "real" object through the normal DOM. But not those defined
>    by the web page. I.e. .firstChild is there on a document, but
>    .myHilariousCounter isn't. Any DOM property that is overridden by content
>    will be ignored and the real DOM property is exposed instead.

This one I understand, it's the API to an object needed by Firebug to correctly manipulate the page state. User-level changes to this API are bad for Firebug and bad for security.

> 
> C) The SJOW (safe js object wrapper) wrapping ther "real" object in A.
>    This is the object FireBug gets handed to it (at least for all DOM objects)
>    when xpcnativewrappers=no is used.
>    This is also the object you'll get when getting .wrappedJSObject from an
>    XPCNativeWrapper.
>    This object exposes all methods and properties defined on the "real". But
>    lets you access them in such a way that getters and methods defined on the
>    object won't be able to get elevated privileges.

Hmm, not a adequate definition for me. If the user-level code defines a getter for property "bar" of object "foo" and Firebug issues "foo.bar", does the getter run?  If yes, this is bad for Firebug, whether or not it is bad for security.

> 
> Also note that there is no difference between enumerating and getting
> properties. If you can get a property then it'll be there when you enumerate
> the object. And the other way around.

What does this rule mean in relation to hasOwnProperty and friends? They don't apply to these wrappers or...?

> 
> 
> So in general you obviously want XPCNativeWrappers. They will act as if the
> webpage hadn't done anything funky on the object.

Well, I would say differently: if Firebug wants to operate on the webpage using the standard API, then it wants XPCNativeWrappers. Firebug also wants to do other things.

> 
> However, if you somewhere really do want to see anything weird that the page
> has modified you should get an SJOW. Preferably by getting .wrappedJSObject off
> of a XPCNativeWrapper. This is useful if you want to display to the user the
> properties that the webpage sees on the "real" object.

I would also say this differently: it's not a matter of extra special desire as in "really", but an essential requirement, just one that is different from the one above. Simply:

To read the properties of the user-level object in the webpage, use the SJOW you get from .wrappedJSObject.

> 
> 
> There are further differences between the XPCNativeWrapper and the SJOW
> wrappers. If you set a property on the XPCNativeWrapper it'll only be set on
> the XPCNativeWrapper. If you set something on a SJOW the set will be forwarded
> to the "real" object.

Ok, now we are getting into "this is not good for me". These kind of rules will make development crazy.

> 
> So now to some harder parts. In general accessing a SJOW or XPCNativeWrapper is
> safe. However, if you use it such a way that you are handing chrome functions
> to content you will no longer be safe. 

But look at this from my perspective: I have 35,000 lines of code worked on by multiple people over a long period of time. The SJOW and XPCNativeWrapper objects are indistinguishable in source from every other object. So anything like the analysis you have here isn't practical. That's what I meant when I said I was worried about "carefully". I cannot tell you if Firebug will or will not hand chrome functions to content.  We need security support to succeed.

> But using xpcnativewrappers=yes is clearly safer, as you have to opt in to the
> less secure wrapper, rather than with xpcnativewrappers=no where you will by
> default get the less secure wrapper and have to opt in to the more secure one.
> 
> Does that make sense? :)

Overall the description is making more sense, but the end-point is not. Firebug will be put in the position of being as safe as we are careful. 

Now I want to look at the Firebug security problem from the Firebug side. By that I mean I want to pretend I never heard of wrappers, and just ask what is needed for Firebug to be effective and secure.  (Rather than pepper this discussion with "I think/believe/understand", I will just assert as fact stuff I kinda understand.)

Since the security holes come from user-level code running with chrome privileges, another way to say this is:  Can Firebug succeed without ever running user-level code outside of the page?  Well, yes, and in fact any case where Firebug currently runs user-level code outside of the page is either a bug or one waiting --- or an exploit.  User-level code is designed to run in the page, the cases where Firebug runs it, eg foo.toString(), amounts to planning to be lucky.  

Firebug needs to control the page. It uses FF APIs in part, but a lot of the control comes through event-handling.  That's why this particular bug report is critical: if addEventListener can be hacked, Firebug cannot succeed. Firebug will never want user-level code for page control.

Firebug needs to read the page.  It uses DOM APIs and walking the object graph. Firebug never wants user-level code involved. In fact it does not want getter functions to run: we want the "machine" representation, not the view created by the developer of the page.

Firebug needs to write into the page. It uses DOM API and javascript object manipulation.  Again Firebug never wants user-level code to run.  No setters. There are two places that Firebug set values on 'window' directly; both of these could be removed.

Firebug would be more correct and more secure if page objects had two APIs:
  1) Standard API, no user-level manipulation. sounds like XPCNativeWrapper
  2) Read-only API. No setters, no getters, no writes allowed.  Does this exist?

With these two APIs Firebug's in-page console and in-page command-line would have to be re-designed. I believe these could be implemented by using DOM APIs to cause in-page functions to run which store state in the page. (That is injecting strings that are scripts in the page that then run and set values on eg window._firebug or invisible elements marked by firebug).  The results would be ferried to the Firebug UI using the Read-only API.

The advantage of a Read-only API over the SJOW is that it allows carelessness. Returning to Jonas' examples: 

// ERROR: cannot call methods on myROWojbect (unsafe for mySJOWobject)
myROWobject.addEventListener("load", myChromeFunction, false)

// Ok and safe
myXPCNativeWrapper.addEventListener("load", myChromeFunction, false)

// ERROR cannot call myROWObject.setUserData()
myROWobject.setUserData("hello", myChromeFunction, null)

// ERROR no such function
myXPCNativeWrapper.setUserData("hello", myChromeFunction, null)

// ERROR, Read only
myROWobject.myOwnProperty = myChromeFunction;
// Ok and safe.
myXPCNativeWrapper.myOwnProperty = myChromeFunction;

I don't know if Read-Only is possible, but it would be better for Firebug than SJOW, as I understand them.

John.

I removed the xcpnativewrappers=no from chrome.manifest and fixed the user-level object content access to use .wrappedJSObject in firebug on branches/firebug1.2 at R440. The testcase attached to this report no longer pops up an alert. The next steps from the Firebug side is to test the result to see if we still have all of the function and it is correct. Functions are now displayed differently in DOM panel, but I don't know if this is a bug or a feature ;-).
Status: NEW → ASSIGNED
Assignee: dveditz → johnjbarton
Status: ASSIGNED → NEW
Great news on removing xpcnativewrapper=no! This is a huge step forward. Thanks a lot for your work.


To answer your other comments:

So first off, the APIs available for extensions can definitely be improved. The XPCNativeWrapper and SJOW wrappers were huge steps, but it's clearly not enough, and we are working on improving things more.

However, extensions should not wait until we are "done" before trying to make themselves secure. We should get firebug as safe as we can as soon as possible. That means working around shortcomings from both directions.

But we should definitely continue discussing what APIs are needed by firebug in order to more easily make it possible to write safe code.

(In reply to comment #13)
> > A) The "real" javascript object.
> >    This is what the webpage usually uses.
> 
> Could you have said: 
> 
> A) A javascript object created in the scope of a web page.

Yes.

> ? "real" is ok, but "usually" just confuses me. I am thinking "ok, so about
> those other cases..."

I was trying to save you from some gory details. But since you asked, here goes :)

For a few objects, specifically window objects, location objects, frame/iframe elements, and document objects, web code doesn't use the "real" object directly, but rather uses a XOW wrapper.

The XOW wrapper is usually 100% transparent and so the page has no idea it's using a wrapper. However, if the wrapped "real" object is from a different domain then the XOW acts like an XPCNativeWrapper and only lets you access DOM methods and properties. User-level properties are hidden. So if a web page accesses

myIframePointingToOtherDomain.contentWindow.location.href = "http://mozilla.org"

Then it will access the real DOM .location and .href properties. Even if the webpage inside the iframe has overridden those properties to do other things.

However if the webpage inside the iframe accessed window.location.href it would get whatever overrides it had done itself.

I don't think this is something that affects FireBug though.

> > C) The SJOW (safe js object wrapper) wrapping ther "real" object in A.
> >    This is the object FireBug gets handed to it (at least for all DOM objects)
> >    when xpcnativewrappers=no is used.
> >    This is also the object you'll get when getting .wrappedJSObject from an
> >    XPCNativeWrapper.
> >    This object exposes all methods and properties defined on the "real". But
> >    lets you access them in such a way that getters and methods defined on the
> >    object won't be able to get elevated privileges.
> 
> Hmm, not a adequate definition for me. If the user-level code defines a getter
> for property "bar" of object "foo" and Firebug issues "foo.bar", does the
> getter run?  If yes, this is bad for Firebug, whether or not it is bad for
> security.

Yes, accessing foo.bar would run the getter. This is by design since we want the SJOW to act as much as possible as the "real" object. That is its purpose.

What would you like to happen? Also, you can check if a getter is defined for a given property by using __lookupGetter__ and avoid calling the getter if one is defined.

However for something like a js-object view I would think that you do in fact want to run getters.

> > Also note that there is no difference between enumerating and getting
> > properties. If you can get a property then it'll be there when you enumerate
> > the object. And the other way around.
> 
> What does this rule mean in relation to hasOwnProperty and friends? They don't
> apply to these wrappers or...?

Not really sure how hasOwnProperty works so I'll leave this one for Blake or Jst.

> To read the properties of the user-level object in the webpage, use the SJOW
> you get from .wrappedJSObject.

or use xpcnativewrappers=no. But yes, that is true.

> > There are further differences between the XPCNativeWrapper and the SJOW
> > wrappers. If you set a property on the XPCNativeWrapper it'll only be set on
> > the XPCNativeWrapper. If you set something on a SJOW the set will be forwarded
> > to the "real" object.
> 
> Ok, now we are getting into "this is not good for me". These kind of rules will
> make development crazy.

So how would you prefer setting properties to work? One alternative that has been discussed is simply not letting you set properties at all on XPCNativeWrappers.

> > So now to some harder parts. In general accessing a SJOW or XPCNativeWrapper is
> > safe. However, if you use it such a way that you are handing chrome functions
> > to content you will no longer be safe. 
> 
> But look at this from my perspective: I have 35,000 lines of code worked on by
> multiple people over a long period of time. The SJOW and XPCNativeWrapper
> objects are indistinguishable in source from every other object. So anything
> like the analysis you have here isn't practical. That's what I meant when I
> said I was worried about "carefully". I cannot tell you if Firebug will or
> will not hand chrome functions to content.  We need security support to
> succeed.

I agree we should do better here, and we are working on it. But I think we should try to get FireBug safe as soon as possible. 

Note that as soon as you remove xpcnativewrappers=no FireBug will be much much better off. There really are very few ways you can screw yourself using XPCNativeWrappers.

> > But using xpcnativewrappers=yes is clearly safer, as you have to opt in to the
> > less secure wrapper, rather than with xpcnativewrappers=no where you will by
> > default get the less secure wrapper and have to opt in to the more secure one.
> > 
> > Does that make sense? :)
> 
> Overall the description is making more sense, but the end-point is not. 
> Firebug will be put in the position of being as safe as we are careful. 

Not really sure I follow what you're saying here.


(In reply to comment #14)
> Since the security holes come from user-level code running with chrome
> privileges, another way to say this is:  Can Firebug succeed without ever
> running user-level code outside of the page? 

Not really sure what you mean by running code "outside of the page". Do you mean "when not requested by the page", or do you mean "run in a security context other than the pages thus making it possible for the page to gain elevated privileges"?

SJOW does run the page code in such a way that it can't get elevated privileges. However it does of course still allow code to run when the extension requests it, not when the website requests it. I.e. if an extension does:

webpageWindowSJOW.funFunction();

funFunction will run, clearly without the webpage requesting it, but it will run in such a way that the webpage can't do anything harmful.

> Well, yes, and in fact any case
> where Firebug currently runs user-level code outside of the page is either a
> bug or one waiting --- or an exploit.  User-level code is designed to run in
> the page, the cases where Firebug runs it, eg foo.toString(), amounts to
> planning to be lucky.  

Not really sure I agree that this always is true. For example in the JS object view I think you do want to run getters.

> Firebug needs to control the page. It uses FF APIs in part, but a lot of the
> control comes through event-handling.  That's why this particular bug report is
> critical: if addEventListener can be hacked, Firebug cannot succeed. Firebug
> will never want user-level code for page control.
> 
> Firebug needs to read the page.  It uses DOM APIs and walking the object graph.
> Firebug never wants user-level code involved. In fact it does not want getter
> functions to run: we want the "machine" representation, not the view created by
> the developer of the page.

Agreed. And if you remove xpcnativewrappers=no that is what you'll get.

> Firebug would be more correct and more secure if page objects had two APIs:
>   1) Standard API, no user-level manipulation. sounds like XPCNativeWrapper
>   2) Read-only API. No setters, no getters, no writes allowed.  Does this
> exist?

2 does not exist no. 1 is XPCNativeWrapper.

> The advantage of a Read-only API over the SJOW is that it allows carelessness.
> Returning to Jonas' examples: 
> 
> // ERROR: cannot call methods on myROWojbect (unsafe for mySJOWobject)
> myROWobject.addEventListener("load", myChromeFunction, false)
> 
> // Ok and safe
> myXPCNativeWrapper.addEventListener("load", myChromeFunction, false)
> 
> // ERROR cannot call myROWObject.setUserData()
> myROWobject.setUserData("hello", myChromeFunction, null)
> 
> // ERROR no such function
> myXPCNativeWrapper.setUserData("hello", myChromeFunction, null)
> 
> // ERROR, Read only
> myROWobject.myOwnProperty = myChromeFunction;
> // Ok and safe.
> myXPCNativeWrapper.myOwnProperty = myChromeFunction;
> 
> I don't know if Read-Only is possible, but it would be better for Firebug than
> SJOW, as I understand them.

ROW seems very hard. The problem is that we'd have to go through every single function in the whole DOM and figure out which ones are 'read only' and which ones are not. And there are lots of DOM functions, with more added continuously.

And really what you should compare is ROW to XPCNativeWrapper. The main advantage of ROW over XPCNativeWrapper seems to be that it lets you get user-level properties without worrying about getters running? Which you could accomplish using .wrappedJSObject and __lookupGetter__. Though I definitely agree that it's more effort, but it isn't really something that should stop you right now.
> > Also note that there is no difference between enumerating and getting
> > properties. If you can get a property then it'll be there when you enumerate
> > the object. And the other way around.
> 
> What does this rule mean in relation to hasOwnProperty and friends? They don't
> apply to these wrappers or...?

Checked with jst and hasOwnProperty is likely not going to usefully work on these wrappers. Not sure which "friends" you are referring to.

Is this a problem?
(In reply to comment #16)
> Great news on removing xpcnativewrapper=no! This is a huge step forward. Thanks
> a lot for your work.

Well its not a step forward unless it turns out that we can get all the missing function back...but it looks promising.

> > Overall the description is making more sense, but the end-point is not. 
> > Firebug will be put in the position of being as safe as we are careful. 
> 
> Not really sure I follow what you're saying here.

I'm saying that I don't think Firebug is safe.  Relying on me or any other Firebug developer to do some analysis on the source is not wise. Its too complex and fluid to expect success.

> 
> 
> (In reply to comment #14)
> > Since the security holes come from user-level code running with chrome
> > privileges, another way to say this is:  Can Firebug succeed without ever
> > running user-level code outside of the page? 
> 
> Not really sure what you mean by running code "outside of the page". Do you
> mean "when not requested by the page", or do you mean "run in a security
> context other than the pages thus making it possible for the page to gain
> elevated privileges"?
> 
> SJOW does run the page code in such a way that it can't get elevated
> privileges. However it does of course still allow code to run when the
> extension requests it, not when the website requests it. I.e. if an extension
> does:
> 
> webpageWindowSJOW.funFunction();
> 
> funFunction will run, clearly without the webpage requesting it, but it will
> run in such a way that the webpage can't do anything harmful.

Ok, but I have no use for this ability.

> 
> > Well, yes, and in fact any case
> > where Firebug currently runs user-level code outside of the page is either a
> > bug or one waiting --- or an exploit.  User-level code is designed to run in
> > the page, the cases where Firebug runs it, eg foo.toString(), amounts to
> > planning to be lucky.  
> 
> Not really sure I agree that this always is true. For example in the JS object
> view I think you do want to run getters.

But I don't want the getters, I want to know that a property has a getter, but I don't want to run it except within the page.

> 
> ROW seems very hard. The problem is that we'd have to go through every single
> function in the whole DOM and figure out which ones are 'read only' and which
> ones are not. And there are lots of DOM functions, with more added
> continuously.

No, you would just not allow any functions to be called.  No calls, no writes, just property traversal.

(In reply to comment #17)
> > > Also note that there is no difference between enumerating and getting
> > > properties. If you can get a property then it'll be there when you enumerate
> > > the object. And the other way around.
> > 
> > What does this rule mean in relation to hasOwnProperty and friends? They don't
> > apply to these wrappers or...?
> 
> Checked with jst and hasOwnProperty is likely not going to usefully work on
> these wrappers. Not sure which "friends" you are referring to.
> 
> Is this a problem?
> 

Firebug uses hasOwnProperty for its own tables, but maybe not for user-level objects.

I do have one problem however, "typeof(val)" is not giving correct values, specifically user defined functions give "object" not "function".
> > Not really sure I agree that this always is true. For example in the JS 
> > object view I think you do want to run getters.
> 
> But I don't want the getters, I want to know that a property has a getter, but
> I don't want to run it except within the page.

It's up to you of course since you are the FireBug developer, but I don't understand why you wouldn't want to display the result of the getter.

If I'm debugging my web app and have defined a property using a getter, I think that I would want to see what that getter returns when I debug my code using FireBug.

> > ROW seems very hard. The problem is that we'd have to go through every single
> > function in the whole DOM and figure out which ones are 'read only' and which
> > ones are not. And there are lots of DOM functions, with more added
> > continuously.
> 
> No, you would just not allow any functions to be called.  No calls, no writes,
> just property traversal.

Note that almost all of the DOM is implemented as getter functions. Node.firstChild, Node.attributes, NodeList.length, etc all are getters. So if you don't call any getters you are basically left with just the constants.

> I do have one problem however, "typeof(val)" is not giving correct values,
> specifically user defined functions give "object" not "function".

Please file a bug on this.
(In reply to comment #20)
> > > Not really sure I agree that this always is true. For example in the JS 
> > > object view I think you do want to run getters.
> > 
> > But I don't want the getters, I want to know that a property has a getter, but
> > I don't want to run it except within the page.
> 
> It's up to you of course since you are the FireBug developer, but I don't
> understand why you wouldn't want to display the result of the getter.

You would if you had a bug in your getter method.

> 
> If I'm debugging my web app and have defined a property using a getter, I think
> that I would want to see what that getter returns when I debug my code using
> FireBug.

Yes, and the right way to do that is to invoke the getter in the page scope, as is true for command-line and "console" object.

> 
> > > ROW seems very hard. The problem is that we'd have to go through every single
> > > function in the whole DOM and figure out which ones are 'read only' and which
> > > ones are not. And there are lots of DOM functions, with more added
> > > continuously.
> > 
> > No, you would just not allow any functions to be called.  No calls, no writes,
> > just property traversal.
> 
> Note that almost all of the DOM is implemented as getter functions.
> Node.firstChild, Node.attributes, NodeList.length, etc all are getters. So if
> you don't call any getters you are basically left with just the constants.

But again I'm not interested in the DOM functions for this purpose, I assumed I would use the xpcnativewrapper.  Unfortunately Firebug is not designed for objects to have interfaces, that is two different APIs.


> 
> > I do have one problem however, "typeof(val)" is not giving correct values,
> > specifically user defined functions give "object" not "function".
> 
> Please file a bug on this.
> 

But as a practical matter can you tell me how to do this? How do I create a test case for an extension?
> > If I'm debugging my web app and have defined a property using a getter, I 
> > think that I would want to see what that getter returns when I debug my
> > code using FireBug.
> 
> Yes, and the right way to do that is to invoke the getter in the page scope,
> as is true for command-line and "console" object.

Not sure what you mean by "invoke the getter in the page scope".

Using JSOW it will be invoked in the scope of the page just as if the page had accessed it itself.

> > > > ROW seems very hard. The problem is that we'd have to go through every single
> > > > function in the whole DOM and figure out which ones are 'read only' and which
> > > > ones are not. And there are lots of DOM functions, with more added
> > > > continuously.
> > > 
> > > No, you would just not allow any functions to be called.  No calls, no writes,
> > > just property traversal.
> > 
> > Note that almost all of the DOM is implemented as getter functions.
> > Node.firstChild, Node.attributes, NodeList.length, etc all are getters. So 
> > if you don't call any getters you are basically left with just the
> > constants.
> 
> But again I'm not interested in the DOM functions for this purpose, I assumed 
> I would use the xpcnativewrapper.  Unfortunately Firebug is not designed for
> objects to have interfaces, that is two different APIs.

My point is, i think ROW would be of very very limited use. The only thing you could use it for would be getting properties explicitly defined by page-level script.

You wouldn't be able to traverse the DOM, you wouldn't be able to access basically any properties other than ones defined on the window object (since that is where almost all user-level properties end up).

If FireBug indeed does need this I think it would be better to write a JS-library that FireBug could invoke since I don't know of any other extension that has asked for this functionality.


Let me know if you need this functionality in order to remove xpcnativewrappers=no, preferably also pointing to the part of FireBug where it would be used, and we'll figure out how to write such a library.


I'll respond to the bug-filing question through email.
(In reply to comment #22)
> My point is, i think ROW would be of very very limited use. The only thing you
> could use it for would be getting properties explicitly defined by page-level
> script.
> 
> You wouldn't be able to traverse the DOM, you wouldn't be able to access
> basically any properties other than ones defined on the window object (since
> that is where almost all user-level properties end up).

xpcnativewrappers give me access to objects unmodified by user-level javascript. This is great.

The other thing I need is access to the objects modified by user-level code in a way that does not allow errors or exploits in user-level code to alter the result.  We don't have that now. And yes, this is something that would be very very rarely needed, an in fact if I am successful it will be needed by exactly one application.  But that is just like saying we don't need a debugger API because we'll only have one debugger.
Now returning to the problem at hand...

Suppose I do
   var insecureObject = object.wrappedJSObject;
   val = insecureObject[name];  // getter is safe
Now what kind of API do I have in val? It seems from the way my code works that 'val.wrappedJSObject' isn't meaningful, though unfortunately its not merely undefined but rather throws an unconventional and largely opaque exception:
[code]=1000;
[INDEX_SIZE_ERR]=1;
[DOMSTRING_SIZE_ERR]=2;
[HIERARCHY_REQUEST_ERR]=3;
[WRONG_DOCUMENT_ERR]=4;
[INVALID_CHARACTER_ERR]=5;
[NO_DATA_ALLOWED_ERR]=6;
[NO_MODIFICATION_ALLOWED_ERR]=7;
[NOT_FOUND_ERR]=8;
[NOT_SUPPORTED_ERR]=9;
[INUSE_ATTRIBUTE_ERR]=10;
[INVALID_STATE_ERR]=11;
[SYNTAX_ERR]=12;
[INVALID_MODIFICATION_ERR]=13;
[NAMESPACE_ERR]=14;
[INVALID_ACCESS_ERR]=15;
[VALIDATION_ERR]=16;
[TYPE_MISMATCH_ERR]=17;
[message]=Security error;
[result]=2152924136;
[name]=NS_ERROR_DOM_SECURITY_ERR;
[filename]=chrome://firebug/content/dom.js;
[lineNumber]=1230;
[columnNumber]=0;
[location]=JS frame :: chrome://firebug/content/dom.js :: getMembers :: line 1230;
[inner]=null;
[data]=null;
(In reply to comment #24)
> Now returning to the problem at hand...
> 
> Suppose I do
>    var insecureObject = object.wrappedJSObject;
>    val = insecureObject[name];  // getter is safe
> Now what kind of API do I have in val?

object is a XPCNativeWrapper
insecureObject is a SJOW
val is a SJOW

In other words, SJOW "wrapps deep". In other words, if you get a property that is an object, or if you call a method that returns an object, on a SJOW, you'll get another SJOW back.

This is sometimes the case for XPCNativeWrappers too. They wrap deeply when they are automatically created (due to the scope having xpcnativewrappers=yes). However they do unfortunately not nest deeply when an XPCNativeWrapper is created explicitly using XPCNativeWrapper(foo). You shouldn't need to do that though once you've removed xpcnativewrappers=no.

> It seems from the way my code works
> that 'val.wrappedJSObject' isn't meaningful, though unfortunately its not
> merely undefined but rather throws an unconventional and largely opaque
> exception:

That is really odd. Though you shouldn't do this anyway as you already have an SJOW.

Do you by any chance get that exception for any property that doesn't exist? Or just for 'wrappedJSObject'? If the latter we probably need another bug.
We want FB 1.x to work with FF3 - so marking this as blocking so we resolve one way or the other. 
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Depends on: 425139
I had occasion to turn on my script tracing for the first time since we started on xpcnativewrappers.  I see a bunch scripts like:

onScriptCreated: 8690@(445-446)XPCSafeJSObjectWrapper.cpp
onScriptCreated name: 'anonymous'
    function () {
        return "" + this;
    }
and

onScriptCreated: 8691@(445-446)XPCSafeJSObjectWrapper.cpp
onScriptCreated name: 'anonymous'
    function () {
        var args = [];
        for (var i = 1; i < arguments.length; i++) {
            args.push(arguments[i]);
        }
        return arguments[0].apply(this, args);
    }

I don't understand why more than one of these needs to be issued.


What you're seeing there is the internals of how XPCSafeJSObjectWrapper works, and that's not going to change any time soon unless there's something horribly wrong with it. If seeing those through Firebug is a problem then I suggest you filter them out in Firebug and not show them to the user of Firebug.
I understand that that these are internals for XPCSaftJSObjectWrapper. What I am trying to point out is that thousands of these will be generated when ever Firebug looks at a page whereas only two are needed. These scripts can be generated once and reused. 

On top of that, filtering is already the number one user of CPU time in firebug and performance is a significant problem for users.

(In reply to comment #29)
> I understand that that these are internals for XPCSaftJSObjectWrapper. What I
> am trying to point out is that thousands of these will be generated when ever
> Firebug looks at a page whereas only two are needed. These scripts can be
> generated once and reused.

They're generated once per wrapper, and those scripts have their security principal compiled into them (which is the whole point of those scripts) and thus can't be shared. With a whole lot of more work we might be able to do smarter caching of those compiled scripts, but now is *not* the time to do that.

> On top of that, filtering is already the number one user of CPU time in firebug
> and performance is a significant problem for users.

If there's anything that could be done in those scripts themselves to make Firebug's life easier (i.e. make them easier to filter out or what not), that may be a possibility. If so, please let me know but don't expect those to go away or be reduced in number any time soon.
(In reply to comment #20)
> > > Not really sure I agree that this always is true. For example in the JS 
> > > object view I think you do want to run getters.
> > 
> > But I don't want the getters, I want to know that a property has a getter, but
> > I don't want to run it except within the page.
> 
> It's up to you of course since you are the FireBug developer, but I don't
> understand why you wouldn't want to display the result of the getter.
> 
> If I'm debugging my web app and have defined a property using a getter, I think
> that I would want to see what that getter returns when I debug my code using
> FireBug.

In trying to fix 423796 I need to redefine firebug's window.console so that this object is defined by code run within the page rather than an object attached by firebug from chrome. The last step in the fix is to preserve the user experience of simply issuing "window.console.log...". For this I tried to define a getter for 'console' that would load the script defining the console functions on its first call.  Among the many reasons this failed is one related to our discussion about how debuggers treat getters: to my surprise the getter was called every page load, whether or not "window.console" was issued.

Why? Because I was running under a debugger that enumerated the properties of the window object and thus triggered the getter every time.

So under the debugger you can't have code like this:

window.__defineGetter__('dontTouch', function() {
    alert("dontTouch has been accessed");
 });

without causing the alert to be triggered under the debugger. (as far as I can tell).
Taking this off the FF blocker list since the fix will be in FireBug.
Flags: blocking1.9+ → blocking1.9-
This no longer works with Firebug1.2 and Firefox 3 (trunk), but may need a branch resolution.
Version: Trunk → 1.8 Branch
This security exploit no longer works, but no error message results.
I apologize for assigning this bug to myself.  I don't know what will happen if I change the assignment value. Would someone who understands the assignment issues please change it?  Thanks.
Assignee: johnjbarton → nobody
John:  Why should you *not* own this bug?  The fix is in Firebug, correct?  (again, error messaging issues should be tracked elsewhere, and *those* might well be owned by someone else)
Yes I fixed this in Firebug. But I don't understand the consequence of operating on this bug report which contains information on an exploit potentially affecting a couple of hundred thousand Firebug 1.0 and 1.1 users
I'm not sure what you mean by "the consequence of operating on this bug"?
Do we need to keep this bug open? Firebug is fixed these days (by no longer opting out of wrappers right?)

Do we have a generic bug on 'UnsafeJSObjectWrapper' to wrap callback functions as appropriate?
(In reply to comment #39)
> Firebug is fixed these days (by no longer
> opting out of wrappers right?)

From Firebug 1.2 on we do not use xpcnativewrapper=no in the Firebug chrome.manifest.
Filed bug 464527 on the generic issue of having wrappers that make it safe to pass callback functions to content.
And marking this fixed since it was fixed on the firebug end.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.