Object properties should be created using defineProperty

[monsanto]

This is how JSON.parse works, and it allows one to bypass setters on Object.prototype such as __proto__. Compare:

> require('js-yaml').safeLoad('{"__proto__": "test"}').__proto__
{}
> JSON.parse('{"__proto__": "test"}').__proto__
'test'

[puzrin]

Seems we need to use Object(null) instead of {} for maps, when available.

[monsanto]

I'm OK with that, personally. Do you think the symmetry with JSON.parse is not worth preserving? An object restored with JSON.parse will still have toString, etc if they aren't overwritten.

[puzrin]

Seems this can't cause security issue, but it worth to be improved anyway.

/cc @dervus

[rlidwka]

An object restored with JSON.parse will still have toString, etc if they aren't overwritten.

Honestly, this behavior of JSON.parse is a security threat. I'd say it is worth avoiding if possible.

[dervus]

Naive implementation by replacing regular assignment with defineProperty results in noticeable performance drop:

Selected samples: (5 of 12)
 > document_application2
 > document_huge
 > document_nodeca_application
 > document_nodeca_database
 > document_nodeca_logger

Sample: document_application2.yaml (360 characters)
 > current x 35,550 ops/sec ±0.91% (92 runs sampled)
 > js-yaml-3.2.7 x 56,118 ops/sec ±0.72% (89 runs sampled)


Sample: document_huge.yaml (7182063 characters)
 > current x 4.68 ops/sec ±4.80% (15 runs sampled)
 > js-yaml-3.2.7 x 4.50 ops/sec ±3.86% (15 runs sampled)


Sample: document_nodeca_application.yaml (2055 characters)
 > current x 26,759 ops/sec ±0.91% (89 runs sampled)
 > js-yaml-3.2.7 x 38,083 ops/sec ±0.13% (101 runs sampled)


Sample: document_nodeca_database.yaml (394 characters)
 > current x 63,604 ops/sec ±3.30% (92 runs sampled)
 > js-yaml-3.2.7 x 99,212 ops/sec ±3.91% (93 runs sampled)


Sample: document_nodeca_logger.yaml (727 characters)
 > current x 19,315 ops/sec ±0.13% (101 runs sampled)
 > js-yaml-3.2.7 x 29,862 ops/sec ±0.17% (99 runs sampled)

Probably we need to collect all these special keys like __proto__ and use defineProperty selectively.

[isaacs]

Note that __proto__ is much less magical in es6. I recommend testing any fix against iojs for future proofing.

[KSXGitHub]

This vulnerability still exists today (Node v13). Is nobody going to fix this?

[puzrin]

Using terms like "vulnerability" need proofs. Do you have those? Something more than single broken object.

[puzrin]

Idea: we could check first bytes of property name and use defineProperty() for _*. This should isolate specific names without breaking performance for normal use.

/cc @rlidwka

[rlidwka]

Fixed in a003121 (in dev branch currently).