Defend WeakMap against leaking its HIDDEN_NAME to proxies.

Browser native Proxies can observe the emulated WeakMap HIDDEN_NAMEs,
which leaks the name and breaks WeakMap's invariants. Therefore to be
reliably safe we must either have all proxies in Caja compatible with
WeakMap, or avoid ever putting a proxy in an emulated WeakMap. The
latter is already the case, and this change merely helps enforce that.

Specifically, no existant platform supports Proxy but not WeakMap.
Native WeakMaps are safe, so we only have to worry about DoubleWeakMap,
which is used to deal with funky host objects (on Firefox) that cannot
be put in a native WeakMap. All WeakMaps in Caja fall into the
categories of either:
  1. containing no funky host objects, or
  2. containing no proxies.
In case 1, DoubleWeakMap can always use the native WeakMap, so it is
safe. In case 2, no proxy is involved in order to observe the hidden
name.

In this change, we loosely enforce the above division: DoubleWeakMap
will refuse to fall back to emulated weak maps unless the weak map has
been flagged to permit it using a privileged operation; the weak maps
used by the taming membrane are so flagged since they encounter said
funky host objects, and do not contain any Caja-created proxies.

Supporting changes:
* If we ever do encounter a platform with Proxy and not WeakMap, delete
  Proxy so that nothing breaks.
* Remove WeakMap magic name detector from Domado, as it is now
  unnecessary and ineffective.
* Fixed an incomplete change in r5039: getOwnPropertyNames hides hidden
  names from any frame, but getPropertyNames does not. This is only
  a problem for multiple interacting SES frames (which we no longer use
  in Caja) on platforms which have Object.getPropertyNames (which does
  not include current Chrome or Firefox), so it is not a current
  vulnerability in Caja.

Fixes <https://code.google.com/p/google-caja/issues/detail?id=1725>.

@r5453

[MarkM, 2013-06-11 01:28:27]

ses/* parts LGTM

https://codereview.appspot.com/10162044/diff/1/src/com/google/caja/ses/WeakMa...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/10162044/diff/1/src/com/google/caja/ses/WeakMa...
src/com/google/caja/ses/WeakMap.js:28: * @provides cajaWeakMapPermitHostObjects
I dont like this global namespace pollution. Why not put this on ses?

https://codereview.appspot.com/10162044/diff/1/src/com/google/caja/ses/WeakMa...
src/com/google/caja/ses/WeakMap.js:512: // those which
Incomplete sentence

[kpreid2, 2013-06-11 16:51:51]

Browser native Proxies can observe the emulated WeakMap HIDDEN_NAMEs,
which leaks the name and breaks WeakMap's invariants. Therefore to be
reliably safe we must either have all proxies in Caja compatible with
WeakMap, or avoid ever putting a proxy in an emulated WeakMap. The
latter is already the case, and this change merely helps enforce that.

Specifically, no existant platform supports Proxy but not WeakMap.
Native WeakMaps are safe, so we only have to worry about DoubleWeakMap,
which is used to deal with funky host objects (on Firefox) that cannot
be put in a native WeakMap. All WeakMaps in Caja fall into the
categories of either:
  1. containing no funky host objects, or
  2. containing no proxies.
In case 1, DoubleWeakMap can always use the native WeakMap, so it is
safe. In case 2, no proxy is involved in order to observe the hidden
name.

In this change, we loosely enforce the above division: DoubleWeakMap
will refuse to fall back to emulated weak maps unless the weak map has
been flagged to permit it using a privileged operation; the weak maps
used by the taming membrane are so flagged since they encounter said
funky host objects, and do not contain any Caja-created proxies.

Supporting changes:
* If we ever do encounter a platform with Proxy and not WeakMap, delete
  Proxy so that nothing breaks.
* Remove WeakMap magic name detector from Domado, as it is now
  unnecessary and ineffective.
* Fixed an incomplete change in r5039: getOwnPropertyNames hides hidden
  names from any frame, but getPropertyNames does not. This is only
  a problem for multiple interacting SES frames (which we no longer use
  in Caja) on platforms which have Object.getPropertyNames (which does
  not include current Chrome or Firefox), so it is not a current
  vulnerability in Caja.

Fixes <https://code.google.com/p/google-caja/issues/detail?id=1725>.

[kpreid2, 2013-06-11 16:52:05]

https://codereview.appspot.com/10162044/diff/1/src/com/google/caja/ses/WeakMa...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/10162044/diff/1/src/com/google/caja/ses/WeakMa...
src/com/google/caja/ses/WeakMap.js:28: * @provides cajaWeakMapPermitHostObjects
On 2013/06/11 01:28:27, MarkM wrote:
> I dont like this global namespace pollution. Why not put this on ses?

My impression was that WeakMap.js was designed to stand alone (its only
reference to ses is conditional). I have now put it on ses only if ses exists,
which is sufficient for our uses.

https://codereview.appspot.com/10162044/diff/1/src/com/google/caja/ses/WeakMa...
src/com/google/caja/ses/WeakMap.js:512: // those which
On 2013/06/11 01:28:27, MarkM wrote:
> Incomplete sentence

Done.

[MarkM, 2013-06-12 14:59:38]

LGTM

[kpreid2, 2013-06-12 16:34:48]

On 2013/06/12 14:59:38, MarkM wrote:
> LGTM

Do you agree that this does not need to be an undisclosed patch, specifically
because the restriction imposed is not violatable by guest code in the absence
of other bugs, and because getPropertyNames is not on any current browser?

Ihab, when you review, please also confirm.

[MarkM, 2013-06-12 17:15:53]

  Cheers
  --MarkM

On Jun 12, 2013, at 9:34 AM, kpreid.switchb.org@gmail.com wrote:

> On 2013/06/12 14:59:38, MarkM wrote:
>> LGTM
> 
> Do you agree that this does not need to be an undisclosed patch,
> specifically because the restriction imposed is not violatable by guest
> code in the absence of other bugs, and because getPropertyNames is not
> on any current browser?

Are you sure? Have you checked?

If so, yes.

> 
> Ihab, when you review, please also confirm.
> 
> https://codereview.appspot.com/10162044/

[ihab.awad, 2013-06-13 19:45:56]

lgtm++

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:827: if ((lookup = this.col_lookup(name)))
{
Can remove an extra level of parens. :) Here & elsewhere.

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/ses/Wea...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/ses/Wea...
src/com/google/caja/ses/WeakMap.js:558: throw new Error('bogus
permitHostObjects___');
More like, bogus _call to_ pHO___?

[kpreid2, 2013-06-13 20:04:47]

Browser native Proxies can observe the emulated WeakMap HIDDEN_NAMEs,
which leaks the name and breaks WeakMap's invariants. Therefore to be
reliably safe we must either have all proxies in Caja compatible with
WeakMap, or avoid ever putting a proxy in an emulated WeakMap. The
latter is already the case, and this change merely helps enforce that.

Specifically, no existant platform supports Proxy but not WeakMap.
Native WeakMaps are safe, so we only have to worry about DoubleWeakMap,
which is used to deal with funky host objects (on Firefox) that cannot
be put in a native WeakMap. All WeakMaps in Caja fall into the
categories of either:
  1. containing no funky host objects, or
  2. containing no proxies.
In case 1, DoubleWeakMap can always use the native WeakMap, so it is
safe. In case 2, no proxy is involved in order to observe the hidden
name.

In this change, we loosely enforce the above division: DoubleWeakMap
will refuse to fall back to emulated weak maps unless the weak map has
been flagged to permit it using a privileged operation; the weak maps
used by the taming membrane are so flagged since they encounter said
funky host objects, and do not contain any Caja-created proxies.

Supporting changes:
* If we ever do encounter a platform with Proxy and not WeakMap, delete
  Proxy so that nothing breaks.
* Remove WeakMap magic name detector from Domado, as it is now
  unnecessary and ineffective.
* Fixed an incomplete change in r5039: getOwnPropertyNames hides hidden
  names from any frame, but getPropertyNames does not. This is only
  a problem for multiple interacting SES frames (which we no longer use
  in Caja) on platforms which have Object.getPropertyNames (which does
  not include current Chrome or Firefox), so it is not a current
  vulnerability in Caja.

Fixes <https://code.google.com/p/google-caja/issues/detail?id=1725>.

[kpreid2, 2013-06-13 20:04:52]

Ihab, do you agree (as discussed earlier in the thread) that this patch need not
be undisclosed? I have re-verified that Firefox and Chrome (even with the
experimental JS flag set) do not have getPropertyNames.

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:827: if ((lookup = this.col_lookup(name)))
{
On 2013/06/13 19:45:56, ihab.awad wrote:
> Can remove an extra level of parens. :) Here & elsewhere.

The parens are an idiom (and in other environments, actual warning suppression)
for "Yes, I am writing an assignment, not a comparison operator."

I tried to find local style advice on this matter and got no clear evidence for
or against it.

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/ses/Wea...
File src/com/google/caja/ses/WeakMap.js (right):

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/ses/Wea...
src/com/google/caja/ses/WeakMap.js:558: throw new Error('bogus
permitHostObjects___');
On 2013/06/13 19:45:56, ihab.awad wrote:
> More like, bogus _call to_ pHO___?

Done.

[MarkM, 2013-06-13 20:18:59]

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/10162044/diff/5001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:827: if ((lookup = this.col_lookup(name)))
{
On 2013/06/13 20:04:52, kpreid2 wrote:
> On 2013/06/13 19:45:56, ihab.awad wrote:
> > Can remove an extra level of parens. :) Here & elsewhere.
> 
> The parens are an idiom (and in other environments, actual warning
suppression)
> for "Yes, I am writing an assignment, not a comparison operator."
> 
> I tried to find local style advice on this matter and got no clear evidence
for
> or against it.

Yes, whether or not this convention is supported for JS specifically, it is
widely recognized in the C-style-syntax languages, including C, C++, and Java,
and should be kept here.


Amusing anecdote for which a link would be nice:

There was an attempted attack on the Linux source tree where someone submitted a
patch with an assignment in an "if" that they hoped would be mistaken for an
equality test. Had it been accepted, it would have introduced an exploitable
vulnerability. It was caught in time because of this rule. Though I don't
remember if it was caught because the patch contained the extra parens in order
to suppress the warning, making the attack too visible, or because it didn't,
provoking the warning.

Does anyone else remember this? Am I getting this history correct?

[ihab.awad, 2013-06-18 20:07:12]

Given Kevin's assessment of the risk to date on actual browsers, I don't think
we need to keep this issue undisclosed.

[Mark S. Miller, 2013-06-18 20:27:57]

+1


On Tue, Jun 18, 2013 at 1:07 PM, <ihab.awad@gmail.com> wrote:

> Given Kevin's assessment of the risk to date on actual browsers, I don't
> think we need to keep this issue undisclosed.
>
>
>
https://codereview.appspot.**com/10162044/<https://codereview.appspot.com/101...
>
> --
> --
> ---You received this message because you are subscribed to the Google
> Groups "caja-discuss-undisclosed" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
caja-discuss-undisclosed+**unsubscribe@googlegroups.com<caja-discuss-undisclo...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>


-- 
    Cheers,
    --MarkM