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

Security Vulnerability fix for helix-core [CVE-2023-38647] #23820

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mehradpk
Copy link
Contributor

@mehradpk mehradpk commented Oct 14, 2024

Description

Fixes the helix-core security vulnerability CVE-2023-38647 by removing helix-core package from presto-pinot.

Motivation and Context

An attacker can use SnakeYAML to deserialize java.net.URLClassLoader and make it load a JAR from a specified URL, and then deserialize javax.script.ScriptEngineManager to load code using that ClassLoader. This unbounded deserialization can likely lead to remote code execution. The code can be run in Helix REST start and Workflow creation. Affect all the versions lower and include 1.2.0. Affected products: helix-core, helix-rest Mitigation: Short term, stop using any YAML based configuration and workflow creation.

Impact

NA

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
* Remove helix-core from presto-pinot to address `CVE-2023-38647 <https://nvd.nist.gov/vuln/detail/CVE-2023-38647>` :pr:`#23820`

@mehradpk mehradpk requested a review from a team as a code owner October 14, 2024 08:35
@mehradpk mehradpk requested a review from presto-oss October 14, 2024 08:35
Copy link

linux-foundation-easycla bot commented Oct 14, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@steveburnett
Copy link
Contributor

Please revise the release note entry to include the PR number, and also to follow the Order of changes in the Release Notes Guidelines.

Maybe something like:

== RELEASE NOTES ==

Security Changes
* Remove helix-core from presto-pinot to address `CVE-2023-38647 <https://nvd.nist.gov/vuln/detail/CVE-2023-38647>`_ :pr:`23820`

@mehradpk
Copy link
Contributor Author

Please revise the release note entry to include the PR number, and also to follow the Order of changes in the Release Notes Guidelines.

Maybe something like:

== RELEASE NOTES ==

Security Changes
* Remove helix-core from presto-pinot to address `CVE-2023-38647 <https://nvd.nist.gov/vuln/detail/CVE-2023-38647>`_ :pr:`23820`

Thanks for pointing out. Updated the release note.

Copy link
Member

@agrawalreetika agrawalreetika left a comment

Choose a reason for hiding this comment

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

Please squash your commits into a single commit may be something like -
Fix CVE-2023-38647 helix-core in presto-pinot connector

Also rebase your PR on lastest master as well.

@@ -30,6 +30,10 @@
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_annotations</artifactId>
</exclusion>
<exclusion>
Copy link
Member

Choose a reason for hiding this comment

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

Are we confident this dependency is not required? Can we not upgrade the dependency to a newer version without the CVE?

Copy link
Member

Choose a reason for hiding this comment

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

I see that the most recent versions of helix-core don't have any CVE - https://mvnrepository.com/artifact/org.apache.helix/helix-core

I also see that newer versions of Pinot are already on one of these versions, we can also explore upgrading Pinot if it's relatively less complex.

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.

4 participants