Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal #56531

Open
vdata1 opened this issue Jan 9, 2025 · 6 comments · May be fixed by #56625
Open

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal #56531

vdata1 opened this issue Jan 9, 2025 · 6 comments · May be fixed by #56625
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. good first issue Issues that are suitable for first-time contributors.

Comments

@vdata1
Copy link

vdata1 commented Jan 9, 2025

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
Copy link
Member

joyeecheung commented Jan 10, 2025

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.

@joyeecheung joyeecheung added good first issue Issues that are suitable for first-time contributors. c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Jan 10, 2025
@cristianstaicu
Copy link

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
Copy link
Member

joyeecheung commented Jan 13, 2025

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 joyeecheung closed this as not planned Won't fix, can't repro, duplicate, stale Jan 13, 2025
@joyeecheung joyeecheung reopened this Jan 13, 2025
@joyeecheung
Copy link
Member

(github button mistakes)

@cristianstaicu
Copy link

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
Copy link

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. 😄

@jazelly jazelly linked a pull request Jan 16, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants