Closed Bug 387390 Opened 15 years ago Closed 15 years ago

The fix for bug 384750 can be circumvented

Categories

(Core :: Security, defect, P1)

1.8 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta3

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

References

Details

(Keywords: verified1.8.1.13, Whiteboard: [sg:critical])

Attachments

(1 file, 3 obsolete files)

It's possible to call __define[GS]etter__() on an implicit XPCNativeWrapper in
a chrome XBL method.
Attached file testcase (obsolete) —
Flags: blocking1.9?
Flags: blocking1.8.1.6?
Flags: blocking1.8.1.5?
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical]
Version: unspecified → 1.8 Branch
How realistic is a fix for this in 1.8.1.5? 
Assignee: dveditz → mrbkap
Depends on: 384750
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9beta1
Attached patch Proposed fix (obsolete) — Splinter Review
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.
Attachment #271579 - Flags: superreview?(brendan)
Attachment #271579 - Flags: review?(jst)
Attached patch patch to see effects (obsolete) — Splinter Review
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 on attachment 271579 [details] [diff] [review]
Proposed fix

Preemptive sr=me pending jst's r=.

/be
Attachment #271579 - Flags: superreview?(brendan) → superreview+
Blocks: 344495
Attachment #271579 - Flags: review?(jst) → review+
Flags: blocking1.8.1.6?
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
I checked this into trunk, hoping for the best wrt Txul and Tp!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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]
************************************************************
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.9? → blocking1.9+
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.
Attached file testcase 2
Attached file testcase 3
This does not require user interaction. (This uses bug 344495's trick.)
I seem to have checked the patch for bug 386695 in with this bug as the bug #.
I'll attach new testcases since testcase 2 and 3 no longer work on trunk.
Attached file testcase 4
Attached file testcase 5
This does not require user interaction. (This uses bug 344495's trick.)
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.
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Attachment #271579 - Attachment is obsolete: true
Any progress here?
This isn't going to make beta 2.  Marking M11.
Target Milestone: mozilla1.9 M8 → mozilla1.9 M11
Renominating since this might not make it.
Status: REOPENED → ASSIGNED
Flags: blocking1.8.1.12+ → blocking1.8.1.12?
Attached patch Another approach (obsolete) — Splinter Review
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.
Attachment #271580 - Attachment is obsolete: true
Attachment #294311 - Flags: superreview?(brendan)
Attachment #294311 - Flags: review?(jst)
Attachment #294311 - Flags: review?(jst) → review+
Flags: wanted1.8.1.x+
"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__.)
Attached file 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.)
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.
Attachment #294311 - Attachment is obsolete: true
Attachment #294552 - Flags: superreview?(brendan)
Attachment #294552 - Flags: review?(jst)
Attachment #294311 - Flags: superreview?(brendan)
Attachment #294552 - Flags: review?(jst) → review+
Whiteboard: [sg:critical] → [sg:critical][patch]
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
Attachment #294552 - Flags: superreview?(brendan) → superreview+
"Fix" checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Not taking bug 384750 in 1.8.1.12, need a combined 1.8 branch patch
Flags: blocking1.8.1.12+ → blocking1.8.1.13+
Whiteboard: [sg:critical][patch] → [sg:critical][need 1.8 patch]
Comment on attachment 294552 [details] [diff] [review]
Don't remove EnsureLegalActivity

This patch applies as-is.
Attachment #294552 - Flags: approval1.8.1.13?
Whiteboard: [sg:critical][need 1.8 patch] → [sg:critical]
Comment on attachment 294552 [details] [diff] [review]
Don't remove EnsureLegalActivity

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #294552 - Flags: approval1.8.1.13? → approval1.8.1.13+
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1.13
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.
Status: RESOLVED → VERIFIED
Flags: blocking1.8.0.15+
Attachment #294552 - Flags: approval1.8.0.15?
Group: security
You need to log in before you can comment on or make changes to this bug.