Security Fix for Arbitrary Code Execution - huntr.dev

[huntr-helper]

https://huntr.dev/users/bbeale has fixed the Arbitrary Code Execution vulnerability 🔨. bbeale has been awarded $25 for fixing the vulnerability through the huntr bug bounty program 💵. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#1
GitHub Issue URL | #12
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/safer-eval/1/README.md

User Comments:

📊 Metadata *

Bounty URL: https://www.huntr.dev/bounties/1-npm-safer-eval

⚙️ Description *

Mitigating potential sandbox breakout by checking user supplied input for statements that might be trying to manipulate system processes.

💻 Technical Description *

After a couple attempts at specifically addressing the RangeError mentioned in the disclosure, I ended up going with a simpler solution of checking the user supplied template literal for statements that might be trying to access system processes that they should not be accessing. This solution involves runInContext() logging a warning to the console and returning undefined if this situation is detected, otherwise it returns vm.runInContext the same as before.

🐛 Proof of Concept (PoC) *

Unit tests based on existing proofs of concept:
https://gist.github.com/JLLeitschuh/609bb2efaff22ed84fe182cf574c023a
#10
#11

    it('tries to breakout', function () {
      const maliciousFunction = function () {
        const f = Buffer.prototype.write
        const ft = {
          length: 10,
          utf8Write () {

          }
        }
        function r (i) {
          var x = 0
          try {
            x = r(i)
          } catch (e) {}
          if (typeof (x) !== 'number') { return x }
          if (x !== i) { return x + 1 }
          try {
            f.call(ft)
          } catch (e) {
            return e
          }
          return null
        }
        var i = 1
        while (1) {
          try {
            i = r(i).constructor.constructor('return process')()
            break
          } catch (x) {
            i++
          }
        }
        return i.mainModule.require('child_process').execSync('id').toString()
      }

      const evil = `(${maliciousFunction})()`
      const res = saferEval(evil)
      assert.strictEqual(res, undefined)
    })

    it('tries to breakout another way', function () {
      const anotherMaliciousFunction = function () {
        const process = clearImmediate.constructor('return process;')()
        return process.mainModule.require('child_process').execSync('whoami').toString()
      }
      const evil = `(${anotherMaliciousFunction})()`
      const res = saferEval(evil)
      assert.strictEqual(res, undefined)
    })

    it('tries to break out yet another way using setInterval', function () {
      const code = "setInterval.constructor('return" +
        " process')().mainModule.require('child_process').execSync('whoami').toString();"
      const res = saferEval(code)
      assert.strictEqual(res, undefined)
    })

    it('tries to break out yet another way using setInterval', function () {
      const code = "Buffer.of.constructor('return" +
        " process')().mainModule.require('child_process').execSync('whoami').toString();"
      const res = saferEval(code)
      assert.strictEqual(res, undefined)
    })

🔥 Proof of Fix (PoF) *

[commenthol]

Thanks for your contribution. I really honor your efforts.
Nonetheless a breakout from the VM is still possible. The only limitation is that child_process is somehow prevented to be required.
If someone tries to obfuscate require("child_process") (greetings from white-space or brain fuck) or uses ES5 import statement together with node@>=10 experimental flags we will be facing the same issue again.