XPCSafeJSObjectWrapper allows regexp variables to be clobbered

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
XPConnect
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sicking, Assigned: bnewman)

Tracking

({verified1.9.0.15, verified1.9.1})

Trunk
mozilla1.9.1
verified1.9.0.15, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.2 +
blocking1.9.1 +
wanted1.9.1 +
wanted1.9.0.x +
wanted1.8.1.x -

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: [sg:moderate] 1.9.0 fix in bug 500644)

Attachments

(3 attachments, 3 obsolete attachments)

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
(Assignee)

Comment 3

9 years ago
Created attachment 370306 [details] [diff] [review]
untested WIP implementation

Straightforward enough, but I'd really like to create a testcase, too.
(Assignee)

Comment 4

9 years ago
Created attachment 370337 [details] [diff] [review]
complete patch with mochitests
Attachment #370306 - Attachment is obsolete: true
Attachment #370337 - Flags: superreview?
Attachment #370337 - Flags: review?(mrbkap)
(Assignee)

Updated

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

Comment 8

9 years ago
Created attachment 371324 [details] [diff] [review]
rooting

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
(Assignee)

Comment 11

9 years ago
Created attachment 371507 [details] [diff] [review]
keeping the API private, friendly

(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

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

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/de94b14328d6
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

9 years ago
Created attachment 379020 [details] [diff] [review]
backport to 1.9.1

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

9 years ago
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
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
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?]
(Assignee)

Comment 19

9 years ago
Created attachment 380969 [details] [diff] [review]
backport to 1.9.0

(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

9 years ago
Attachment #380969 - Flags: review?(mrbkap) → review+

Updated

9 years ago
Attachment #380969 - Flags: superreview?(jst) → superreview+

Updated

9 years ago
Flags: blocking1.9.2? → blocking1.9.2+
(Assignee)

Comment 20

9 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

Updated

9 years ago
Depends on: 500644
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

9 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.
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Whiteboard: [sg:moderate?] → [sg:moderate] 1.9.0 fix in bug 500644
(Assignee)

Updated

9 years ago
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.
Keywords: fixed1.9.0.15, fixed1.9.1 → verified1.9.0.15, verified1.9.1
Group: core-security
You need to log in before you can comment on or make changes to this bug.