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

Object.freeze(NODEJS_BUILTIN_GLOBAL_AND_ITS_PROTOTYPE) may lead to crash #45336

Closed
loynoir opened this issue Nov 6, 2022 · 3 comments · Fixed by #45344
Closed

Object.freeze(NODEJS_BUILTIN_GLOBAL_AND_ITS_PROTOTYPE) may lead to crash #45336

loynoir opened this issue Nov 6, 2022 · 3 comments · Fixed by #45344
Labels
confirmed-bug Issues with confirmed bugs.

Comments

@loynoir
Copy link

loynoir commented Nov 6, 2022

Version

v18.12.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

Protect from prototype pollution inspired by https://github.com/snyk-labs/nopp/blob/main/index.js

reproduce.mjs

import globals from "globals";

for (const k of [...new Set(Object.values(globals).map(x => Object.keys(x)).flat())]) {
  if (k in globalThis) {
    const v = globalThis[k]
    try { Object.freeze(v) } catch { }
    try { Object.freeze(v.prototype) } catch { }
  }
}

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

No response

What is the expected behavior?

No error.

What do you see instead?

$ node
Welcome to Node.js v18.12.0.
> void await import('./reproduce.mjs')
Uncaught TypeError: Cannot delete property 'crypto' of #<Object>
> crypto
Uncaught TypeError: Cannot delete property 'crypto' of #<Object>
    at get (node:internal/modules/cjs/helpers:181:23)
> globalThis.crypto
Uncaught TypeError: Cannot delete property 'crypto' of #<Object>
    at get (node:internal/modules/cjs/helpers:181:23)

Additional information

No response

@loynoir loynoir changed the title Uncaught TypeError: Cannot delete property 'crypto' of #<Object> prototype protection Nov 6, 2022
@aduh95
Copy link
Contributor

aduh95 commented Nov 6, 2022

I don't understand you are asking, are you:

  • reporting an instance where prototype pollution makes Node.js crash, in which case, can you share the content of reproduce.mjs?
  • Asking for Node.js to freeze the built-in objects, in which case, have you heard of --frozen-intrinsics?

@loynoir
Copy link
Author

loynoir commented Nov 6, 2022

@aduh95

  1. Below is reproduce.mjs which is modified from snyk-labs/nopp
import globals from "globals";

for (const k of [...new Set(Object.values(globals).map(x => Object.keys(x)).flat())]) {
  if (k in globalThis) {
    const v = globalThis[k]
    try { Object.freeze(v) } catch { }
    try { Object.freeze(v.prototype) } catch { }
  }
}
  1. Ah, just know --frozen-intrinsics now.

@loynoir loynoir changed the title prototype protection Object.freeze(NODEJS_BUILTIN_GLOBAL_AND_ITS_PROTOTYPE) may lead to crash Nov 6, 2022
@aduh95
Copy link
Contributor

aduh95 commented Nov 6, 2022

Here's a smaller repro:

$ node --no-experimental-global-webcrypto --use-strict -e 'Object.defineProperty(global, "crypto", { configurable: false });crypto'
node:internal/modules/cjs/helpers:181
        delete object[name];
                      ^

TypeError: Cannot delete property 'crypto' of #<Object>
    at get (node:internal/modules/cjs/helpers:181:9)
    at [eval]:1:66
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:307:38)
    at node:internal/process/execution:83:21
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:82:62)
    at evalScript (node:internal/process/execution:104:10)
    at node:internal/main/eval_string:50:3

Node.js v20.0.0-pre

@aduh95 aduh95 added the confirmed-bug Issues with confirmed bugs. label Nov 6, 2022
aduh95 added a commit to aduh95/node that referenced this issue Nov 6, 2022
nodejs-github-bot pushed a commit that referenced this issue Nov 17, 2022
Fixes: #45336
PR-URL: #45344
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this issue Nov 21, 2022
Fixes: #45336
PR-URL: #45344
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Nov 23, 2022
Fixes: nodejs#45336
PR-URL: nodejs#45344
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
Fixes: #45336
PR-URL: #45344
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
Fixes: #45336
PR-URL: #45344
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
Fixes: #45336
PR-URL: #45344
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this issue Jan 4, 2023
Fixes: #45336
PR-URL: #45344
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants