wilk / microjob

A tiny wrapper for turning Node.js worker threads into easy-to-use routines for heavy CPU loads.
https://wilk.github.io/microjob/
MIT License
2.02k stars 47 forks source link

Security issue: injecting any JS code #2

Closed awto closed 6 years ago

awto commented 6 years ago

If context variable value is received from its remote user (e.g. HTML form), and I would expect this will happen often, it is easy to execute any JS expression:

Here's your example (a bit reduced):

(async () => {
  const { job } = require('microjob')
    // this function will be executed in another thread
  const counter = 1000000
  const res = await job(() => {
    return counter
  }, {ctx: {counter:process.env.DATA_FROM_USER}})

})()

Now running it with:

$ DATA_FROM_USER="Z';console.log(\"hi\");'" node  --experimental-worker .\test.js
hi

DATA_FROM_USER here simulates remote data

wilk commented 6 years ago

Very good catch, thank you @awto !

Yes, this is a real security issue. microjob needs to sanitize the context before evaluating it but I've no solution for it right now.

Any idea to implement it is welcome, of course!

awto commented 6 years ago

I think the best solution is to await a message after worker starts, and send the data in that message

wilk commented 6 years ago

Unfortunately, this cannot be done via message passing. In fact, worker threads use v8.serialize function (https://nodejs.org/api/v8.html#v8_v8_serialize_value) and elements like functions and classes are not allowed.

With "ctx" I want to give the capability to emulate the current context inside the anonymous function passed to the job.

So, I'll just mention the thing inside the docs for now.

awto commented 6 years ago

I haven't noticed you want indeed pass functions there, in that case, some strong warning in docs will be enough.

I actually have a transpiler to make functions serializable including closure captured values, shared references etc, no docs yet though.

wilk commented 6 years ago

improved docs here: https://github.com/wilk/microjob/commit/1e10c998d7002946f7a1b4b20eb388afb3e89a99 I'm closing this issue but I'll reopen it if someone finds a better solution.