Skip to content

timeout isn't safe. Potential DDOS issue #20

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

Closed
exx8 opened this issue Mar 19, 2021 · 1 comment
Closed

timeout isn't safe. Potential DDOS issue #20

exx8 opened this issue Mar 19, 2021 · 1 comment

Comments

@exx8
Copy link

exx8 commented Mar 19, 2021

I've read a bit of the code.
And I believe I've found a vulnerability:
For example consider this:

const { StaticPool } = require("node-worker-threads-pool");

const staticPool = new StaticPool({
  size: 1,
  task: (n) => {while(n){console.log("a");}
return n;
},
timeout:10
});
staticPool.exec(0).then((result) => {
  console.log("result from thread pool:", result); // result will be 0.
});
staticPool.exec(1).then((result) => {
  console.log("result from thread pool:", result); //will never return
});
staticPool.exec(0).then((result) => {
  console.log("result from thread pool:", result); // will never be executed
});

The worker thread is now will be hijacked till node restart in the malicious task which never ends.
Moreover the library never says to the user that something went wrong.
The process will keep running, with the exhausted poll (the attacker only need to make sure that he sends as much tasks as available workers).
In dynamic pool, attacker might cause a server CPU runout.
For summary:
The user which tries to protect its process from an expensive task taking down its service, might be tricked to use timeout feature, which supposedly guarantees that this never happens.
The attacker only need to find a case where task is expensive to compute (for example an edge case of regex).
The threads are hijacked to the expensive case, leading to a potential DDOS.
The user is never informed that something went wrong, while the pool never executes following tasks.

@SUCHMOKUO SUCHMOKUO added bug Something isn't working and removed bug Something isn't working labels Mar 23, 2021
@SUCHMOKUO
Copy link
Owner

SUCHMOKUO commented Mar 23, 2021

@exx8 Hi, thank you for commenting. There is no timeout setting on the options of StaticPool, user should use the createExecutor api to set timeout for each task. code below works:

const { StaticPool } = require("node-worker-threads-pool");

const staticPool = new StaticPool({
  size: 1,
  task: (n) => {
    while (n) {
      console.log("a");
    }
    return n;
  }
});

staticPool.exec(0).then((result) => {
  console.log("result from thread pool:", result);
});

staticPool.createExecutor().setTimeout(10).exec(1).then((result) => {
  console.log("result from thread pool:", result);
}).catch(() => console.error('timeout'));

staticPool.exec(0).then((result) => {
  console.log("result from thread pool:", result);
});

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

No branches or pull requests

2 participants