456271 - Make it impossible to create script-exposed documents without script handling objects

Status: VERIFIED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now, any document without a script handling object that is exposed to content script is a security bug.  At the same time, it's trivial to create such documents: by default documents have no script handling object.  Not only that, but even chokepoint methods like nsContentUtils::CreateDocument happily allow creation of documents with no script handling object.

We presumably need that for all the proper functioning of XHR from C++, so how about making sure that:

1)  An nsINode whose GetOwnerDoc() has no script handling object cannot be
    JS-wrapped.
2)  Adopting or importing a node from a document with a script handling object
    into one without fails.

That should at least make sure we don't accidentally expose such documents to script (something that at least one set of patches in progress was pretty close to doing, since none of this stuff is documented anywhere).  Of course ideally this script handling object bogosity will go away...  smaug says there might be a bug on that.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: something like this

I haven't really verified this works, because I don't know how a script could
get access to problematic documents.
So I need to write a C++ test case for this and return
a document without script handling object to JS.

Especially I'm not familiar with quickstubs, so if that changes something here, 
a hint how to fix that would be useful.

Comment 2

[Boris Zbarsky [:bzbarsky]]

quickstubs don't affect PreCreate so far, so we're good there.

You could verify this works by disabling the setting of the script handling object in XHR and checking that you can no longer get the responseXML from the XHR object.   Not sure we can write a test for this in general easily.

Still need to fix the import/adopt thing, btw.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Only adopt, since that is the only case where event listeners are moved to a
new document.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Or, hmm, right. Clone/import may cause something bad too, because of
onfoo attributes.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: or maybe this

I tested this by making .createDocument to create non-scriptable-documents.
Content couldn't access those but chrome could.
Also the following works in chrome
Components.classes["@mozilla.org/xml/xml-document;1"].createInstance(Components.interfaces.nsIDOMDocument); and this is why we still need to allow trusted scripts to access those documents, I think.
Not perfect solution, but should prevent possible security problems where
content's event listeners run with chrome privileges.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I guess import is not a problem because you won't be able to JS-wrap the new node.  So you're right, just have to deal with adopt.

I'm not sure about allowing chrome to access these documents: they could leak into content.

The fact that documents are creatable with a classid is more or less an accident; it creates bogus documents, and we want to remove it one of these days when we get around to it.  I see no reason to make sure it works from chrome script.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #6)
> I'm not sure about allowing chrome to access these documents: they could leak
> into content.
But would they use the same wrappers then?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sure, if the chrome passes its wrapper to the content code.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

But wouldn't that case bring other security problems anyway, I mean even with
documents which have script handling object. Wrappers' scope is set in
PreCreate.
And if chrome gives a document with a chrome script handling object to content
and content can somehow modify it, event listeners will run with chrome privileges.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, it probably would.  I guess I just don't see a usecase in special-casing chrome here, and would rather not add the extra code.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I don't want to break any extensions, which may use .createInstance.

Comment 12

[Boris Zbarsky [:bzbarsky]]

We've said loud and clear that those shouldn't be used.  In fact, I'd support removing them entirely for 1.9.1.  If extensions are using them, they need to stop anyway.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> We've said loud and clear that those shouldn't be used.
We have? Where?

I'd like to get the fix to 1.9.0 too, so would be great to get the same
patch first to 1.9.1 to get some baking time.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> We have? Where?

Any time someone brought it up in the newsgroups, for one thing...

I can see the 1.9.0 argument.  Let's do this your way for now, then, and file a followup to remove the code before we get too far into the 1.9.1 betas, maybe?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 339696 [details] [diff] [review]
or maybe this

>+  // If we have a document, make sure

 ... one of these is true

r+sr=bzbarsky with that.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +"one of these is true"

Comment 17

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/36efcc70a426

Comment 18

[Daniel Veditz [:dveditz]]

smaug(In reply to comment #13)
> I'd like to get the fix to 1.9.0 too, so would be great to get the same
> patch first to 1.9.1 to get some baking time.

Seems well-baked, how much of this and which regression fixes (bug 465767?) do we want on the 1.9.0 branch? Were there any extensions affected?

Comment 19

[Daniel Veditz [:dveditz]]

Unless we know specific documents we're exposing unsafely this is more of a preventive action than a "critical" security bug, but seems an easy mistake to make so guessing at "moderate" severity.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified based on checkins. Landed on trunk as http://hg.mozilla.org/mozilla-central/rev/36efcc70a426

Can we have a test here?

Comment 21

[Daniel Veditz [:dveditz]]

Since the regression situation is unclear and there hasn't been any response I'm guessing this isn't making the 1.9.0.8 code-freeze :-(

Comment 22

[Daniel Veditz [:dveditz]]

Given the still-open dependencies/regressions we'll wait on this for the 1.9.0 branch

Comment 23

[Daniel Veditz [:dveditz]]

Is regression bug 460460 going to get fixed? Or is that now the intended behavior and addon authors will simply have to deal with it?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Bug 460460 is going to get fixed eventually, I hope. But I'm not yet sure how.
Hopefully http://mxr-test.konigsberg.mozilla.org/bonsai/cvsblame.cgi?file=js/src/xpconnect/src/xpccallcontext.cpp&xrev=913aaa2f38d3&mark=95-102#93 gets changed or security checks changed so that they don't rely on the JSContext.