Skip to content

Disable __proto__ #31951

Closed
#32279
Closed
Disable __proto__#31951
#32279
Labels
securityIssues and PRs related to security.

Description

@mcollina
mcollina
opened on Feb 25, 2020

There have been quite a few CVE related to __proto__ in the last while. I think it would be good to have a flag to enable/disable it.

A quick example:

const payload = '{"__proto__": null}'
const a = {}
console.log("Before : " + a) // this works
Object.assign(a, JSON.parse(payload))
console.log("After : " + a) // this crashes

(It's not strictly related to JSON, as it can also apply to multipart data or other serialization format).

Some vulnerabilities:

I don't know if this is fixable / manageable on our side (vs V8), but __proto__ still causes significant vulnerabilities.

Note that there are some modules to help with this, including https://github.com/hapijs/bourne.

Activity

  1. mcollina
    added
    securityIssues and PRs related to security.
    on Feb 25, 2020

  2. mcollina commented on Feb 25, 2020

    @mcollina
    mcollina
    on Feb 25, 2020
    SponsorMemberAuthor

    cc @nodejs/tsc @nodejs/security @nodejs/v8

  3. hueniverse commented on Feb 25, 2020

    @hueniverse
    hueniverse
    on Feb 25, 2020

    From a security / dev perspective, there really is no reason not to disable the getter/setter for __proro__ on Object. If this is done in a major breaking release, modules that use it can easily move to better ways of manipulating the prototype.

    The core issue here is that even the most security-aware developers still get caught not thinking about it when using external inputs. It's impossible to completely seal internal logic from some external input coming in that contains __proto__ directly or after some string manipulation.

    I am seriously considering deleting the getter/setter when hapi loads and being done with it, letting any other non-hapi module that requires it to fail and get people to fix it. But this would be much better dealt with for everyone at the node core level (or v8). It's just a getter/setter on the Object prototype.

  4. devsnek commented on Feb 25, 2020

    @devsnek
    devsnek
    on Feb 25, 2020
    Member

    There are two parts to __proto__:

    • Object.prototype.__proto__
    • Object literals with __proto__

    The second one is extremely popular for creating object literals with a null prototype, so I doubt we could disable it. I don't think the Object.prototype.__proto__ has anywhere near as much usage, but I would guess it is still a lot more than we could deal with.

  5. jasnell commented on Feb 25, 2020

    @jasnell
    jasnell
    on Feb 25, 2020
    Member

    I'm not convinced there's anything reasonable we can do at the Node.js level here by default without causing massive backwards compat issues. I wouldn't be opposed to running an experiment tho so see if that's wrong.

  6. ljharb commented on Feb 25, 2020

    @ljharb
    ljharb
    on Feb 25, 2020
    SponsorMember

    It's part of JS; I don't think node dot JS should ever be in the position of disabling parts of the language, even with a flag.

    (also most of these vulnerabilities are due to doing dangerous things with unsanitized user input, which is a bad practice that causes problems with or without __proto__)

  7. bakkot commented on Feb 25, 2020

    @bakkot
    bakkot
    on Feb 25, 2020
    Contributor

    Drive-by comment:

    I don't know if this is fixable / manageable on our side

    For the vulnerabilities listed, which are about the setter on Object.prototype, I think it would be straightforward for Node to just delete that setter during startup when the flag was set. (Possibly it would be a little more complicated to propagate that behavior to new contexts; I lack the relevant knowledge of the internals.)

    Object.defineProperty(Object.prototype, '__proto__', { set: void 0 });

  8. mscdex commented on Feb 25, 2020

    @mscdex
    mscdex
    on Feb 25, 2020
    Contributor

    IMO there isn't anything node can/should do about this. Developers just need to implement better patterns. The typical scenario I've seen that relates to this kind of issue is people using {} as a data storage mechanism instead of a Map or Object.create(null), both of which have been available for quite some time now and are more suitable for storing arbitrary data.

    Additionally, I'm not entirely sure why someone would be attempting the kind of operation in the OP ('string' + nullProtoObj). Outside of edge cases like that, objects with null prototypes work just fine, even when calling JSON.stringify():

    > JSON.stringify({ foo: Object.create(null) })
'{"foo":{}}'

  9. vdeturckheim commented on Feb 25, 2020

    @vdeturckheim
    vdeturckheim
    on Feb 25, 2020
    Member

    For context, here is a real life RCE in Kibana using prototype pollution: https://research.securitum.com/prototype-pollution-rce-kibana-cve-2019-7609/

  10. bmeck commented on Feb 25, 2020

    @bmeck
    bmeck
    on Feb 25, 2020
    Member

    I know we have --frozen-intrinsics and --experimental-policies, perhaps we could make a more cohesive single set of defaults instead of a flag for each. This would be similar to the difficulty TypeScript faces with all its flags, and strict mode for TS opts into the desired but breaking defaults.

  11. kanongil commented on Feb 26, 2020

    @kanongil
    kanongil
    on Feb 26, 2020
    Contributor

    I recently looked into the feasibility of doing a delete Object.prototype.__proto__ for my serverside projects. It can work, but it is definitely still in use. This was evidenced when I tried to benchmark if it caused any changes in performance, and the benchmark tool failed.

    It's part of JS; I don't think node dot JS should ever be in the position of disabling parts of the language, even with a flag.

    It's not really, though. At least, as it is currently specced in the optional for non-browsers section.

    There are two parts to __proto__:

    • Object.prototype.__proto__
    • Object literals with __proto__

    The second one is extremely popular for creating object literals with a null prototype, so I doubt we could disable it. I don't think the Object.prototype.__proto__ has anywhere near as much usage, but I would guess it is still a lot more than we could deal with.

    The second form could still be valid, and is not a security concern, as you can't accidentally get user input into a literal.

  12. guybedford commented on Feb 26, 2020

    @guybedford
    guybedford
    on Feb 26, 2020
    Contributor

    Note that the specific JSON issue quoted in the issue is blocked when enabling the --frozen-intrinsics flag, as it is not possible to write to any builtin __proto__ properties with this.

    Edit: it's just --frozen-intrinsics

  13. Jack-Works commented on Feb 26, 2020

    @Jack-Works
    Jack-Works
    on Feb 26, 2020

    It's part of JS; I don't think node dot JS should ever be in the position of disabling parts of the language, even with a flag.

    (also most of these vulnerabilities are due to doing dangerous things with unsanitized user input, which is a bad practice that causes problems with or without __proto__)

    It's not part of JS. __proto__ is defined in section B and Node.JS is not a browser so it's safe even __proto__ is not implemented🤔

    When the ECMAScript host is a web browser the following additional properties of the standard built-in objects are defined. (section B)

  14. kanongil commented on Feb 26, 2020

    @kanongil
    kanongil
    on Feb 26, 2020
    Contributor

    The --frozen-intrinsics flag doesn't really work. It does prevent the more serious issue, where the global prototype is modified. It does not prevent changing an existing object prototype through assignment to the obj.__proto__ property itself.

  15. ljharb commented on Feb 26, 2020

    @ljharb
    ljharb
    on Feb 26, 2020
    SponsorMember

    In practice, most of Annex B is not really optional if you want to write or use portable code, but yes, that would be the loophole that would allow node to remove it, using the method described upthread.

  16. 24 remaining items

  17. devsnek
    added a commit that references this issue on Mar 18, 2020

    lib: add option to disable __proto__

    7a742ec
  18. devsnek
    closed this as completedin 7a742econ Mar 18, 2020
  19. MylesBorins
    added 2 commits that reference this issue on Mar 19, 2020

    lib: add option to disable __proto__

    7a2400d

    lib: add option to disable __proto__

    36ba54e
  20. targos
    added 2 commits that reference this issue on Apr 25, 2020

    lib: add option to disable __proto__

    dafa9c7

    lib: add option to disable __proto__

    b598321

  21. GrosSacASac commented on Jan 11, 2021

    @GrosSacASac
    GrosSacASac
    on Jan 11, 2021
    Contributor

    What is the state of this in v15.5.1 Current. As I understand the setter has been disabled ?

  22. bmeck commented on Jan 11, 2021

    @bmeck
    bmeck
    on Jan 11, 2021
    Member

    @GrosSacASac it is enabled by default but can be disabled via CLI flag

  23. ChALkeR commented on Jan 26, 2021

    @ChALkeR
    ChALkeR
    on Jan 26, 2021
    Member

    I believe babel/babel#12693 should fix the babel side, that was the last part in babel codebase that generated __proto__-first code afaik.

  24. GrosSacASac commented on Jan 26, 2021

    @GrosSacASac
    GrosSacASac
    on Jan 26, 2021
    Contributor

    Since what version ? I could not find it in the changelogs ?

  25. ChALkeR commented on Jan 26, 2021

    @ChALkeR
    ChALkeR
    on Jan 26, 2021
    Member

    @GrosSacASac If you are speaking about Babel fix — that PR I linked is not merged yet.

    That is, that PR only affects loose: true class transforms.
    Without loose mode, Babel was already fine since babel/babel#7675 (which was landed in v7.0.0) afaik.

  26. GrosSacASac commented on Jan 26, 2021

    @GrosSacASac
    GrosSacASac
    on Jan 26, 2021
    Contributor

    Sorry, I was asking about Node

  27. targos commented on Jan 26, 2021

    @targos
    targos
    on Jan 26, 2021
    Member

    --disable-proto was added in v13.12.0 and backported to v12.17.0: https://nodejs.org/dist/latest-v15.x/docs/api/cli.html#cli_disable_proto_mode

  28. GrosSacASac
    added a commit that references this issue on Jan 26, 2021

    chore: up node version

    f16ce74

  29. ChALkeR commented on Feb 3, 2021

    @ChALkeR
    ChALkeR
    on Feb 3, 2021
    Member

    Babel v7.12.13 should be fine even in loose mode.
    Outside of loose mode, anything from v7.0.0 onwards appears to fine afaik.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    securityIssues and PRs related to security.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions