-
Notifications
You must be signed in to change notification settings - Fork 18
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
Patch fixes prototype pollution issue #17 #19
Conversation
…tion-security-fix Fix as proposed by Johns Hopkins researchers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch! This is definitely worth fixing, but I don't think we've gone about it the right way here.
With this code change, we'd potentially be returning incorrect data (see comment left inline). Instead, we should either error when given a restricted keyword or return undefined
.
To avoid breaking existing apps as much as possible, IMO the best solution is to return undefined
.
Example:
var restricted = ['__proto__', 'prototype', 'constructor'];
var isRestricted = function(key){
for (var i = 0; i < restricted.length; i++) {
if (key == restricted[i]) {
return true
}
}
return false
}
exports.parse = function(str){
if ('string' != typeof str) return {};
str = trim(str);
if ('' == str) return {};
if ('?' == str.charAt(0)) str = str.slice(1);
var obj = {};
var pairs = str.split('&');
for (var i = 0; i < pairs.length; i++) {
var parts = pairs[i].split('=');
var key = decode(parts[0]);
var m;
if (isRestricted(key)) {
continue
}
if (m = pattern.exec(key)) {
obj[m[1]] = obj[m[1]] || [];
obj[m[1]][m[2]] = decode(parts[1]);
continue;
}
obj[parts[0]] = null == parts[1]
? ''
: decode(parts[1]);
}
return obj;
};
* | ||
* @param {String} str | ||
* @return {String} | ||
* @api public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not part of the public API.
* @api public | |
* @api private |
|
||
var sanitizeObjKey = function(str) { | ||
if (str && ["__proto__", "constructor", "prototype"].indexOf(str.toLowerCase()) > -1) { | ||
return str.toUpperCase() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why convert to uppercase? If the user (for some weird reason) wanted the value of prototype
, they'd get PROTOTYPE
instead, which would either return unexpected (incorrect) results or undefined
. Consider the following:
var q = querystring.parse('prototype=hi&PROTOTYPE=bye')
console.log(q.prototype)
// => bye
console.log(q.PROTOTYPE)
// => bye
IMO we should either error or return undefined
.
Solution as proposed by Johns Hopkins researchers.
If querystring key contains strings that can cause prototype pollution in the browser, e.g.
__proto__
, transform to uppercase.fixes #17