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

New Strategic Initiative on Primordials #1439

Open
benjamingr opened this issue Sep 13, 2023 · 16 comments
Open

New Strategic Initiative on Primordials #1439

benjamingr opened this issue Sep 13, 2023 · 16 comments

Comments

@benjamingr
Copy link
Member

benjamingr commented Sep 13, 2023

Primordials are a way we use to add robustness to certain areas in our code and there has been a lot of debate about whether or not the issues they create regarding performance and ease of contribution are worth it in different contexts.


Context: #1438 nodejs/node#18795


I'd like to proposes a (ideally short lived) new strategic initiative around deciding their status in the project.

I'm happy to drive this as a strategic initiative if there is interest from the TSC with the following format:

  • We (all members invited) will meet a few times to write use cases and then derive tests from those use cases that will run during CI to enforce we don't break any primordials guarantees (e.g exit handler) . We will try to keep the scope small.
  • We will add a warning to the docs outlining the robustness model regarding primordials and what guarantees Node makes. We will make a recommendation and an official "Node wants X" for TC39 discussion.
  • We will recommend where to enforce primordials and where not to enforce them and what the PR flow around them should be.
  • The TSC will approve every step above either by consensus (good) or by vote (bad).
  • All this should happen in the timespan of a month or two, ideally. We rush nothing, hear everyone out and adapt/amend when needed.

I would really like it if collaborators who have spent a lot of time thinking about this participate, namely @aduh95 , @ronag @joyeecheung and @anonrig but there are a lot of others here like @isaacs and @ljharb that are welcome too weigh in and participate.

I think our opinions don't differ that much honestly and I think once we iron out the specifics we can come to a good resolution.

@benjamingr
Copy link
Member Author

benjamingr commented Sep 13, 2023

Stuff to do once the TSC approves, ideally:

  • create a "primordials-use-cases" repository like we did for https://github.com/nodejs/promise-use-cases to help write use cases down we think are important.
  • create a "primordials" team and send a doodle for a meeting to discuss the use cases
    • have a discussion around the use cases we believe are critical and bring those to the TSC
    • create pull requests for said use cases
    • write/edit a recommendation around primordials
    • create a docs PR regarding what Node.js explicitly does not guarantee
  • build consensus around primordials in the code:
    • can we all agree that primordials are not a security feature?
    • can we all agree robustness is important and should be prioritized for certain use cases?
    • can we all agree that in certain places primordials create a contribution burden on some people?
    • can we discuss what we want to ask of TC39 and/or V8 around primordials?
    • communicate our recommendation to the TSC in order to have the TSC agree to it.

It's really important to me that people feel safe to participate in this so I ask that people make an even bigger effort to come with an open mind to discuss this in good faith for the benefit of the Node.js project and its users.

@ljharb
Copy link
Member

ljharb commented Sep 13, 2023

Primordials are sometimes a security feature - just not always.

I doubt anyone would argue that primordials create no contribution burden :-)

@benjamingr
Copy link
Member Author

Primordials are sometimes a security feature - just not always.

"sometimes security" is no security IMO. I feel strongly that given our current threat model primordials do not provide any additional security guarantees without extensive static escape analysis that shows that the code is actually "safe" and tooling to enforce that on dependencies.

I think it would be a lot more productive to focus on robustness and explicitly make security a non-goal since it would be a lot easier to build consensus around that and define that clearly. A use case describing something like "If a prototype is modified and an error is thrown - the uncaught error handler still fires" is measurable, enforceable and easy to discuss. We simply don't have a way to do that with "security".

@joyeecheung
Copy link
Member

joyeecheung commented Sep 13, 2023

A strategic initiative SGTM but I wonder if we should concentrate on primordials - primordials are means to achieve robustness against prototype pollution, not the ends. Perhaps it can be named "prototype pollution" instead, and there are a range of solutions to this end - testing, using primordials, moving to native, some TC39 proposal (e.g. https://github.com/tc39/proposal-symbol-proto) to enhance the language, etc. And the goal of the initiative is just to find a balance between robustness and maintainability.

Note that primordials aren't the only thing in scope currently even, there's also __proto__: null everywhere and I think they should be included - if primordials aren't always enforced there isn't much point enforcing the null prototype either...speaking of which, URLs are probably where this should be enforced, I remember it could cause security issues there when not used carefully. But that's just URL.

@ljharb
Copy link
Member

ljharb commented Sep 13, 2023

A moat-based defense is appropriate in security; it’s not all or nothing.

@benjamingr
Copy link
Member Author

primordials are means to achieve robustness against prototype pollution, not the ends.

More generally altering builtins and not just prototype pollution, e.g. the user changing Math.max.

there are a range of solutions to this end - testing, using primordials, moving to native, some TC39 proposal (e.g. https://github.com/tc39/proposal-symbol-proto) to enhance the language, etc. And the goal of the initiative is just to find a balance between robustness and maintainability.

Agreed though I'd like to focus on the use of primordials and come up with a recommendation about the other areas (e.g when we should prefer native code, what we want from TC39 etc). I'm not sure what "testing" is here though.

Note that primordials aren't the only thing in scope currently even, there's also proto: null everywhere and I think they should be included - if primordials aren't always enforced there isn't much point enforcing the null prototype either

I'm not sure why since:

  • __proto__: null and Object.craete(null) generally don't create nearly as big of an issue (compared to lets say, promise chains) with primordials. It's a lot less work and a much smaller maintainability burden.
  • Primordials, __proto__: null and a bunch of other stuff should be enforced IMO in very specific cases where said trade off is "worth it". Additionally to __proto__: null we also need to do a bunch of other checks (e.g. guard against getters that throw in user-created objects).

The topic is "primordials" and not "robustness" since the initial focus I had in mind was to build consensus around when/how we use primordials rather than solve the larger issue though I think we can do the former and then tackle the later.


@ljharb

A moat-based defense is appropriate in security; it’s not all or nothing.

An unaudited and unverified security mechanism is a bad one that just creates a false sense of security and leads users to assume something that isn't secure is secure. It's "rolling your own crypto", in order to claim a piece of code is covered by primordials we'd need to formally prove it over the language with escape analysis, it's possible it's just not something anyone volunteered to do.

Node is a big platform trusted by many companies and users and I won't feel comfortable with the project claiming security or isolation without verification much like I wouldn't want to ship our own unaudited scrypt.

@ljharb
Copy link
Member

ljharb commented Sep 13, 2023

I totally agree that node shouldn't claim security externally without those verifications; that doesn't mean there's no value in internally attempting to mitigate security issues, whether partially or fully. Formal proofs and security audits are important but they're not the gate for something that improves security.

@benjamingr
Copy link
Member Author

benjamingr commented Sep 14, 2023

As we did not have time to discuss this in the TSC meeting and get synchronous consensus I'll fall back to lazy consensus, if this does not have any -1s after 7 days I'll go ahead and start bootstrapping this.

cc @nodejs/tsc

@jasnell
Copy link
Member

jasnell commented Sep 14, 2023

+1 to lazy consensus fall back.

@mcollina
Copy link
Member

I'm +1 with the approach.

@isaacs
Copy link

isaacs commented Sep 15, 2023

@benjamingr I think this is an extremely reasonable approach that most likely will strike a good balance between the competing goals involved. It's a tricky problem which will likely always be some sort of unsatisfying compromise solution, but hopefully a better compromise can be found. Or if not, at least there'll be more clarity around why it is the way it is and how to minimize the downsides :)

@benjamingr
Copy link
Member Author

Opened nodejs/node#49706 if no one objects in two days I'll follow through with the steps outlined in #1439 (comment)

@benjamingr
Copy link
Member Author

I've created the team https://github.com/orgs/nodejs/teams/primordials please let me know if you want to be added (or feel free to add yourself if TSC member). I will send a doodle ±next week

I already pinged the people above.

@ljharb
Copy link
Member

ljharb commented Sep 20, 2023

I'd like to be added to the team.

@atlowChemi
Copy link
Member

I'd be happy to join as well

@iambary44

This comment was marked as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants