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

Possible to improve the usage of snakeyaml due to CVE-2022-1471 #4747

Closed
winniegy opened this issue Jan 9, 2023 · 14 comments
Closed

Possible to improve the usage of snakeyaml due to CVE-2022-1471 #4747

winniegy opened this issue Jan 9, 2023 · 14 comments

Comments

@winniegy
Copy link

winniegy commented Jan 9, 2023

Is your enhancement related to a problem? Please describe

A critical severity CVE, CVE-2022-1471, is discovered recently on snakeyaml, which is a transit-dependency for kubernetes-client-api.
The issue is finally accepted by the community of snakeyaml and a fix has been done on its master branch.
However, there is no plan for a quick release for now.
More details could be found here. https://bitbucket.org/snakeyaml/snakeyaml/issues/561/cve-2022-1471-vulnerability-in

Therefore, I would like to kindly ask if kubernetes-client community could help to add a workaround for this issue so that kubernetes-client itself then is not vulnerable to this CVE.

After following the above issue discussion, the issue finally turns out to be solved by not parsing global tags by default.
As it is now in kubernetes-client-api, most likely the usage is vulnerable.

Describe the solution you'd like

According to the issue https://bitbucket.org/snakeyaml/snakeyaml/issues/561/cve-2022-1471-vulnerability-in on snakeyaml, it is clear that the solution is to ignore global tags from yaml by default so as not to read class names or attempt to instantiate classes from the global tags. Then it would be enough to address the CVE.

In kubernetes-client-api source code, the Yaml class from snakeyaml is initiated here.
https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/Serialization.java#L257

I notice that SafeConstructor is used, and there is CustomYamlTagResolverWithLimit, which helps to safely parse the boolean tag.

I am not experienced in yaml nor in snakeyaml, but I just wonder if it is possible to add yet another ImplicitResolver to simply ignore global tag.

I am not sure if the above suggestion would help but really appreciate it if anyone else could help to suggest a better solution.
As the CVE is critical on snakeyaml, I believe it impacts most of the snakeyaml users.

Thanks in advance.

Describe alternatives you've considered

uplift snakeyaml but they do not plan for a release soon. At least there is no date communicated yet.
I understand kubernetes-client only parses yamls in the response from k8s API, but yet it might be possible to make kubernetes-client not vulnerable to ignore global tags.

Additional context

No response

@manusa
Copy link
Member

manusa commented Jan 9, 2023

I started to follow the SnakeYaml discussion before Christmas holidays, but it was clear at the moment that the CVE was a false positive.

The fix to disable Global tags by default in SnakeYaml is here: https://bitbucket.org/snakeyaml/snakeyaml/issues/565/do-not-allow-global-tags-by-default

AFAIR the use of SafeConstructor should safeguard against this, but maybe I'm wrong.

@winniegy
Copy link
Author

winniegy commented Jan 10, 2023

Hi @manusa,

Thanks for the answer.
My understanding is that by default global tags are allowed. That's probably why snakeyaml made a fix to ignore global tags by default.
And I found the wiki provided by snakeyaml.
https://bitbucket.org/snakeyaml/snakeyaml/wiki/CVE-2022-1471

I am also confused if SafeConstructor would help or not after reading the issue https://bitbucket.org/snakeyaml/snakeyaml/issues/561/cve-2022-1471-vulnerability-in.

Anyway, I post a question on this issue, and hopefully, we can get an answer soon. :)

@winniegy
Copy link
Author

winniegy commented Jan 10, 2023

Hi again @manusa,
Thanks to Andrey from snakeyaml, I got a confirmation it is safe by using SafeConstructor.
More details could be found here. https://bitbucket.org/snakeyaml/snakeyaml/issues/561/cve-2022-1471-vulnerability-in.
And thank you for the reply. No improvement is needed and I will close this improvement then.

@asomov
Copy link
Contributor

asomov commented Jan 11, 2023

@manusa I can contribute the migration to SnakeYAML Engine which is not affected by this CVE (and many others)

@asomov
Copy link
Contributor

asomov commented Jan 11, 2023

The release of SnakeYAML 2.0 with backwards incompatible changes is not coming now because of the Android issue (SnakeYAML has 2 deliverables, for JVM and for Android).
SnakeYAML Engine has only one deliverable (it works for Android without modifications)

@manusa
Copy link
Member

manusa commented Jan 11, 2023

@manusa I can contribute the migration to SnakeYAML Engine which is not affected by this CVE (and many others)

That'd be awesome.

You deserve a monument BTW 🙌

@asomov
Copy link
Contributor

asomov commented Jan 11, 2023

@manusa here is the PR
It fails locally, but it is not directly related to my change. Can you please have a look ?

@manusa
Copy link
Member

manusa commented Jan 11, 2023

Your changes look OK. Our tests are really flaky these days (we're doing some structural refactorings). If the failures you see are not related with your changes, it should be OK, we'll wait for the CI.

@sebastien-workday
Copy link

sebastien-workday commented Mar 13, 2023

Hi. My security scanning tool is still detecting the vulnerability as SnakeYaml 1.33 is still being pulled via jackson:

org.yaml:snakeyaml:1.33
\--- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.14.2
     +--- io.fabric8:kubernetes-client-api:6.5.0

For reference, maven also detects the vulnerability in the dependencies for jackson-dataformat-yaml: https://mvnrepository.com/artifact/com.fasterxml.jackson.dataformat/jackson-dataformat-yaml/2.14.2.

Are there plans to address this? Thanks.

@asomov
Copy link
Contributor

asomov commented Mar 13, 2023

@sebastien-workday why not to report a bug for your scanning tool ? (we are all sick and tired of the low quality of this "scan")

@sebastien-workday
Copy link

sebastien-workday commented Mar 13, 2023

The scan tool is correct as the vulnerable version of snakeyaml is being pulled in via a transitive dependency.

@asomov
Copy link
Contributor

asomov commented Mar 13, 2023

@sebastien-workday please check the CVE itself, not the low quality tool. Jackson does not use SnakeYAML in the way which might be vulnerable. But the low quality tool does not care.

@sebastien-workday
Copy link

Well, I would say that the scan tool is not able to tell how SnakeYAML is being used, or that it is being used in a way that does not trigger the vulnerability.

Thanks for the answer.

@asomov
Copy link
Contributor

asomov commented Mar 14, 2023

@sebastien-workday exactly, the low quality tool does not respect the corresponding CVE and fires a false positive.
It costs us all a lot of time for nothing.
(only the first part of your statement is correct - they simply ignore the most important part of CVE)

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

No branches or pull requests

4 participants