Revision: 5453
Author: [email protected]
Date: Tue Jun 18 13:18:13 2013
Log: Defend WeakMap against leaking its HIDDEN_NAME to proxies.
https://codereview.appspot.com/10162044
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>.
[email protected], [email protected]
http://code.google.com/p/google-caja/source/detail?r=5453
Modified:
/trunk/src/com/google/caja/plugin/domado.js
/trunk/src/com/google/caja/plugin/es53-frame-group.js
/trunk/src/com/google/caja/plugin/ses-frame-group.js
/trunk/src/com/google/caja/plugin/taming-membrane.js
/trunk/src/com/google/caja/plugin/taming-schema.js
/trunk/src/com/google/caja/ses/WeakMap.js
/trunk/tests/com/google/caja/plugin/es53-test-scan-guest.js
=======================================
--- /trunk/src/com/google/caja/plugin/domado.js Mon Jun 17 12:21:19 2013
+++ /trunk/src/com/google/caja/plugin/domado.js Tue Jun 18 13:18:13 2013
@@ -162,19 +162,6 @@
|| getPropertyDescriptor(Object.getPrototypeOf(o), n);
}
}
-
- /**
- * The Caja WeakMap emulation magic property name.
- *
- * This name can only be seen by using a proxy handler, but we need our
proxy
- * handlers to permit it even on frozen-seeming proxies, so we have to
obtain
- * it here, which we do by using a proxy to observe it.
- *
- * If proxies are unavailable or if WeakMaps do not use a magic
property, then
- * weakMapMagicName will be a value unequal to any string.
- */
- var weakMapMagicName = {};
- // The actual initialization of weakMapMagicName occurs after
ProxyHandler.
/**
* This is a simple forwarding proxy handler. Code copied 2011-05-24 from
@@ -193,12 +180,6 @@
// Object.getOwnPropertyDescriptor(proxy, name) -> pd | undefined
getOwnPropertyDescriptor: function(name) {
- if (name === weakMapMagicName) {
- // Caja WeakMap emulation internal property; must be redirected to
- // avoid glitches involving the proxy being treated as identical
to its
- // target.
- return Object.getOwnPropertyDescriptor(this, 'weakMapMagic');
- }
var desc = Object.getOwnPropertyDescriptor(this.target, name);
if (desc !== undefined) { desc.configurable = true; }
return desc;
@@ -223,26 +204,12 @@
// Object.defineProperty(proxy, name, pd) -> undefined
defineProperty: function(name, desc) {
- if (name === weakMapMagicName) {
- if (desc.get) {
- throw new Error('WeakMap compatibility doesn\'t expect
accessors');
- }
- if (desc.set === null) {
- // TODO(kpreid): stale?
- desc.set = undefined;
- }
- Object.defineProperty(this, 'weakMapMagic', desc);
- return true;
- }
Object.defineProperty(this.target, name, desc);
return true;
},
// delete proxy[name] -> boolean
'delete': function(name) {
- if (name === weakMapMagicName) {
- return delete this.weakMapMagic;
- }
return delete this.target[name];
},
@@ -272,11 +239,6 @@
// proxy[name] -> any
get: function(proxy, name) {
- if (name === weakMapMagicName) {
- // Caja WeakMap emulation internal property
- // Note: Not correct in the presence of a getter (checked above)
- return this.weakMapMagic;
- }
return this.target[name];
},
@@ -312,39 +274,6 @@
};
cajaVM.def(ProxyHandler);
- // Find the weakMapMagicName. This occurs after ProxyHandler is defined
- // because it uses ProxyHandler.
- (function() {
- if (!proxiesAvailable) {
- // unobservable so doesn't matter
- return;
- }
-
- // Create a proxy whose handler will stash away the magic name.
- var handler = new ProxyHandler({});
- handler.getOwnPropertyDescriptor = function(name) {
- if (/^weakmap:/.test(name)) { weakMapMagicName = name; }
- return ProxyHandler.prototype.getOwnPropertyDescriptor.call(this,
name);
- };
- handler.get = function(receiver, name) {
- if (/^weakmap:/.test(name)) { weakMapMagicName = name; }
- return ProxyHandler.prototype.get.call(this, receiver, name);
- };
- var proxy = Proxy.create(handler);
-
- // Cause the proxy to be used as a key.
- var w = new WeakMap();
- try {
- w.get(proxy);
- } catch (e) {
- console.error('Domado internal error: failed in WeakMap name
setup:', e);
- }
-
- // At this point, we have either obtained the magic name, or there is
no
- // observable magic name, in which case weakMapMagicName is left at its
- // initial {} value which is not === to any property name.
- })();
-
function makeOverrideSetter(object, prop) {
return innocuous(function overrideSetter(newValue) {
if (object === this) {
@@ -867,7 +796,7 @@
CollectionProxyHandler.prototype.getOwnPropertyDescriptor =
function (name) {
var lookup;
- if (name !== weakMapMagicName && (lookup = this.col_lookup(name))) {
+ if ((lookup = this.col_lookup(name))) {
return {
configurable: true, // proxy invariant check
enumerable: true, // TODO(kpreid): may vary
@@ -880,7 +809,7 @@
};
CollectionProxyHandler.prototype.get = function(receiver, name) {
var lookup;
- if (name !== weakMapMagicName && (lookup = this.col_lookup(name))) {
+ if ((lookup = this.col_lookup(name))) {
return this.col_evaluate(lookup);
} else {
return ProxyHandler.prototype.get.call(this, receiver, name);
@@ -893,9 +822,7 @@
};
CollectionProxyHandler.prototype['delete'] = function(name) {
var lookup;
- if (name === weakMapMagicName) {
- return false;
- } else if ((lookup = this.col_lookup(name))) {
+ if ((lookup = this.col_lookup(name))) {
return false;
} else {
return ProxyHandler.prototype['delete'].call(this, name);
=======================================
--- /trunk/src/com/google/caja/plugin/es53-frame-group.js Wed May 8
09:50:54 2013
+++ /trunk/src/com/google/caja/plugin/es53-frame-group.js Tue Jun 18
13:18:13 2013
@@ -54,7 +54,8 @@
'banNumerics', markCallableWithoutMembrane(banNumerics),
'USELESS', tamingWin.___.USELESS,
'BASE_OBJECT_CONSTRUCTOR', tamingWin.___.BASE_OBJECT_CONSTRUCTOR,
- 'getValueOf', markCallableWithoutMembrane(getValueOf));
+ 'getValueOf', markCallableWithoutMembrane(getValueOf),
+ 'weakMapPermitHostObjects', tamingWin.cajaVM.identity);
var frameGroupTamingSchema = TamingSchema(tamingHelper);
var frameGroupTamingMembrane =
=======================================
--- /trunk/src/com/google/caja/plugin/ses-frame-group.js Wed Jun 5
20:35:14 2013
+++ /trunk/src/com/google/caja/plugin/ses-frame-group.js Tue Jun 18
13:18:13 2013
@@ -14,15 +14,16 @@
/**
* @provides SESFrameGroup
+ * @requires bridalMaker
* @requires cajaVM
* @requires cajaFrameTracker
* @requires Domado
* @requires GuestManager
* @requires Q
- * @requires URI
+ * @requires ses
* @requires TamingSchema
* @requires TamingMembrane
- * @requires bridalMaker
+ * @requires URI
* @overrides window
*/
@@ -50,7 +51,8 @@
banNumerics: function() {},
USELESS: USELESS,
BASE_OBJECT_CONSTRUCTOR: BASE_OBJECT_CONSTRUCTOR,
- getValueOf: function(o) { return o.valueOf(); }
+ getValueOf: function(o) { return o.valueOf(); },
+ weakMapPermitHostObjects: ses.weakMapPermitHostObjects
});
var frameGroupTamingSchema = TamingSchema(tamingHelper);
=======================================
--- /trunk/src/com/google/caja/plugin/taming-membrane.js Wed May 29
10:27:13 2013
+++ /trunk/src/com/google/caja/plugin/taming-membrane.js Tue Jun 18
13:18:13 2013
@@ -25,6 +25,7 @@
var feralByTame = new WeakMap();
var tameByFeral = new WeakMap();
+ privilegedAccess.weakMapPermitHostObjects(tameByFeral);
// Useless value provided as a safe 'this' value to functions.
feralByTame.set(privilegedAccess.USELESS, privilegedAccess.USELESS);
=======================================
--- /trunk/src/com/google/caja/plugin/taming-schema.js Mon Mar 25 13:24:51
2013
+++ /trunk/src/com/google/caja/plugin/taming-schema.js Tue Jun 18 13:18:13
2013
@@ -70,12 +70,19 @@
READ_ONLY_RECORD: 'read_only_record'
});
- var tameAs = new WeakMap();
+ // All WeakMaps we use deal in host objects, so have a shortcut
+ function makeWeakMap() {
+ var map = new WeakMap();
+ privilegedAccess.weakMapPermitHostObjects(map);
+ return map;
+ }
- var tameFunctionName = new WeakMap();
- var tameCtorSuper = new WeakMap();
+ var tameAs = makeWeakMap();
+
+ var tameFunctionName = makeWeakMap();
+ var tameCtorSuper = makeWeakMap();
- var functionAdvice = new WeakMap();
+ var functionAdvice = makeWeakMap();
function applyFeralFunction(f, self, args) {
return initAdvice(f)(self, args);
@@ -91,7 +98,7 @@
}
}
- var fixed = new WeakMap();
+ var fixed = makeWeakMap();
function checkCanControlTaming(f) {
var to = typeof f;
=======================================
--- /trunk/src/com/google/caja/ses/WeakMap.js Mon May 6 13:38:03 2013
+++ /trunk/src/com/google/caja/ses/WeakMap.js Tue Jun 18 13:18:13 2013
@@ -23,8 +23,8 @@
* quite conform, run <code>repairES5.js</code> first.
*
* @author Mark S. Miller
- * @requires ses, crypto, ArrayBuffer, Uint8Array
- * @overrides WeakMap, WeakMapModule, navigator
+ * @requires crypto, ArrayBuffer, Uint8Array, navigator
+ * @overrides WeakMap, ses, Proxy
*/
/**
@@ -97,6 +97,29 @@
// already too broken, so give up
return;
}
+
+ /**
+ * In some cases (current Firefox), we must make a choice betweeen a
+ * WeakMap which is capable of using all varieties of host objects as
+ * keys and one which is capable of safely using proxies as keys. See
+ * comments below about HostWeakMap and DoubleWeakMap for details.
+ *
+ * This function (which is a global, not exposed to guests) marks a
+ * WeakMap as permitted to do what is necessary to index all host
+ * objects, at the cost of making it unsafe for proxies.
+ *
+ * Do not apply this function to anything which is not a genuine
+ * fresh WeakMap.
+ */
+ function weakMapPermitHostObjects(map) {
+ // identity of function used as a secret -- good enough and cheap
+ if (map.permitHostObjects___) {
+ map.permitHostObjects___(weakMapPermitHostObjects);
+ }
+ }
+ if (typeof ses !== 'undefined') {
+ ses.weakMapPermitHostObjects = weakMapPermitHostObjects;
+ }
// Check if there is already a good-enough WeakMap implementation, and
if so
// exit without replacing it.
@@ -183,6 +206,12 @@
return (u8 % 36).toString(36);
}).join('') + '___';
}
+
+ function isNotHiddenName(name) {
+ return !(
+ name.substr(0, HIDDEN_NAME_PREFIX.length) == HIDDEN_NAME_PREFIX &&
+ name.substr(name.length - 3) === '___');
+ }
/**
* Monkey patch getOwnPropertyNames to avoid revealing the
@@ -198,11 +227,7 @@
*/
defProp(Object, 'getOwnPropertyNames', {
value: function fakeGetOwnPropertyNames(obj) {
- return gopn(obj).filter(function(name) {
- return !(
- name.substr(0, HIDDEN_NAME_PREFIX.length) == HIDDEN_NAME_PREFIX
&&
- name.substr(name.length - 3) === '___');
- });
+ return gopn(obj).filter(isNotHiddenName);
}
});
@@ -214,9 +239,7 @@
var originalGetPropertyNames = Object.getPropertyNames;
defProp(Object, 'getPropertyNames', {
value: function fakeGetPropertyNames(obj) {
- return originalGetPropertyNames(obj).filter(function(name) {
- return name !== HIDDEN_NAME;
- });
+ return originalGetPropertyNames(obj).filter(isNotHiddenName);
}
});
}
@@ -484,6 +507,14 @@
// we know all entries actually stored are entered in 'hmap'.
var omap = undefined;
+ // Hidden-property maps are not compatible with proxies because
proxies
+ // can observe the hidden name and either accidentally expose it
or fail
+ // to allow the hidden property to be set. Therefore, we do not
allow
+ // arbitrary WeakMaps to switch to using hidden properties, but
only
+ // those which need the ability, and unprivileged code is not
allowed
+ // to set the flag.
+ var enableSwitching = false;
+
function dget(key, opt_default) {
if (omap) {
return hmap.has(key) ? hmap.get(key)
@@ -498,11 +529,15 @@
}
function dset(key, value) {
- try {
+ if (enableSwitching) {
+ try {
+ hmap.set(key, value);
+ } catch (e) {
+ if (!omap) { omap = new OurWeakMap(); }
+ omap.set___(key, value);
+ }
+ } else {
hmap.set(key, value);
- } catch (e) {
- if (!omap) { omap = new OurWeakMap(); }
- omap.set___(key, value);
}
}
@@ -515,7 +550,14 @@
get___: { value: constFunc(dget) },
has___: { value: constFunc(dhas) },
set___: { value: constFunc(dset) },
- delete___: { value: constFunc(ddelete) }
+ delete___: { value: constFunc(ddelete) },
+ permitHostObjects___: { value: constFunc(function(token) {
+ if (token === weakMapPermitHostObjects) {
+ enableSwitching = true;
+ } else {
+ throw new Error('bogus call to permitHostObjects___');
+ }
+ })}
});
}
DoubleWeakMap.prototype = OurWeakMap.prototype;
@@ -530,6 +572,15 @@
});
})();
} else {
+ // There is no host WeakMap, so we must use the emulation.
+
+ // Emulated WeakMaps are incompatible with native proxies (because
proxies
+ // can observe the hidden name), so we must disable Proxy usage (in
+ // ArrayLike and Domado, currently).
+ if (typeof Proxy !== 'undefined') {
+ Proxy = undefined;
+ }
+
WeakMap = OurWeakMap;
}
})();
=======================================
--- /trunk/tests/com/google/caja/plugin/es53-test-scan-guest.js Mon Jun 17
12:21:19 2013
+++ /trunk/tests/com/google/caja/plugin/es53-test-scan-guest.js Tue Jun 18
13:18:13 2013
@@ -1352,7 +1352,8 @@
argsBySuffix('WeakMap<CONSTRUCT>().get___',
argsBySuffix('WeakMap<CONSTRUCT>().has___',
argsBySuffix('WeakMap<CONSTRUCT>().set___',
- G.none)))); // known implementation details leak
+ argsBySuffix('WeakMap<CONSTRUCT>().permitHostObjects___',
+ G.none))))); // known implementation details leak. TODO abuse
argsByIdentity(RegExp, genNew(genRegex));
argsByIdentity(RegExp.prototype.exec,
freshResult(genMethod(genString)));
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
