FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal

[vdata1]

Version

v23.6.0

Platform

Linux SMP Debian 5.10.103-1 (2022-03-07) x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Hi,

I would like to report a bug, it can be reproduced by running the PoC below:

const {exec} = require('child_process');

Object.defineProperty(Array.prototype, "2", {
  set: function () {},
});

(async function () {
  exec('pwd', (err, stdout, stderr) => {
    console.log(stdout);
  });
})();

Regards,

AH

How often does it reproduce? Is there a required condition?

It reproduces anytime by simply running the given PoC on the given Node.js version.

What is the expected behavior? Why is that the expected behavior?

It is a crash.

What do you see instead?

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal

Additional information

No response

[joyeecheung]

In general we provide no guarantee about the usability of the process once builtin prototypes like this are modified, so it's not technically a bug, but poor UX at worst. It would be a UX improvement to at least not crash, if it's not too invasive a change to defend against it.

Marking as good first issues, though just for those who are willing to debug the C++ internals.

[cristianstaicu]

Thanks for the clarification! Imo, crashing the runtime like this is always bad. We had a discussion a while back about supporting the execution of untrusted code in Node.js: #40718. While not necessary true for this particular payload shared by @vdata1 because of the privileged child_process API call, but imagine a use case like Cloudflare's Workers (https://developers.cloudflare.com/workers/reference/security-model/) in which mutually untrusted, low-privilege code share the same process. Attackers can crash co-located Isolates with such payloads. What is the team's view on this? Are these assumptions well documented somewhere?

[joyeecheung]

but imagine a use case like Cloudflare's Workers in which mutually untrusted, low-privilege code share the same process.

You are referencing cloudflare's security model, which differs from the Node.js threat model.

Node.js trusts everything else. Examples include:
...
The code it is asked to run, including JavaScript, WASM and native code, even if said code is dynamically loaded, e.g., all dependencies installed from the npm registry. The code run inherits all the privileges of the execution user.

Like vm, child_process is just another regular API that should not be accessible to untrusted code. If a Node.js application allows code provided by a potential attacker to run in the process, the security risk is on them, not on Node.js. Using Node.js for an architecture similar to cloudflare's worker is in itself, a design with security flaws, as Node.js was never built with this security model in mind and after >10 years of organic growth, it would have countless loopholes even if one wants to start amending it for this model now, so it's already a lost cause.

[joyeecheung]

(github button mistakes)

[cristianstaicu]

Wow, that is a very bold claim in Node.js' threat model! "Node.js trusts everything else. Examples include: ... The code it is asked to run." 😄 Sure, I am aware that Cloudflare runs on a dedicated runtime, not on Node.js, that is why I said "a use case like". Nonetheless, many of us thought that running untrusted code inside a worker or something like isolated-vm is probably safe (see the discussion I linked above). However, that is only the case if the runtime is crash-resilient against arbitrary code, which you say the team does not aim to guarantee. I now see that the isolated-vm project is struggling exactly with this type of issues: https://github.com/laverdet/isolated-vm?tab=readme-ov-file#wishlist

[cristianstaicu]

Thanks for the updated comment above. If I understand correctly, it is the combination of a privileged/Node.js API with a crash that makes this a low-priority issue, then. If the same effect can be obtained by a modified prototype like above with something like Math.log(), that would be more interesting for you guys; but, then, we would likely need to take such issues to the v8 team directly. 😄