add review notes
MODIFY
secreview-notes.txt
secreview-notes.txt
@@ -80,3 +80,13 @@ primal realm. Might let us avoid the weakmaps?
make the Realm *constructor* be the weakmap key, not the prototype?
+Two reasons why the function. constructor of the root realm should always throw:
+* the mike samuel attack: adversary-controlled data fed into an innocent
+ program can cause it to accidentally navigate to the root realm's funtion
+ constructor and then feed a string to it which is code, result is a mild
+ form of injection attack. The injection doesn't abuse authority, but it
+ causes unreviewed behavior in the realm
+* getting to the constructor via the .constructor link doesn't lead to a
+ compartment-speciifc behavior, so if it has any non-erroneous behavior at
+ all, it would be invisibly incompatible, which would cause confusion. if we
+ can't be compatible, we should thrrow an error.

MODIFY
shim/src/accessors.js
shim/src/accessors.js
@@ -61,7 +61,7 @@ export function repairAccessors(unsafeRec) {
},
__lookupGetter__: {
value(prop) {
- prop = asPropertyName(prop);
+ prop = asPropertyName(prop); // sanitize property name/symbol
let base = this;
let desc;
while (base && !(desc = getOwnPropertyDescriptor(base, prop))) {
@@ -72,7 +72,7 @@ export function repairAccessors(unsafeRec) {
},
__lookupSetter__: {
value(prop) {
- prop = asPropertyName(prop);
+ prop = asPropertyName(prop); // sanitize property name/symbol
let base = this;
let desc;
while (base && !(desc = getOwnPropertyDescriptor(base, prop))) {

MODIFY
shim/src/commons.js
shim/src/commons.js
// improves both consistency and minification. Unused declarations are
// dropped by the tree shaking process.
+// we capture these, not just for brevity, but for security. If any code
+// modifies Object to change what 'assign' points to, the Realm shim would be
+// corrupted.
+
export const {
assign,
create,
@@ -21,9 +25,12 @@ export const {
} = Reflect;
// See http://wiki.ecmascript.org/doku.php?id=conventions:safe_meta_programming
+// which only lives at http://web.archive.org/web/20160805225710/http://wiki.ecmascript.org/doku.php?id=conventions:safe_meta_programming
const bind = Function.prototype.bind;
const uncurryThis = bind.bind(bind.call);
+// We also capture these for security: changes to Array.prototype after the
+// Realm shim runs shouldn't affect subsequent Realm operations.
export const objectHasOwnProperty = uncurryThis(Object.prototype.hasOwnProperty),
arrayPush = uncurryThis(Array.prototype.push),
arrayPop = uncurryThis(Array.prototype.pop),

MODIFY
shim/src/stdlib.js
shim/src/stdlib.js
@@ -83,6 +83,9 @@ export function getUnsafeGlobalDescs(unsafeGlobal) {
for (const name of sharedGlobalPropertyNames) {
descriptors[name] = {
+ // todo: if there's a get/accessor on the global, do we want to invoke
+ // it or throw an error?
+ // todo: get a descriptor here, so we can check
value: unsafeGlobal[name],
writable: true,
configurable: true

MODIFY
shim/test/realm/confinement.js
shim/test/realm/confinement.js
@@ -18,6 +18,13 @@ test('confinement evaluation constructor', t => {
t.throws(() => {
r.evaluate('({}).constructor.constructor("return this")()');
}, Error);
+
+ // Error is a function, so Error.__proto__ is Function.prototype . The
+ // unpatched Function.prototype.constructor used to point at the unsafe
+ // 'Function' object, which would provide access to the primal realm's
+ // globals, so it must be kept out of the hands of any child realm. We
+ // replace that '.constructor' with a safe replacement (which always
+ // throws). Here we test that this constructor has been replaced.
t.throws(() => {
r.evaluate('Error.__proto__.constructor("return this")()');
}, Error);

add review notes