484107 - XPCSafeJSObjectWrapper allows regexp variables to be clobbered

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 1

[Johnny Stenback (:jst)]

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.

Comment 2

[Johnny Stenback (:jst)]

Actually reassigning, duh.

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

Attachment: untested WIP implementation

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

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

Attachment: complete patch with mochitests

Comment 5

[Ben Newman (:bnewman) (:benjamn)]

Tried it, liked it: https://build.mozilla.org/tryserver-builds/2009-03-31_16:[e-mail]-try-a5e24bd03f7/

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 7

[Johnny Stenback (:jst)]

(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...

Comment 8

[Ben Newman (:bnewman) (:benjamn)]

Attachment: 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.

Comment 9

[Brendan Eich [:brendan]]

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

[Brendan Eich [:brendan]]

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

Comment 11

[Ben Newman (:bnewman) (:benjamn)]

Attachment: 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.

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Ben Newman (:bnewman) (:benjamn)]

http://hg.mozilla.org/mozilla-central/rev/de94b14328d6

Comment 14

[Ben Newman (:bnewman) (:benjamn)]

Attachment: 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.

Comment 15

[Johnny Stenback (:jst)]

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

Comment 16

[Daniel Holbert [:dholbert]]

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

[Johnny Stenback (:jst)]

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

[Daniel Veditz [:dveditz]]

I assume we need this on the 1.9.0 branch as well? But not 1.8 which didn't have XPCSafeJSObjectWrapper.

Comment 19

[Ben Newman (:bnewman) (:benjamn)]

Attachment: 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.

Comment 20

[Ben Newman (:bnewman) (:benjamn)]

(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

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

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)

Comment 22

[Ben Newman (:bnewman) (:benjamn)]

(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.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

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.