313566 - privilege escalation using commandDispatcher.focusedElement

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[shutdown]

In XUL documents, document.commandDispatcher.focusedElement
allows attackers to access privileged chrome DOM objects.

Comment 1

[shutdown]

Attachment: testcase

Works on:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051022 Firefox/1.6a1
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b5) Gecko/20051022 Firefox/1.5

Comment 2

[Daniel Veditz [:dveditz]]

Assigning to Blake because ultimately it comes down to another eval() case, but maybe it's a tabbrowser issue. Note that step 2 in the testcase refers to the tab widget for the current exploit page; exploiting this would thankfully require some "social engineering". Clicking another tab and clicking back does't trigger the exploit, but then clicking a second time in the exploit tab widget shows the exploit is still there waiting to get you.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

The problem here isn't eval, per se, but instead that content can get its greasy hands on chrome objects (the opposite of what XPCNativeWrapper was designed to do). I'm patching the XUL command dispatcher to not return chrome (or other, bad, objects) as focus elements, but it would appear that we need to make sure that there are not other avenues of attack through other node-getting functions.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: As described

Comment 5

[Scott MacGregor]

Comment on attachment 200673 [details] [diff] [review]
As described

drive by comment:

+  if (NS_FAILED(rv)) {
+    return rv;
+  }

This should be
NS_ENSURE_SUCCESS(rv, rv);

Comment 6

[Hixie (not reading bugmail)]

Shouldn't attempting to use an Element from a different domain (such as chrome://-
anything from http://-anything) raise a security exception?

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 200673 [details] [diff] [review]
As described

r=jst

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Shouldn't attempting to use an Element from a different domain (such as
> chrome://-
> anything from http://-anything) raise a security exception?

Yes, but we don't access-check on every property access between arbitrary objects. In the web same-origin model, the only paths in the object graph between different trust domains go through window (frame, iframe) articulation points, and those objects' getters and setters check.

There are some hard cases outside of these articulation points in the object graph that must be covered by ad-hoc checks.  And here is one big honking one, thanks to XUL polluting (IMHO) every content window's document object with commandDispatcher.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> There are some hard cases outside of these articulation points in the object
> graph that must be covered by ad-hoc checks.  And here is one big honking one,
> thanks to XUL polluting (IMHO) every content window's document object with
> commandDispatcher.

Let me reemphasize how bad this is.  When we were automating XPCNativeWrappers to protect chrome that accesses content DOM, we said "no content script can get at a chrome DOM object" -- but the joke's on us.  Anyone know of other such cases?  What else did XUL add to the HTML DOM that shows up in content windows?

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Ian, we do special stuff in DOM classinfo in order to do those security checks. Because all of the elements in this testcase are either XUL or XBL, we bypass the security checks. I'm looking into adding security checks there as well.

I made mscott's suggested change.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 200673 [details] [diff] [review]
As described

Oops, need to handle null better.

Also, jst and I both think that GetFocusedWindow needs similar treatment.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Protecting GetFocusedWindow

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 200684 [details] [diff] [review]
Protecting GetFocusedWindow

r=jst

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 200684 [details] [diff] [review]
Protecting GetFocusedWindow

>Index: content/xul/document/src/nsXULCommandDispatcher.cpp
> nsXULCommandDispatcher::GetFocusedElement(nsIDOMElement** aElement)

>+  if (*aElement && !nsContentUtils::CanCallerAccess(*aElement)) {
>+    // XXX This might want to return null, but we use that return value
>+    // to mean "there is no focused element," so to be clear, throw an
>+    // exception.
>+    return NS_ERROR_DOM_SECURITY_ERR;

I think we also want NS_RELEASE(*aElement) here.  Otherwise caller might not know to release it (since our params might be garbage on a failure return).

>+  // Make sure the caller can access this window. The caller can access this
>+  // window iff the window's document has the same origin.

"iff he can access the document".  We're not doing a same-origin check, nor should we be.

>+  if (domdoc && !nsContentUtils::CanCallerAccess(domdoc)) {
>+    return NS_ERROR_DOM_SECURITY_ERR;

Again, NS_RELEASE(*aWindow), please.

Add similar checks to nsIDOMXULDocument.popupNode/tooltipNode, perhaps?  Just to be sure?

sr=bzbarsky

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 200692 [details] [diff] [review]
Updated to comments

This is an updated version of a patch that has r and sr. It simply adds a couple of security checks to prevent scripts from accessing nodes that they should not be able to access.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Checked into trunk.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Checked into MOZILLA_1_8_BRANCH.

Comment 19

[Boris Zbarsky [:bzbarsky]]

This caused bug 319434

Comment 20

[Boris Zbarsky [:bzbarsky]]

Specifically, there are cases when this keeps C++ layout code from accessing these nodes too, which breaks that code.