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

Prevent prototype pollution in $.deparam #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cee-chen
Copy link

@cee-chen cee-chen commented Oct 7, 2020

See #62 for a more in-depth description of how to reproduce the prototype pollution.

Using Object.create(null) prevents prototype pollution because the objects it creates have no prototype/constructor, and therefore does not pollute the global prototype.

You can test/confirm this for yourself by comparing the outputs of:

  • Object.create(null).__proto__.test = 'polluted' (should simply fail)
  • vs. ({}).__proto__.test = 'polluted'

jQuery/browser compatability

As a reminder, if you fork this or care about prototype pollution in jQuery BBQ, you must also be on jQuery 3.4.0+, otherwise you're still vulnerable via jQuery's $.extend.

Please also note that Object.create(null) is only supported on IE9+. This fork therefore drops support for IE6-8 (which should be the case anyway if you are on jQuery 3.4.0, which does not support $.browser).

@jayaddison
Copy link

@constancecchen This looks like a good change; I had some trouble getting this to pass the unit tests though. Could you try running those locally? (it should be a case of opening the unit/index.html file in your preferred browser)

@jayaddison
Copy link

@constancecchen A small reminder about the unit tests here, if & when you have time - or let me know if you're likely to be too busy to take more of a look.

@cee-chen
Copy link
Author

cee-chen commented Apr 6, 2021

Super sorry about the slow response!! I've been swamped and totally procrastinated on this. It would be awesome to get some help on the unit tests, if anyone's around. For what it's worth also we ran into another issue with prototype pollution even with this PR:

The issue there is when an obj is initially defined as a non-object (e.g. a string or an array), the prototype of the prototype of that string/array can then by still be polluted (yay, JS). I think the issue is somewhere around line 521 of the code but I could be wrong.

With that in mind a more blunt-force solution instead ofObject.create(null) might be to simply return early if someone tries to pass a key of __proto__ at any point - they're probably up to no good 🤷 Thoughts?

@marcovtwout
Copy link

@cee-chen This issue was fixed like you suggested in LimeSurvey: LimeSurvey/LimeSurvey@0c4651b

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