Closed
Bug 569162
Opened 14 years ago
Closed 14 years ago
GC hazard with lookupProperty callers with scripted proxy handlers
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status2.0 | --- | unaffected |
blocking1.9.2 | --- | .13+ |
status1.9.2 | --- | .13-fixed |
blocking1.9.1 | --- | .16+ |
status1.9.1 | --- | .16-fixed |
People
(Reporter: gal, Assigned: igor)
References
Details
(Whiteboard: [sg:critical?][ccbr][critsmash:patch][stale][1.9.x branches])
Attachments
(1 file, 1 obsolete file)
3.37 KB,
patch
|
brendan
:
review+
christian
:
approval1.9.2.13+
christian
:
approval1.9.1.16+
|
Details | Diff | Splinter Review |
lookupProperty and other object ops calls might not root obj.
Reporter | ||
Comment 1•14 years ago
|
||
Assignee: general → gal
Attachment #448307 -
Flags: review?(brendan)
Reporter | ||
Updated•14 years ago
|
Reporter | ||
Comment 2•14 years ago
|
||
Proxies didn't exist in 192 and 191, so no need to block.
blocking1.9.2: ? → ---
status1.9.1:
? → ---
Comment 3•14 years ago
|
||
Comment on attachment 448307 [details] [diff] [review] patch >diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp >--- a/js/src/jsproxy.cpp >+++ b/js/src/jsproxy.cpp >@@ -513,40 +513,43 @@ ReturnedValueMustNotBePrimitive(JSContex > return true; > } > > bool > JSScriptedProxyHandler::getPropertyDescriptor(JSContext *cx, JSObject *proxy, jsid id, > JSPropertyDescriptor *desc) > { > JSObject *handler = JSVAL_TO_OBJECT(proxy->getProxyHandler()); >+ AutoObjectRooter objr(cx, proxy); > AutoValueRooter tvr(cx); > return FundamentalTrap(cx, handler, ATOM(getPropertyDescriptor), tvr.addr()) && > TryHandlerTrap(cx, proxy, Trap1(cx, handler, tvr.value(), id, tvr.addr())) && > ReturnedValueMustNotBePrimitive(cx, proxy, ATOM(getPropertyDescriptor), tvr.value()) && > ParsePropertyDescriptorObject(cx, proxy, id, tvr.value(), desc); etc. Ok, who is violating caller-roots? What caller(s) passed unrooted proxy into this and similar methods? /be
Reporter | ||
Comment 4•14 years ago
|
||
Quoting Igor: "Most if not all all callers of lookupProperty pass pointer to unrooted JSObject * from the native stack for the objp parameter." I can dig into it a bit and find out where this happens. There aren't that many call sites (25ish). We should take the discussion to the other bug. If we decide to fix that one we can invalidate this one.
Reporter | ||
Comment 5•14 years ago
|
||
Comment on attachment 448307 [details] [diff] [review] patch We will try to fix the general case.
Attachment #448307 -
Flags: review?(brendan)
Updated•14 years ago
|
status1.9.2:
--- → unaffected
Whiteboard: [sg:critical?]
Updated•14 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][ccbr][critsmash:investigating]
Updated•14 years ago
|
Whiteboard: [sg:critical?][ccbr][critsmash:investigating] → [sg:critical?][ccbr][critsmash:patch]
Comment 6•14 years ago
|
||
(In reply to comment #5) > (From update of attachment 448307 [details] [diff] [review]) > We will try to fix the general case. What's the status here?
Updated•14 years ago
|
status1.9.1:
--- → unaffected
Comment 7•14 years ago
|
||
ping...?
Updated•14 years ago
|
Comment 8•14 years ago
|
||
we have a conservative stack scanner on 1.9.3, so this doesn't bite there. the patch here isn't the greatest, so more research needed.
Updated•14 years ago
|
Whiteboard: [sg:critical?][ccbr][critsmash:patch] → [sg:critical?][ccbr][critsmash:patch][stale]
Comment 9•14 years ago
|
||
What specific action is next here?
Comment 10•14 years ago
|
||
Am I understanding the bug state correctly? - we no longer have a security problem in 1.9.3 (comment 8) - despite lack of proxies this is potentially exploitable in 1.9.1 and 1.9.2? (due to LiveConnect?) Need a new patch for branches only?
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10) > Am I understanding the bug state correctly? > - we no longer have a security problem in 1.9.3 (comment 8) Yes - the conservative scanner has fixed this nicely. > - despite lack of proxies this is potentially exploitable in 1.9.1 and 1.9.2? > (due to LiveConnect?) Yes. > > Need a new patch for branches only? Yes.
Updated•14 years ago
|
Comment 12•14 years ago
|
||
Andreas: Is the patch in this bug a valid start for an older branch patch or should it be marked obsolete?
blocking1.9.1: ? → .13+
blocking1.9.2: ? → .10+
Updated•14 years ago
|
status2.0:
--- → unaffected
Whiteboard: [sg:critical?][ccbr][critsmash:patch][stale] → [sg:critical?][ccbr][critsmash:patch][stale][1.9.x branches]
Comment 13•14 years ago
|
||
Sayrer to follow up here per critkill meeting.
Reporter | ||
Comment 14•14 years ago
|
||
Igor, what do you think what we should do here? I don't think its worth fixing this "the right way" by ensuring caller-roots. My patch should be simple enough to backport. Brendan, any objections?
Reporter | ||
Updated•14 years ago
|
Attachment #448307 -
Flags: feedback?(igor)
Assignee | ||
Comment 15•14 years ago
|
||
The bad pattern to watch is cases like any_of_lookupProperty_variants(..., &pobj, &prop) code_that_potentialy_can_run_gc pobj->dropProperty(); After some greping I have found a potential issues in JS_SetWatchPoint and obj_toSource. But if the hazards would be confirmed, then at those places we also have an issue where a GC may run with obj locked, so the hazards may exists even on the trunk we have not yet eliminated support for thread-shared objects. So I think the suggested approach would be to look at all dropProperty calls (there about 100 of them on 1.9.2 branch but the vast mayoralty of them are trivially safe), locate the above pattern and try to reorganize the code to avoid the potential GC calls fixing the bugs.
Comment 16•14 years ago
|
||
The GC doesn't wait for title locks, it knows it is serialized with all requests. I do agree with coment 15 last paragraph, but who will do the work? The attached patch is for jsproxy.cpp, new in fx4 -- nothing to backport. What am I missing? Anyway, callee-roots is generally unsound. /be
Assignee | ||
Comment 17•14 years ago
|
||
Although the callee roots do not work in general, to solve the GC hazards here it is sufficient that liveconnect would copy the pobj result into the newborn root. As regarding running the GC with the title lock taken, that GC comes from a GC thing allocator. That may also report OOM reports and it is good practice to ensure that all locks are released during those.
Assignee: gal → igor
Assignee | ||
Comment 18•14 years ago
|
||
The patch puts the result of lookupProperty in liveconnect into newborn roots. This should protect the result against bugs where the last-ditch can happen before the caller calls obj->dropProperty. I will file as separated bug the issues of running the GC or doing OOM reporting with title lock held.
Attachment #448307 -
Attachment is obsolete: true
Attachment #479783 -
Flags: review?(brendan)
Attachment #448307 -
Flags: feedback?(igor)
Comment 19•14 years ago
|
||
Comment on attachment 479783 [details] [diff] [review] newborn rooting in liveconnect Fragile, but this code will EOL soon enough. Thanks, /be
Attachment #479783 -
Flags: review?(brendan) → review+
Comment 20•14 years ago
|
||
can we land this?
Assignee | ||
Updated•14 years ago
|
Attachment #479783 -
Flags: approval1.9.2.12?
Attachment #479783 -
Flags: approval1.9.1.15?
Assignee | ||
Comment 21•14 years ago
|
||
(In reply to comment #20) > can we land this? The patch is for 1.8.* branches only. It does not exist on the trunk due to the conservative scanner.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•14 years ago
|
||
(In reply to comment #21) > The patch is for 1.8.* branches only. I meant 1.9.* branches
Comment 23•14 years ago
|
||
Comment on attachment 479783 [details] [diff] [review] newborn rooting in liveconnect a=LegNeato for 1.9.2.12 and 1.9.1.15. Please land only on the mozilla-1.9.2 default branch and mozilla-1.9.1 default branch, *not* the relbranches.
Attachment #479783 -
Flags: approval1.9.2.12?
Attachment #479783 -
Flags: approval1.9.2.12+
Attachment #479783 -
Flags: approval1.9.1.15?
Attachment #479783 -
Flags: approval1.9.1.15+
Assignee | ||
Comment 24•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/48d24bce1c77 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/166a45227d1c
Updated•14 years ago
|
Group: core-security
Updated•14 years ago
|
Attachment #479783 -
Attachment is private: false
You need to log in
before you can comment on or make changes to this bug.
Description
•