search

clj-commons / clj-yaml

YAML encoding and decoding for Clojure
Other
122 stars 26 forks source link

Explore Upgrading to SnakeYAML 2.x #86

Closed lread closed 1 year ago

lread commented 1 year ago

SnakeYAML 2.0 has been released:

Perceived advantages to upgrade:

I'll start with a PR to explore

lread commented 1 year ago

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 commented 1 year ago

Option 2 sounds good to me

arichiardi commented 1 year ago

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

lread commented 1 year ago

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 commented 1 year ago

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.