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 merging __proto__ property #48

Merged
merged 1 commit into from Jul 19, 2018

Conversation

mnespor
Copy link
Contributor

@mnespor mnespor commented Apr 24, 2018

Hi there! Would you be open to preventing deep cloning of properties named __proto__ to avoid the attack described in JavaScript Prototype Poisoning Vulnerabilities in the Wild?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 24, 2018

I think calling this an "attack" is a significant overexaggeration.

This library is meant to be an exact copy of jQuery's algorithm, warts and all. I'm not solely convinced by this article that this is a real problem in practice - one worth deviating from jQuery's original algorithm.

Separately, if this isn't going to get the actual [[Prototype]], I'd expect it to end up matching the JSON object - in other words, having an own __proto__ property of { george: 1 }.

@mnespor
Copy link
Contributor Author

@mnespor mnespor commented Apr 24, 2018

Thanks so much for looking at this!

I'm definitely with you on giving the target an own __proto__ property. That's less surprising than ignoring __proto__ for sure.

I do still think it has the potential to be a problem in practice. A well-behaved app ought to sanitize user-provided JSON before sending it through extend() or _.cloneDeep(), but until reading that article, I never realized that __proto__ needed to be stripped. I'd been using the OWASP JSON sanitizer as a model.

index.js Outdated
@@ -31,6 +31,29 @@ var isPlainObject = function isPlainObject(obj) {
return typeof key === 'undefined' || hasOwn.call(obj, key);
};

// If name is '__proto__', define it as an own property on target
var setProperty = function setProperty(target, options) {
if (options.name === '__proto__') {
Copy link
Collaborator

@ljharb ljharb Apr 24, 2018

Choose a reason for hiding this comment

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

what happens if Object.defineProperty is unavailable, like in ES3 engines?

Copy link
Contributor Author

@mnespor mnespor Apr 25, 2018

Choose a reason for hiding this comment

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

I've been thinking about that a bit. It might be good to check for the existence of Object.defineProperty and fall back to regular assignment.

This means it would still be possible to modify prototypes in pre-ES5 environments. That seems fine in practice. Some browsers will fall back, but Node won't have to. iojs and Node 0.8 have Object.defineProperty available.

(To be frank, Node 0.8 has bigger security concerns anyway)

index.js Outdated
// Return a new object instead of __proto__ if '__proto__' is not an own property
var getProperty = function getProperty(obj, name) {
if (name === '__proto__' && !hasOwn.call(obj, name)) {
return {};
Copy link
Collaborator

@ljharb ljharb Apr 24, 2018

Choose a reason for hiding this comment

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

should this not be undefined in that case?

Copy link
Contributor Author

@mnespor mnespor Apr 25, 2018

Choose a reason for hiding this comment

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

Yes, definitely.

test/index.js Outdated
var malicious = JSON.parse('{ "fred": 1, "__proto__": { "george": 1 } }');
var target = {};
extend(true, target, malicious);
t.notOk(target.george);
t.ok(Object.prototype.hasOwnProperty.call(target, '__proto__'));
var name = '__proto__';
t.equal(target[name].george, 1);
Copy link
Collaborator

@ljharb ljharb Apr 24, 2018

Choose a reason for hiding this comment

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

t.deepEqual(target.__proto__, { george: 1 }) would also work

Copy link
Contributor Author

@mnespor mnespor Apr 25, 2018

Choose a reason for hiding this comment

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

For sure!

The reason for the var name = '__proto__' gymnastics: the linter discourages both target.__proto__ (no-proto) and target['__proto__'] (dot-notation). After some thought, I want to drop the gymnastics and use a comment to disable no-proto on that line.

@mnespor
Copy link
Contributor Author

@mnespor mnespor commented Apr 25, 2018

I'm digging into the CI checks a little bit. So far, I've learned:

  • npm@6 dropped support for node@4. Environments older than node@6 might need special cases like the one for "${TRAVIS_NODE_VERSION}" = "0.6" so that they can install an older npm.
  • eslint@3 uses an arrow function. Linting doesn't work under Nodes older than v4 on my machine, but most of the other CI steps still work.
  • covert hangs on the "deep clone; arrays are merged" test case on node@5. It succeeds on both older and newer Nodes.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 25, 2018

@mnespor nvm install-latest-npm will be fixed with npm 6 when the next version of nvm is released and used by travis; I'll rebase this PR at that time.

Regarding eslint, and covert, and the general testing landscape: I'll update travis.yml in master and rebase this PR again in a few hours.

@ljharb ljharb force-pushed the feature/prototype-poisoning branch from ebd56b3 to 3f92a15 Compare Apr 25, 2018
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 25, 2018

Rebased; tests are now failing in 0.8 and 0.10, and they seem to be failing properly.

Note that when you're testing locally, only npm run tests-only should be run in anything besides "latest node"; see the travis.yml for details.

test/index.js Outdated
t.ok(Object.prototype.hasOwnProperty.call(target, '__proto__'));
t.deepEqual(target.__proto__, { george: 1 }); // eslint-disable-line no-proto
// this test isn't valid for earlier versions of V8, which strip __proto__ during JSON.parse()
if (Object.prototype.hasOwnProperty.call(malicious, '__proto__')) {
Copy link
Collaborator

@ljharb ljharb Apr 26, 2018

Choose a reason for hiding this comment

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

Perhaps instead of this, we could do var malicious = { fred: 1 }; if (Object.defineProperty) { Object.defineProperty(malicious, '__proto__', { value: { george: 1 } }; }?

JSON.parse is just one way a malicious/surprising object could appear, but we should try to test this in earlier v8s too.

Copy link
Contributor Author

@mnespor mnespor Apr 26, 2018

Choose a reason for hiding this comment

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

I like that a lot. It's doing something I didn't expect in 0.8, however:

> var malicious = { fred: 1 }
undefined
> Object.defineProperty(malicious, '__proto__', { value: { george: 1 }, enumerable: true })
{ fred: 1, [__proto__]: { george: 1 } }
> malicious.__proto__
{}
> malicious.hasOwnProperty('__proto__')
true
> Object.keys(malicious)
[ 'fred', '__proto__' ]
> malicious[Object.keys(malicious)[0]]
1
> malicious[Object.keys(malicious)[1]]
{}

(in newer Nodes, it does this instead):

> Object.defineProperty(malicious, '__proto__', { value: { george: 1 }, enumerable: true })
{ fred: 1, __proto__: { george: 1 } }
> malicious.__proto__
{ george: 1 }

Copy link
Collaborator

@ljharb ljharb Apr 26, 2018

Choose a reason for hiding this comment

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

interesting - it seems in node 0.8, Object.getOwnPropertyDescriptor(malicious, '__proto__').value gives the expected output. This seems to be a bug in these node versions; but it'd be fine to use this approach in the tests.

Copy link
Contributor Author

@mnespor mnespor Apr 27, 2018

Choose a reason for hiding this comment

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

That's very interesting. It might not be fixable from the test side alone. The library code would also have to do copy = Object.getOwnPropertyDescriptor(options, name).value instead of copy = options[name] to get the real value of an own __proto__ on Node 0.8.

Pure druthers, I'd change the test to use Object.defineProperty() instead of JSON.parse(), but continue to skip this test case on 0.8 and 0.10.

Alternatively, this could go into the library and the test would pass on 0.8:

var getProperty = function getProperty(obj, name) {
	if (name === '__proto__') {
		if (!hasOwn.call(obj, name)) {
			return undefined;
		} else if (Object.getOwnPropertyDescriptor) {
			return Object.getOwnPropertyDescriptor(obj, name).value;
		}
	}

	return obj[name];
};

The performance characteristics of Object.getOwnPropertyDescriptor() aren't ideal, but I trust that branch wouldn't run often in production.

What do you think?

Copy link
Collaborator

@ljharb ljharb Apr 27, 2018

Choose a reason for hiding this comment

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

I don't think performance is particularly important here anyways :-) let's go for it.

@mnespor
Copy link
Contributor Author

@mnespor mnespor commented May 3, 2018

@ljharb That change ought to get the test suite passing on Node 0.8 and 0.10. If you get some time free, could you give it a review, please?

@mnespor
Copy link
Contributor Author

@mnespor mnespor commented Jun 19, 2018

Hi, just wanted to check in. Is this ready to merge?

@ljharb ljharb force-pushed the feature/prototype-poisoning branch from 1a5c464 to 0e68e71 Compare Jul 19, 2018
@ljharb ljharb merged commit 0e68e71 into justmoon:master Jul 19, 2018
1 check passed
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jul 19, 2018

v3.0.2 and v2.0.2 have been released with this change.

@mnespor
Copy link
Contributor Author

@mnespor mnespor commented Jul 22, 2018

Thank you kindly!

This was referenced Mar 15, 2021
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.

None yet

2 participants