Fix PRISMA-2023-0067 upgrade jackson core to 2.15.4 #23753

Mariamalmesfer · 2024-10-01T09:18:53Z

Description

fixes the jackson-core security vulnerability PRISMA-2023-0067 by upgrading from version 2.11.0 to 2.15.4
PRISMA-2023-0067: Details

Motivation and Context

Reasons for upgradation:
This PR upgrades jackson-core to address a Denial of Service (DoS) vulnerability caused by improper input validation during numeric type conversions. This upgrade prevents resource exhaustion by ensuring proper input validation.

Impact

Image Scan showed the vulnerability have been removed.
Image scan report :
Image-scan-report-9 Sep.csv

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
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

 == RELEASE NOTES == =

Security Changes
* Upgrade Jackson-core to 2.15.4 :pr:`23753`

linux-foundation-easycla · 2024-10-01T09:18:58Z

The committers listed above are authorized under a signed CLA.

✅ login: Mariamalmesfer / name: Mariam AlMesfer (de07533)

elharo

Why not a later version like 2.17.1?

elharo · 2024-10-01T13:17:21Z

I know @steveburnett disagrees with me about this, but I really don't think we should be including dependency upgrades in the release notes.

elharo · 2024-10-01T19:36:17Z

Looks like the most recent version is 2.18.0

Mariamalmesfer · 2024-10-03T17:08:51Z

Looks like the most recent version is 2.18.0

The CVS scan suggests upgrading to version 2.15.0, as that's where the vulnerability was fixed. But if we want the latest updates, we can go with 2.18.0. Let me know your preference

elharo

I'd prefer to go to the latest (honestly I'd prefer not to depend on jackson at all) but this is still an improvement.

agrawalreetika · 2024-10-07T09:24:49Z

@Mariamalmesfer @elharo Maybe we can update to the one version before the very latest which is 2.17.2 what do you think?

Mariamalmesfer · 2024-10-07T10:02:22Z

@Mariamalmesfer @elharo Maybe we can update to the one version before the very latest which is 2.17.2 what do you think?

Sounds good to me. We can go with 2.17.2 if that works for everyone.
cc: @elharo

elharo · 2024-10-07T13:25:23Z

pom.xml

@@ -76,7 +76,7 @@
        <dep.ratis.version>2.2.0</dep.ratis.version>
        <dep.errorprone.version>2.18.0</dep.errorprone.version>
        <dep.guava.version>32.1.0-jre</dep.guava.version>
-        <dep.jackson.version>2.11.0</dep.jackson.version>
+        <dep.jackson.version>2.17.2</dep.jackson.version>


why not 2.18.0?

@Mariamalmesfer @elharo Maybe we can update to the one version before the very latest which is 2.17.2 what do you think?

We can proceed with either version, but since Reetika mentioned updating to 2.17.2, Let me know what you think.

@elharo Since version 2.18.0 is the very latest, I was thinking we could use the previous version instead to avoid potential issues with the latest release.

@elharo, @agrawalreetika pointed out the option to update to 2.17.2 instead of the latest 2.18.0. Please advise on the preferred version.

tdcmeehan · 2024-10-07T13:29:56Z

redis-hbo-provider/pom.xml

                    <ignoredClassPatterns>
                        <ignoredClassPattern>module-info</ignoredClassPattern>
+                        <ignoredClassPattern>META-INF.versions.9.module-info</ignoredClassPattern>


Can you make <ignoredClassPatterns combine.children="append"> so we don't duplicate this in the submodules?

steveburnett · 2024-10-07T13:58:59Z

Please update the release note entry to the version being updated to. For example

 == RELEASE NOTES == =

Security Changes
* Upgrade Jackson-core to 2.17.2 :pr:`#23753`

elharo · 2024-10-10T15:49:07Z

I prefer 2.18.0 but I'm not sure others agree with this. Don't overthink it. Either 2.17.2 or 2.18 is better than what we have now.

tdcmeehan · 2024-10-23T14:07:39Z

pom.xml

                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-enforcer-plugin</artifactId>
+                    <version>3.3.0</version>
+                    <dependencies>


Why do we need this addition to the enforcer plugin?

to prevent issues with different Java versions. Java 8 was trying to load JDK 11-specific classes

Where were the the JDK 11 specific classes being loaded, inside the enforcer plugin? By the way, why is the version of the enforcer plugin upgraded as well?

The JDK 11-specific classes were being loaded as part of Jackson's multi-release JAR (META-INF/versions/11/). These classes are intended to be loaded only on JDK 11 and above, but Maven was attempting to load them in my JDK 8 environment, causing compatibility issues.

Can you please root cause why we are attempting to load JDK 11 classes and revert this change?

The root cause of the issue lies in the multi-release JAR feature in Jackson. The JDK 11-specific classes (located under META-INF/versions/11/) were being loaded even though the build was running on Java 8. This happened because Maven was attempting to load these classes during the build, which caused compatibility issues.

Upgrading the Enforcer plugin to version 3.3.0, along with the extra-enforcer-rules dependency, ensures that Java 8 doesn't attempt to load these JDK 11 classes, as it properly handles multi-release JARs by excluding versioned classes not compatible with the current JDK version.

Can you not override the configuration, and use plugin management to update the version?

The enforcer plugin and its version upgrade are already managed in the pluginManagement section

I have removed the section, but I suggest keeping the extra-enforcer-rules dependency. It helps resolve compatibility issues with multi-release JARs.This prevents build failures. Would it be okay to keep it for stability?

tdcmeehan · 2024-10-23T14:07:55Z

pom.xml

@@ -2357,6 +2408,7 @@
                                    <exclude>com.fasterxml.jackson.core:jackson-annotations</exclude>
                                    <exclude>com.fasterxml.jackson.core:jackson-core</exclude>
                                    <exclude>com.fasterxml.jackson.core:jackson-databind</exclude>
+                                    <exclude>com.google.api.grpc:proto-google-common-protos</exclude>


How is this related to the Jackson upgrade?

to prevent conflicts between multiple versions. Different versions were causing a RequireUpperBoundDeps error in the build, and excluding it ensures the correct version is used.

Where was the conflict originating from? I don't understand how a Jackson upgrade causes a Google proto conflict.

The conflict originates from transitive dependencies in Google Cloud libraries, specifically google-cloud-bigquery and related modules like google-cloud-core and gax-grpc, which bring in different versions of proto-google-common-protos. When upgrading Jackson, the build system caught these conflicts due to the RequireUpperBoundDeps rule, which enforces the use of a single version for shared dependencies. Excluding the older version ensures that the build uses the latest, compatible version of proto-google-common-protos.

Isn't this excluding the enforcement of duplicate dependencies, not "excluding the older version"?

You're right that this is more about managing duplicate dependencies than directly "excluding the older version." When I say "excluding the older version," I mean that by using the tag, we're preventing Maven from including conflicting transitive versions of proto-google-common-protos. This ensures that the build system selects the correct, non-conflicting version that is compatible with the rest of the dependencies.

I do not follow. Isn't this just excluding enforcement of upper bound dependencies? What tag?

Sorry for the confusion. What I meant is that the tag in the enforcer plugin is used to manage conflicting transitive dependencies by preventing Maven from enforcing the upper-bound rule on specific dependencies like proto-google-common-protos. This way, the build process doesn’t fail due to multiple versions being pulled in from different Google Cloud libraries. ensures that Maven doesn’t enforce a version mismatch for proto-google-common-protos, allowing the correct version to be used without causing a conflict.Let me know if this clears things up!

Sorry but I still don't understand, how does this exclusion ensure only one version is being used? Aren't multiple being used and we're just ignoring the dependency? What happens if you remove this line of code?

The exclusion is not just ignoring the dependency it helps resolve a version conflict that occurs specifically in the presto-bigquery module. This conflict arises because different transitive dependencies (like google-cloud-core, gax-grpc, and others) pull in multiple versions of proto-google-common-protos. Without the exclusion, Maven fails the build due to version conflicts, as seen in the error log for presto-bigquery.

By excluding this from the enforcer's upper-bound rule, Maven allows the correct version to be selected during dependency resolution. This issue only occurs in presto-bigquery, so we could limit this exclusion to that module in the enforcer configuration.

Would it be okay to apply it just to presto-bigquery ?

tdcmeehan · 2024-10-25T19:13:02Z

pom.xml

@@ -2357,6 +2408,7 @@
                                    <exclude>com.fasterxml.jackson.core:jackson-annotations</exclude>
                                    <exclude>com.fasterxml.jackson.core:jackson-core</exclude>
                                    <exclude>com.fasterxml.jackson.core:jackson-databind</exclude>
+                                    <exclude>com.google.api.grpc:proto-google-common-protos</exclude>


Sorry but I still don't understand, how does this exclusion ensure only one version is being used? Aren't multiple being used and we're just ignoring the dependency? What happens if you remove this line of code?

tdcmeehan · 2024-10-28T19:21:51Z

LGTM but please fix this typo in the commit message: [Upgraded jakcson-core to 2.15.4](https://github.com/prestodb/presto/pull/23753/commits/7b94b0459c7024b8e3880a06dbb3dbcecc49d8a0). jakcson->jackson

agrawalreetika · 2024-11-04T03:55:55Z

presto-hive/pom.xml

@@ -258,6 +258,7 @@
        <dependency>
            <groupId>com.fasterxml.jackson.core</groupId>
            <artifactId>jackson-databind</artifactId>
+            <version>2.14.3</version>


I see this is already included in root pom - https://github.com/prestodb/presto/pull/23753/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R795

Do we still need version here? Is it coming in higher version as transitive dependency from other place?

The specific issue with jackson-databind 2.15.0 and above affects Presto-Hive tests. Downgrading jackson-databind to 2.14.3 here has stabilized the tests without impacting the rest of the engine.

Seems the serialized test data should be updated instead. This is going in the wrong direction

pom.xml

steveburnett · 2024-11-04T15:00:22Z

Nit I didn't notice earlier, please remove the # from the release note entry

 == RELEASE NOTES == =

Security Changes
* Upgrade Jackson-core to 2.15.4. :pr:`23753`

Mariamalmesfer marked this pull request as ready for review October 1, 2024 09:20

Mariamalmesfer requested review from shangxinli, a team and jaystarshot as code owners October 1, 2024 09:20

Mariamalmesfer requested a review from presto-oss October 1, 2024 09:20

Mariamalmesfer force-pushed the jackson-core-fix branch from e4bfb0c to 6023bad Compare October 1, 2024 09:36

elharo reviewed Oct 1, 2024

View reviewed changes

elharo changed the title ~~Fix PRISMA-2023-0067 upgrade jackson core to 2.0.15~~ Fix PRISMA-2023-0067 upgrade jackson core to 2.15.0 Oct 1, 2024

elharo previously approved these changes Oct 4, 2024

View reviewed changes

sumi-mathew mentioned this pull request Oct 7, 2024

Upgraded the Jackson version to address CVE vulnerabilities from jackson-databind #23752

Closed

6 tasks

Mariamalmesfer dismissed elharo’s stale review via fb101c9 October 7, 2024 10:01

Mariamalmesfer force-pushed the jackson-core-fix branch 3 times, most recently from 6531651 to 840291d Compare October 7, 2024 11:54

elharo changed the title ~~Fix PRISMA-2023-0067 upgrade jackson core to 2.15.0~~ Fix PRISMA-2023-0067 upgrade jackson core to 2.17.2 Oct 7, 2024

elharo reviewed Oct 7, 2024

View reviewed changes

tdcmeehan reviewed Oct 7, 2024

View reviewed changes

tdcmeehan self-assigned this Oct 7, 2024

Mariamalmesfer force-pushed the jackson-core-fix branch from 840291d to 8a1d0b4 Compare October 14, 2024 08:08

Mariamalmesfer requested review from steveburnett, hantangwangd and ZacBlanco as code owners October 14, 2024 08:08

tdcmeehan reviewed Oct 23, 2024

View reviewed changes

Mariamalmesfer force-pushed the jackson-core-fix branch from 9ab076f to fc0bbdd Compare October 25, 2024 17:08

tdcmeehan requested changes Oct 25, 2024

View reviewed changes

Mariamalmesfer force-pushed the jackson-core-fix branch 2 times, most recently from cae6933 to 7b94b04 Compare October 25, 2024 23:10

Mariamalmesfer force-pushed the jackson-core-fix branch 2 times, most recently from 66e4caa to 9aca2fa Compare October 28, 2024 20:45

tdcmeehan previously approved these changes Oct 28, 2024

View reviewed changes

Mariamalmesfer marked this pull request as draft October 29, 2024 15:56

Mariamalmesfer dismissed tdcmeehan’s stale review via fee537a November 3, 2024 16:56

Mariamalmesfer force-pushed the jackson-core-fix branch from 9aca2fa to fee537a Compare November 3, 2024 16:56

Mariamalmesfer marked this pull request as ready for review November 3, 2024 20:23

agrawalreetika requested changes Nov 4, 2024

View reviewed changes

Mariamalmesfer force-pushed the jackson-core-fix branch 8 times, most recently from 1b2a46e to 8208d1e Compare November 4, 2024 11:23

Mariamalmesfer force-pushed the jackson-core-fix branch 5 times, most recently from 592392b to b0b5b17 Compare November 4, 2024 22:04

Upgraded jackson-core to 2.15.4

de07533

Mariamalmesfer force-pushed the jackson-core-fix branch from b0b5b17 to de07533 Compare November 6, 2024 10:59

Fix PRISMA-2023-0067 upgrade jackson core to 2.15.4 #23753

Are you sure you want to change the base?

Fix PRISMA-2023-0067 upgrade jackson core to 2.15.4 #23753

Conversation

Mariamalmesfer commented Oct 1, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

linux-foundation-easycla bot commented Oct 1, 2024 • edited Loading

elharo left a comment

Choose a reason for hiding this comment

elharo commented Oct 1, 2024

elharo commented Oct 1, 2024

Mariamalmesfer commented Oct 3, 2024

elharo left a comment

Choose a reason for hiding this comment

agrawalreetika commented Oct 7, 2024

Mariamalmesfer commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented Oct 7, 2024

elharo commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mariamalmesfer Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan commented Oct 28, 2024

Choose a reason for hiding this comment

Mariamalmesfer Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveburnett commented Nov 4, 2024

Mariamalmesfer commented Oct 1, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 1, 2024 •

edited

Loading

Mariamalmesfer Oct 23, 2024 •

edited

Loading

Mariamalmesfer Nov 4, 2024 •

edited

Loading