Upgrade vulnerable version of reload4j and aws-java-sdk dependencies #24606

ShahimSharafudeen · 2025-02-21T13:15:33Z

Description

Upgrade the aws-java-sdk version to 1.12.782 to fix the ion-java vulnerability - CVE-2024-21634
Remove the reload4j dependency to fix the vulnerability WS-2022-0467

Log4j was removed from the reload4j version 1.2.18.4. Therefore, if we upgrade this dependency, it will cause a Cannot instantiate class TestNGException in the Presto-accum module test classes. To resolve this issue, we are adding the latest version of the Apache Log4j 1.x Compatibility API.

Motivation and Context

To resolve the vulnerability issues on the aws-java-sdk and reload4j dependencies.

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 aws-java-sdk version to 1.12.782 in response to `CVE-2024-21634 <https://nvd.nist.gov/vuln/detail/cve-2024-21634>`_. 
* Remove reload4j dependency in response to `WS-2022-0467 <https://www.mend.io/vulnerability-database/WS-2022-0467>`_.

steveburnett · 2025-02-24T14:50:06Z

Thanks for the release note entry! Suggest linking to the relevant CVEs. See Phrasing in the Release Notes Guidelines for an example.

Security Changes

* Upgrade aws-java-sdk version to 1.12.640 in response to `CVE-2024-21634 <https://nvd.nist.gov/vuln/detail/cve-2024-21634>`_. 
* Upgrad reload4j version to 1.2.22 in response to `WS-2022-0467 <https://github.com/qos-ch/reload4j/issues/53>`_.

ShahimSharafudeen · 2025-02-24T16:57:20Z

Thanks for the release note entry! Suggest linking to the relevant CVEs. See Phrasing in the Release Notes Guidelines for an example.
Security Changes

* Upgrade aws-java-sdk version to 1.12.640 in response to `CVE-2024-21634 <https://nvd.nist.gov/vuln/detail/cve-2024-21634>`_. 
* Upgrad reload4j version to 1.2.22 in response to `WS-2022-0467 <https://github.com/qos-ch/reload4j/issues/53>`_. 

Thanks, @steveburnett for your feedback. I have updated the release note based on your suggestions.

steveburnett · 2025-02-24T19:34:33Z

Thanks for the release note entry! Suggest linking to the relevant CVEs. See Phrasing in the Release Notes Guidelines for an example.
Security Changes

* Upgrade aws-java-sdk version to 1.12.640 in response to `CVE-2024-21634 <https://nvd.nist.gov/vuln/detail/cve-2024-21634>`_. 
* Upgrad reload4j version to 1.2.22 in response to `WS-2022-0467 <https://github.com/qos-ch/reload4j/issues/53>`_. 
Thanks, @steveburnett for your feedback. I have updated the release note based on your suggestions.

Thanks for updating the release note entry! Sorry about making a mistake in my suggested draft - could you fix my typo in the second line? "Upgrad reload4j" should be "Upgrade reload4j".

agrawalreetika · 2025-02-25T18:37:20Z

pom.xml

        <dep.slice.version>0.38</dep.slice.version>
        <dep.testing-mysql-server-5.version>0.6</dep.testing-mysql-server-5.version>
-        <dep.aws-sdk.version>1.12.560</dep.aws-sdk.version>
+        <dep.aws-sdk.version>1.12.640</dep.aws-sdk.version>


@ShahimSharafudeen Looks like even version 1.12.640 has CVEs for artifact aws-java-sdk-core that uses this SDK version in Presto.
https://mvnrepository.com/artifact/com.amazonaws/aws-java-sdk-core/1.12.640 (But it looks like those CVEs are in test dependencies)

Other 2 AWS artifact aws-java-sdk-glue & aws-java-sdk-s3 are CVEs free even with existing aws version 1.12.560

@agrawalreetika - If we check version 1.12.560, we can see that an ion-java vulnerability exists there. In version 1.12.640, it was resolved and has no vulnerabilities. So, we used the resolved version here.

We should just upgrade to the latest version (1.12.782) to ensure it solves as many CVEs as possible.

agrawalreetika · 2025-02-25T18:54:17Z

presto-accumulo/pom.xml

        <dep.accumulo.version>1.10.1</dep.accumulo.version>
        <dep.curator.version>2.12.0</dep.curator.version>
-        <dep.reload4j.version>1.2.18.3</dep.reload4j.version>
+        <dep.reload4j.version>1.2.22</dep.reload4j.version>


Why this upgrade? I see exsiting and new both the version has CVEs coming from test dependencies-
https://mvnrepository.com/artifact/ch.qos.reload4j/reload4j/1.2.18.3
https://mvnrepository.com/artifact/ch.qos.reload4j/reload4j/1.2.22

I don't see that's resolved even on latest version.

@agrawalreetika - Regarding the reload4j dependency, we can see a vulnerability identified by the Mend tool scan (OSS Scan - WS-2022-0467), which is also described in the PR description. The fix is only available in reload4j version 1.2.22

We should just upgrade to the latest (1.2.26) if it does actually resolve a CVE. However, as Reetika pointed out, the mvnrepository link shows the CVE has not been resolved in any new version.

@ShahimSharafudeen can you link to the specific CVE the scan was reporting. The github issue linked has no attached CVE.

@ShahimSharafudeen What's your response to Reetika and Zac's comments? I see this line is deleted from the second commit, so here the change should be reverted because it doesn't fix the CVE

@yingsu00 - When upgrading the Reload4j version, Zookeeper fails to start in the test runs due to a missing Log4j dependency, and we couldn't fix it. Therefore, @ZacBlanco suggested removing the Reload4j dependency and adding the org.apache.logging.log4j dependencies instead, to ensure Zookeeper loads as expected.

Just to add - reload4j was just a way to allow accumulo and zookeeper to use their log4j 1.x API even when the implementation is 2.0. Since reload4j was the dependency with the CVE, I recommended we remove it entirely to resolve the CVE and just use the log4j 1.X API facade dependency instead

presto-accumulo/pom.xml

ZacBlanco · 2025-02-27T20:43:16Z

presto-accumulo/pom.xml

        <dep.accumulo.version>1.10.1</dep.accumulo.version>
        <dep.curator.version>2.12.0</dep.curator.version>
-        <dep.reload4j.version>1.2.18.3</dep.reload4j.version>
+        <dep.reload4j.version>1.2.22</dep.reload4j.version>


We should just upgrade to the latest (1.2.26) if it does actually resolve a CVE. However, as Reetika pointed out, the mvnrepository link shows the CVE has not been resolved in any new version.

@ShahimSharafudeen can you link to the specific CVE the scan was reporting. The github issue linked has no attached CVE.

ZacBlanco · 2025-02-27T20:45:23Z

pom.xml

        <dep.slice.version>0.38</dep.slice.version>
        <dep.testing-mysql-server-5.version>0.6</dep.testing-mysql-server-5.version>
-        <dep.aws-sdk.version>1.12.560</dep.aws-sdk.version>
+        <dep.aws-sdk.version>1.12.640</dep.aws-sdk.version>


We should just upgrade to the latest version (1.12.782) to ensure it solves as many CVEs as possible.

ZacBlanco · 2025-02-28T20:14:42Z

I played around with this myself. I think I have a better solution for the reload4j bits. We can remove reload4j entirely and just migrate to using the log4j-1.2 facade instead. Here is a diff. We might want to move the log4j dependencies out of the test scoped dependency section though

diff --git a/presto-accumulo/pom.xml b/presto-accumulo/pom.xml
index bc6ccad6f6..3d614af547 100644
--- a/presto-accumulo/pom.xml
+++ b/presto-accumulo/pom.xml
@@ -16,7 +16,6 @@
         <air.main.basedir>${project.parent.basedir}</air.main.basedir>
         <dep.accumulo.version>1.10.1</dep.accumulo.version>
         <dep.curator.version>2.12.0</dep.curator.version>
-        <dep.reload4j.version>1.2.22</dep.reload4j.version>
     </properties>

     <dependencyManagement>
@@ -265,20 +264,6 @@
             <artifactId>jackson-databind</artifactId>
         </dependency>

-        <dependency>
-            <groupId>ch.qos.reload4j</groupId>
-            <artifactId>reload4j</artifactId>
-            <version>${dep.reload4j.version}</version>
-        </dependency>
-
-        <!-- log4j removed from  reload4j version 1.2.18.4-->
-        <dependency>
-            <groupId>org.apache.logging.log4j</groupId>
-            <artifactId>log4j-1.2-api</artifactId>
-            <version>2.24.3</version>
-            <scope>test</scope>
-        </dependency>
-
         <!-- Presto SPI -->
         <dependency>
             <groupId>com.facebook.presto</groupId>
@@ -359,6 +344,22 @@
             <artifactId>testng</artifactId>
             <scope>test</scope>
         </dependency>
+
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-core</artifactId>
+        </dependency>
+
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-api</artifactId>
+        </dependency>
+
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-1.2-api</artifactId>
+            <version>${dep.log4j.version}</version>
+        </dependency>
     </dependencies>

     <profiles>
@@ -377,21 +378,4 @@
             </build>
         </profile>
     </profiles>
-
-    <build>
-        <plugins>
-            <plugin>
-                <groupId>org.basepom.maven</groupId>
-                <artifactId>duplicate-finder-maven-plugin</artifactId>
-                <configuration>
-                    <ignoredResourcePatterns>
-                        <ignoredResourcePattern>org/apache/log4j/xml/log4j.dtd</ignoredResourcePattern>
-                    </ignoredResourcePatterns>
-                    <ignoredClassPatterns>
-                        <ignoredClassPattern>org.apache.log4j.*</ignoredClassPattern>
-                    </ignoredClassPatterns>
-                </configuration>
-            </plugin>
-        </plugins>
-    </build>
 </project>

Also, can you split this PR into two separate commits - one for the log4j upgrade, one for the AWS SDK upgrade? In case there are issues, it will make a revert easier in the future.

ShahimSharafudeen · 2025-03-03T08:36:16Z

I played around with this myself. I think I have a better solution for the reload4j bits. We can remove reload4j entirely and just migrate to using the log4j-1.2 facade instead. Here is a diff. We might want to move the log4j dependencies out of the test scoped dependency section though

diff --git a/presto-accumulo/pom.xml b/presto-accumulo/pom.xml
index bc6ccad6f6..3d614af547 100644
--- a/presto-accumulo/pom.xml
+++ b/presto-accumulo/pom.xml
@@ -16,7 +16,6 @@
         <air.main.basedir>${project.parent.basedir}</air.main.basedir>
         <dep.accumulo.version>1.10.1</dep.accumulo.version>
         <dep.curator.version>2.12.0</dep.curator.version>
-        <dep.reload4j.version>1.2.22</dep.reload4j.version>
     </properties>

     <dependencyManagement>
@@ -265,20 +264,6 @@
             <artifactId>jackson-databind</artifactId>
         </dependency>

-        <dependency>
-            <groupId>ch.qos.reload4j</groupId>
-            <artifactId>reload4j</artifactId>
-            <version>${dep.reload4j.version}</version>
-        </dependency>
-
-        <!-- log4j removed from  reload4j version 1.2.18.4-->
-        <dependency>
-            <groupId>org.apache.logging.log4j</groupId>
-            <artifactId>log4j-1.2-api</artifactId>
-            <version>2.24.3</version>
-            <scope>test</scope>
-        </dependency>
-
         <!-- Presto SPI -->
         <dependency>
             <groupId>com.facebook.presto</groupId>
@@ -359,6 +344,22 @@
             <artifactId>testng</artifactId>
             <scope>test</scope>
         </dependency>
+
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-core</artifactId>
+        </dependency>
+
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-api</artifactId>
+        </dependency>
+
+        <dependency>
+            <groupId>org.apache.logging.log4j</groupId>
+            <artifactId>log4j-1.2-api</artifactId>
+            <version>${dep.log4j.version}</version>
+        </dependency>
     </dependencies>

     <profiles>
@@ -377,21 +378,4 @@
             </build>
         </profile>
     </profiles>
-
-    <build>
-        <plugins>
-            <plugin>
-                <groupId>org.basepom.maven</groupId>
-                <artifactId>duplicate-finder-maven-plugin</artifactId>
-                <configuration>
-                    <ignoredResourcePatterns>
-                        <ignoredResourcePattern>org/apache/log4j/xml/log4j.dtd</ignoredResourcePattern>
-                    </ignoredResourcePatterns>
-                    <ignoredClassPatterns>
-                        <ignoredClassPattern>org.apache.log4j.*</ignoredClassPattern>
-                    </ignoredClassPatterns>
-                </configuration>
-            </plugin>
-        </plugins>
-    </build>
 </project>

Also, can you split this PR into two separate commits - one for the log4j upgrade, one for the AWS SDK upgrade? In case there are issues, it will make a revert easier in the future.

Ya sure @ZacBlanco .

ShahimSharafudeen · 2025-03-03T13:02:06Z

@ZacBlanco - Updated the changes and commits as as per above comment.

ZacBlanco

last thing

presto-accumulo/pom.xml

ZacBlanco

Thanks for the improvements @ShahimSharafudeen !

imjalpreet · 2025-03-04T13:29:34Z

@yingsu00 / @jaystarshot Can you please have a look whenever you get a chance? Thanks.

ZacBlanco · 2025-03-11T04:41:26Z

@ShahimSharafudeen you'll need to rebase your PR in order to merge due to the updated github status checks

yingsu00 · 2025-03-11T05:47:34Z

@ShahimSharafudeen Test failures are relevant to the change. Can you please fix?

Remove the reload4j dependency and add the org.apache.logging.log4j dependency for log4j support in response to WS-2022-0467

ShahimSharafudeen · 2025-03-11T17:38:41Z

@ShahimSharafudeen you'll need to rebase your PR in order to merge due to the updated github status checks

@ZacBlanco - Rebased the PR and all checks are passed.

ShahimSharafudeen · 2025-03-11T17:40:02Z

@ShahimSharafudeen Test failures are relevant to the change. Can you please fix?

@yingsu00 - Fixed all test failures after rebase.

ZacBlanco · 2025-03-11T17:40:50Z

@yingsu00 please look again and merge when convenient, thanks!

prestodb-ci added the from:IBM PR from IBM label Feb 21, 2025

ShahimSharafudeen force-pushed the prestodb_forwardfit_CVE_fix_ion-java branch 2 times, most recently from ca2dee0 to 46e1d2a Compare February 24, 2025 07:25

ShahimSharafudeen marked this pull request as ready for review February 24, 2025 12:04

ShahimSharafudeen requested a review from a team as a code owner February 24, 2025 12:04

ShahimSharafudeen requested a review from jaystarshot February 24, 2025 12:04

imjalpreet self-requested a review February 25, 2025 09:25

agrawalreetika requested changes Feb 25, 2025

View reviewed changes

ZacBlanco requested changes Feb 27, 2025

View reviewed changes

ShahimSharafudeen force-pushed the prestodb_forwardfit_CVE_fix_ion-java branch from 46e1d2a to 68749f7 Compare March 3, 2025 09:51

ZacBlanco requested changes Mar 3, 2025

View reviewed changes

presto-accumulo/pom.xml Outdated Show resolved Hide resolved

ShahimSharafudeen force-pushed the prestodb_forwardfit_CVE_fix_ion-java branch from 68749f7 to fd5a0fd Compare March 3, 2025 18:30

ZacBlanco previously approved these changes Mar 3, 2025

View reviewed changes

agrawalreetika previously approved these changes Mar 4, 2025

View reviewed changes

imjalpreet previously approved these changes Mar 4, 2025

View reviewed changes

yingsu00 previously approved these changes Mar 11, 2025

View reviewed changes

Upgrade aws-java-sdk version to 1.12.782 in response to CVE-2024-21634

61721e3

ShahimSharafudeen force-pushed the prestodb_forwardfit_CVE_fix_ion-java branch from fd5a0fd to 283e7d4 Compare March 11, 2025 05:44

yingsu00 self-requested a review March 11, 2025 05:46

ShahimSharafudeen dismissed imjalpreet’s stale review via e82f2bd March 11, 2025 06:13

ShahimSharafudeen dismissed stale reviews from agrawalreetika, yingsu00, and ZacBlanco via e82f2bd March 11, 2025 06:13

ShahimSharafudeen force-pushed the prestodb_forwardfit_CVE_fix_ion-java branch 3 times, most recently from 0892107 to 455cf97 Compare March 11, 2025 09:46

Remove reload4j dependency in response to WS-2022-0467

3bdc0dd

Remove the reload4j dependency and add the org.apache.logging.log4j dependency for log4j support in response to WS-2022-0467

ShahimSharafudeen force-pushed the prestodb_forwardfit_CVE_fix_ion-java branch from 455cf97 to 3bdc0dd Compare March 11, 2025 10:03

yingsu00 approved these changes Mar 12, 2025

View reviewed changes

yingsu00 merged commit e98ef49 into prestodb:master Mar 12, 2025
92 checks passed

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

ethanyzhang added the Security label Apr 3, 2025

prestodb-ci requested review from a team, ScrapCodes and imsayari404 and removed request for a team April 3, 2025 04:04

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

Upgrade vulnerable version of reload4j and aws-java-sdk dependencies #24606

Upgrade vulnerable version of reload4j and aws-java-sdk dependencies #24606

Uh oh!

Conversation

ShahimSharafudeen commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett commented Feb 24, 2025

Uh oh!

ShahimSharafudeen commented Feb 24, 2025

Uh oh!

steveburnett commented Feb 24, 2025

Uh oh!

agrawalreetika Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShahimSharafudeen Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacBlanco commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShahimSharafudeen commented Mar 3, 2025

Uh oh!

ShahimSharafudeen commented Mar 3, 2025

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet commented Mar 4, 2025

Uh oh!

ZacBlanco commented Mar 11, 2025

Uh oh!

yingsu00 commented Mar 11, 2025

Uh oh!

ShahimSharafudeen commented Mar 11, 2025

Uh oh!

ShahimSharafudeen commented Mar 11, 2025

Uh oh!

ZacBlanco commented Mar 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ShahimSharafudeen commented Feb 21, 2025 •

edited

Loading

agrawalreetika Feb 25, 2025 •

edited

Loading

ShahimSharafudeen Feb 26, 2025 •

edited

Loading

ZacBlanco commented Feb 28, 2025 •

edited

Loading