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

Explore Upgrading to SnakeYAML 2.x #86

Closed
lread opened this issue Feb 26, 2023 · 5 comments · Fixed by #87
Closed

Explore Upgrading to SnakeYAML 2.x #86

lread opened this issue Feb 26, 2023 · 5 comments · Fixed by #87

Comments

@lread
Copy link
Collaborator

lread commented Feb 26, 2023

SnakeYAML 2.0 has been released:

Perceived advantages to upgrade:

  • users would not get the current CVE warning for snakeyaml 1.33
  • there might be some security fixes included in 2.0 that we did not entirely grok

I'll start with a PR to explore

@lread lread changed the title Explore Upgrading to SnakeYAML 2.0 Explore Upgrading to SnakeYAML 2.x Mar 26, 2023
@lread
Copy link
Collaborator Author

lread commented Mar 26, 2023

We have decided to wait until SnakeYAML 2.1 is released before upgrading.
The idea is that the dust should have settled by then.

TrustedTagInspector is being removed in 2.1

SnakeYAML 2.1, which at the time of this writing, has yet to be released, moves TrustedTagInspector from their release artifact to a not-released example test class.

Note: the TrustedTagInspector might have been better named TrustingTagInspector, it simply allows any and all yaml global tags.

I made use of the TrustedTagInspector class in my SnakeYAML 2.0 PR to mimic SnakeYAML 1.x behaviour to support clj-yaml :unsafe feature.

Option 1 - Include a TrustedTagInspector.java in clj-yaml

Clj-yaml has Java sources already, just bring this wee class over and use it.

Option 2 - Same as 1 but do it in Clojure

Just because clj-yaml has Java sources does not mean it should have more.

We can express the equivalent of TrustedTagInspector inline via a reify in clj-yaml.

Option 3 - Force clj-yaml users to create their own trusted tag inspector.

With all the SnakeYAML CVE turmoil, I can see why team SnakeYAML turfed the TrustedTagInspector. They want to force their users to be very deliberate when doing unsafe things.

Since clj-yaml is already safe by default, and only : unsafe on request, I don't see a need to follow the SnakeYAML strategy here with a breaking change.

We also want to remain babashka-friendly.

Proposal

Option 2 makes the most sense to me.
@borkdude any thoughts, or concerns?

@borkdude
Copy link
Collaborator

Option 2 sounds good to me

@arichiardi
Copy link

arichiardi commented May 19, 2023

This is particularly important also because of a critical vulnerability in snakeyaml 1.33.

@lread
Copy link
Collaborator Author

lread commented May 19, 2023

Hi @arichiardi, we feel that clj-yaml is not impacted by CVE-2022-1471 because clj-yaml is safe by default.

That said, various security analysis tooling will rightly chirp and whistle because they don't know how clj-yaml is using SnakeYAML.

Because 2.0 was a big change for SnakeYAML, and some changes seemed to be in flux, we decided to wait until SnakeYAML 2.1 is released before making the move to SnakeYAML 2.x.

@lread
Copy link
Collaborator Author

lread commented Aug 4, 2023

Looks like v2.1 was released today: https://central.sonatype.com/artifact/org.yaml/snakeyaml/2.1

Might as well wait a few days more just to learn if there are any major issues with v2.1 before taking the plunge.

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 a pull request may close this issue.

3 participants