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

runtime-manager: lock down pooled threads #120

Closed
josephjclark opened this issue Dec 6, 2022 · 5 comments
Closed

runtime-manager: lock down pooled threads #120

josephjclark opened this issue Dec 6, 2022 · 5 comments
Assignees
Labels

Comments

@josephjclark
Copy link
Collaborator

The runtime manager uses thread pooling to run jobs. This reduces the overhead of starting up a new process for every job - idle threads will be re-used.

There is a security issue here: because threads are shared, their environment is ALSO shared. That means a job could corrupt a worker's environment and compromise the next job to run in that thread.

There are two solutions which we should explore more nearer the time:

  1. use Object.freeze() or ses.harden() to freeze the global scope
  2. use vm.runInNewContext() on every job (probably inside the worker's run function) to isolate each global scope.

Breakouts are presumably a theoretical concern in both cases. I don't know which is more secure. If we can't find a reasonable solution then we'll have to abandon node pooling (which makes you wonder if threading is even worth it)

@josephjclark josephjclark changed the title runtime-manager: lockdown pooled threads runtime-manager: lock down pooled threads Dec 6, 2022
@josephjclark
Copy link
Collaborator Author

@josephjclark
Copy link
Collaborator Author

You know, the runtime currently creates a new context object (basically a global scope) for each run. So actually, subsequent jobs in the same worker thread should already have their own contexts. This needs checking and testing.

When we create a context, it's probably a good idea to freeze or harden it before running the job. With slightly tighter control over what goes in that scope (see #104) it's probably a pretty secure starting point.

@josephjclark
Copy link
Collaborator Author

josephjclark commented Dec 6, 2022

Ok, confirmed. Each runtime creates its own context, so two runs can't actually interfere with each other. This means we don't need to sandbox threads any more (unless someone can break out of the vm context into the parenting thread scope)

See unit tests around this stuff here: https://github.com/OpenFn/kit/blob/security-tests/packages/runtime/test/security.test.ts

Note that while the scope is isolated, it may still not be entirely secure. We're doing nothing to harden it, so a job can override eg the array prototype. To what end I am not sure as it can't affect any other job...

@josephjclark
Copy link
Collaborator Author

So I think this one is gonna stay open.

I've been testing around this most of the day. Where we are now is:

Jobs running in pooled threads are totally dependent on the runtime sandbox to preserve their environment.

We can't really freeze the global scope to prevent writes (freezing global is easy, but you have to freeze every variable in the top module scope, and that includes things which can't be frozen, like the typed arrays). So if someone was to break out of the job jail, they'd be able to scribble to the top worker scope and set off a long running function or interval or something to spy on the runner's environment.

Having broken out, though, I don't think there's any way to break back in to the next job.

I can confirm that the parent process is not available to the child.

The child worker can optionally see the parent's env, but I have disabled this, so the env is secure.

I don't know. I'm not particularly worried about this, but I would like to invest a bit more time. If we can run jobs in a) a secure, sandboxed worker thread and b) a secure sandbox inside that worker, then that's a really tight double layer of security. Double jail. So I'd still like to take some action here.

@josephjclark josephjclark self-assigned this Oct 26, 2023
@josephjclark
Copy link
Collaborator Author

More thoughts on this.

In workerpool, the environment is not terribly secure if you break out of the sandbox.

But this week we have learned more and added more mitgations:

Also, the sandbox itself is a reasonable level of protection; threads cannot read or communicate with other threads or their parent process; and per the above the environment is protected (and the parent environment private).

Closing as I do not plan to do any more work on this (beyond the other open issues)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant