Closed
Bug 484107
Opened 15 years ago
Closed 15 years ago
XPCSafeJSObjectWrapper allows regexp variables to be clobbered
Categories
(Core :: XPConnect, defect, P2)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla1.9.1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: sicking, Assigned: mozilla+ben)
References
Details
(Keywords: verified1.9.0.15, verified1.9.1, Whiteboard: [sg:moderate] 1.9.0 fix in bug 500644)
Attachments
(3 files, 3 obsolete files)
8.04 KB,
patch
|
mrbkap
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
11.49 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
11.59 KB,
patch
|
mrbkap
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
I think that it's currently possible for a XPCSafeJSObjectWrapper-wrapped function to clobber the regexp variables of the calling context. This could lead to security issues. For example (though unlikely) the caller could eval() the result of a previous regexp search.
Flags: wanted1.9.1+
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Comment 1•15 years ago
|
||
Reassigning to bnewman. Brendan wants to completely get rid of regexp statics (as they're not in ECMA), but we won't have time to do that and dela with the outcome any time real soon, so until then we should add something like JS_SaveRegexpStatics()/JS_RestoreRegexpStatics() that we can use in our wrapper code when calling from chrome into content etc.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Assignee | ||
Comment 3•15 years ago
|
||
Straightforward enough, but I'd really like to create a testcase, too.
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #370306 -
Attachment is obsolete: true
Attachment #370337 -
Flags: superreview?
Attachment #370337 -
Flags: review?(mrbkap)
Assignee | ||
Updated•15 years ago
|
Attachment #370337 -
Flags: superreview?
Assignee | ||
Comment 5•15 years ago
|
||
Tried it, liked it: https://build.mozilla.org/tryserver-builds/2009-03-31_16:19-bnewman@mozilla.com-try-a5e24bd03f7/
Comment 6•15 years ago
|
||
Comment on attachment 370337 [details] [diff] [review] complete patch with mochitests >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp >+JS_SaveRegExpStatics(JSContext *cx, JSRegExpStatics *statics) >+{ >+ JS_ASSERT(statics); Don't bother asserting here. The JS engine doesn't assert non-NULL unless the actual point of failure is far away. If you pass NULL into this function, you'll crash immediately. >+ CHECK_REQUEST(cx); >+ *statics = cx->regExpStatics; >+ JS_ClearRegExpStatics(cx); We need to come up with some sort of rooting scheme for these statics. One solution would be to add a new type of JSTempValueRooter (in jscntxt.h). That's not entirely sufficiently, actually, since then there's a hole in the API that you can only save and restore RegExp statics under a certain stack frame... Maybe these things should know how to root themselves? brendan might also have ideas. >+JS_PUBLIC_API(void) >+JS_RestoreRegExpStatics(JSContext *cx, const JSRegExpStatics *statics) >+{ >+ JS_ASSERT(statics); Ditto on this assertion. >diff --git a/js/src/jsapi.h b/js/src/jsapi.h >+struct JSRegExpStatics; Instead of adding this here, move the typedef from jsprvtd.h to jspubtd.h... >+extern JS_PUBLIC_API(void) >+JS_SaveRegExpStatics(JSContext *cx, struct JSRegExpStatics *statics); Then you can name the type name without the 'struct' clarification. >+extern JS_PUBLIC_API(void) >+JS_RestoreRegExpStatics(JSContext *cx, const struct JSRegExpStatics *statics); Here too. We should file followup bugs on using this new API in workers (I seem to recall that they lose regexp statics in some cases) and the DOM, which loses them when we navigate back and forth.
Attachment #370337 -
Flags: review?(mrbkap) → review-
Comment 7•15 years ago
|
||
(In reply to comment #6) > We should file followup bugs on using this new API in workers (I seem to recall > that they lose regexp statics in some cases) and the DOM, which loses them when > we navigate back and forth. Well, brendan was arguing we should remove support for regexp statics completely (down the road, too much work to do for 1.9.1), so I'm not sure it's worth adding more users of this, unless there's reasons I'm unaware of to do so...
Assignee | ||
Comment 8•15 years ago
|
||
Adding a JSTempValueRooter field to JSRegExpStatics. Maybe a waste of space for JSRegExpStatics that don't need it, but this seems cleaner than passing a JSTempValueRooter* into JS_{Save,Restore}RegExpStatics.
Attachment #370337 -
Attachment is obsolete: true
Attachment #371324 -
Flags: review?(mrbkap)
Comment 9•15 years ago
|
||
Don't want JSRegExpStatis in jspubtd.h -- can't the callers be FRIENDs and include jsprvtd.h and use JS_FRIEND_API additions to a non-public header (e.g., jsregexp.h)? Also, the tvr embedding requires Save be followed by Restore in a balanced fashion on the exact same cx, but this is not strictly required by all potential uses of the new APIs. Other Save/Restore APIs do not hide state in cx, they use a return value from Save and pass it to Restore (Suspend/Resume, equivalently). If balanced calling should be required, or in any event, better to make the API caller pass in the root for input. /be
Comment 10•15 years ago
|
||
Although again, with a JS_FRIEND_API, you can require balance and use a TVR all without pain. Just include those private .h files and away you go. Still seems best to avoid embedding a TVR in JSContext just for this case. Make the caller supply it. /be
Assignee | ||
Comment 11•15 years ago
|
||
(In reply to comment #9) > can't the callers be FRIENDs and > include jsprvtd.h and use JS_FRIEND_API additions to a non-public header (e.g., > jsregexp.h)? I like that idea a lot better than adding new functions to jsapi.h.
Attachment #371324 -
Attachment is obsolete: true
Attachment #371507 -
Flags: superreview?(brendan)
Attachment #371507 -
Flags: review?(mrbkap)
Attachment #371324 -
Flags: review?(mrbkap)
Updated•15 years ago
|
Attachment #371507 -
Flags: review?(mrbkap) → review+
Updated•15 years ago
|
Attachment #371507 -
Flags: superreview?(brendan) → superreview+
Comment 12•15 years ago
|
||
Comment on attachment 371507 [details] [diff] [review] keeping the API private, friendly Looks great. Only thought, for mrbkap and the long term: should we be using a different cx? That would dodge this bug and better isolate the callee's activation. /be
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/de94b14328d6
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•15 years ago
|
||
Pushed to tryserver: http://hg.mozilla.org/try/rev/9eaeb47eb978 Applied cleanly with the exception of mochitest Makefile.in bit-rot.
Attachment #379020 -
Flags: superreview?(jst)
Attachment #379020 -
Flags: review?(jst)
Updated•15 years ago
|
Attachment #379020 -
Flags: superreview?(jst)
Attachment #379020 -
Flags: superreview+
Attachment #379020 -
Flags: review?(jst)
Attachment #379020 -
Flags: review+
Comment 15•15 years ago
|
||
Fixed on 1.9.1. Had to un-bitrot the test Makefile again since it conflicted with the changes in bug 478438. http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c7adc8fdcb4
Keywords: fixed1.9.1
Comment 16•15 years ago
|
||
Looks like this caused a 1.39% regression in Ts on XP. m.d.tree-management post: http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/e42908b7a8449d70 However, it also appears to have caused a 12.58% Txul decrease: http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/11561903767e909f
Comment 17•15 years ago
|
||
Hmm, odd. I find that very hard to believe (either the slowdown or the speedup). This has been on the trunk for a while, and as best as I know there was no performance changes when it landed there. Ben, can you spend some time looking over the graphs for trunk here around the time this landed there?
Comment 18•15 years ago
|
||
I assume we need this on the 1.9.0 branch as well? But not 1.8 which didn't have XPCSafeJSObjectWrapper.
Flags: wanted1.9.0.x?
Flags: wanted1.8.1.x-
Whiteboard: [sg:moderate?]
Assignee | ||
Comment 19•15 years ago
|
||
(In reply to comment #18) > I assume we need this on the 1.9.0 branch as well? But not 1.8 which didn't > have XPCSafeJSObjectWrapper. Gross for all the usual reasons. Apparently XPC_SJOW_Create didn't exist in 1.9.0, so that test had to be disabled.
Attachment #380969 -
Flags: superreview?(jst)
Attachment #380969 -
Flags: review?(mrbkap)
Updated•15 years ago
|
Attachment #380969 -
Flags: review?(mrbkap) → review+
Updated•15 years ago
|
Attachment #380969 -
Flags: superreview?(jst) → superreview+
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Assignee | ||
Comment 20•15 years ago
|
||
(In reply to comment #16) > Looks like this caused a 1.39% regression in Ts on XP. m.d.tree-management > post: > > http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/e42908b7a8449d70 That seems like noise to me. If someone was hounding you about it, Daniel, I'm sorry (also confused). > However, it also appears to have caused a 12.58% Txul decrease: > http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/11561903767e909f Who knows. Probably not my doing :) (In reply to comment #17) > Ben, can you spend some time looking over the graphs for trunk here around the > time this landed there? Nothing interesting (see revision de94b14328d6): http://graphs.mozilla.org/#tests=[{%22test%22:%2216%22,%22branch%22:%221%22,%22machine%22:%2271%22}]&sel=1239741326,1239800677
Depends on: CVE-2009-3372
Comment 21•15 years ago
|
||
Mass change: adding fixed1.9.2 keyword (This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)
Keywords: fixed1.9.2
Assignee | ||
Comment 22•15 years ago
|
||
(In reply to comment #19) > Created an attachment (id=380969) [details] > backport to 1.9.0 Apparently this never landed, yikes. On the positive side, that means 1.9.0 was never subject to the exploitable crash that I sneaked in with this patch (bug 500644). This patch should land along with my patch for bug 500644, which fixes the exploit.
Updated•15 years ago
|
status1.9.2:
--- → beta1-fixed
Keywords: fixed1.9.2
Updated•15 years ago
|
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Whiteboard: [sg:moderate?] → [sg:moderate] 1.9.0 fix in bug 500644
Assignee | ||
Updated•15 years ago
|
Keywords: fixed1.9.0.15
Comment 23•15 years ago
|
||
Verified fixed for 1.9.1.4 and 1.9.0.15 based on checked in test passing in logs. I don't see another way to verify this.
Updated•15 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•