326501 - Content can implement tree views and trigger memory corruption

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 325236.  The testcase there exploits a bug in page info, but content can just implement a tree view itself and stick alerts in it, thus leading to dead rendering surfaces.  Jag has a testcase he's going to attach.

On trunk, cairo will make the gfx code in question die, but we're still left with arbitrary JS running (and triggering DOM mutations, reflow, etc) while inside Paint().  That's really not a good idea.

Options I see:

1)  Disallow unprivileged tree views.
2)  Change trees somehow to not call into the view during Paint()

Any other ideas?

This probably affects at least the 1.8 branch; we might be safe on 1.7 with the old tree API; not sure.

Comment 1

[jag (Peter Annema)]

Attachment: Careful, this may crash your browser. alert() inside JS getCellText()

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

2) would be ideal. We'd like to support trees for untrusted content.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Agreed.  The problem is I'm not sure how much of the tree arch (and especially performance) is tied to calling getCellText() only in Paint()...

Note also that any getCellText() call can destroy the tree frame.  So doing anything after calling getCellText() is no good, pretty much.

Comment 4

[Jan Varga [:janv]]

The whole tree arch depends on calling tree view methods during Paint().
If you want to avoid it, you will have to cache it somehow, but that would lead to much increased memory footprint IMO.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> The whole tree arch depends on calling tree view methods during Paint().
> If you want to avoid it, you will have to cache it somehow, but that would lead
> to much increased memory footprint IMO.

One solution is to disallow unprivileged tree models, and then provide one (or more) tree model implementations that unprivileged content can use.  For example, one that lets you set up # of columns, and then has addrow/deleterow/setvalue operations on it; all the TreeView callbacks would go to the well-behaved model impl, which would have all the data in memory.  I don't think unprivileged content will be doing all that interesting things with native trees anyway..

Comment 6

[georgi - hopefully not receiving bugspam]

is this linux only?

don't crash on win32/wine

Comment 7

[georgi - hopefully not receiving bugspam]

don't crash on mac osx

Comment 8

[[e-mail]]

(In reply to comment #5)
>One solution is to disallow unprivileged tree models, and then provide one (or
>more) tree model implementations that unprivileged content can use.
Trees default to the DOM model (there are cases where they get the RDF model but that's only useful for sorting). How would they access these other models?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #5)
> I don't think unprivileged content will be doing all that interesting things
> with native trees anyway..

I don't want to assume this.

However, it seems to me that the best architecture for "asychronous" untrusted tree views (where tree cell data is not fetched during Paint, but Paint triggers loading of cell data with update of the tree when the data arrives, and cell data is cached and prefetched for performance/responsiveness) would be to have a regular synchronous, trusted treeview implementation which delegates to the untrusted one --- keeps the complexity out of the already-nasty tree code. So then, I say we go for requiring treeviews to be trusted.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So perhaps when a view is being set we should just check whether the caller context is system and if it's not wrap the view being set in a trusted one?  If we could check whether the view being set is already trusted that would be nice too; perhaps if it is get the underlying view and rewrap it (I don't think we want to assume anything about the claims of a tree view to being trusted).

Comment 11

[[e-mail]]

So, how is this trusted view supposed to work? At the moment, I'm guessing:
* The frame makes a request
* The view logs that the request was made
* The view queues an event (if there isn't one already queued)
* The view returns the last cached value
* The event fires
* The view queries the script for the actual value(s)
* For each value that has changed, the view requests a repaint

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, that sounds about right to me.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Though the view also needs to have some provisions for dropping data at times, possibly...  otherwise it seems like unbounded memory growth can happen easily.

Comment 14

[georgi - hopefully not receiving bugspam]

modifying the testcase

getImageSrc: function(row,col){ return "javascript:alert(this)"; },

gives a strange warning:

JavaScript error: , line 0: uncaught exception: Permission denied to get property Window.alert

don't know in what context the js is tried to execute.

Comment 15

[georgi - hopefully not receiving bugspam]

Attachment: Window.alert exception on javascript: tree image

Comment 16

[Boris Zbarsky [:bzbarsky]]

> don't know in what context the js is tried to execute.

The JS context was that of the window involved.  The security context was the one created from the "javascript:" URI (basically, as close to the non-principal as we can get right now).  See http://lxr.mozilla.org/seamonkey/source/dom/src/jsurl/nsJSProtocolHandler.cpp#245

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> So, how is this trusted view supposed to work? At the moment, I'm guessing:
> * The frame makes a request
> * The view logs that the request was made
> * The view queues an event (if there isn't one already queued)
> * The view returns the last cached value
> * The event fires
> * The view queries the script for the actual value(s)
> * For each value that has changed, the view requests a repaint

Something like that, but that discussion belongs in a different bug. We should make this bug just be about requiring tree views to be trusted.

Comment 18

[Jan Varga [:janv]]

(In reply to comment #13)
> Though the view also needs to have some provisions for dropping data at times,
> possibly...  otherwise it seems like unbounded memory growth can happen easily.
> 

We might end up with building real content and frames after all :)
The tree widget is quite complex and "nasty" because it implements many things on its own for performance reasons.

The old tree widget was buggy and slow, but it supported almost the same API as other widgets, so it didn't confuse XUL developers as the current widget does.
The outliner/new tree widget is very fast, but it has grown into a monster and doesn't fit the current layout code or plans for other layout changes in future.

There are other things like in-line editing (textbox, menulist), arbitrary content in rows, rows with different height.
These things are hard to implement and would make things even more complex.

Maybe it's time to design tree widget v3 :)

My $0.02

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it should be possible to design a new high-performance, high-function tree widget with little or no Gecko support. Make the core of it an overflow:hidden box. On scrolling, just shuffle around the content children and/or repopulate them with new data. But that discussion also belongs elsewhere...

Comment 20

[Jan Varga [:janv]]

(In reply to comment #19)
> I think it should be possible to design a new high-performance, high-function
> tree widget with little or no Gecko support. Make the core of it an
> overflow:hidden box. On scrolling, just shuffle around the content children
> and/or repopulate them with new data. But that discussion also belongs
> elsewhere...
> 

Yeah, that would be great.
I just wanted to express my opinion on that.

Comment 21

[[e-mail]]

(In reply to comment #19)
>I think it should be possible to design a new high-performance, high-function tree
>widget with little or no Gecko support. Make the core of it an overflow:hidden box.
Isn't that what a <listbox> is trying to be?

Comment 22

[Jan Varga [:janv]]

(In reply to comment #21)
> (In reply to comment #19)
> >I think it should be possible to design a new high-performance, high-function tree
> >widget with little or no Gecko support. Make the core of it an overflow:hidden box.
> Isn't that what a <listbox> is trying to be?
> 

I think listbox is overflow-y:auto

Someone please file a new bug for new tree design.

Comment 23

[georgi - hopefully not receiving bugspam]

making the testcase a little more invalid produces this bad sounding assertion:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file /opt/firefox-cvs/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 589
Break: at file /opt/firefox-cvs/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 589
###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

Comment 24

[georgi - hopefully not receiving bugspam]

Attachment: assert no Components

Comment 25

[georgi - hopefully not receiving bugspam]

"assert no Components" must be reloaded for the assertion.

Comment 26

[Daniel Veditz [:dveditz]]

Isn't looking like a 1.8.0.2 patch is a reasonable expectation.

Comment 27

[Darin Fisher]

Any thoughts on how to workaround this bug for 1.8.0.x?  Any takers?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment 11 is about it, as far as I can tell.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I thought we were just going to go with comment #10 and disable tree views from untrusted content.

Comment 30

[Darin Fisher]

OK.  Is someone willing to cut a patch for that?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm working on it. Actually we only need to disable untrusted content setting a non-native tree view.

Comment 32

[Darin Fisher]

-> roc

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Fairly simple. The psuedointerface approach to marking treeviews as usable by untrusted script isn't ideal, perhaps, but it's good enough as far as I can tell.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note that if we land this, we need to tell XUL developers somehow.

Comment 35

[Neil Deakin]

What is this code trying to accomplish? Disable using custom nsITreeViews from remote content?

If so, allowing setting it to anything doesn't serve any purpose for remote content. You seem to be explicitly allowing it to be set to the builder view, which will already be set automatically if needed, and the sql database view (not built by default), which doesn't make any sense since it can't be used from remote content anyway.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK. Is it okay to assume that we won't add any other tree views that we want to make accessible to remote XUL?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Given the number of bugs we've had filed that used custom tree views, I'm not sure whether disabling custom tree views will break xul-based intranet apps...  I say "not sure" because those testcases may have been reduced from extensions, of course.

Hence the proposal to have a built-in wrapper for these custom views -- it would allow us to make this safe without regressing functionality (hopefully).

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That is considerably more work and I hadn't really intended to suggest that approach as a security fix. Getting it right wouldn't be easy, and I suspect that it'd be simpler and more reliable for intranet apps to just get UniveralBrowserWrite.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216077 [details] [diff] [review]
fix

OK, fair enough.

>Index: content/base/src/nsContentUtils.cpp

>+  if (nsContentUtils::IsCallerChrome())
>+    return PR_TRUE;

Not sure it's really worth doing this check up front, since for chrome all capabilities are always enabled and figuring that out is pretty fast.  I'd just do the capability checks here.

r+sr=bzbarsky with that.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 216077 [details] [diff] [review]
fix

Need branch approvals for this 1.8.1/1.8.0.3 blocker.

Comment 42

[georgi - hopefully not receiving bugspam]

today linux trunk doesn't crash on the testcases and gives error

Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "chrome://global/content/bindings/tree.xml Line: 0"]

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 216077 [details] [diff] [review]
fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed on branches.

Comment 45

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060420 Firefox/1.5.0.2, no crash, but I see the same exception as Georgi:
Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "chrome://global/content/bindings/tree.xml Line: 0"]

If there are any other testcases that will test whether or not this will break existing functionality, please let us know.