laverdet / isolated-vm

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

How would one go about making an http request. #434

Closed savoygrizzly closed 10 months ago

savoygrizzly commented 10 months ago

So I tried cloning node-fetch into the isolate context without luck (and by good security design).

Is there any way to allow untrusted code a possibility to send an http request from the isolate, and how would one go about it (code samples welcome).

Thanks.

laverdet commented 10 months ago

Hi there it's answered in the frequently asked question section of the readme: https://github.com/laverdet/isolated-vm?tab=readme-ov-file#frequently-asked-question

savoygrizzly commented 10 months ago

Well, since that answer didn't help except in telling me that I am one of the "people who probably should not be using this library".

Here is a code example I scraped together using [this issue](https://github.com/laverdet/isolated-vm/issues/396).

The goal is to provide users with a way to write simple scripts to perform simple tasks as well as to send fetch requests if needed. The wrapper function "simulates" the existence of the native fetch function.

const ivm = require("isolated-vm");
const { EventEmitter } = require("events");

class HostObject {
  async httpCall(url, data) {
    try {
      const response = await fetch(url, data);
      const contentType = response.headers.get("content-type");
      if (contentType && contentType.indexOf("application/json") !== -1) {
        return await response.json();
      } else {
        return await response.json();
      }
    } catch (err) {
      throw new Error(`Error, failed to fetch ${url}: ${err?.cause || err}`);
    }
  }
}

const runCode = async (code, data) => {
  const results = [];

  const emitter = new EventEmitter();
  emitter.on("event", (data) => {
    results.push(data);
  });

  const emit = (data) => {
    emitter.emit("event", data);
  };

  const isolate = new ivm.Isolate({});

  const context = isolate.createContextSync();

  context.global.setSync("emit", function (...args) {
    emit(...args);
  });
  context.global.setSync("log", function (...args) {
    console.log(...args);
  });
  context.global.setSync("data", new ivm.ExternalCopy(data).copyInto());

  const hostObjectRef = new HostObject();

  context.global.setSync(
    "__hostObject_httpCall",
    new ivm.Reference(async function (...args) {
      const result = await hostObjectRef.httpCall(args[0], args[1]);
      return new ivm.ExternalCopy(result);
    })
  );

  context.global.setSync("hostObject", hostObjectRef, {
    reference: true,
  });
  const script = await isolate.compileScript(`
        (async()=>{ 

            const url = 'https://dummyjson.com/products/1';
            const body = {method: 'GET'};

            //Create a dummy fetch function that wraps __hostObject_httpCall
            const fetch = async (dummyUrl, dummyData) => {
                const result = await __hostObject_httpCall.apply(undefined, [dummyUrl, dummyData], {
                    arguments: { copy: true },
                    result: { promise: true },
                });
                return result.copyInto(); // copy the results back into the isolate
            }

            const fetchedData = await fetch(url, body);
            emit(fetchedData);

            ${code}
            emit('values users want to return');

        })();     
    `);

  try {
    await script.run(context, { promise: true });
    return results;
  } catch (error) {
    console.log("error");
    return error;
  }
};

runCode('',{});

@laverdet if you would be so kind as to point out to any eventual mishap or mistakes it would be very much appreciated.

Thank you for your time.

laverdet commented 10 months ago

There is a section in the README under SECURITY that says:

At a minimum you should take care not to leak any instances of isolated-vm objects (Reference, ExternalCopy, etc) to untrusted code.

__hostObject_httpCall is a Reference and fetchedData is an ExternalCopy and the untrusted code has access to both. I mean it's probably fine since they were kind of hardened, but you should not do that.

Also as a general guideline you should avoid concating untrusted code. You can use Function or AsyncFunction for this.

savoygrizzly commented 9 months ago

Thanks for your feedback,

I edited this to prevent the ExternalCopy to be leaked into the VM.

await jail.set(
    "__hostObject_httpCall",
    new ivm.Reference(async function (url,data) {
      const result = await hostObjectRef.httpCall(url, data);
      return new ivm.ExternalCopy(result).copyInto();
    })
  );

However because hostObjectRef.httpCall is returning a Promise I am unable to call deref on the Reference.

As for avoiding the concatenation I don't see how I could do it differently in this use case.

Security wise this is gonna run as a separated service on a hardened docker running on a separated vm. Also the runner is meant to run untrusted code but for privileged users, therefore probably not the biggest threat vector.

laverdet commented 9 months ago

As for avoiding the concatenation I don't see how I could do it differently in this use case.

I linked it in my previous comment. The built-in Function and AsyncFunction constructors.

therefore probably not the biggest threat vector.

If you violate the invariants of a system then none of the promises it made to you can be relied on. If your other protections [vm, docker] are good enough then what do you gain by adding isolated-vm?

savoygrizzly commented 9 months ago

If a theoretical attacker was running code directly in a docker or firecracker he would then gain inside access to the VPC and to the launcher spinning up docker as well. It would also open up the possibility of outside influences on code being run considering a single docker would be spinning up for every X requests. Besides modifying the design of the stack to give entire control of a full VM, spun up on demand for someone to run a 2 liner code snippet just to make one API call feels like nailing a painting with a sledgehammer.

Moreover isolated-vm also gives a timeout and max memory control in a faster approach than it would configuring it in the docker itself.

I know I am not as literate in the inner JS engine's working nor as good as a programmer as you, and that thus you don't really feel like providing code samples. But could you achieve passing an async function to the user allowing http calls in isolated-vm without violating invariants ? I have searched but never could find someone who had it working (except for this example, that leaks a Reference)

laverdet commented 9 months ago
import ivm from 'isolated-vm';
const isolate = new ivm.Isolate;
const context = await isolate.createContext();
context.evalClosure(
    'globalThis.request = (...args) => $0.apply(undefined, args, { arguments: { copy: true }, promise: true, result: { copy: true, promise: true } })',
    [
        new ivm.Reference(async url => {
            console.log('request', url);
            return {
                body: '<!doctype>'
            };
        })
     ]);

const run = code => context.evalClosure(`return (async function() {
        const AsyncFunction = async function () {}.constructor;
        return new AsyncFunction($0)();
    }())`, [ code ], { result: { copy: true, promise: true } })

console.log(await run('return request("https://example.com")'));

You would fill in the blank with however you want to dispatch a request.

Please do not concat code. Imagine you have this:

context.evalClosure(`try { ${code}; } finally { do_important_cleanup(); }`);

Now imagine code is something like " } evil_code(); return; try {". In this case the user could make it so your postamble doesn't run. Maybe it doesn't matter in your example but these errors accumulate into security vulnerabilities.