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

fix(runtime): implement __proto__ getter, ignore and warn on setter #16775

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

khrj
Copy link
Contributor

@khrj khrj commented Nov 23, 2022

A lot of npm modules depend on using __proto__, either directly or indirectly through and old dependency they're using. This causes silent __proto__ key creations which then lead to completely unrelated errors that take a lot of time to debug.

This PR adds a getter for __proto__ using Object.getPrototypeOf(), and warns (while not actually setting the prototype) when trying to set __proto__, since allowing __proto__ to be set creates security vulnerabilities (as per #4341)

Examples of modules broken:

@CLAassistant
Copy link

CLAassistant commented Nov 23, 2022

CLA assistant check
All committers have signed the CLA.

@khrj khrj changed the title Warn on __proto__ fix(runtime): implement __proto__ getter, ignore and warn on setter Nov 23, 2022
@khrj khrj force-pushed the warn-on-proto branch 2 times, most recently from 37c56cf to 31026f3 Compare November 23, 2022 11:31
@khrj

This comment was marked as outdated.

@khrj khrj marked this pull request as ready for review November 23, 2022 11:40
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test case demonstrating that package like chalk@3 works with this change? Eg. you can add an integration test in npm_tests.rs

cli/tsc/99_main_compiler.js Outdated Show resolved Hide resolved
runtime/js/99_main.js Outdated Show resolved Hide resolved
@khrj

This comment was marked as outdated.

@khrj

This comment was marked as outdated.

@khrj
Copy link
Contributor Author

khrj commented Nov 23, 2022

Since the __proto__ accessor isn't specifically npm related, I've added a new generic __proto__ accessor test, to check if the output matches Object.getPrototypeOf(). I've also updated the proto_exploit test to include the warning output, to make sure a warning is emitted.

runtime/js/99_main.js Outdated Show resolved Hide resolved
@khrj khrj requested review from dsherret and bartlomieju and removed request for bartlomieju and dsherret November 24, 2022 10:50
@Lesiuk
Copy link

Lesiuk commented Nov 25, 2022

Vite with babel is failing on any code change without this PR, so I would love to get it merged soon.

@KnorpelSenf
Copy link
Contributor

This closes #16833.

@bartlomieju
Copy link
Member

Does the output contain the full stack trace where the error is coming from?

@khrj
Copy link
Contributor Author

khrj commented Nov 30, 2022

Does the output contain the full stack trace where the error is coming from?

It's the same stacktrace that's produced when any error is thrown.

Examples:

Next.js

image

pm2

image

@kt3k
Copy link
Member

kt3k commented Nov 30, 2022

This doesn't resolve the usages of chalk in next.js and pm2, but only improves the error messages. I'm not sure it's worth to land. (There might be more confusion if we provide non-standard readonly __proto__

Update: next.js doesn't use chalk in a way that it requires __proto__ support. Probably this PR isn't relevant to next.js

@khrj
Copy link
Contributor Author

khrj commented Nov 30, 2022

Maybe __proto__ reads should show a warning too then? (With or without actually returning the property)

I feel like silently creating/returning __proto__ keys is a worse solution since it leads to headaches in debugging, especially for people who don't know that __proto__ isn't implemented

It's also possible to emit the warning only once, with a longer explanation on why the error occurred and what the user can do -- perhaps that's a better solution?

Something along the lines of

An attempt to access/set __proto__ was made within your application or one of its dependencies.
__proto__ is not implemented in Deno due to security concerns over unsanitized inputs setting prototypes maliciously

  1. If your own code uses __proto__, replace it with Object.getPrototypeOf or Object.setPrototypeOf
  2. If one of your dependencies uses or depends on something that uses __proto__, you will need to suggest changes and ask the dependency maintainer to update their code
  3. If __proto__ has been replaced in a newer version of a module that is depended upon by a module you depend upon, you will need to suggest changes and ask them to update their dependencies.

@KnorpelSenf
Copy link
Contributor

(Note that getters and setters will look to "__proto__" in obj checks as if the property was present. This makes it especially hard to detect the non-standard implementation of __proto__.)

@Jakob5358
Copy link

Jakob5358 commented Nov 30, 2022

(Note that getters and setters will look to "__proto__" in obj checks as if the property was present. This makes it especially hard to detect the non-standard implementation of __proto__.)

What about a warning when deno downloads the npm package?

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

Successfully merging this pull request may close these issues.

bug: __proto__ behaves differently from Chrome, Node, and Firefox
8 participants