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

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: moz_bug_r_a4, Assigned: smaug)

Tracking

({fixed1.8.0.15, testcase, verified1.8.1.12})

unspecified
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments)

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).
Posted file testcase 1
This works on trunk, fx2.0.0.x and fx1.5.0.x.
Posted file 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.
Assignee: dveditz → jst
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.9?
Whiteboard: [sg:critical]
I have some code (for Bug 382636), which may fix this one too. I'll test...
Taking. My patch does fix both testcases. Will clean it up a bit before uploading.
Assignee: jst → Olli.Pettay
Posted patch the ideaSplinter Review
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?
Btw, I think the cxpushers in ESM could be removed, but didn't do that here
to reduce the regressions risk.
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.
Attachment #278324 - Flags: review?(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?
Feel free to review :)
Flags: blocking1.9? → blocking1.9+
Status: NEW → ASSIGNED
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...
Attachment #278324 - Flags: review?(jst) → review+
Comment on attachment 278324 [details] [diff] [review]
the idea

peterv has probably shorter review queue.
Attachment #278324 - Flags: superreview?(peterv)
Peterv, would be great to get sr before M9 ;)
Seconding Smaug's request for SR.  Please Peter?   :-)
Blocks: 393761
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?
Attachment #278324 - Flags: superreview?(peterv) → superreview+
Any ideas for better naming?
Get/SetScriptHandlingContext ? (a bit ugly)
Get/SetScriptHandlingObject ? (- "" -)
Get/SetExecutionContext? (maybe)
Get/SetScriptExecutionContext? (maybe)
Posted patch better namingSplinter Review
Checked in
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 393761
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.
Flags: blocking1.8.1.9?
Not sure we could safely pull this into a short 1.8.1.10 release. Smaug, jst: your thoughts?
Flags: blocking1.8.1.11?
Flags: blocking1.8.1.11+
Flags: blocking1.8.1.10?
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.
Flags: blocking1.8.1.10?
Whiteboard: [sg:critical] → [sg:critical][need 1.8 patch]
Smaug: any progress on the branch version? A patch this large makes us nervous and we want it in sooner rather than later.
Verified using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007121107 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
The branch version is on my todo-list, now that all other relevant bugs have
trunk patches.
Posted patch for 1.8Splinter Review
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.
Attachment #294964 - Flags: review?(jst)
Attachment #294964 - Flags: review?(jst) → review+
Attachment #294964 - Flags: superreview?(jonas)
Comment on attachment 294964 [details] [diff] [review]
for 1.8

Or perhaps peterv could sr
Attachment #294964 - Flags: superreview?(jonas) → superreview?(peterv)
Flags: in-testsuite?
Whiteboard: [sg:critical][need 1.8 patch] → [sg:critical][need 1.8 sr=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).
Attachment #294964 - Flags: superreview?(peterv) → superreview+
(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.
Attachment #294964 - Flags: approval1.8.1.12?
Blocks: 360529
Whiteboard: [sg:critical][need 1.8 sr=peterv] → [sg:critical]
(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.

FWIW, I think our policy so far has been that it's ok to modify _BRANCH interfaces.
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?
Attachment #294964 - Flags: approval1.8.1.12? → approval1.8.1.12+
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...
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
Blocks: 382636
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.
Group: security
Posted patch for 1.8.0Splinter Review
smaug, please take a look if this a good port for 1.8.0
Attachment #310230 - Flags: review?(Olli.Pettay)
Flags: blocking1.8.0.15+
Attachment #310230 - Flags: review?(Olli.Pettay) → review+
Attachment #310230 - Flags: approval1.8.0.15?
Comment on attachment 310230 [details] [diff] [review]
for 1.8.0

a=caillon for 1.8.0.15
Attachment #310230 - Flags: approval1.8.0.15? → approval1.8.0.15+
Committed to the 1.8.0 branch
Keywords: fixed1.8.0.15
Depends on: 252542
You need to log in before you can comment on or make changes to this bug.