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

For Object, stringify mapping key as YAML rather than JSON #104

Merged
merged 5 commits into from Apr 3, 2019

Conversation

eemeli
Copy link
Owner

@eemeli eemeli commented Mar 30, 2019

With this change, mapping keys are stringified during YAML.parse() or YAML.Document#toJSON() as YAML rather than JSON representations:

YAML.parse('{ 1: 2 }: foo')
// was: { '{"1":2}': 'foo' }
// now: { '{ 1: 2 }': 'foo' }

The reason for this change is this npm advisory, which is related to this issue: nodeca/js-yaml#475 -- which also affects this library. Essentially, it's possible to abuse aliases to create an exponential tree of nodes consisting of collections that each have multiple references. When such a tree is used as the key of a mapping that is then converted to a JS object, JSON.stringify() was previously called on it, and the process runs out of memory creating the string.

Stringifying the key as YAML rather than JSON allows for the exponential explosion to be averted, as the repeated references use the same anchors as in the original input.

This change will be a part of the next minor release. It could arguably require a major version bump, but I figure that if you're working with structures that may include collections or aliases as keys and you care about the contents of said keys, you're almost certainly already using mapAsMap: true to avoid the stringification, and so this change should not break any reasonable implementations.

While this fix patches the library's own vulnerability to this type of attack, I'm considering whether I need to do something more when parsing this type of input, as the library user may not expect it to be able to return structures that are not safe to e.g. stringify as JSON.

@eemeli
Copy link
Owner Author

eemeli commented Apr 2, 2019

Ended up adding a new option maxAliasDepth, and throwing an error during the document -> JS object stage when the reference depth exceeds the limit. By default three levels of aliasing are allowed, but that can be adjusted or disabled.

@eemeli eemeli merged commit d42b492 into master Apr 3, 2019
@eemeli eemeli deleted the yamly-keys branch April 3, 2019 06:48
eemeli added a commit that referenced this pull request Apr 4, 2019
@eemeli
Copy link
Owner Author

eemeli commented Apr 4, 2019

Ended up replacing the option with maxAliasCount, and actually tracking and limiting the count of how many times any one piece of data may be aliased rather than just the depth of the reference structure. By default, this is 100 (including the original), which ought to be enough for most cases, but still limiting enough that exponential or quadratic attacks are thwarted -- effectively the expansion is now at most linear at 100x the input size.

Setting maxAliasCount to 0 or 1 will now complain if the input contains any alias nodes.

eemeli added a commit to eemeli/yaml-docs that referenced this pull request Apr 4, 2019
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

1 participant