Skip to content

Commit

Permalink
Merge pull request #252 from XmiliaH/fix-proxy-trap-errors
Browse files Browse the repository at this point in the history 
Fix access to frozen or unconfigurable properties
  • Loading branch information
@patriksimek
patriksimek committed Mar 21, 2020
2 parents 8c9143d + dfaafd1 commit 3ba34db
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 212 deletions.
246 changes: 56 additions & 190 deletions lib/contextify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,31 @@ function instanceOf(value, construct) {
}
}

const SHARED_ARROW = ()=>{};
function SHARED_FUNC() {}
const SHARED_ARRAY = [];
const SHARED_OBJECT = {__proto__: null};

function getBaseObject(obj) {
if (typeof obj === 'function') {
try {
// eslint-disable-next-line no-new
new new host.Proxy(obj, {
__proto__: null,
construct() {
return this;
}
})();
} catch (e) {
return SHARED_ARROW;
}
return SHARED_FUNC;
} else if (host.Array.isArray(obj)) {
return SHARED_ARRAY;
}
return SHARED_OBJECT;
}

/**
* VMError definition.
*/
Expand Down Expand Up @@ -84,192 +109,6 @@ function throwCallerCalleeArgumentsAccess(key) {
return new VMError('Unreachable');
}

/*
* Proxy Helper
*
* Here we track proxy creations so that we know for every proxy in the VM the
* target. If the proxy is given to decontextify we are going to lookup
* the target and using this non proxy as target for the decontextify proxy.
*
*/

const ProxyHelper = host.Object.create(null);

// Marker for revoked proxy objects
ProxyHelper.revoked = 'Revoked';

// Tracks for every proxy the target.
ProxyHelper.tracker = new host.WeakMap();

// Gets the target of a proxy recursively until target is not any more a proxy
ProxyHelper.getTarget = (proxy) => {
let obj = proxy;
let next;
while ((next = ProxyHelper.tracker.get(obj))!==undefined) {
obj = next;
}
// Target could be revoked.
if (obj === ProxyHelper.revoked) {
obj = host.Object.create(null);
}
return obj;
};


// This is not so nice, I would prefer globalThis.Proxy but globalThis is relatively new
Proxy = ((ProxyFunc) => {
// Handle Proxy.revocable()
const ProxyRevocableHandler = host.Object.create(null);
ProxyRevocableHandler.apply = (target, thiz, args) => {
const proxyTarget = args[0];
const ret = local.Reflect.apply(target, thiz, args);
const proxy = ret.proxy;
ProxyHelper.tracker.set(proxy, proxyTarget);
const revokeHandler = host.Object.create(null);
revokeHandler.apply = (rTarget, rThiz, rArgs) => {
const rRet = local.Reflect.apply(rTarget, rThiz, rArgs);
ProxyHelper.tracker.set(proxy, ProxyHelper.revoked);
return rRet;
};
ret.revoke = new host.Proxy(ret.revoke, revokeHandler);
return ret;
};
ProxyFunc.revocable = new host.Proxy(Proxy.revocable, ProxyRevocableHandler);
// Handle new Proxy()
const ProxyHandler = host.Object.create(null);
ProxyHandler.construct = (target, args, newTarget) => {
const proxyTarget = args[0];
const proxy = local.Reflect.construct(target, args, newTarget);
ProxyHelper.tracker.set(proxy, proxyTarget);
return proxy;
};
return new host.Proxy(ProxyFunc, ProxyHandler);
})(Proxy);

/*
* Shared proxy handler for inspect proxy.
* This is needed since node's inspect method used in console.log
* will strip one proxy layer away. The second layer will use this
* handler.
*/

const INSPECT_PROXY_HANDLER = host.Object.create(null);

INSPECT_PROXY_HANDLER.apply = (target, context, args) => {
context = Contextify.value(context);

// Set context of all arguments to vm's context.
args = Contextify.arguments(args);

try {
return Decontextify.value(target.apply(context, args));
} catch (e) {
throw Decontextify.value(e);
}
};
INSPECT_PROXY_HANDLER.construct = (target, args, newTarget) => {
throw new host.VMError('Inspect tried to create new object');
};
INSPECT_PROXY_HANDLER.get = (target, key, receiver) => {
try {
return Decontextify.value(target[key]);
} catch (e) {
throw Decontextify.value(e);
}
};
INSPECT_PROXY_HANDLER.set = (target, key, value, receiver) => {
// Inspect should not set properties
return false;
};
INSPECT_PROXY_HANDLER.getOwnPropertyDescriptor = (target, prop) => {
let def;

try {
def = host.Object.getOwnPropertyDescriptor(target, prop);
} catch (e) {
throw Decontextify.value(e);
}

// Following code prevents V8 to throw
// TypeError: 'getOwnPropertyDescriptor' on proxy: trap reported non-configurability for property '<prop>'
// which is either non-existant or configurable in the proxy target

if (!def) {
return undefined;
} else if (def.get || def.set) {
return {
get: Decontextify.value(def.get) || undefined,
set: Decontextify.value(def.set) || undefined,
enumerable: def.enumerable === true,
configurable: def.configurable === true
};
} else {
return {
value: Decontextify.value(def.value),
writable: def.writable === true,
enumerable: def.enumerable === true,
configurable: def.configurable === true
};
}
};
INSPECT_PROXY_HANDLER.defineProperty = (target, key, descriptor) => {
// Inspect should not set properties
return false;
};
INSPECT_PROXY_HANDLER.deleteProperty = (target, prop) => {
// Inspect should not set properties
return false;
};
INSPECT_PROXY_HANDLER.getPrototypeOf = (target) => {
try {
return Decontextify.value(local.Object.getPrototypeOf(target));
} catch (e) {
throw Decontextify.value(e);
}
};
INSPECT_PROXY_HANDLER.setPrototypeOf = (target) => {
// Inspect should not set properties
return false;
};
INSPECT_PROXY_HANDLER.has = (target, key) => {
try {
return Decontextify.value(local.Reflect.has(target, key));
} catch (e) {
throw Decontextify.value(e);
}
};
INSPECT_PROXY_HANDLER.isExtensible = target => {
try {
return Decontextify.value(local.Object.isExtensible(target));
} catch (e) {
throw Decontextify.value(e);
}
};
INSPECT_PROXY_HANDLER.ownKeys = target => {
try {
const keys = local.Reflect.ownKeys(target);
if (host.Array.isArray(target)) {
// Do this hack so that console.log(decontextify([1,2,3])) doesn't write the properties twice
// a la [1,2,3,'0':1,'1':2,'2':3]
return Decontextify.value(keys.filter(key=>typeof key!=='string' || !key.match(/^\d+$/)));
}
return Decontextify.value(keys);
} catch (e) {
throw Decontextify.value(e);
}
};
INSPECT_PROXY_HANDLER.preventExtensions = target => {
// Inspect should not call this
return false;
};
INSPECT_PROXY_HANDLER.enumerate = target => {
try {
return Decontextify.value(local.Reflect.enumerate(target));
} catch (e) {
throw Decontextify.value(e);
}
};

/**
* Decontextify.
*/
Expand Down Expand Up @@ -521,11 +360,38 @@ Decontextify.object = (object, traps, deepTraps, flags, mock) => {
}
};

const resolvedTarget = ProxyHelper.getTarget(object);
const proxy = new host.Proxy(resolvedTarget, INSPECT_PROXY_HANDLER);
host.Object.assign(base, traps, deepTraps);

let shallow;
if (host.Array.isArray(object)) {
const origGet = base.get;
shallow = {
__proto__: null,
ownKeys: base.ownKeys,
get: origGet
};
base.ownKeys = target => {
try {
const keys = local.Reflect.ownKeys(object);
// Do this hack so that console.log(decontextify([1,2,3])) doesn't write the properties twice
// a la [1,2,3,'0':1,'1':2,'2':3]
return Decontextify.value(keys.filter(key=>typeof key!=='string' || !key.match(/^\d+$/)));
} catch (e) {
throw Decontextify.value(e);
}
};
base.get = (target, key, receiver) => {
if (key === host.Symbol.toStringTag) return;
return origGet(target, key, receiver);
};
} else {
shallow = SHARED_OBJECT;
}

const proxy = new host.Proxy(getBaseObject(object), base);
Decontextified.set(proxy, object);
// We need two proxys since nodes inspect just removes one.
const proxy2 = new host.Proxy(proxy, host.Object.assign(base, traps, deepTraps));
const proxy2 = new host.Proxy(proxy, shallow);
Decontextify.proxies.set(object, proxy2);
Decontextified.set(proxy2, object);
return proxy2;
Expand Down Expand Up @@ -852,7 +718,7 @@ Contextify.object = (object, traps, deepTraps, flags, mock) => {
}
};

const proxy = new host.Proxy(object, host.Object.assign(base, traps, deepTraps));
const proxy = new host.Proxy(getBaseObject(object), host.Object.assign(base, traps, deepTraps));
Contextify.proxies.set(object, proxy);
Contextified.set(proxy, object);
return proxy;
Expand Down
2 changes: 1 addition & 1 deletion lib/main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ class VM extends EventEmitter {
*/
protect(value, globalName) {
this._internal.Contextify.protected(value);
if (globalName) this._internal.Contextify.globalValue(globalName, value);
if (globalName) this._internal.Contextify.setGlobal(globalName, value);
return value;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/wildcard.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const match = (wildcard, s) => {
const regexString = wildcard.replace(/\*/, '\\S*').replace(/\?/g, '.');
const regexString = wildcard.replace(/\*/g, '\\S*').replace(/\?/g, '.');
const regex = new RegExp(regexString);
return regex.test(s);
};
Expand Down
44 changes: 25 additions & 19 deletions test/vm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,18 @@ describe('VM', () => {
assert.throws(() => vm2.run('Function')('async function(){}'), /Async not available/, '#9');
});

it('frozen unconfigurable access', () => {
const vm2 = new VM();

assert.doesNotThrow(()=>{
vm2.run('x => x.prop')(Object.freeze({prop: 'good'}));
});

assert.doesNotThrow(()=>{
vm2.run('x => x.prop')(Object.defineProperty({}, 'prop', {value: 'good'}));
});
});

it('various attacks #1', () => {
const vm2 = new VM({sandbox: {log: console.log, boom: () => {
throw new Error();
Expand Down Expand Up @@ -654,26 +666,20 @@ describe('VM', () => {

vm2 = new VM();

assert.throws(() => vm2.run(`
var process;
try {
Object.defineProperty(Buffer.from(""), "", {
value: new Proxy({}, {
getPrototypeOf(target) {
if(this.t) {
throw Buffer.from;
}
this.t=true;
return Object.getPrototypeOf(target);
assert.doesNotThrow(() => vm2.run(`
Object.defineProperty(Buffer.from(""), "", {
value: new Proxy({}, {
getPrototypeOf(target) {
if(this.t) {
throw Buffer.from;
}
})
});
} catch (e) {
process = e.constructor("return process")();
}
process.mainModule.require("child_process").execSync("whoami").toString()
`), /e\.constructor\(\.\.\.\) is not a function/, '#4');
this.t=true;
return Object.getPrototypeOf(target);
}
})
});
`), '#4');

vm2 = new VM();

Expand Down
2 changes: 1 addition & 1 deletion test/wildcard.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const assert = require('assert');

describe('wildcard matching', () => {
it('handles * correctly', () => {
assert.strictEqual(match('some*g', 'something'), true);
assert.strictEqual(match('s*th*g', 'something'), true);
});

it('handles ? correctly', () => {
Expand Down

0 comments on commit 3ba34db

Please sign in to comment.