laverdet / isolated-vm

Secure & isolated JS environments for nodejs
ISC License
2.19k stars 154 forks source link

Question about timeout option #475

Closed JeanSamGirard closed 5 months ago

JeanSamGirard commented 5 months ago

I'd like to understand better how the timeout option works in isolated-vm, especially in asynchronous scenarios.

I have something a little like this:

import ivm from 'isolated-vm';

const isolate = new ivm.Isolate();
const context = await isolate.createContext();

// Set a reference to a function outside of the isolate
context.global.set(
    'externalFunctionRef',
    new ivm.Reference(async (params) => {
        // Asynchronous code here (database query, http request, etc...)
    })
);

// Make a function in the isolate to abstract the call to the external function
context.evalIgnored(`const externalFunction = async (params) => {
    return await externalFunctionRef.apply(null, [params], {
        arguments: { copy: true },
        result: { copy: true, promise: true },
    });
};`);

// Later on...

// Run the actual code inside the isolate

const code =
    'const myfunc = async ()=>{while(true){await externalFunction("yo")}}; myfunc();';

try {
    console.log(
        'RESULT: ' +
            (await context.eval(code, {
                timeout: 5000,
                promise: true,
                copy: true,
            }))
    );
} catch (e) {
    console.error(e);
}

This seems to never timeout?

Is it because the time spent waiting for the external promise to resolve isn't counted towards the timeout? (aka timeout is CPU time only)

I already have an idea for a workaround for my use case, but if there's a better way I'd love to hear it.


My workaround: When calling externalFunction before going out of the isolate compare Date.now to the Date.now at the start of execution, if too much time has passed, throw. (this is not a real timeout, but would be sufficient for my use case)

laverdet commented 5 months ago

The timeout only applies to the initial synchronous execution. In this case your anonymous function is invoked, it invokes externalFunction which immediately returns a promise. Then myfunc() returns a promise and evaluation has finished.

The promise-related convenience utilities present a challenge when invoked against untrusted code which needs to timeout.

JeanSamGirard commented 5 months ago

This makes so much sense, can't believe I didn't notice that...

Also makes me realize that my workaround doesn't really work :

import ivm from 'isolated-vm';

const isolate = new ivm.Isolate();
const context = await isolate.createContext();

// Set a reference to a function outside of the isolate
context.global.set(
    'externalFunctionRef',
    new ivm.Reference(async (params) => {
        // Asynchronous code here (database query, http request, etc...)
    })
);

// Make a function in the isolate to abstract the call to the external function
context.evalIgnored(`
let started;
const externalFunction = async (params) => {
    if(Date.now() > started + 5000) throw "Timeout reject the promise";
    return await externalFunctionRef.apply(null, [params], {
        arguments: { copy: true },
        result: { copy: true, promise: true },
    });
};`);

// Later on...

// Run the actual code inside the isolate

const code = `started = Date.now(); // I inject this before the "untrusted code" (in my actual use case, the code is transpiled so I have control on the variable names used in the user code, they wouldn't be able to alter this value)

    const myfunc = async ()=>{
        let firstTime = true;
        while(true){
         if(firstTime) await externalFunction(); // myfunc will return a promise here
         firstTime = false; // The promise never resolves and can't be rejected by externalFunction because externalFunction doesn't get called anymore...
    }}; myfunc();`;

try {
    console.log(
        'RESULT: ' +
            (await context.eval(code, {
                timeout: 5000,
                promise: true,
                copy: true,
            }))
    );
} catch (e) {
    console.error(e);
}

I guess I could add the same timeout check that I do in externalFunction at the beginning of while and for loops during transpilation.

Would that cover everything to prevent infinite code execution ?

They could still write recursive code but they'll hit the max call stack size and stop eventually.

JeanSamGirard commented 5 months ago

Would calling isolate.dispose() force the pending promise to reject ?

laverdet commented 5 months ago

If you want to have a timeout which is cumulative between multiple entrances of the isolate, and also includes asynchronous wall time, then you'll need to handle that bookkeeping outside of isolated-vm.

The problem is not simple. For example you need to make sure that await Promise.all([ externalFunction(), externalFunction() ]) doesn't "charge" the user for 2x wall time.

My advice here is to pretend that { promise: true } does not exist and instead pass around your own resolver functions.

JeanSamGirard commented 5 months ago

Thanks for the help.

I think for my use case I'll go with this for now:

import ivm from 'isolated-vm';

const isolate = new ivm.Isolate();
const context = await isolate.createContext();

// Set a reference to a function outside of the isolate
context.global.set(
    'externalFunctionRef',
    new ivm.Reference(async (params) => {
        // Asynchronous code here (database query, http request, etc...)
    })
);

// Make a function in the isolate to abstract the call to the external function
context.evalIgnored(`
let started;
const externalFunction = async (params) => {
    if(Date.now() > started + 5000) throw "Timeout reject the promise";
    return await externalFunctionRef.apply(null, [params], {
        arguments: { copy: true },
        result: { copy: true, promise: true },
    });
};`);

// Later on...

// Run the actual code inside the isolate

const code = `started = Date.now(); // I inject this before the "untrusted code" (in my actual use case, the code is transpiled so I have control on the variable names used in the user code, they wouldn't be able to alter this value)

    const myfunc = async ()=>{
        let firstTime = true;
        while(true){
         if(firstTime) await externalFunction(); // myfunc will return a promise here
         firstTime = false; // The promise never resolves and can't be rejected by externalFunction because externalFunction doesn't get called anymore...
    }}; myfunc();`;

let promiseRef;

try {
    promiseRef = context.eval(code, {
        timeout: 5000,
        promise: true,
        copy: true,
    });

    console.log(
        'RESULT: ' +
            (await Promise.race([
                promiseRef,
                new Promise((resolve, reject) => {
                    setTimeout(() => reject('TIMED OUT'), 5000);
                }),
            ]))
    );
} catch (e) {
    console.error(e);
} finally {
    if (!isolate.isDisposed) isolate.dispose();
    setTimeout(() => console.log(promiseRef), 1000);
}

I don't mind the infinite loop running for a little while longer in the isolate as long as it doesn't stop my main process awaiting it and the Promise gets rejected when the isolate is disposed of.