The fix for bug 384750 can be circumvented

VERIFIED FIXED in mozilla1.9beta3

Status

()

Core
Security
P1
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({verified1.8.1.13})

1.8 Branch
mozilla1.9beta3
verified1.8.1.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

11 years ago
Created attachment 271501 [details]
testcase
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
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9beta1
(Assignee)

Comment 4

11 years ago
Created attachment 271579 [details] [diff] [review]
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.
Attachment #271579 - Flags: superreview?(brendan)
Attachment #271579 - Flags: review?(jst)
(Assignee)

Comment 5

11 years ago
Created attachment 271580 [details] [diff] [review]
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 on attachment 271579 [details] [diff] [review]
Proposed fix

Preemptive sr=me pending jst's r=.

/be
Attachment #271579 - Flags: superreview?(brendan) → superreview+

Updated

11 years ago
Blocks: 344495

Updated

11 years ago
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+
(Assignee)

Comment 7

11 years ago
I checked this into trunk, hoping for the best wrt Txul and Tp!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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 → ---

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
(Reporter)

Comment 9

11 years ago
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.
(Reporter)

Comment 10

11 years ago
Created attachment 273952 [details]
testcase 2
(Reporter)

Comment 11

11 years ago
Created attachment 273953 [details]
testcase 3

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

Comment 12

11 years ago
I seem to have checked the patch for bug 386695 in with this bug as the bug #.
(Reporter)

Comment 13

10 years ago
I'll attach new testcases since testcase 2 and 3 no longer work on trunk.
(Reporter)

Comment 14

10 years ago
Created attachment 281435 [details]
testcase 4
(Reporter)

Comment 15

10 years ago
Created attachment 281436 [details]
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+
(Assignee)

Updated

10 years ago
Attachment #271579 - Attachment is obsolete: true
Priority: -- → P1
Any progress here?
This isn't going to make beta 2.  Marking M11.
Target Milestone: mozilla1.9 M8 → mozilla1.9 M11
(Assignee)

Comment 19

10 years ago
Renominating since this might not make it.
Status: REOPENED → ASSIGNED
Flags: blocking1.8.1.12+ → blocking1.8.1.12?
(Assignee)

Comment 20

10 years ago
Created attachment 294311 [details] [diff] [review]
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.
Attachment #271580 - Attachment is obsolete: true
Attachment #294311 - Flags: superreview?(brendan)
Attachment #294311 - Flags: review?(jst)

Updated

10 years ago
Attachment #294311 - Flags: review?(jst) → review+
Flags: wanted1.8.1.x+
(Reporter)

Comment 21

10 years ago
"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__.)
(Reporter)

Comment 22

10 years ago
Created attachment 294521 [details]
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.)
(Assignee)

Comment 23

10 years ago
Created attachment 294552 [details] [diff] [review]
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.
Attachment #294311 - Attachment is obsolete: true
Attachment #294552 - Flags: superreview?(brendan)
Attachment #294552 - Flags: review?(jst)
Attachment #294311 - Flags: superreview?(brendan)

Updated

10 years ago
Attachment #294552 - Flags: review?(jst) → review+
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 25

10 years ago
"Fix" checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago10 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]
(Assignee)

Comment 27

10 years ago
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+
(Assignee)

Comment 29

10 years ago
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
Keywords: fixed1.8.1.13 → verified1.8.1.13

Updated

10 years ago
Flags: blocking1.8.0.15+

Updated

10 years ago
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.