326931 - Content should not be able to call SetBoxObjectFor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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]]

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 [:bzbarsky]]

> 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]]

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 [:bzbarsky]]

> 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]]

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

[[e-mail]]

For the branches, could we just disallow a non-null boxObject?
That would only require some code rearrangement in nsDocument.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That might work, yeah....

Comment 9

[[e-mail]]

Attachment: Branch-safe patch

Comment 10

[[e-mail]]

Attachment: -w version of above

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

[[e-mail]]

Attachment: Updated for review comments

Comment 13

[[e-mail]]

Fix checked in to the branch for 1.8.1

Comment 14

[[e-mail]]

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]

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 [:bzbarsky]]

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]

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

[[e-mail]]

(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]

OK, that works too.  Please forgive my ignorance :)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Er... SetBoxObjectFor seems to exist on the trunk....  Were we going to remove it in some other bug or something?

Comment 21

[[e-mail]]

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 [:bzbarsky]]

> 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

[[e-mail]]

Fix checked in to the branch for 1.8.0.3

Comment 24

[[e-mail]]

Attachment: "Scary" trunk patch

Because it also includes the nsSupportsHashtable to nsInterfaceHashtable update.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 219278 [details] [diff] [review]
"Scary" trunk patch

Excellent

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 219278 [details] [diff] [review]
"Scary" trunk patch

r=jst

Comment 27

[[e-mail]]

Trunk fix checked in.

Comment 28

[Jay Patel [:jay]]

v.fixed based on comments and code inspection.

Comment 29

[chris hofmann]

could this have caused trunk regression described in https://bugzilla.mozilla.org/show_bug.cgi?id=338630