Prevent merging __proto__ property

[mnespor]

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]

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]

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.

[ljharb]

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

[mnespor]

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)

[ljharb]

should this not be undefined in that case?

[mnespor]

Yes, definitely.

[ljharb]

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

[mnespor]

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]

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]

@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]

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.

[ljharb]

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.

[mnespor]

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 }

[ljharb]

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.

[mnespor]

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?

[ljharb]

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

[mnespor]

@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]

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

[ljharb]

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

[mnespor]

Thank you kindly!