Skip to content

Security Vulnerability fix for snappy-0.2(CVE-2024-36124)#24159

Closed
namya28 wants to merge 1 commit intoprestodb:masterfrom
namya28:snappy_vulfix
Closed

Security Vulnerability fix for snappy-0.2(CVE-2024-36124)#24159
namya28 wants to merge 1 commit intoprestodb:masterfrom
namya28:snappy_vulfix

Conversation

@namya28
Copy link
Contributor

@namya28 namya28 commented Nov 27, 2024

Description

This PR is fixing the security vulnerability for the library "snappy-0.2" (CVE-2024-36124) https://mvnrepository.com/artifact/org.iq80.snappy/snappy/0.2. The library has been upgraded to the version 0.5 (https://mvnrepository.com/artifact/org.iq80.snappy/snappy/0.5). This fixes CVE-2024-36124.

Motivation and Context

The snappy library currently used has a security vulnerability for the version 0.2. This PR focuses on upgrading the version to 0.5 to fix the current vulnerability.

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Security Changes

* Upgrade the snappy version to 0.5 in response to `CVE-2024-36124 <https://github.com/advisories/GHSA-8wh2-6qhj-h7j9>`_. :pr:`24159`

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Nov 27, 2024
@prestodb-ci prestodb-ci requested review from a team, infvg and nishithakbhaskaran and removed request for a team November 27, 2024 09:45
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 27, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: namya28 / name: Namya Sehgal (f6e2e60)

<exclusion>
<groupId>org.iq80.snappy</groupId>
<artifactId>snappy</artifactId>
</exclusion>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we excluding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was an upper bound dependencies error for org.iq80.snappy:snappy:0.2 from this module(Attaching below for reference) :

+-com.facebook.presto:presto-hive:0.291-SNAPSHOT
+-com.facebook.hive:hive-dwrf:0.8.5
+-org.iq80.snappy:snappy:0.2
and
+-com.facebook.presto:presto-hive:0.291-SNAPSHOT
+-com.facebook.presto:presto-hive-metastore:0.291-SNAPSHOT
+-org.iq80.snappy:snappy:0.5
and
+-com.facebook.presto:presto-hive:0.291-SNAPSHOT
+-com.facebook.presto:presto-hive-metastore:0.291-SNAPSHOT
+-org.iq80.snappy:snappy:0.5

<artifactId>snappy</artifactId>
<version>0.5</version>
</dependency>

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of bringing in this dependency, have you tried adding this upgrade to the root pom dependency management?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a transitive dependency coming from this module. I will try adding in the root pom as well. Thanks for the suggestion.

@infvg
Copy link
Contributor

infvg commented Nov 27, 2024

Could you please squash the commits and modify the commit message to follow the guidelines?

The release note should also contain the CVE as per the guidelines
eg:
* Upgrade okio to 3.6.0 in response to `CVE-2023-3635 <https://github.com/advisories/GHSA-w33c-445m-f8w7>`_. :pr:`23796

<artifactId>snappy</artifactId>
<version>0.5</version>
</dependency>

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also get this in the root pom?

This upgrade eliminates the vulnerability present in the version 0.2 which fixes CVE-2024-36124.
@namya28
Copy link
Contributor Author

namya28 commented Nov 28, 2024

Hi @infvg , thank you for your suggestions and comments. I have addressed all the review comments. Could you please re-review the PR once again.

@namya28 namya28 requested a review from infvg November 28, 2024 16:57
@namya28 namya28 marked this pull request as ready for review November 28, 2024 16:58
@namya28 namya28 requested a review from a team as a code owner November 28, 2024 16:58
@namya28 namya28 requested a review from presto-oss November 28, 2024 16:58
Copy link
Contributor

@infvg infvg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nishithakbhaskaran nishithakbhaskaran left a comment

Choose a reason for hiding this comment

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

Looks good


<dependencyManagement>
<dependencies>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is ultimately bringing in and using this dependency?

Copy link
Contributor Author

@namya28 namya28 Dec 4, 2024

Choose a reason for hiding this comment

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

@tdcmeehan The version earlier used was 0.2 which was present transitively across modules , causing the vulnerability. By bringing in this dependency and the version 0.5 fixes the vulnerability as 0.5 is the vulnerable free version for this library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but my question is what is using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is being transitively used by the modules presto-hive-hadoop2 , presto-hive , presto-native-execution and presto-hive-metastore . (Attaching a screenshot for reference)
Screenshot 2024-12-04 at 9 43 18 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update hive-dwrf instead of overriding it at the top level here?

Copy link
Contributor Author

@namya28 namya28 Dec 5, 2024

Choose a reason for hiding this comment

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

@tdcmeehan , introducing the latest version of hive-dwrf introduces a lot of vulnerabilities. (https://mvnrepository.com/artifact/com.facebook.hive/hive-dwrf/0.18.9). (Attaching the screenshot from the maven repository). Although even if we try upgrading hive-dwrf, it does not resolve the vulnerability directly as it still introduces the 0.2 version of snappy transitively.
Screenshot 2024-12-05 at 8 49 08 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tdcmeehan , could you please have a look at the comments and re-review if possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is, simply fix the vulnerabilities in hive-dwrf, cut a release, then update here. https://github.com/prestodb/presto-hive-dwrf

@namya28 namya28 requested a review from tdcmeehan December 10, 2024 08:27
@namya28
Copy link
Contributor Author

namya28 commented Feb 25, 2025

Closing this PR as this CVE was fixed by upgrading the snappy version in the new hive-dwrf release.
Attaching the PRs:
The PR Link to prestodb/presto: #24461
The PR link to upgrade snappy in prestodb/presto-hive-dwrf : prestodb/presto-hive-dwrf#7

@namya28 namya28 closed this Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:IBM PR from IBM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants