Vulnerability

[saltimbanc]

You can escape the sandbox using something like this:

const compiler = require('@nx-js/compiler-util');

const code = compiler.compileCode('} { return global.process.env.LOGNAME');
const user = code({});
console.log(user);

[mgesmundo]

Hi,
as possible workaround you could clean the source string before to put into the compiler:

const compiler = require('@nx-js/compiler-util');

const sourceCode = '} { return global.process.env.LOGNAME';
const cleanSourceCode = sourceCode.replace(/^([\n\r\t\s;]*}[\n\r\t\s;]*{)*/, '').trim();
const code = compiler.compileCode(cleanSourceCode);
const user = code({});
console.log(user);

[saltimbanc]

@mgesmundo That will not be enough, you could use '1 + 1 } { return global.process.env.LOGNAME' (or any other preceding valid JavaScript before the first closing curly bracket) to bypass it.
To prevent this, you'll have to parse all the code and balance curly brackets.

[mgesmundo]

@saltimbanc this updated RegEx /^([\w\W\d\s]*}.*{)*/ removes all code before the dangerous couple of curly brackets } {.

[natarius]

Would transpiling user code from typescript to JavaScript help mitigate such attacks?

[saltimbanc]

@mgesmundo That RegExp matches valid code, something like function one(){return "something"}function two(){return "another thing"}, also, you can break it by using a new line between the opening and closing brackets, }\n{ because . doesn't match new lines if you don't add the s flag to the RegExp.

I don't think you can fix this with regex, you could use /^([^{]*}.*{)*/gms but even this can be broken by '{'}{, you could extend the regex by making sure the opening curly bracket is not inside a string literal but I don't know if there is any reliable way to do that in RegExp.

RegExp is not my forte, I think you could check for { characters inside string literals with something like this: /^((('|`|"){\1)|[^{]*}.*{)*/gms but you can break it using '\'{'}{ (escaping the quote inside the literal).

[saltimbanc]

@natarius Probably. This exploit uses invalid syntax so I guess the code will not be transpiled.

[natarius]

yeah, seems like typescript transpiling catches this!

here are the options I used:

const compilerOptions = {
            allowJs: true,
            target: ts.ScriptTarget.ES2020,
            lib: ["ES2020"],
            module: ts.ModuleKind.CommonJS,
            noEmitOnError: true,
        };


         const jsCode = ts.transpileModule(tsCode, {compilerOptions});
         return jsCode.outputText;

[mgesmundo]

The TipeScript solution is interesting. The updated RegEx /^([\w\W\d\s]*}[\w\d\s\n\r\t]*{)*/ cover the @saltimbanc issue. My previous RegEx was wrong because the . handles every chars, included the bracket. The vulnerability depends on the first closed bracket followed by an opened bracket. All code before this first (closed) bracket and the second (opened) should be removed anyway. The code after the second opened bracket will be safe again. I don't see IMHO any reason to keep some valide code inside a dangerous bracket couple } { or before the first one only.
It would be interesting to write some test case.

[natarius]

@mgesmundo will you commit the regex to the repo?

[mgesmundo]

@solkimicreb do you like a PR with this proposed fix and its tests? Do you have some other solution other than the RegEx and TypeScript?

[saltimbanc]

@mgesmundo You can break that RegExp using "{";}"";{
Full example: "{";}"";{ return global.process.env.LOGNAME

[mgesmundo]

@saltimbanc For my needs I use an async wrapper:

const compiler = require('@nx-js/compiler-util')

const sleep = (milliseconds) => {
  return new Promise(resolve => setTimeout(resolve, milliseconds))
}

compiler.expose = [ 'console', 'Promise' ]

const test = async (src) => {
  // Fix vulnerability: https://github.com/nx-js/compiler-util/issues/12
  const cleanSrc = src.replace(/^([\w\d\s\n\r\t]*}[\w\d\s\n\r\t]*{)*/, '').trim();
  const source = `
    const $wrapperFunction = async () => {
      ${cleanSrc}
    }
    return $wrapperFunction()
  `

  const code = compiler.compileCode(source);

  return code({}, { sleep });
}

const source1 = `
  } {
  return async ms => {
    await sleep(ms);
    return 'Hello World!';
  }
`
const source2 = `"{";}"";{ return global.process.env.LOGNAME`

const getResult = async () => {
  try {
    const r = await test(source1);
    console.log('>>> result is', await r(1000));
  } catch (e) {
    console.log('>>> invalid source code')
  }
  try {
    const r = await test(source2);
    console.log('>>> result is', await r(1000));
  } catch (e) {
    console.log('>>> invalid source code')
  }
  const code = compiler.compileCode(source2);
  console.log('>>> vulnerable', code({}))
}

getResult();

// after 1s
// >>> result is Hello World!
// >>> invalid source code
// >>> vulnerable marcello

[nerochiaro]

Wouldn't it be possible to prevent this vulnerability by simply changing the "wrapper" to include a return statement ?

Right now what the wrapper does is essentially with (sandbox) { code }.
But it could be changed to with (sandbox) { return code }

This way the code block can't inject a closing curly brace to escape the sandbox and then execute other code, because the other code after the closing brace would never be reached due to the return statement.

Obviously this will change the semantics a bit, because to execute multiple statements you would have to wrap them in an immediately invoked function expression (IIFE), but it sounds like a minor price to pay for having a less vulnerable sandbox.

If the worry is breaking existing code that relies on the existing behavior, a new "safer" method can be added that implements this, and people can choose which one to use (simpler + less safe) or (more boilerplate + safer)

[nerochiaro]

Also wouldn't it be also possible to escape the sandbox by simply getting hold of any function, reaching its constructor (i.e. Function), then using it to create a new un-sandboxed evaluation.
See this for example: https://runkit.com/feralgeometry/5f7dcaf7cc4527001aed3119