Last Comment Bug 326501 - Content can implement tree views and trigger memory corruption
: Content can implement tree views and trigger memory corruption
Status: RESOLVED FIXED
[sg:critical?] [need testing on 1.7 b...
: fixed1.8.1, testcase, verified1.8.0.4
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All Linux
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 325236
Blocks: 327126
  Show dependency treegraph
 
Reported: 2006-02-08 20:28 PST by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2008-07-31 03:20 PDT (History)
22 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2-
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Careful, this may crash your browser. alert() inside JS getCellText() (1.13 KB, application/vnd.mozilla.xul+xml)
2006-02-08 20:34 PST, jag (Peter Annema)
no flags Details
Window.alert exception on javascript: tree image (1.16 KB, application/vnd.mozilla.xul+xml)
2006-02-09 08:53 PST, georgi - hopefully not receiving bugspam
no flags Details
assert no Components (1.20 KB, application/vnd.mozilla.xul+xml)
2006-02-13 08:14 PST, georgi - hopefully not receiving bugspam
no flags Details
fix (12.10 KB, patch)
2006-03-23 18:46 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
bzbarsky: superreview+
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2006-02-08 20:28:22 PST
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) 2006-02-08 20:34:40 PST
Created attachment 211229 [details]
Careful, this may crash your browser. alert() inside JS getCellText()
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-02-08 21:53:44 PST
2) would be ideal. We'd like to support trees for untrusted content.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-02-08 22:21:28 PST
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] 2006-02-09 00:01:41 PST
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] 2006-02-09 00:14:02 PST
(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 2006-02-09 02:00:02 PST
is this linux only?

don't crash on win32/wine
Comment 7 georgi - hopefully not receiving bugspam 2006-02-09 02:12:36 PST
don't crash on mac osx
Comment 8 neil@parkwaycc.co.uk 2006-02-09 02:44:41 PST
(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) (Exited; email my personal email if necessary) 2006-02-09 08:09:09 PST
(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 [:bz] (Out June 25-July 6) 2006-02-09 08:20:12 PST
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 neil@parkwaycc.co.uk 2006-02-09 08:47:15 PST
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 [:bz] (Out June 25-July 6) 2006-02-09 08:48:54 PST
Yeah, that sounds about right to me.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-02-09 08:49:51 PST
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 2006-02-09 08:51:40 PST
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 2006-02-09 08:53:54 PST
Created attachment 211268 [details]
Window.alert exception on javascript: tree image
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-02-09 09:03:55 PST
> 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) (Exited; email my personal email if necessary) 2006-02-09 09:21:31 PST
(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] 2006-02-09 11:10:20 PST
(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) (Exited; email my personal email if necessary) 2006-02-09 11:16:13 PST
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] 2006-02-09 11:32:26 PST
(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 neil@parkwaycc.co.uk 2006-02-09 12:15:07 PST
(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] 2006-02-09 12:35:43 PST
(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 2006-02-13 08:12:01 PST
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 2006-02-13 08:14:19 PST
Created attachment 211727 [details]
assert no Components
Comment 25 georgi - hopefully not receiving bugspam 2006-02-13 08:15:24 PST
"assert no Components" must be reloaded for the assertion.
Comment 26 Daniel Veditz [:dveditz] 2006-02-16 12:38:44 PST
Isn't looking like a 1.8.0.2 patch is a reasonable expectation.
Comment 27 Darin Fisher 2006-03-23 11:06:57 PST
Any thoughts on how to workaround this bug for 1.8.0.x?  Any takers?
Comment 28 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-03-23 13:37:15 PST
Comment 11 is about it, as far as I can tell.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-23 16:33:40 PST
I thought we were just going to go with comment #10 and disable tree views from untrusted content.
Comment 30 Darin Fisher 2006-03-23 17:16:42 PST
OK.  Is someone willing to cut a patch for that?
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-23 17:29:29 PST
I'm working on it. Actually we only need to disable untrusted content setting a non-native tree view.
Comment 32 Darin Fisher 2006-03-23 17:57:12 PST
-> roc
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-23 18:46:01 PST
Created attachment 216077 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2006-03-23 18:46:50 PST
Note that if we land this, we need to tell XUL developers somehow.
Comment 35 Neil Deakin 2006-03-23 19:01:00 PST
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) (Exited; email my personal email if necessary) 2006-03-23 19:32:49 PST
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 [:bz] (Out June 25-July 6) 2006-03-23 21:28:25 PST
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) (Exited; email my personal email if necessary) 2006-03-24 01:16:52 PST
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 [:bz] (Out June 25-July 6) 2006-03-24 10:02:32 PST
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) (Exited; email my personal email if necessary) 2006-03-26 12:59:28 PST
checked in.
Comment 41 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-03-26 13:00:51 PST
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 2006-03-27 00:20:14 PST
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] 2006-04-03 12:12:41 PDT
Comment on attachment 216077 [details] [diff] [review]
fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-04-03 17:13:43 PDT
Fixed on branches.
Comment 45 Jay Patel [:jay] 2006-04-20 14:44:47 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.