393762 - Arbitrary code execution using an event handler attached to an element whose owner document has no script global object

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

Please see bug 383424.

A document created by DOMImplementation.createDocument() does not have a script
global object.  With such a document, nsCxPusher::Push() does not push a JS
context.  And it's possible to change the owner document of an element by using
adoptNode() (or appendChild() on 1.8/1.8.0 branches).

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1

This works on trunk, fx2.0.0.x and fx1.5.0.x.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2

This takes advantage of bug 393761's XSS trick.
This works on trunk, fx2.0.0.x, fx1.5.0.x, sm-trunk, sm1.1.x and sm1.0.x.

Comment 3

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

I have some code (for Bug 382636), which may fix this one too. I'll test...

Comment 4

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

Taking. My patch does fix both testcases. Will clean it up a bit before uploading.

Comment 5

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

Attachment: the idea

This is the idea to fix this bug and Bug 393761 and Bug 382636.
Mark cloned and createDocument documents as "data". Make them to have
"eventhandlingcontext", which is basically the |window| of the original document.
Use eventhandlingcontext in nsCxPusher, and make sure there is always some 
valid cx to push. Exception is the case when document is created without
eventhandlingcontext (jscomponent creating domparser, for example). In that case
the context should ok anyway.
Passes mochitest, chrome and TUnit.
Btw, I was doing this even before the security bugs to fix document.clone().
Comments?

Comment 6

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

Btw, I think the cxpushers in ESM could be removed, but didn't do that here
to reduce the regressions risk.

Comment 7

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

Comment on attachment 278324 [details] [diff] [review]
the idea

After a night this still feels reasonable solution.
Asking the review to get at least comments about the idea.
Binding XHR and DOMParser to |window| (like in Bug 372964) would have made the patch a bit cleaner, but should work fine even without that.

Comment 8

[Johnny Stenback (:jst)]

This does seem like a sane approach to me. Do you want me to do a proper review on this already, or is there more work that should be done here before this is ready for prime time?

Comment 9

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

Feel free to review :)

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 278324 [details] [diff] [review]
the idea

Looks good to me. You should get someone else to do an sr on this one though, peterv or jonas come to mind...

Comment 11

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

Comment on attachment 278324 [details] [diff] [review]
the idea

peterv has probably shorter review queue.

Comment 12

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

Peterv, would be great to get sr before M9 ;)

Comment 13

[Damon Sicore (:damons)]

Seconding Smaug's request for SR.  Please Peter?   :-)

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 278324 [details] [diff] [review]
the idea

Not sure Get/SetEventHandlingContext are the right names, given that it returns the object from which to get the context, also from the change in nsTextBoxFrame.cpp this is used for more than event handling.
Would it make sense to pass in the eventHandlingContext as an argument to NS_NewDOMDocument?

Comment 15

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

Any ideas for better naming?
Get/SetScriptHandlingContext ? (a bit ugly)
Get/SetScriptHandlingObject ? (- "" -)
Get/SetExecutionContext? (maybe)
Get/SetScriptExecutionContext? (maybe)

Comment 16

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

Attachment: better naming

Comment 17

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

Checked in

Comment 19

[Daniel Veditz [:dveditz]]

We'll want this on the 1.8 branch, but given the size of the patch and the need to rework the interface changes (_MOZILLA_1_8_BRANCH uglification) we want to land early in a cycle rather than at the end.

Comment 20

[Daniel Veditz [:dveditz]]

Not sure we could safely pull this into a short 1.8.1.10 release. Smaug, jst: your thoughts?

Comment 21

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

Haven't heard any regressions on trunk and it should be pretty safe.
I think event listeners on data documents aren't that common.
I'll try to come up with a branch patch within next few days.

Comment 22

[Daniel Veditz [:dveditz]]

Smaug: any progress on the branch version? A patch this large makes us nervous and we want it in sooner rather than later.

Comment 23

[Samuel Sidler (old account; do not CC)]

Verified using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007121107 Minefield/3.0b2pre

Comment 24

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

The branch version is on my todo-list, now that all other relevant bugs have
trunk patches.

Comment 25

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

Attachment: for 1.8

Since the code has change quite a lot on trunk, the branch patch really
needs good reviews.
The main difference is that on branch there isn't nsContentUtils::CreateDocument
and so nsXMLHttpRequest and nsDOMParser create documents in a bit different way.

Comment 26

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

Comment on attachment 294964 [details] [diff] [review]
for 1.8

Or perhaps peterv could sr

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 294964 [details] [diff] [review]
for 1.8

>Index: content/base/public/nsIDocument.h
>===================================================================

>+#define NS_IDOCUMENT_MOZILLA_1_8_BRANCH3_IID      \
>+{ 0x23da8ffb, 0x095d, 0x4215, \
>+  { 0x9a, 0x93, 0x3e, 0x0f, 0xe4, 0x10, 0x89, 0x0b } }
>+
>+class nsIDocument_MOZILLA_1_8_BRANCH3 : public nsISupports

Not really sure why we need this instead of just changing the IID of nsIDocument_MOZILLA_1_8_BRANCH2.

> NS_NewDOMDocument(nsIDOMDocument** aInstancePtrResult,
>                   const nsAString& aNamespaceURI, 
>                   const nsAString& aQualifiedName, 
>                   nsIDOMDocumentType* aDoctype,
>                   nsIURI* aBaseURI);
>+

Pointless whitespace change.

> nsresult

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

>+    nsCOMPtr<nsIDocument_MOZILLA_1_8_BRANCH3> branch3doc =
>+      do_QueryInterface(document);
>+    NS_ASSERTION(branch3doc,
>+                 "Document must implement nsIDocument_MOZILLA_1_8_BRANCH3!!!");
>+    PRBool hasHadScriptObject = PR_TRUE;
>+    sgo = branch3doc->GetScriptHandlingObject(hasHadScriptObject);
>+    // It is bad if the document doesn't have event handling context,
>+    // but it used to have one.
>+    NS_ENSURE_TRUE(sgo || !hasHadScriptObject, PR_FALSE);

BTW, on trunk this seems to be triggered by just running the bloat test, see for example: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1200858300.1200858794.17250.gz&fulltext=1 (look for hasHadScriptObject).

Comment 28

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

(In reply to comment #27)
> Not really sure why we need this instead of just changing the IID of
> nsIDocument_MOZILLA_1_8_BRANCH2.
I wasn't sure about that either so I created the BRANCH3, just in case
someone is already using BRANCH2 in some extensions

> BTW, on trunk this seems to be triggered by just running the bloat test
I'm aware of that. Seems to happen during shutdown, during the time when
some |window| *really* shouldn't be accessed anymore.

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to comment #28)
> (In reply to comment #27)
> > Not really sure why we need this instead of just changing the IID of
> > nsIDocument_MOZILLA_1_8_BRANCH2.
> I wasn't sure about that either so I created the BRANCH3, just in case
> someone is already using BRANCH2 in some extensions

I've been told that if we've shipped one of these on the branch, then we need to increment the BRANCHX part, just in case someone used it.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, I think our policy so far has been that it's ok to modify _BRANCH interfaces.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 294964 [details] [diff] [review]
for 1.8

approved for 1.8.1.12, a=dveditz for release-drivers

What areas are at risk from this patch, that should get extra testing?

Comment 32

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

I believe risks aren't that bad. Afaik event listeners on data documents aren't
very common.
But the risks there are, are possibly XHR handling, feed handling?, DOMParser, 
search bar (it uses XHR), then something related to shutdown when
inner-outer windows aren't connected anymore...

Comment 33

[Samuel Sidler (old account; do not CC)]

Both testcases no longer work using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/2008012820 Firefox/2.0.0.12

Comment 34

[Al Billings [:abillings - ex-MoCo]]

Re-verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.12) Gecko/2008013015 Firefox/2.0.0.12. Still fixed. I checked with 2.0.0.11 and verified the repro cases on this machine as well.

Comment 35

[Alexander Sack]

Attachment: for 1.8.0

smaug, please take a look if this a good port for 1.8.0

Comment 36

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 310230 [details] [diff] [review]
for 1.8.0

a=caillon for 1.8.0.15

Comment 37

[Christopher Aillon (sabbatical, not receiving bugmail)]

Committed to the 1.8.0 branch