Last Comment Bug 326931 - Content should not be able to call SetBoxObjectFor
: Content should not be able to call SetBoxObjectFor
Status: RESOLVED FIXED
[sg:critical]
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-12 12:10 PST by Boris Zbarsky [:bz]
Modified: 2013-04-04 13:53 PDT (History)
13 users (show)
timr: blocking1.7.13-
dveditz: blocking1.7.14+
timr: blocking‑aviary1.0.8-
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
Branch-safe patch (1.61 KB, patch)
2006-03-05 09:01 PST, neil@parkwaycc.co.uk
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
-w version of above (1.40 KB, patch)
2006-03-05 09:02 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Updated for review comments (1.64 KB, patch)
2006-03-05 11:45 PST, neil@parkwaycc.co.uk
neil: approval‑aviary1.0.9?
neil: approval1.7.14?
bzbarsky: approval‑branch‑1.8.1+
darin.moz: approval1.8.0.4+
Details | Diff | Review
"Scary" trunk patch (8.90 KB, patch)
2006-04-21 03:16 PDT, neil@parkwaycc.co.uk
jst: review+
bzbarsky: superreview+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2006-02-12 12:10:18 PST
We have all sorts of code that assumes that it can rely on the box object in the table to be correct... Unfortunately, content can set this value to an object of its choice.  Not good.  :(
Comment 1 Boris Zbarsky [:bz] 2006-02-12 12:13:19 PST
Oh, to be precise what this means is that it should be easy to get into situations where we're calling virtual methods on deleted frames.

So I think to fix this we should move the method to nsIDocument (from nsIDOMNSDocument), since the only callers are nsGenericElement and nsXULElement.

The problem is that I really don't have time to write (and test!) the 3+ versions of the patch this will require (for 1.7, 1.8, trunk, with new interfaces added on 1.7/1.8 because we don't want to change nsIDocument) any time in the next few days.

Anyone else willing to help?  :(
Comment 2 Tim Riley [:timr] 2006-02-13 09:25:08 PST
Can we get a security group reading on the security exposure of this?

We're well past code freeze deadline for 1.0.8/1.7.13.  I am minusing for these releases and nominating it for the next 1.0.x/1.7.x release.
Comment 3 Boris Zbarsky [:bz] 2006-02-13 10:27:29 PST
> Can we get a security group reading on the security exposure of this?

As I said in comment 1, "get into situations where we're calling virtual methods on deleted frames".  That means arbitrary code execution (probabilistically) if the attacker can prefill memory with a known byte pattern.

If we're close to shipping 1.7.x we shouldn't hold for this, I guess...
Comment 4 Tim Riley [:timr] 2006-02-13 10:45:54 PST
Comment 3 helps understand what the attack can be.  I am still not sure how easy it is to exploit.  How easy is it for an attacker to pre-fill memory and then to call that memory?  Is this vulnerability low, medium, or high risk of exploit?
Comment 5 Boris Zbarsky [:bz] 2006-02-13 10:54:02 PST
> How easy is it for an attacker to pre-fill memory

Trivial -- create a large array in JavaScript.  We'll put it in memory somewhere.

> and then to call that memory?

This is a little more difficult to answer, especially since we arena-allocate frames.  David, do you know whether we ever shrink the frame arena during the lifetime of a page?  Basically, if we deallocate the frame's memory while it's still in the tree, we can put the JS array described above on top of it, and then we're screwed.  If we don't shrink the arena, we might be ok.

> Is this vulnerability low, medium, or high risk of exploit?

I guess I'd rate it as medium for now pending figuring out exactly what the arena behavior is, I guess.
Comment 6 Daniel Veditz [:dveditz] 2006-02-21 14:39:05 PST
Would love a fix in 1.8.0.2, ask for approval if a patch gets created. Until then this isn't looking like it could make it.
Comment 7 neil@parkwaycc.co.uk 2006-03-03 13:01:51 PST
For the branches, could we just disallow a non-null boxObject?
That would only require some code rearrangement in nsDocument.
Comment 8 Boris Zbarsky [:bz] 2006-03-03 21:03:50 PST
That might work, yeah....
Comment 9 neil@parkwaycc.co.uk 2006-03-05 09:01:27 PST
Created attachment 214092 [details] [diff] [review]
Branch-safe patch
Comment 10 neil@parkwaycc.co.uk 2006-03-05 09:02:26 PST
Created attachment 214093 [details] [diff] [review]
-w version of above
Comment 11 Boris Zbarsky [:bz] 2006-03-05 10:16:14 PST
Comment on attachment 214092 [details] [diff] [review]
Branch-safe patch

>Index: nsDocument.cpp
>+  if (!mBoxObjectTable) {
>+    mBoxObjectTable = new nsSupportsHashtable(12);

I know this code didn't use to handle OOM.... but why don't we fix it to deal while we're here?

r+sr=bzbarsky with that; on trunk I'd still like to move the method to nsIDocument (and perhaps rename to "ClearBoxObjectFor"?).
Comment 12 neil@parkwaycc.co.uk 2006-03-05 11:45:08 PST
Created attachment 214101 [details] [diff] [review]
Updated for review comments
Comment 13 neil@parkwaycc.co.uk 2006-03-05 14:05:02 PST
Fix checked in to the branch for 1.8.1
Comment 14 neil@parkwaycc.co.uk 2006-03-05 14:23:16 PST
Comment on attachment 214101 [details] [diff] [review]
Updated for review comments

Stop people messing around with our table of box objects potentially causing arbitrary code execution.
Comment 15 Darin Fisher 2006-04-07 12:48:40 PDT
Comment on attachment 214101 [details] [diff] [review]
Updated for review comments

 NS_IMETHODIMP
 nsDocument::SetBoxObjectFor(nsIDOMElement* aElement, nsIBoxObject* aBoxObject)
 {
+  if (aBoxObject) {
+    return NS_ERROR_INVALID_ARG;
+  }

So, a non-null aBoxObject is an error?
Comment 16 Boris Zbarsky [:bz] 2006-04-07 13:01:22 PDT
That's correct.  We don't want content to override the box object that _should_ be there, but clearing one is ok (since on next get we'll just create a correct one).  The point is, we need to return the right _types_ of box objects in all cases from GetBoxObjectFor.
Comment 17 Darin Fisher 2006-04-17 12:01:03 PDT
Comment on attachment 214101 [details] [diff] [review]
Updated for review comments

a=darin on behalf of drivers for MOZILLA_1_8_0_BRANCH.  We'd like to see a trunk patch as well before 1.8.0.3 is released.
Comment 18 neil@parkwaycc.co.uk 2006-04-17 13:01:25 PDT
(In reply to comment #17)
>We'd like to see a trunk patch as well before 1.8.0.3 is released.
That'll be a bit tricky; obviously the patch has baked on the 1.8.0 branch for six weeks but this function no longer exists on the trunk.
Comment 19 Darin Fisher 2006-04-17 13:04:05 PDT
OK, that works too.  Please forgive my ignorance :)
Comment 20 Boris Zbarsky [:bz] 2006-04-17 13:11:13 PDT
Er... SetBoxObjectFor seems to exist on the trunk....  Were we going to remove it in some other bug or something?
Comment 21 neil@parkwaycc.co.uk 2006-04-17 13:15:50 PDT
My bad, I was sure we were going to replace it with ClearBoxObjectFor.

darin, this means that the patch has in fact baked on the trunk too :-)
Comment 22 Boris Zbarsky [:bz] 2006-04-17 13:27:55 PDT
> My bad, I was sure we were going to replace it with ClearBoxObjectFor.

Well, we still should.  I don't think this patch got checked in to trunk...  Certainly not according to lxr.
Comment 23 neil@parkwaycc.co.uk 2006-04-21 02:58:59 PDT
Fix checked in to the branch for 1.8.0.3
Comment 24 neil@parkwaycc.co.uk 2006-04-21 03:16:51 PDT
Created attachment 219278 [details] [diff] [review]
"Scary" trunk patch

Because it also includes the nsSupportsHashtable to nsInterfaceHashtable update.
Comment 25 Boris Zbarsky [:bz] 2006-04-21 11:22:14 PDT
Comment on attachment 219278 [details] [diff] [review]
"Scary" trunk patch

Excellent
Comment 26 Johnny Stenback (:jst, jst@mozilla.com) 2006-04-24 20:41:05 PDT
Comment on attachment 219278 [details] [diff] [review]
"Scary" trunk patch

r=jst
Comment 27 neil@parkwaycc.co.uk 2006-04-26 05:42:47 PDT
Trunk fix checked in.
Comment 28 Jay Patel [:jay] 2006-05-11 13:01:57 PDT
v.fixed based on comments and code inspection.
Comment 29 chris hofmann 2006-05-26 12:05:35 PDT
could this have caused trunk regression described in https://bugzilla.mozilla.org/show_bug.cgi?id=338630

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