Closed Bug 484107 Opened 11 years ago Closed 11 years ago

XPCSafeJSObjectWrapper allows regexp variables to be clobbered

Categories

(Core :: XPConnect, defect, P2)

defect

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)

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?
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
Actually reassigning, duh.
Assignee: nobody → bnewman
Attached patch untested WIP implementation (obsolete) — Splinter Review
Straightforward enough, but I'd really like to create a testcase, too.
Attached patch complete patch with mochitests (obsolete) — Splinter Review
Attachment #370306 - Attachment is obsolete: true
Attachment #370337 - Flags: superreview?
Attachment #370337 - Flags: review?(mrbkap)
Attachment #370337 - Flags: superreview?
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-
(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...
Attached patch rooting (obsolete) — Splinter Review
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)
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
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
(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)
Attachment #371507 - Flags: review?(mrbkap) → review+
Attachment #371507 - Flags: superreview?(brendan) → superreview+
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
http://hg.mozilla.org/mozilla-central/rev/de94b14328d6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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)
Attachment #379020 - Flags: superreview?(jst)
Attachment #379020 - Flags: superreview+
Attachment #379020 - Flags: review?(jst)
Attachment #379020 - Flags: review+
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
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?
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?]
(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)
Attachment #380969 - Flags: review?(mrbkap) → review+
Attachment #380969 - Flags: superreview?(jst) → superreview+
Flags: blocking1.9.2? → blocking1.9.2+
(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
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
(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.
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Whiteboard: [sg:moderate?] → [sg:moderate] 1.9.0 fix in bug 500644
Keywords: fixed1.9.0.15
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.