Closed
Bug 387390
Opened 17 years ago
Closed 17 years ago
The fix for bug 384750 can be circumvented
Categories
(Core :: Security, defect, P1)
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)
1.10 KB,
patch
|
jst
:
review+
brendan
:
superreview+
dveditz
:
approval1.8.1.13+
asac
:
approval1.8.0.next?
|
Details | Diff | Splinter Review |
It's possible to call __define[GS]etter__() on an implicit XPCNativeWrapper in a chrome XBL method.
Reporter | ||
Comment 1•17 years ago
|
||
Updated•17 years ago
|
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
Comment 2•17 years ago
|
||
How realistic is a fix for this in 1.8.1.5?
Assignee: dveditz → mrbkap
Depends on: 384750
Assignee | ||
Comment 3•17 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•17 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9beta1
Assignee | ||
Comment 4•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 271579 [details] [diff] [review] Proposed fix Preemptive sr=me pending jst's r=. /be
Attachment #271579 -
Flags: superreview?(brendan) → superreview+
Updated•17 years ago
|
Attachment #271579 -
Flags: review?(jst) → review+
Updated•17 years ago
|
Flags: blocking1.8.1.6?
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Updated•17 years ago
|
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Assignee | ||
Comment 7•17 years ago
|
||
I checked this into trunk, hoping for the best wrt Txul and Tp!
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 8•17 years ago
|
||
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•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Reporter | ||
Comment 9•17 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•17 years ago
|
||
Reporter | ||
Comment 11•17 years ago
|
||
This does not require user interaction. (This uses bug 344495's trick.)
Assignee | ||
Comment 12•17 years ago
|
||
I seem to have checked the patch for bug 386695 in with this bug as the bug #.
Reporter | ||
Comment 13•17 years ago
|
||
I'll attach new testcases since testcase 2 and 3 no longer work on trunk.
Reporter | ||
Comment 14•17 years ago
|
||
Reporter | ||
Comment 15•17 years ago
|
||
This does not require user interaction. (This uses bug 344495's trick.)
Comment 16•17 years ago
|
||
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.
Updated•17 years ago
|
Flags: blocking1.8.1.8+ → blocking1.8.1.9+
Assignee | ||
Updated•17 years ago
|
Attachment #271579 -
Attachment is obsolete: true
Priority: -- → P1
Comment 17•17 years ago
|
||
Any progress here?
Comment 18•17 years ago
|
||
This isn't going to make beta 2. Marking M11.
Target Milestone: mozilla1.9 M8 → mozilla1.9 M11
Assignee | ||
Comment 19•17 years ago
|
||
Renominating since this might not make it.
Status: REOPENED → ASSIGNED
Flags: blocking1.8.1.12+ → blocking1.8.1.12?
Assignee | ||
Comment 20•17 years ago
|
||
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•17 years ago
|
Attachment #294311 -
Flags: review?(jst) → review+
Updated•17 years ago
|
Flags: wanted1.8.1.x+
Reporter | ||
Comment 21•17 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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
Attachment #294552 -
Flags: review?(jst) → review+
Assignee | ||
Updated•17 years ago
|
Whiteboard: [sg:critical] → [sg:critical][patch]
Comment 24•17 years ago
|
||
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•17 years ago
|
||
"Fix" checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Flags: in-testsuite?
Updated•17 years ago
|
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Comment 26•17 years ago
|
||
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•17 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?
Updated•17 years ago
|
Whiteboard: [sg:critical][need 1.8 patch] → [sg:critical]
Comment 28•17 years ago
|
||
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+
Comment 30•17 years ago
|
||
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•17 years ago
|
Flags: blocking1.8.0.15+
Updated•17 years ago
|
Attachment #294552 -
Flags: approval1.8.0.15?
Updated•17 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•