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(encoding/jsonc): avoid prototype pollution in Node.js and Browser #3077

Merged
merged 8 commits into from
Jan 13, 2023

Conversation

ayame113
Copy link
Contributor

@ayame113 ayame113 commented Jan 2, 2023

related: CVE-2022-46175

Using encoding/jsonc with Node.js and browsers is currently vulnerable to prototype pollution (in the form described in the CVE-2022-46175).

This PR changes the behavior of jsonc.parse when trying to set __proto__ as a key in Node.js and browsers.

This PR does not change behavior when running in Deno. This is because __proto__ has been removed within Deno and is already robust against prototype pollution attacks.

encoding/jsonc_test.ts Outdated Show resolved Hide resolved
@lino-levan
Copy link
Contributor

Not sure if this makes sense for the std. It seems to be making the code harder to read with very little gain for its intended place of consumption. I do see an argument for browsers since iirc this module is considered browser compatible but given that it is non-trivial to get this running directly in a browser due to typescript, I would not be concerned about __proto__ vulnerabilities personally. I'm not super strongly against this change, just my 2 cents.

Also, I'm personally -1 for adding a dependency to node here.

@ayame113
Copy link
Contributor Author

ayame113 commented Jan 3, 2023

We can use esb.deno.dev, deno.land/x/emit and dnt to run this module in the browser or Node.js.

In my opinion, when it comes to code readability and security, security should take precedence. (For reference, the content of the fix is the same as the fix done in JSON5.)

Also, the behavior in current browsers simply looks buggy. i want to fix this.

image

Also, I'm personally -1 for adding a dependency to node here.

I agree with this.
I thought about this again, by overriding the setter of __proto__ and testing it, it may be possible to test without relying on Node.js. I'll try later.

Edit: I have done this, and it works great. There is no dependency on Node.js anymore.

encoding/jsonc_test.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Jan 4, 2023

Fixing for browser security makes sense to me

@lino-levan
Copy link
Contributor

Sorry about not having time to review this earlier. In its new form, this LGTM!

My biggest concern before was regarding CI but now that this has been fixed, this seems like an obvious fix.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@kt3k kt3k merged commit 329d164 into denoland:main Jan 13, 2023
@ayame113 ayame113 deleted the fix-jsonc-proto branch January 13, 2023 09:51
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.

3 participants