Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

Defend against malicious plugins #57

Closed
mariusandra opened this issue Dec 9, 2020 · 5 comments
Closed

Defend against malicious plugins #57

mariusandra opened this issue Dec 9, 2020 · 5 comments

Comments

@mariusandra
Copy link
Collaborator

We should figure out what are the limits of the plugin system and find ways to protect against malicious plugins, deliberate or not. For example plugins that:

  • Await for 300sec and then sends on the event
  • Block the thread indefinitely with a long running for loop
  • Take up 100GB of RAM
  • Mint crypto
  • etc

Ideally none of these cases should bring down the server and long running tasks should get killed in some way.

@Twixes
Copy link
Collaborator

Twixes commented Jan 13, 2021

Digging for ideas: what can we do in this regard?

@mariusandra
Copy link
Collaborator Author

We could set resource limits to something sane to avoid workers using up all memory.

As for long running tasks, we can just capture and measure how long each task takes in the main thread. Piscina natively supports aborting tasks, but this won't help us here, as it only works when concurrentTasksPerWorker is 1, but we set it higher in order to not block all tasks when a plugin is async waiting for an API to return a value.

We can keep track of how long any task takes and if we find something taking too long, we can terminate the entire worker (probably after some PRs to piscina to get this low level access). This would also terminate any other tasks that are blocked inside the worker thread (either also async'ing or just blocked because of a long running sync task that makes them queue up). I'm not sure there's much we can do about that, other than trying our best to retry.

We could/should also have some form of punishment for plugins that exceed resource limits (either time or memory) and prevent them from starting again after restarting the worker.

And all of this should come with tests obviously :).

There's probably more we can do, but these seem to be the lowest hanging fruit.

@mariusandra
Copy link
Collaborator Author

The currently relevant issue #154 is directly connected to this issue. When processEvent gets stuck for long, it takes down the entire build pipeline. In the case of #154 it's probably due to an uncaught rejected promise, but it could be something else. Thus we need to implement some timeouts.

Here's an interesting and relevant article: https://medium.com/@bvjebin/js-infinite-loops-killing-em-e1c2f5f2db7f

@mariusandra mariusandra mentioned this issue Feb 15, 2021
2 tasks
@mariusandra
Copy link
Collaborator Author

A lot of the common CPU-based attacks got handled with PR #155. Out of memory errors are still possible though.

@mariusandra
Copy link
Collaborator Author

I made a new issue #159 against out of memory problems. Unless we discover something new, this issue is rather resolved.

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

No branches or pull requests

2 participants