3
\$\begingroup\$

As per the title, I'm making a webapp that is intended to use as a JS exercise platform. Problems are shown to users, they submit code, that code is run against a few test cases, and a report of the outcome is given back to the user.

I'm using Django for my backend, but to run the code I have also set up a node script, which is called via subprocess in Django.

The script is given a string containing the user code and a list of assertions. I'm trying to see if the way I structured that script is sound.

Objective: I need to return to Django a list of objects, one for each assertion passed to node, where each object looks like this:

{ 
    id: Number,
    assertion: String,
    public: Boolean,
    passed: Boolean,
    error: String,
} 

Django pretty much needs to know which test cases were passed and which failed.

My idea is the following: in Node, I take each assertion passed by Django, and I turn it into a try ... catch in which I create an object I run that assertion and, if it fails, I collect the error in the object (otherwise I collect the positive outcome), then I push that object to an array which is what I ultimately return. I then take all the try ... catch strings and I inline them after the user code in the code I run in my vm.

As you might imagine, the possible issues with this are all the cases where the user might tamper with the array prototype and whatnot.

So here's my Node code. I'd like some input and feedback on what could be improved, what kind of tampering it might still be vulnerable to, and other ideas as to how I could make it generally safer andb better.

/*
usage: 
node runWithAssertions.js programCode assertions
arguments:
programCode is a STRING containing a js program
assertions is an ARRAY of strings representing assertions made using node assertions
output: 
an array printed to the console (and collected by Django via subprocess.check_output()) where each entry 
corresponds to an assertion and is an object:
{ 
    id: Number,
    assertion: String,
    public: Boolean,
    passed: Boolean,
    error: String,
} 
where id is the id of the assertion (as in the Django database),
assertion is the string containing the assertion verbatim,
public indicates whether the assertion is to be shown to the user or it's secret,
passed represents the outcome of running the assertion on the program,
and error is only present if the assertion failed
*/

// The VM2 module allows to execute arbitrary code safely using a sandboxed, secure virtual machine
const { VM } = require('vm2')
const assert = require('assert')
const AssertionError = require('assert').AssertionError
const timeout = 1000

// instantiation of the vm that'll run the user-submitted program
const safevm = new VM({
  timeout, // set timeout to prevent endless loops from running forever
  sandbox: {
    prettyPrintError,
    prettyPrintAssertionError,
    assert,
    AssertionError
  }
})


function prettyPrintError (e) {
  const tokens = e.stack.split(/(.*)at (new Script(.*))?vm.js:([0-9]+)(.*)/)
  const rawStr = tokens[0] // error message

  if (rawStr.match(/execution timed out/)) {
    // time out: no other information available
    return `Execution timed out after ${timeout} ms`
  }

  const formattedStr = rawStr.replace(
    /(.*)vm.js:([0-9]+):?([0-9]+)?(.*)/g,
    function (a, b, c, d) {
      return `on line ${parseInt(c) - 1}` + (d ? `, at position ${d})` : '')
    }
  ) // actual line of the error is one less than what's detected due to an additional line of code injected in the vm
  return formattedStr
}

// does the same as prettyPrintError(), but it's specifically designed to work with AssertionErrors
function prettyPrintAssertionError (e) {
  const expected = e.expected
  const actual = e.actual
  const [errMsg, _] = e.stack.split('\n')
  return (
    errMsg +
    ' expected value ' +
    JSON.stringify(expected) +
    ', but got ' +
    JSON.stringify(actual)
  )
}

const userCode = process.argv[2]

const assertions = JSON.parse(process.argv[3])

// turn array of strings representing assertion to a series of try-catch's where those assertions
// are evaluated and the result is pushed to an array - this string will be inlined into the program
// that the vm will run
const assertionString = assertions
  .map(
    (
      a // put assertion into a try-catch
    ) =>
      `
        ran = {id: ${a.id}, assertion: '${a.assertion}', is_public: ${a.is_public}}
        try {
            ${a.assertion} // run the assertion
            ran.passed = true
        } catch(e) {
            ran.passed = false
            if(e instanceof AssertionError) {
                ran.error = prettyPrintAssertionError(e)
            } else {
                ran.error = prettyPrintError(e)
            }
        }
        output_wquewoajfjoiwqi.push(ran)
    `
  )
  .reduce((a, b) => a + b, '') // reduce array of strings to a string

// support for executing the user-submitted program
// contains a utility function to stringify errors, the user code, and a series of try-catch's
// where assertions are ran against the user code; the program evaluates to an array of outcomes
// resulting from those assertions
const runnableProgram = `const output_wquewoajfjoiwqi = []; const arr_jiodferwqjefio = Array; const push_djiowqufewio = Array.prototype.push; const shift_dfehwioioefn = Array.prototype.shift
${userCode}
// USER CODE ENDS HERE
// restore array prototype and relevant array methods in case user tampered with them
Array = arr_jiodferwqjefio
Array.prototype.push = push_djiowqufewio;
Array.prototype.shift = shift_dfehwioioefn;
if(Object.isFrozen(output_wquewoajfjoiwqi)) {
    // abort if user intentionally froze the output array
    throw new Error("Malicious user code froze vm's output array")
}
while(output_wquewoajfjoiwqi.length) {
    output_wquewoajfjoiwqi.shift() // make sure the output array is empty
}
// inline assertions
${assertionString}
// output outcome object to console
output_wquewoajfjoiwqi`

try {
  const outcome = safevm.run(runnableProgram) // run program
  console.log(JSON.stringify({ tests: outcome })) // output outcome so Django can collect it
} catch (e) {
  console.log(JSON.stringify({ error: prettyPrintError(e) }))
}

The full repo of this project is available at https://github.com/samul-1/js-exercise-platform/ if you want to take a look at the whole thing. Advice is welcomed :)

\$\endgroup\$

1 Answer 1

1
+50
\$\begingroup\$

what kind of tampering it might still be vulnerable to

I am working on testing this locally and in the process of getting it set up so I am not sure if these things are actual areas of concern but test tampering with globals like JSON, prettyPrintError , assert, Error, etc... I tried modifying JSON.parse() and was able to change its implementation...

It would be wise to setup unit tests if you haven’t already. Common frameworks like mocha/chai, jest, etc. offer techniques for ensuring exceptions are or aren’t thrown.

The code tests if the variable output_wquewoajfjoiwqi is frozen, but I assigned it to a different value and it threw TypeError: Assignment to constant variable. If the goal is to use a variable not used within the user code, then perhaps it would be wise to generate the hash dynamically - repeating if necessary until the user code does not contain the hash.

Bearing in mind the code is run in a VM instead of a browser, you might be interested to read about how the StackExchange snippets are designed to prevent XSS attacks. I tried to find a list of restrictions for those but haven't found anything comprehensive yet.

Other review points

  • constant style: most style guides recommend constant names be in all caps so anyone reading it can distinguish variables from constants. So instead of:

    const timeout = 1000
    

    use all capitals - e.g.

    const TIMEOUT = 1000 
    
  • repeated require for dependency assertionError: instead of

    const AssertionError = require('assert').AssertionError` 
    

    just use:

    const AssertionError = assert.AssertionError
    

    since the previous line already loaded assert

  • regular expressions class \d can be used in place of [0-9]

  • scope of variable Is ran declared with keyword, or okay as global?

  • arrow function for callback The callback to rawStr.replace() could be simplified to an arrow function

  • assigning ran a in the callback to .map() in assertionString can be stringified with JSON.stringify()- so instead of:

    ran = {id: ${a.id}, assertion: '${a.assertion}', is_public: ${a.is_public}}
    

    just do this:

    const ran = ${JSON.stringify(a)} 
    

    initially I was thinking that destructuring a - i.e. {id, assertion, is_public} since a is never passed in its entirety. Before I realized that ran is within a template literal I was thinking the object initializer shorthand notation could allow for the keys to be omitted - but within the template literal that doesn't seem to make sense.

  • call Array.join() instead of Array.reduce() After the assertions are mapped to an array, instead of calling reduce to join them together to a string, Array.join() can be used.

\$\endgroup\$
2
  • \$\begingroup\$ Excellent answer. The idea of randomly generating the name for the output array is brilliant. One exploit I found this script is vulnerable to is the following code: gist.github.com/samul-1/f4115ad1047bdca6bde66c4f6adbcd2c using this, you can override the push method and cause the script to tell the Django backend you passed all tests. How would you tackle this? One thing I thought of is to create an alias of the array prototype (with a random name as well) and use that for the pushing of outcome objects. I'm open to exploring other paths. Thank you! \$\endgroup\$
    – Samuele B.
    May 2, 2021 at 9:44
  • 1
    \$\begingroup\$ Hmm... I see the original code assigns Array to arr_jiodferwqjefio... I would see if freezing Array would solve that issue. Otherwise there are alternatives to the push method - e.g. elements could be pushed into the array by using the bracket notation: e.g. output_wquewoajfjoiwqi[output_wquewoajfjoiwqi.length] = ran \$\endgroup\$ May 3, 2021 at 18:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.