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

feat: allow to set a maximum amount of aliases while parsing to protect against billion laughs attack and similar #620

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

Vampire
Copy link
Contributor

@Vampire Vampire commented Sep 23, 2024

No description provided.

@Vampire
Copy link
Contributor Author

Vampire commented Sep 24, 2024

In the current code of the PR the cumulated alias weight of all nodes so far is used as a criterion.
An alternative could be to stop parsing if an individual node exceeds a given weight limit.

@charleskorn
Copy link
Owner

Is this necessary? YamlConfiguration already has a allowAnchorsAndAliases property that defaults to false. I think adding what is proposed here adds a lot of complexity and I worry that it provides a false sense of security.

@charleskorn charleskorn changed the title Allow to set a maximum amount of aliases while parsing to protect against billion laughs attack and similar feat: allow to set a maximum amount of aliases while parsing to protect against billion laughs attack and similar Oct 30, 2024
@Vampire
Copy link
Contributor Author

Vampire commented Oct 30, 2024

Yes, this is definitely necessary.

If you set allowAnchorsAndAliases to false you cannot use anchors and aliases.

If you need them and thus set it to true, you are one blink away from a DoS attack which is very trivial.

With this protection, the DoS attack is prevented. It is a similar protection e.g. snakeyaml has built in if you use the high-level API (you use the low-level one), or also the YAML NPM package.

@Vampire
Copy link
Contributor Author

Vampire commented Oct 30, 2024

If you have a better idea for a protection, your welcome to not merge this and do that other idea instead of course. :-)

@Vampire Vampire force-pushed the max-alias-count branch 2 times, most recently from 64513da to ba60015 Compare November 4, 2024 03:58
@Vampire Vampire requested a review from charleskorn November 4, 2024 03:58
Copy link
Owner

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

Two small suggestions, and then I think this is good to go!

src/commonMain/kotlin/com/charleskorn/kaml/Yaml.kt Outdated Show resolved Hide resolved
@charleskorn
Copy link
Owner

@Vampire could you please fix the linting failure?

@Vampire
Copy link
Contributor Author

Vampire commented Nov 5, 2024

Whoops, sorry, did the uglification now. :-)

Copy link
Owner

@charleskorn charleskorn left a comment

Choose a reason for hiding this comment

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

Thanks @Vampire!

@charleskorn charleskorn merged commit 1b93b80 into charleskorn:main Nov 14, 2024
6 checks passed
Copy link

🎉 This PR is included in version 0.64.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@Vampire Vampire deleted the max-alias-count branch December 6, 2024 09:38
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.

2 participants