387390 - The fix for bug 384750 can be circumvented

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

It's possible to call __define[GS]etter__() on an implicit XPCNativeWrapper in
a chrome XBL method.

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[Daniel Veditz [:dveditz]]

How realistic is a fix for this in 1.8.1.5? 

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I'm not sure when that is. I suspect that this is actually the same problem as bug 344495, but will confirm that later today.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

The trick here is that we end up in a cloned XBL method (cloned into content privileges) whose filename is still chrome://... Because of this, EnsureLegalActivity gets confused and allows access (since it looks at the script filename first). This patch makes us ignore the filename for cloned function objects and go straight to the slow path for such frames.

I'm worried about performance with this patch, however, since security checks are expensive (see bug 376291 for proof). This patch makes us do far more security checks when operating on native wrappers. This penalizes against the common case, which seems bad to me.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch to see effects

This patch implements a simple counter to tell how many calls to EnsureLegalActivity were for cloned functions. For a simple test (startup and shutdown, I get):
There were 236 calls and 94 clones (39.83%)

For a slightly more intensive case (starting DOMI and page info, loading pages from history, etc), I get:
There were 2486 calls and 2157 clones (86.77%)

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 271579 [details] [diff] [review]
Proposed fix

Preemptive sr=me pending jst's r=.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I checked this into trunk, hoping for the best wrt Txul and Tp!

Comment 8

[Jeff Walden [:Waldo]]

I had to back this out.  It was causing layout/reftests/bugs/336736-1.html and layout/reftests/bugs/336736-1-ref.html to render equivalently, when currently they're known to not render equivalently on Macs.  Before this patch landed, they both rendered an "a", but subpixel errors caused them to display differently.  After, the following security exception is thrown (in both versions, so this is probably a general marquee problem) and causes nothing to be displayed, and you can't quite get subpixel errors doing that.  :-)  I'll hazard a guess that this patch breaks marquee completely.

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Security Manager vetoed action"  nsresult: "0x80570027 (NS_ERROR_XPC_SECURITY_MANAGER_VETO)"  location: "JS frame :: chrome://xbl-marquee/content/xbl-marquee.xml :: _doMove :: line 439"  data: no]
************************************************************

Comment 9

[moz_bug_r_a4]

Due to the fix in bug 388121, privilege escalation testcases that use location
setter and javascript: url no longer work without change.  I'll attach new
testcases that load a chrome: url before loading a javascript: url to
circumvent the fix.

Comment 10

[moz_bug_r_a4]

Attachment: testcase 2

Comment 11

[moz_bug_r_a4]

Attachment: testcase 3

This does not require user interaction. (This uses bug 344495's trick.)

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I seem to have checked the patch for bug 386695 in with this bug as the bug #.

Comment 13

[moz_bug_r_a4]

I'll attach new testcases since testcase 2 and 3 no longer work on trunk.

Comment 14

[moz_bug_r_a4]

Attachment: testcase 4

Comment 15

[moz_bug_r_a4]

Attachment: testcase 5

This does not require user interaction. (This uses bug 344495's trick.)

Comment 16

[Daniel Veditz [:dveditz]]

Blake: do we have a branch fix here, or do we need to start over given the most recent testcases? 1.8.1.8 code freeze is Oct 3.

Comment 17

[Damon Sicore (:damons)]

Any progress here?

Comment 18

[Damon Sicore (:damons)]

This isn't going to make beta 2.  Marking M11.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Renominating since this might not make it.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Another approach

This fixes this bug by disallowing scripted getters or setters on XPCNativeWrappers at all. I'm hoping that nobody actually tries to do this in an extension, but there's no way to tell. The browser still runs and appears to work with this patch and given that nothing broke when I removed XPCNativeWrapper.__defineGetter__, I don't think this should be a problem for simple browser chrome.

Comment 21

[moz_bug_r_a4]

"Another approach" patch regresses bug 384750.  The testcase in bug 384750 gets
an implicit XPCNativeWrapper by using a chrome xbl method, and then modifies it
_without_ using a chrome xbl method.  And, bug 384750 was fixed by introducing
EnsureLegalActivity(), which blocks "modifies it _without_ using a chrome xbl
method" part.  But, with "Another approach" patch, which removes
EnsureLegalActivity(), content script can set (non getter/setter) properties
and methods on an implicit XPCNativeWrapper _without_ using a chrome xbl
method.  I'll attach a testcase that works with "Another approach" patch.

The testcases in this bug modify an implicit XPCNativeWrapper by calling
__defineGetter__ within a chrome xbl method to circumvent
EnsureLegalActivity().  Actually, no need to use __defineGetter__ if there is a
chrome xbl method that can be used to set properties/methods without using
__defineGetter__.  (But I couldn't find such a method, therefore I used
__defineGetter__.)

Comment 22

[moz_bug_r_a4]

Attachment: testcase 6

This works on trunk with "Another approach" patch.
(also works on 1.8/1.8.0 branches since bug 384750 is fixed only on trunk for
now.)

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Don't remove EnsureLegalActivity

All right, this keeps the worst of both worlds. I played with trying to make XBL functions be bound methods to avoid exposing implicit XPCNativeWrappers to content at all, but that just broke things.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 294552 [details] [diff] [review]
Don't remove EnsureLegalActivity

>+  JSObject *objp;

Canonical name is pobj (second choice: obj2 -- so use "pobj" ;-).

>+  jsid idAsId;
>+
>+  if (!::JS_ValueToId(cx, id, &idAsId) ||
>+      !OBJ_LOOKUP_PROPERTY(cx, obj, idAsId, &objp, &prop)) {
>+    return JS_FALSE;
>+  }
>+
>+  // Do not allow scripted getters or setters on XPCNativeWrappers.
>+  NS_ASSERTION(prop && objp == obj, "Wasn't this property just added?");
>+  JSScopeProperty *sprop = (JSScopeProperty *) prop;

Should assert that pobj is native too, before this sprop deref:

>+  if (sprop->attrs & (JSPROP_GETTER | JSPROP_SETTER)) {
>+    OBJ_DROP_PROPERTY(cx, objp, prop);
>+    return ThrowException(NS_ERROR_ILLEGAL_VALUE, cx);
>+  }
>+
>+  OBJ_DROP_PROPERTY(cx, objp, prop);

Could save attrs in a local and consolidate the OBJ_DROP_PROPERTY.

sr=me with these nits picked.

/be

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

"Fix" checked into trunk.

Comment 26

[Daniel Veditz [:dveditz]]

Not taking bug 384750 in 1.8.1.12, need a combined 1.8 branch patch

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 294552 [details] [diff] [review]
Don't remove EnsureLegalActivity

This patch applies as-is.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 294552 [details] [diff] [review]
Don't remove EnsureLegalActivity

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 30

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

Verified for 1.8 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/2008031114 Firefox/2.0.0.13 after repro'ing bugs in 2.0.0.12.