2

I took a look at one of the rules from eslint-plugin-security and found that user input could in theory lead to a Remote Code Execution bug.

const a = class {};
console.log(a['constructor']);
a['constructor']('console.log(1)')();

function b() {}
console.log(b['constructor']);
b['constructor']('console.log(2)')();

const c = {}
console.log(c['constructor'])
console.log(c['constructor']('console.log(3)')());

From the snippet it's easy to see that constructors of classes and functions seem to parse strings and evaluate them as valid code. Objects for some reason don't exhibit this behaviour.

Why is this even allowed to happen? What feature of JavaScript needs this behaviour from function/class constructors? I'm assuming it's integral to the way JavaScript works otherwise I don't see why it hasn't been removed from the language.

Louis Christopher
  • 308
  • 3
  • 17
  • 1
    Because `a['constructor'] === Function`. You're calling `Function('console.log(1)')` and then executing it. Just in a roundabout way. – VLAZ Feb 07 '20 at 12:18
  • Also, I'm not clear - how would user input make it to there? It certainly *could* but it would be very strange - you have to trust the user to know what method they want, so you fetch it dynamically (user supplies `"constructor"`) and then you also have to trust the user knows what value that method takes and just give it without ever checking it (user supplies `'console.log(1)'` here). Oh, and you also *execute the result*. The set of circumstances that allow a user to execute arbitrary code mean there is a bigger problem at hand. – VLAZ Feb 07 '20 at 12:23

1 Answers1

6

The problem is that the .constructor of a class is Function, and calling the Function constructor with a string creates a function from that string, and then calling that function results in the string's code being executed:

const a = class {};
console.dir(a['constructor'] === Function);
a['constructor']('console.log(1)')();

This isn't really any different from

Function('console.log(1)')();

It's just that a class's constructor property happens to point to the same thing.

Objects can exhibit the same property, if you navigate up to the .constructor property twice (first one accesses the Object constructor, second accesses the Function constructor):

const a = {};
console.dir(a['constructor'].constructor === Function);
a['constructor'].constructor('console.log(1)')();

If you allow arbitrary access to properties of any object, and also allow those properties to be called with arbitrary arguments, pretty much anything can be executed. The prototype chain (and .constructor) properties are useful, but like many things, they can be misused.

CertainPerformance
  • 356,069
  • 52
  • 309
  • 320
  • "*If you allow arbitrary access to properties of any object, and also allow those properties to be called with arbitrary arguments, pretty much anything can be executed*" like I mentioned on the question - I'm really not clear how that's a security concern. Not that people *can't* abuse that if you're allowing them free access to run arbitrary code but there are multiple failed steps to even get to that point. So, if your app allows arbitrary code execution *in this fashion*, you've already failed before even getting to here. Patching just this very likely leaves you with other problems. – VLAZ Feb 07 '20 at 12:29
  • @CertainPerformance Can we theoretically change this behaviour of `Function`? I know it's part of the language but I'm finding it hard to understand exactly why it exists. Can't we just change this one behaviour of `Function` and have no problems arise as a result? I'm not sure if I should ask this as a new question or continue further discussion here – Louis Christopher Feb 08 '20 at 04:46
  • Yes, you *could* pretty easily overwrite `window.Function`. The need to create a function dynamically from a string is *pretty rare*. Overwriting built-in prototypes is [rarely a good idea though](https://stackoverflow.com/q/6223449) - if you're at the point that you want to remove `window.Function`, it'd probably be better to take a step back and reconsider the entire approach. Overwriting `window.Function` might not be a good idea, but it is quite easy to do, at least – CertainPerformance Feb 08 '20 at 04:49
  • I agree with you that overwriting built-in prototypes is a bad idea. This indeed is a rare edge case and I don't have code that actually does this. I'm just trying to understand why someone from EcmaScript hasn't proposed removal of this behaviour given how niche and unnecessary it actually is. If someone really needs to create a function dynamically from a string they can always use `eval` or have another `createFunctionFromString` that isn't part of `Function` – Louis Christopher Feb 08 '20 at 04:55
  • 2
    The function constructor is a bit better then eval, and has some (rare) uses: https://stackoverflow.com/q/3026089 *or have another createFunctionFromString that isn't part of Function* Thought that Function and eval were the only ways to do something like that, at least in vanilla JS (environments can provide weird alternative ways...) – CertainPerformance Feb 08 '20 at 05:00