809652 - (CVE-2013-0773) Have SecurityWrapper deny access for various sketchy SM extensions

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

It looks like there was originally an intention to do this for things like nativeCall and objectClassIs, but it was thwarted by document.domain. We can do it now, though, once bug 800915 lands, and it'll be good defense-in-depth.

Comment 1

[Luke Wagner [:luke]]

\o/

Comment 2

[Bobby Holley (:bholley)]

I'm going to mark this as s-s given its relation to bug 813901 comment 2, and given that the testcase here might give people ideas.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Jorendorff, can you think of other ways to test this, especially the nativeCall stuff?

Comment 4

[Bobby Holley (:bholley)]

Attachment: Deny access to certain SM extensions for SecurityWrapper. v1

Jorendorff, are there other SM extension we should also forbid?

Comment 5

[Bobby Holley (:bholley)]

We're probably going to delay checking in this testcase one way or another, so I'm going to mark this as in-testsuite? to avoid forgetting.

We're probably going to want this on esr17, given that munging the prototypes on COWs is probably not good. This probably means backporting the first one or two patches from bug 800915. This shouldn't be so bad, though.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 684292 [details] [diff] [review]
Tests. v1

Review of attachment 684292 [details] [diff] [review]:
-----------------------------------------------------------------

You can also test this with any other builtin non-generic function. That is, anything that calls CallNonGenericMethod. js::regexp_exec calls it, so:

  var s = Cu.Sandbox("http://www.example.com/");
  s.re = /f/;
  Cu.evalInSandbox("RegExp.prototype.exec.call(re, 'abcdefg').index", s);

Run this as chrome code and it should throw; instead it runs and returns the correct answer (5).

Methods of Map and Date are also non-generic.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 684293 [details] [diff] [review]
Deny access to certain SM extensions for SecurityWrapper. v1

Review of attachment 684293 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure what else to forbid. All the extensions are weird. I don't understand why Function.prototype.toString is not implemented in terms of nativeCall, for example.

::: js/src/jswrapper.cpp
@@ +795,5 @@
>  bool
>  SecurityWrapper<Base>::nativeCall(JSContext *cx, IsAcceptableThis test, NativeImpl impl,
>                                    CallArgs args)
>  {
> +    JS_ReportError(cx, "Permission denied to unwrap object");

In js/src, we ordinarily use js.msg for strings, but r=me either way.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Deny access to certain SM extensions for SecurityWrapper. v2 r=jorendorff

Comment 9

[Bobby Holley (:bholley)]

Attachment: Tests. v3 r=jorendorff

Comment 10

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a2c372e782f7

Comment 11

[Bobby Holley (:bholley)]

Arg, so this went all orange over SOWs. Looks like they need to be able to use nativeCall for system callers, which suggests that a dynamic check via enter() is what we need here. Working up some new patches.

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=c469b68a62d0

Comment 13

[Bobby Holley (:bholley)]

Sigh, so this is a much bigger problem than I thought it was. Luke and I spent some time talking about this yesterday in MV, and I'll try to summarize here.

Theoretically, nativeCall only comes into effect when people try to call/apply a native-backed JSFunction with a |this| argument in another compartment. If the native operates on the object generically, this works fine. But if it does anything non-generic (like check the JSClass of the object), it won't work. So nativeCall is a mechanism designed to make this work.

However, when we did that trick to remap the prototypes for COWs into the home compartment, we thrust nativeCall much more into the spotlight. Now, _any_ nongeneric standard method that we invoke on a COW will go over nativeCall. So we're stuck between a rock and a hard place in terms of making this stuff transparent and making it secure.

Luke and I discussed this, and he thinks that COWs are generally a bad mechanism to expose real APIs to content. I tend to agree, though what should replace them is sort of up for debate. Regardless, while that's being hashed out, we still need to hand the security issues at play here.

It seems like the best we can do is to whitelist the natives we want to allow, and deny everything else. This is a bit tedious, but not so bad. Especially if we're no longer in the business of trying to make COWs seem 100% like real objects, and instead are just trying to make them secure without breaking anything that people have already started to use them for.

I'm going to make a list of all the instances of CallNonGenericMethod, and annotate which ones I think we should whitelist.

Comment 14

[Bobby Holley (:bholley)]

Quoted text is copy-pasted from an MXR search for CallNonGenericMethod.

> /content/xbl/src/nsXBLBinding.cpp (View Hg log or Hg annotations)
>
>     line 246 -- return JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldGetterImpl>
>     line 280 -- return JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldSetterImpl>

Don't need these.

> /js/src/builtin/MapObject.cpp (View Hg log or Hg annotations)
> 
>     line 918 -- return CallNonGenericMethod(cx, is, next_impl, args);
>     line 1122 -- return CallNonGenericMethod<MapObject::is, MapObject::size_impl>(cx, args);
>     line 1144 -- return CallNonGenericMethod<MapObject::is, MapObject::get_impl>(cx, args);
>     line 1162 -- return CallNonGenericMethod<MapObject::is, MapObject::has_impl>(cx, args);
>     line 1185 -- return CallNonGenericMethod<MapObject::is, MapObject::set_impl>(cx, args);
>     line 1217 -- return CallNonGenericMethod<MapObject::is, MapObject::delete_impl>(cx, args);
>     line 1236 -- return CallNonGenericMethod(cx, is, iterator_impl, args);
>     line 1255 -- return CallNonGenericMethod(cx, is, clear_impl, args);
>     line 1385 -- return CallNonGenericMethod(cx, is, next_impl, args);
>     line 1521 -- return CallNonGenericMethod<SetObject::is, SetObject::size_impl>(cx, args);
>     line 1539 -- return CallNonGenericMethod<SetObject::is, SetObject::has_impl>(cx, args);
>     line 1561 -- return CallNonGenericMethod<SetObject::is, SetObject::add_impl>(cx, args);
>     line 1584 -- return CallNonGenericMethod<SetObject::is, SetObject::delete_impl>(cx, args);
>     line 1603 -- return CallNonGenericMethod(cx, is, iterator_impl, args);
>     line 1622 -- return CallNonGenericMethod(cx, is, clear_impl, args);

I doubt anyone is exposing maps to content. Let's deny unless it breaks something.

> /js/src/builtin/RegExp.cpp (View Hg log or Hg annotations)
> 
>     line 317 -- return CallNonGenericMethod<IsRegExp, regexp_compile_impl>(cx, args);
>     line 361 -- return CallNonGenericMethod<IsRegExp, regexp_toString_impl>(cx, args);
>     line 570 -- /* Step 1 (b) was performed by CallNonGenericMethod. */
>     line 638 -- /* Step 1 (a) was performed by CallNonGenericMethod. */
>     line 660 -- return CallNonGenericMethod(cx, IsRegExp, regexp_exec_impl, args);
>     line 678 -- return CallNonGenericMethod(cx, IsRegExp, regexp_test_impl, args);

same here.

> /js/src/builtin/ParallelArray.cpp (View Hg log or Hg annotations)
> 
>     line 366 -- return CallNonGenericMethod(cx, ParallelArrayObject::is, impl, args);

Rivertrail stuff. Waldo says it probably isn't relevant here.


> /js/src/vm/GlobalObject.cpp (View Hg log or Hg annotations)
> 
>     line 90 -- return CallNonGenericMethod(cx, TestProtoGetterThis, ProtoGetterImpl, args);
>     line 174 -- return CallNonGenericMethod(cx, TestProtoSetterThis, ProtoSetterImpl, args);

Definitely don't want those. That's what prompted this bug.

> /js/src/jsarray.cpp (View Hg log or Hg annotations)
> 
>     line 1448 -- return CallNonGenericMethod<IsArray, array_toSource_impl>(cx, args);

This ends up toSource-ing the underlying objects. Since nativeCall makes this all happen in the content compartment, this will all happen without regard for __exposedProps__ and other access protections. Yikes. Let's kill it.

> /js/src/jsiter.cpp (View Hg log or Hg annotations)
> 
>     line 806 -- return CallNonGenericMethod<IsIterator, iterator_next_impl>(cx, args);
>     line 896 -- return CallNonGenericMethod(cx, IsElementIterator, next_impl, args);
>     line 1669 -- return CallNonGenericMethod<IsGenerator, generator_send_impl>(cx, args);
>     line 1696 -- return CallNonGenericMethod<IsGenerator, generator_next_impl>(cx, args);
>     line 1727 -- return CallNonGenericMethod<IsGenerator, generator_throw_impl>(cx, args);
>     line 1761 -- return CallNonGenericMethod<IsGenerator, generator_close_impl>(cx, args);

I'm not totally sure here, but my sense is that we shouldn't allow custom iterators / generators to work, because we don't know how to censor them.

> /js/src/jsbool.cpp (View Hg log or Hg annotations)
> 
>     line 80 -- return CallNonGenericMethod<IsBoolean, bool_toSource_impl>(cx, args);
>     line 99 -- return CallNonGenericMethod<IsBoolean, bool_toString_impl>(cx, args);
>     line 117 -- return CallNonGenericMethod<IsBoolean, bool_valueOf_impl>(cx, args);

No harm in allowing these, I'd think.

> /js/src/jsdate.cpp (View Hg log or Hg annotations)
> 
>     line 1409 -- return CallNonGenericMethod<IsDate, date_getTime_impl>(cx, args);
>     line 1436 -- return CallNonGenericMethod<IsDate, date_getYear_impl>(cx, args);
>     line 1455 -- return CallNonGenericMethod<IsDate, date_getFullYear_impl>(cx, args);
>     line 1475 -- return CallNonGenericMethod<IsDate, date_getUTCFullYear_impl>(cx, args);
>     line 1494 -- return CallNonGenericMethod<IsDate, date_getMonth_impl>(cx, args);
>     line 1511 -- return CallNonGenericMethod<IsDate, date_getUTCMonth_impl>(cx, args);
>     line 1530 -- return CallNonGenericMethod<IsDate, date_getDate_impl>(cx, args);
>     line 1550 -- return CallNonGenericMethod<IsDate, date_getUTCDate_impl>(cx, args);
>     line 1569 -- return CallNonGenericMethod<IsDate, date_getDay_impl>(cx, args);
>     line 1589 -- return CallNonGenericMethod<IsDate, date_getUTCDay_impl>(cx, args);
>     line 1608 -- return CallNonGenericMethod<IsDate, date_getHours_impl>(cx, args);
>     line 1628 -- return CallNonGenericMethod<IsDate, date_getUTCHours_impl>(cx, args);
>     line 1647 -- return CallNonGenericMethod<IsDate, date_getMinutes_impl>(cx, args);
>     line 1667 -- return CallNonGenericMethod<IsDate, date_getUTCMinutes_impl>(cx, args);
>     line 1688 -- return CallNonGenericMethod<IsDate, date_getUTCSeconds_impl>(cx, args);
>     line 1710 -- return CallNonGenericMethod<IsDate, date_getUTCMilliseconds_impl>(cx, args);
>     line 1736 -- return CallNonGenericMethod<IsDate, date_getTimezoneOffset_impl>(cx, args);
>     line 1762 -- return CallNonGenericMethod<IsDate, date_setTime_impl>(cx, args);
>     line 1824 -- return CallNonGenericMethod<IsDate, date_setMilliseconds_impl>(cx, args);
>     line 1856 -- return CallNonGenericMethod<IsDate, date_setUTCMilliseconds_impl>(cx, args);
>     line 1896 -- return CallNonGenericMethod<IsDate, date_setSeconds_impl>(cx, args);
>     line 1935 -- return CallNonGenericMethod<IsDate, date_setUTCSeconds_impl>(cx, args);
>     line 1979 -- return CallNonGenericMethod<IsDate, date_setMinutes_impl>(cx, args);
>     line 2023 -- return CallNonGenericMethod<IsDate, date_setUTCMinutes_impl>(cx, args);
>     line 2072 -- return CallNonGenericMethod<IsDate, date_setHours_impl>(cx, args);
>     line 2121 -- return CallNonGenericMethod<IsDate, date_setUTCHours_impl>(cx, args);
>     line 2155 -- return CallNonGenericMethod<IsDate, date_setDate_impl>(cx, args);
>     line 2188 -- return CallNonGenericMethod<IsDate, date_setUTCDate_impl>(cx, args);
>     line 2247 -- return CallNonGenericMethod<IsDate, date_setMonth_impl>(cx, args);
>     line 2286 -- return CallNonGenericMethod<IsDate, date_setUTCMonth_impl>(cx, args);
>     line 2346 -- return CallNonGenericMethod<IsDate, date_setFullYear_impl>(cx, args);
>     line 2390 -- return CallNonGenericMethod<IsDate, date_setUTCFullYear_impl>(cx, args);
>     line 2435 -- return CallNonGenericMethod<IsDate, date_setYear_impl>(cx, args);
>     line 2506 -- return CallNonGenericMethod<IsDate, date_toGMTString_impl>(cx, args);
>     line 2535 -- return CallNonGenericMethod<IsDate, date_toISOString_impl>(cx, args);
>     line 2804 -- return CallNonGenericMethod<IsDate, date_toLocaleString_impl>(cx, args);
>     line 2833 -- return CallNonGenericMethod<IsDate, date_toLocaleDateString_impl>(cx, args);
>     line 2850 -- return CallNonGenericMethod<IsDate, date_toLocaleTimeString_impl>(cx, args);
>     line 2879 -- return CallNonGenericMethod<IsDate, date_toLocaleFormat_impl>(cx, args);
>     line 2896 -- return CallNonGenericMethod<IsDate, date_toTimeString_impl>(cx, args);
>     line 2913 -- return CallNonGenericMethod<IsDate, date_toDateString_impl>(cx, args);
>     line 2941 -- return CallNonGenericMethod<IsDate, date_toSource_impl>(cx, args);
>     line 2957 -- return CallNonGenericMethod<IsDate, date_toString_impl>(cx, args);
>     line 2975 -- return CallNonGenericMethod<IsDate, date_valueOf_impl>(cx, args);

This is kind of a tricky one. If we allow anything, I think we need to allow date_valueOf_impl, which means that all the information from the getters is also exposed. We definitely shouldn't expose the setters though. So we should either whitelist only the readonly methods, or blacklist everything. The latter approach involves some small fixing in the B2G contacts API where date is valueOf-ed via a COW. Whitelisting the getterse is probably safest here.


> /js/src/jstypedarray.cpp (View Hg log or Hg annotations)
> 
>     line 129 -- return CallNonGenericMethod<IsArrayBuffer, byteLengthGetterImpl>(cx, args);
>     line 167 -- return CallNonGenericMethod<IsArrayBuffer, fun_slice_impl>(cx, args);
>     line 460 -- return CallNonGenericMethod<IsArrayBuffer, createDataViewForThisImpl>(cx, args);
>     line 1681 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass,
>     line 1758 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_subarray_impl>(cx, args);
>     line 1821 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_move_impl>(cx, args);
>     line 1886 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_set_impl>(cx, args);
>     line 2398 -- return CallNonGenericMethod<IsArrayBuffer, createTypedArrayFromBufferImpl<T> >(cx, args);
>     line 2744 -- return CallNonGenericMethod<is, getInt8Impl>(cx, args);
>     line 2765 -- return CallNonGenericMethod<is, getUint8Impl>(cx, args);
>     line 2786 -- return CallNonGenericMethod<is, getInt16Impl>(cx, args);
>     line 2807 -- return CallNonGenericMethod<is, getUint16Impl>(cx, args);
>     line 2828 -- return CallNonGenericMethod<is, getInt32Impl>(cx, args);
>     line 2849 -- return CallNonGenericMethod<is, getUint32Impl>(cx, args);
>     line 2871 -- return CallNonGenericMethod<is, getFloat32Impl>(cx, args);
>     line 2893 -- return CallNonGenericMethod<is, getFloat64Impl>(cx, args);
>     line 2913 -- return CallNonGenericMethod<is, setInt8Impl>(cx, args);
>     line 2933 -- return CallNonGenericMethod<is, setUint8Impl>(cx, args);
>     line 2953 -- return CallNonGenericMethod<is, setInt16Impl>(cx, args);
>     line 2973 -- return CallNonGenericMethod<is, setUint16Impl>(cx, args);
>     line 2993 -- return CallNonGenericMethod<is, setInt32Impl>(cx, args);
>     line 3013 -- return CallNonGenericMethod<is, setUint32Impl>(cx, args);
>     line 3033 -- return CallNonGenericMethod<is, setFloat32Impl>(cx, args);
>     line 3053 -- return CallNonGenericMethod<is, setFloat64Impl>(cx, args);
>     line 3503 -- return CallNonGenericMethod<is, getterImpl<ValueGetter> >(cx, args);

We allow indexed access to typed arrays, so all of the "array-ish" data is rw accessible automatically. I'm still a bit fuzzy on what we should allow here, so I'm going to CC sfink. Definitely the length getter though.
 
> /js/src/jsstr.cpp (View Hg log or Hg annotations)
> 
>     line 518 -- return CallNonGenericMethod<IsString, str_toSource_impl>(cx, args);
>     line 538 -- return CallNonGenericMethod<IsString, str_toString_impl>(cx, args);

These should be fine.

> /js/src/jsweakmap.cpp (View Hg log or Hg annotations)
> 
>     line 171 -- return CallNonGenericMethod<IsWeakMap, WeakMap_has_impl>(cx, args);
>     line 203 -- return CallNonGenericMethod<IsWeakMap, WeakMap_get_impl>(cx, args);
>     line 236 -- return CallNonGenericMethod<IsWeakMap, WeakMap_delete_impl>(cx, args);
>     line 292 -- return CallNonGenericMethod<IsWeakMap, WeakMap_set_impl>(cx, args);

I don't see a good reason to expose weakmaps across boundaries. Let's deny.

> /js/src/jsnum.cpp (View Hg log or Hg annotations)
> 
>     line 503 -- return CallNonGenericMethod<IsNumber, num_toSource_impl>(cx, args);
>     line 626 -- return CallNonGenericMethod<IsNumber, num_toString_impl>(cx, args);
>     line 759 -- return CallNonGenericMethod<IsNumber, num_toLocaleString_impl>(cx, args);
>     line 774 -- return CallNonGenericMethod<IsNumber, num_valueOf_impl>(cx, args);
>     line 837 -- return CallNonGenericMethod<IsNumber, num_toFixed_impl>(cx, args);
>     line 863 -- return CallNonGenericMethod<IsNumber, num_toExponential_impl>(cx, args);
>     line 901 -- return CallNonGenericMethod<IsNumber, num_toPrecision_impl>

These should be fine.

Comment 15

[Steve Fink [:sfink] [:s:]]

(In reply to Bobby Holley (:bholley) from comment #14)
> > /js/src/jstypedarray.cpp (View Hg log or Hg annotations)
> > 
> >     line 129 -- return CallNonGenericMethod<IsArrayBuffer, byteLengthGetterImpl>(cx, args);
> >     line 167 -- return CallNonGenericMethod<IsArrayBuffer, fun_slice_impl>(cx, args);
> >     line 460 -- return CallNonGenericMethod<IsArrayBuffer, createDataViewForThisImpl>(cx, args);
> >     line 1681 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass,
> >     line 1758 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_subarray_impl>(cx, args);
> >     line 1821 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_move_impl>(cx, args);
> >     line 1886 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_set_impl>(cx, args);
> >     line 2398 -- return CallNonGenericMethod<IsArrayBuffer, createTypedArrayFromBufferImpl<T> >(cx, args);
> >     line 2744 -- return CallNonGenericMethod<is, getInt8Impl>(cx, args);
> >     line 2765 -- return CallNonGenericMethod<is, getUint8Impl>(cx, args);
> >     line 2786 -- return CallNonGenericMethod<is, getInt16Impl>(cx, args);
> >     line 2807 -- return CallNonGenericMethod<is, getUint16Impl>(cx, args);
> >     line 2828 -- return CallNonGenericMethod<is, getInt32Impl>(cx, args);
> >     line 2849 -- return CallNonGenericMethod<is, getUint32Impl>(cx, args);
> >     line 2871 -- return CallNonGenericMethod<is, getFloat32Impl>(cx, args);
> >     line 2893 -- return CallNonGenericMethod<is, getFloat64Impl>(cx, args);
> >     line 2913 -- return CallNonGenericMethod<is, setInt8Impl>(cx, args);
> >     line 2933 -- return CallNonGenericMethod<is, setUint8Impl>(cx, args);
> >     line 2953 -- return CallNonGenericMethod<is, setInt16Impl>(cx, args);
> >     line 2973 -- return CallNonGenericMethod<is, setUint16Impl>(cx, args);
> >     line 2993 -- return CallNonGenericMethod<is, setInt32Impl>(cx, args);
> >     line 3013 -- return CallNonGenericMethod<is, setUint32Impl>(cx, args);
> >     line 3033 -- return CallNonGenericMethod<is, setFloat32Impl>(cx, args);
> >     line 3053 -- return CallNonGenericMethod<is, setFloat64Impl>(cx, args);
> >     line 3503 -- return CallNonGenericMethod<is, getterImpl<ValueGetter> >(cx, args);
> 
> We allow indexed access to typed arrays, so all of the "array-ish" data is
> rw accessible automatically. I'm still a bit fuzzy on what we should allow
> here, so I'm going to CC sfink. Definitely the length getter though.

I looked through these, and I think I'd allow all of them. .byteLength is similar to .length, slice and subarray are just accessing the contents. Sharing the underlying buffer with other views probably can't be avoided, because you need it for casting and structure packing/unpacking.

Comment 16

[Steve Fink [:sfink] [:s:]]

Actually, it seems like you could at least make them read-only by disallowing

> > >     line 1821 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_move_impl>(cx, args);
> > >     line 1886 -- return CallNonGenericMethod<ThisTypeArray::IsThisClass, ThisTypeArray::fun_set_impl>(cx, args);
> > >     line 2913 -- return CallNonGenericMethod<is, setInt8Impl>(cx, args);
> > >     line 2933 -- return CallNonGenericMethod<is, setUint8Impl>(cx, args);
> > >     line 2953 -- return CallNonGenericMethod<is, setInt16Impl>(cx, args);
> > >     line 2973 -- return CallNonGenericMethod<is, setUint16Impl>(cx, args);
> > >     line 2993 -- return CallNonGenericMethod<is, setInt32Impl>(cx, args);
> > >     line 3013 -- return CallNonGenericMethod<is, setUint32Impl>(cx, args);
> > >     line 3033 -- return CallNonGenericMethod<is, setFloat32Impl>(cx, args);
> > >     line 3053 -- return CallNonGenericMethod<is, setFloat64Impl>(cx, args);

Being able to write doesn't seem that critical.

Comment 17

[Bobby Holley (:bholley)]

sicking says that B2G assumes it can expose typedarrays for read-write access, and doesn't do anything with ArrayBuffer and DataView. So I'm going to allow access for all the TypedArray stuff, and none of the other stuff.

Comment 18

[Bobby Holley (:bholley)]

Alright, so there are two big security issues that this fixes:

1 - Array.toSource can be used to bypass __exposedProps__ restrictions and view the contents of a chrome array without any filtering. Theoretically this is kind of bad, but in practice I doubt anyone's really relying on it.

2 - COW and SOW prototypes are mutable by content. This is a pretty big deal. Even though we fixed the __exposedProps__ inheritance issue in bug 813901, there's still a host of other problems with letting content munge the prototypes of chrome objects.

I'm going to go with sec-high here.

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 689964 [details] [diff] [review]
Deny access to certain SM extensions for SecurityWrapper. v2 r=jorendorff

Taking another stab at this.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 1 - Have SecurityWrapper::enter default to deny, and override SecurityWrapper::objectClassIs. v3

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add gross one-off predicates to jsfriendapi. v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 3 - Deny nativeCall for SecurityWrapper except under specific circumstances. v1

Comment 23

[Bobby Holley (:bholley)]

Attachment: Tests. v5

I beefed these up significantly since the last iteration.

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 691168 [details] [diff] [review]
Part 2 - Add gross one-off predicates to jsfriendapi. v1

Review of attachment 691168 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsdate.cpp
@@ +3274,5 @@
> +};
> +
> +namespace js {
> +JS_FRIEND_API(bool)
> +IsReadOnlyDateMethod(IsAcceptableThis test, NativeImpl method)

Style nit: instead of wrapping in 'namespace js {}', do this:

> JS_FRIEND_API(bool)
> js::IsReadOnlyDateMethod(IsAcceptableThis test, NativeImpl method)

::: js/src/jstypedarray.cpp
@@ +3394,5 @@
> +namespace js {
> +
> +#define CHECK(t, a) { if (t == a::IsThisClass) return true; }
> +JS_FRIEND_API(bool)
> +IsTypedArrayThisCheck(JS::IsAcceptableThis test)

Same nit here.

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 691169 [details] [diff] [review]
Part 3 - Deny nativeCall for SecurityWrapper except under specific circumstances. v1

Review of attachment 691169 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jswrapper.h
@@ +188,5 @@
>      virtual bool objectClassIs(JSObject *obj, ESClassValue classValue, JSContext *cx) MOZ_OVERRIDE;
>      virtual bool regexp_toShared(JSContext *cx, JSObject *proxy, RegExpGuard *g) MOZ_OVERRIDE;
> +
> +    /*
> +     * Allow our subclasses to select the supeclass behavior they want without

"superclass"

Comment 26

[Jason Orendorff [:jorendorff]]

Comment on attachment 691171 [details] [diff] [review]
Tests. v5

Review of attachment 691171 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/tests/unit/test_bug809652.js
@@ +53,5 @@
> +    do_check_eq(Cu.evalInSandbox("(new this[curr.constructor.name]).__lookupGetter__('buffer').call(curr)", sb), sb.ab);
> +    do_check_eq(Cu.evalInSandbox("(new this[curr.constructor.name]).__lookupGetter__('byteOffset').call(curr)", sb), t.byteOffset);
> +    do_check_eq(Cu.evalInSandbox("(new this[curr.constructor.name]).__lookupGetter__('byteLength').call(curr)", sb), t.byteLength);
> +  }
> +  sb.ta.forEach(testForTypedArray);

It could just be a for-of loop instead of a closure, if you want.

for (let t of sb.ta) {
  sb.curr = t;
  ...
}

Comment 27

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1d2dcd6790cc

Comment 28

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eae731552b88
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/48300e0e0da8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3dd1c70b310

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/eae731552b88
https://hg.mozilla.org/mozilla-central/rev/48300e0e0da8
https://hg.mozilla.org/mozilla-central/rev/c3dd1c70b310

Comment 30

[Alex Keybl [:akeybl]]

Bobby/Jason - how difficult or risky do you think this would be to backport to Beta19/ESR17/B2G18? Is this even needed for B2G18?

Comment 31

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

Is there a reason why sec-approval was not asked for before checkin?

Comment 32

[Bobby Holley (:bholley)]

(In reply to Alex Keybl [:akeybl] from comment #30)
> Bobby/Jason - how difficult or risky do you think this would be to backport
> to Beta19/ESR17/B2G18?

Per comment 5, I think we should backport this, even though it's some work.

> Is this even needed for B2G18?

It affects B2G18 probably more than Desktop in the sense that there are more b2g APIs that use COWs (like contacts). The IPC hardening mitigates risk there, but it's still a big problem.

(In reply to Al Billings [:abillings] from comment #31)
> Is there a reason why sec-approval was not asked for before checkin?

I don't remember. It was probably a mistake. :-(

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for beta uplift with a risk assessment on the backport.

Comment 34

[Bobby Holley (:bholley)]

Spent some hours backporting everything. Here's a try push:

https://tbpl.mozilla.org/?tree=Try&rev=af0ef08a7cdc

Comment 35

[Bobby Holley (:bholley)]

Attachment: Part 1 - ESR17 backport. v1 r=jorendorff

Comment 36

[Bobby Holley (:bholley)]

Attachment: Part 2 - ESR17 backport. r=jorendorff

Comment 37

[Bobby Holley (:bholley)]

Attachment: Part 3 - Deny nativeCall for SecurityWrapper except under specific circumstances (on esr17). r=jorendorff

Comment 38

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Found nothing wrong here either.

Comment 39

[Bobby Holley (:bholley)]

Comment on attachment 691169 [details] [diff] [review]
Part 3 - Deny nativeCall for SecurityWrapper except under specific circumstances. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Longstanding
User impact if declined: Potential security hole
Testing completed (on m-c, etc.): Been on m-c and m-a for a while. This patch landed just after the merge, so the state of m-b now should be pretty similar to the state of the tree when this landed on m-c.

Risk to taking this patch (and alternatives if risky): Medium risk. We should get it in soon if we're going to go for it.
String or UUID changes made by this patch: None

Comment 40

[bhavana bajaj [:bajaj]]

Comment on attachment 691169 [details] [diff] [review]
Part 3 - Deny nativeCall for SecurityWrapper except under specific circumstances. v1

Approving on beta, sec-high, been on m-c m-a for a while. Looks like you accidentally set the patch as "review -".

Also, can you please get the esr/b2g patches ready for backport ?Thanks !

Comment 41

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/deb2580ab7cf
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/e30b3c95b3ba
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/148a94419660

Comment 42

[Bobby Holley (:bholley)]

(In reply to bhavana bajaj [:bajaj] from comment #40)
> Also, can you please get the esr/b2g patches ready for backport ?Thanks !

The esr patches are ready to go, pending approval. Can someone confirm whether we're taking this on b2g18? I don't want to spend the cycles writing a backport unless we're actually going to take it.

Comment 43

[Alex Keybl [:akeybl]]

(In reply to Bobby Holley (:bholley) from comment #42)
> (In reply to bhavana bajaj [:bajaj] from comment #40)
> > Also, can you please get the esr/b2g patches ready for backport ?Thanks !
> 
> The esr patches are ready to go, pending approval. Can someone confirm
> whether we're taking this on b2g18? I don't want to spend the cycles writing
> a backport unless we're actually going to take it.

Yes, we'll need this on mozilla-b2g18 - we're still discussing the risk of whether to land for v1.0.0

Comment 44

[Alex Keybl [:akeybl]]

(lucas adamski will be getting in touch bholley)

Comment 45

[Lucas Adamski [:ladamski]]

Its pretty late in the game to take defense in depth fixes for 1.0.0 esp. for a patch of this complexity, but maybe I misunderstand the bug.  Is there a specific vulnerability we are fixing here?

Comment 46

[bhavana bajaj [:bajaj]]

(In reply to Bobby Holley (:bholley) from comment #39)
> Comment on attachment 691169 [details] [diff] [review]
> Part 3 - Deny nativeCall for SecurityWrapper except under specific
> circumstances. v1
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Longstanding
> User impact if declined: Potential security hole
> Testing completed (on m-c, etc.): Been on m-c and m-a for a while. This
> patch landed just after the merge, so the state of m-b now should be pretty
> similar to the state of the tree when this landed on m-c.
> 
> Risk to taking this patch (and alternatives if risky): Medium risk. We
> should get it in soon if we're going to go for it.
> String or UUID changes made by this patch: None

:bholley, considering the risk evaluation here , anyways to mitigate it ? Or can you help point areas to lookout for potential regressions here ?

Comment 47

[Bobby Holley (:bholley)]

(In reply to Lucas Adamski from comment #45)
> Its pretty late in the game to take defense in depth fixes for 1.0.0 esp.
> for a patch of this complexity, but maybe I misunderstand the bug.  Is there
> a specific vulnerability we are fixing here?

This does fix a vulnerability, which is that content can arbitrarily change the proto of any privileged object it can get its hands on. Whether or not that's a major security issue depends on the details of all the chrome objects b2g exposes to content (of which there are many).

Blake and I just talked about this in person. He was on the fence, but eventually said that he doesn't believe this is critical to fix for b2g18.

Comment 48

[Lucas Adamski [:ladamski]]

Marking TEF- per comment 47.

Comment 49

[Bobby Holley (:bholley)]

(In reply to bhavana bajaj [:bajaj] from comment #46)
> :bholley, considering the risk evaluation here , anyways to mitigate it ? Or
> can you help point areas to lookout for potential regressions here ?

The main risk here would be addon breakage. In particular, some addon might be using COWs and expect to be able to do certain operations on those objects that are now denied. This isn't super likely though, because these operations only became available in Mozilla 17, and weren't widely publicized. The "moderate" risk rating here mostly has to do with the scope of the change, and that it might have various consequences with external code that are hard to predict.

Comment 50

[bhavana bajaj [:bajaj]]

"Wontfix" for b2g18 given comment# 45/47.

Comment 51

[Bobby Holley (:bholley)]

Comment on attachment 691169 [details] [diff] [review]
Part 3 - Deny nativeCall for SecurityWrapper except under specific circumstances. v1

Now that bug 800915 landed for esr17, requesting approval-esr17. See comment 39 for the details of the bug at hand here. The only major difference is that the backport here was trickier, but most of that complexity was captured in bug 800915.

Comment 52

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/39d86f5ae5a3
remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/6734f9d96b9c
remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/c58385dc7712

Comment 53

[Bobby Holley (:bholley)]

Pushed the tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba2cd25663b

Comment 54

[Ryan VanderMeulen [:RyanVM]]

(In reply to Bobby Holley (:bholley) from comment #53)
> Pushed the tests:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba2cd25663b

https://hg.mozilla.org/mozilla-central/rev/3ba2cd25663b