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

Missing Prototype Pollution Check in ASVS #1563

Closed
ImanSharaf opened this issue Mar 1, 2023 · 11 comments · Fixed by #1843
Closed

Missing Prototype Pollution Check in ASVS #1563

ImanSharaf opened this issue Mar 1, 2023 · 11 comments · Fixed by #1843
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V10 tmp code-review Temporary label for grouping code review or program code related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@ImanSharaf
Copy link
Collaborator

I noticed that the ASVS (Application Security Verification Standard) does not include a check for Prototype Pollution vulnerabilities. Prototype Pollution is a critical vulnerability that can allow attackers to manipulate an application's JavaScript objects and properties, leading to serious security issues such as unauthorized access to data, privilege escalation, and even remote code execution.

As JavaScript applications become more complex, Prototype Pollution attacks are becoming more common. It is therefore essential that the ASVS includes a dedicated check to help ensure that web applications are protected from this type of vulnerability.

I strongly recommend that the ASVS be updated to include a Prototype Pollution check, a sample check could be this one:

  • Check that no user-supplied input is used to modify the prototype property of an object.
@elarlang elarlang added the V10 tmp code-review Temporary label for grouping code review or program code related issues label Mar 10, 2023
@tghosth
Copy link
Collaborator

tghosth commented Mar 15, 2023

https://twitter.com/JoshCGrossman/status/1635987227736961025

Agree we should have this, I have reached out to @hackvertor for his opinion on your wording:

Verify that no user-supplied input is used to modify the prototype property of an object.

@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 labels Mar 15, 2023
@hackvertor
Copy link

Here are my thoughts on protecting against this attack. I would recommend that developers use new Set() or new Map() instead of using object literals:

let allowedTags = new Set();
allowedTags.add('b');
if(allowedTags.has('b')){
  //...
}

let options = new Map();
options.set('spaces', 1);
let spaces = options.get('spaces')

If objects have to be used then they should be created using the Object.create(null) API to ensure they don't inherit from the Object prototype:

let obj = Object.create(null);

If object literals are required then as a last resort you could use the __proto__ property:

let obj = {__proto__:null};

You can also use the Object.freeze() and Object.seal() APIs to prevent built in prototypes from being modified however this can break the application if the libraries they use modify the built in prototypes.

Node also offers the ability to remove the __proto__ property completely using the --disable-proto=delete flag. Note this is a defence in depth measure. Prototype pollution is still possible using constructor.prototype properties but removing __proto__ helps reduce attack surface and prevent certain attacks.

@hackvertor
Copy link

Sanitizing keys is a bad approach since you have to use a denylist of bad values. It's far better to use new Set() or new Map() since these APIs do not have prototype pollution vulnerabilities provided they are used correctly.

@ImanSharaf
Copy link
Collaborator Author

Thank you, @tghosth and @hackvertor, for your valuable input on the Prototype Pollution check. I appreciate the detailed recommendations on how developers can protect against this type of attack.
Based on your feedback, I would like to propose an updated item for the ASVS that highlights these best practices for preventing Prototype Pollution vulnerabilities:

[NEW] Verify that the application mitigates Prototype Pollution vulnerabilities by employing one or more of the following strategies:

  • Using new Set() or new Map() for storing data instead of object literals.
  • Creating objects using Object.create(null) to prevent inheritance from the Object prototype.
  • If object literals are required, using the proto: null property as a last resort.
  • Considering Object.freeze() and Object.seal() APIs to prevent modification of built-in prototypes, but being cautious of potential application breakage.
  • For Node.js applications, using the --disable-proto=delete flag as a defense-in-depth measure to remove the proto property and reduce the attack surface.

@tghosth
Copy link
Collaborator

tghosth commented May 31, 2023

So this is still a little too detailed.

I would propose:

Verify that JavaScript code is written in a way that prevents prototype pollution by using Set() or Map() instead of object literals or other protection mechanisms.

What do you think @ImanSharaf ?

I am separately going to try and get the detailed guidance into a cheatsheet.

@elarlang
Copy link
Collaborator

For category it waits: #1643

@tghosth
Copy link
Collaborator

tghosth commented Sep 21, 2023

So this is still a little too detailed.

I would propose:

Verify that JavaScript code is written in a way that prevents prototype pollution by using Set() or Map() instead of object literals or other protection mechanisms.

What do you think @ImanSharaf ?

I am separately going to try and get the detailed guidance into a cheatsheet.

Do you agree with this @ImanSharaf?

I will discuss category with elar afterwards,

@ImanSharaf
Copy link
Collaborator Author

To address prototype pollution, I recommend that we do not confine our solutions to exclusively using Set() or Map(). It would be nice to be more flexible in this regard.

@tghosth
Copy link
Collaborator

tghosth commented Sep 26, 2023

Ok so what wording do you suggest @ImanSharaf ?

I think the originally proposed wording is too general...

Check that no user-supplied input is used to modify the prototype property of an object.

Note that there is now a cheatsheet on this: https://cheatsheetseries.owasp.org/cheatsheets/Prototype_Pollution_Prevention_Cheat_Sheet.html

@tghosth
Copy link
Collaborator

tghosth commented Jan 24, 2024

Opened #1843 @ImanSharaf please let know what you think

@ImanSharaf
Copy link
Collaborator Author

So this is still a little too detailed.

I would propose:

Verify that JavaScript code is written in a way that prevents prototype pollution by using Set() or Map() instead of object literals or other protection mechanisms.

What do you think @ImanSharaf ?

I am separately going to try and get the detailed guidance into a cheatsheet.

That works for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet V10 tmp code-review Temporary label for grouping code review or program code related issues _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants