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

Upgrade to SnakeYAML 2.1 #87

Merged
merged 11 commits into from
Aug 10, 2023
Merged

Upgrade to SnakeYAML 2.1 #87

merged 11 commits into from
Aug 10, 2023

Conversation

lread
Copy link
Collaborator

@lread lread commented Feb 26, 2023

See commits for details.

Up for debate: my approach to dealing with unsafe YAML should be questioned.

An interesting alternative would be to also expose the new TagInspectors when using clj-yaml's :unsafe option. But, I think (correct me if I am wrong) we might already have this feature-wise via :unknown-tag-fn.

Closes #86

Adapt to changes to exception messages in SnakeYAML 2.0
SnakeYAML 2.0 responded to CVE-2022-1471 by disallowing global tags by
default for any usage.

Global tags were never allowed when using the SnakeYAML SafeConstructor
which clj-yaml uses by default. This is why we felt we were not affected
by CVE-2022-1471.

When the user requests `:unsafe` YAML, clj-yaml use the SnakeYAML
Constructor. We adapt to 2.0 changes by continuing to allow globally
unsafe construction under this usage via the TrustedTagInspector, which
allows any and all tags to be created.

For fine grained tag creation permissions, users can opt to use clj-yaml's
`:unknown-tag-fn`.
@lread
Copy link
Collaborator Author

lread commented Feb 26, 2023

New nvd scan CVEs are suppressed for SnakeYAML 1.33, I'll suppress them for SnakeYAML 2.0 as well. Or maybe all versions of SnakeYAML.

@borkdude
Copy link
Collaborator

@lread If I understand correctly, you passed unsafe to the loader and made the change there to make the existing tests pass, right?

@lread
Copy link
Collaborator Author

lread commented Feb 26, 2023

@lread If I understand correctly, you passed unsafe to the loader and made the change there to make the existing tests pass, right?

Thanks for asking @borkdude.
Yes, but only for :unsafe mode.
I've modified clj-yaml :unsafe behaviour to mimic SnakeYAML 1.33 behaviour.
SnakeYAML 2.0 now has tag inspectors.
The default tag inspector will deny all global tags.
When using :unsafe we mimic SnakeYAML 1.33 and allow all global tags via the TrustedTagInspector

@borkdude
Copy link
Collaborator

That seems a good solution to me. Is there anything you are uncertain about?

@lread
Copy link
Collaborator Author

lread commented Feb 26, 2023

That seems a good solution to me. Is there anything you are uncertain about?

I've claimed that clj-yaml's :unknown-tag-fn is equivalent-ish feature-wise to what SnakeYAML's 2.0 tag inspectors offer. I haven't fully validated that claim.

But... as long as :unknown-tag-fn continues to work, that's probably not important at this time.

* master:
  dev: support running nvd scan locally (#89)
I previously determined that 2 false positives did not apply to
SnakeYAML but I configured these false positives to be ignored for v1.33
of SnakeYAML only. Globally ignore them instead to that they apply to
all versions of SnakeYAML.
@lread
Copy link
Collaborator Author

lread commented Feb 28, 2023

I think it might be a good idea to let the dust settle a bit on the SnakeYAML 2.0 release.

Maybe wait a week? Is anybody in a hurry?

For example, this new commit removes a new SnakeYAML class I used in this PR. Easy to adapt, but there might be other things like this coming in the near term.

@borkdude
Copy link
Collaborator

Yes, let's wait a month.

@frenchy64
Copy link
Contributor

frenchy64 commented Mar 8, 2023

Just to confirm my understanding: this PR is not necessary to resolve any security vulnerabilities because it's already safe to use snakeyaml 1.33 via clj-yaml 1.0.26 since clj-yaml is untrusting by default?

@borkdude
Copy link
Collaborator

borkdude commented Mar 8, 2023

@frenchy64 by default, yes

@lread
Copy link
Collaborator Author

lread commented Mar 8, 2023

@frenchy64, yup, that's our understanding and position.

We do plan to upgrade to SnakeYAML 2, but felt it best to wait for the dust to settle.

@frenchy64
Copy link
Contributor

frenchy64 commented Mar 8, 2023

Thanks. Is there any way to parse untrusted yaml with clj-yaml 1.0.26? Want to audit my code base for such code so I can definitively rule out CVE-2022-1471.

@lread
Copy link
Collaborator Author

lread commented Mar 8, 2023

Thanks. Is there any way to parse untrusted yaml with clj-yaml 1.0.26?

See the user guide: https://github.com/clj-commons/clj-yaml/blob/master/doc/01-user-guide.adoc#unsafe

@borkdude
Copy link
Collaborator

borkdude commented Mar 8, 2023

Yes, via an option:

:unsafe true
:unknown-tag-fn ...

Check docs for more info.

@geekingfrog
Copy link

FYI: don't forget to change the project version there
I'm currently using this branch in our builds and had to patch it with sed. (not complaining, it's really a filthy workaround for the security scanner at my org)

@lread
Copy link
Collaborator Author

lread commented Mar 9, 2023

FYI: don't forget to change the project version there

@geekingfrog, do you mean the clj-yaml version?
If so, that is automatically handled by the clj-yaml release workflow and therefore automatically updated when we next release.

@geekingfrog
Copy link

Yes, the clj-yaml version. I missed this workflow, so you're all good.

@lread
Copy link
Collaborator Author

lread commented Mar 25, 2023

@borkdude, it has been about a month... but maybe we'll just wait until SnakeYAML 2.1 is released?

In the short term, for this PR, I'll adapt to current 2.1 changes and sync with master.

@borkdude
Copy link
Collaborator

Yes. I also want to downgrade ordered before a new clj-yaml release since it had some issues with native image.

@lread
Copy link
Collaborator Author

lread commented Mar 25, 2023

Yes. I also want to downgrade ordered before a new clj-yaml release since it had some issues with native image.

If I get all the privs I need, we'll get a release out of ordered with your fix for clj-commons/ordered#71 for native-image sometime soon.

@lread lread changed the title Upgrade to SnakeYAML 2.0 Upgrade to SnakeYAML 2.x Mar 26, 2023
* master:
  bump ordered to 1.15.11 (#99)
  Fix #96: bump ordered (#97)
  Doc typo (#95)
  nvd scan: missed one change (#92)
  maint: github actions deprecations (#91)
  security scan: always use latest tooling (#90)
SnakeYAML 2.1 will remove TrustedTagInspector.

Do effectively the same work via a reify-ed TagInspector.
@lread lread changed the title Upgrade to SnakeYAML 2.x Upgrade to SnakeYAML 2.1 Aug 10, 2023
@lread
Copy link
Collaborator Author

lread commented Aug 10, 2023

Seems like we can finally merge this one! Agreed @borkdude?

@borkdude
Copy link
Collaborator

Yeap!

@lread lread merged commit 2799a9b into master Aug 10, 2023
9 checks passed
@borkdude borkdude deleted the lread-snakeyaml-two-dot-o branch August 10, 2023 13:05
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.

Explore Upgrading to SnakeYAML 2.x
4 participants