-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 Fix: Okio CVE-2023-3635 + OkHttp Jar Update #23796
Conversation
|
1e17bcc
to
8bed2f9
Compare
8bed2f9
to
4bd3bc1
Compare
f8715a8
to
e3b47f0
Compare
e13efb9
to
dab801a
Compare
Thanks for the release note entry! Nit to remove
|
af5b4c3
to
ef04384
Compare
5de7a82
to
a0525f5
Compare
5549d70
to
472111e
Compare
1ed5d8a
to
406b1d1
Compare
9b55b09
to
83aa236
Compare
83aa236
to
cd95fc3
Compare
presto-client/src/main/java/okhttp/internal/tls/DistinguishedNameParser.java
Show resolved
Hide resolved
85bca20
to
f3d345d
Compare
presto-client/src/main/java/okhttp/internal/tls/DistinguishedNameParser.java
Outdated
Show resolved
Hide resolved
72a9e67
to
d4aee3e
Compare
…n 4.12.0 to address CVE-2023-3635.
d4aee3e
to
4bbd3de
Compare
@@ -2352,6 +2352,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>org.jetbrains.kotlin:kotlin-stdlib-jdk8</exclude> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mariamalmesfer Please do not ignore depdendency conflicts. It causes problems in projects using various Presto libraries as a dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ignored this because OkHttp brings in two versions of this dependency. It seems to be entirely within the OkHttp libraries: #23796 (comment)
If our enforcer plugin complains about this, how would you fix it? It seems to be a problem with the upstream dependency and not something Presto should control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a known issue upstream but they don't seem to plan on fixing it in the 4.x line. square/okhttp#8288 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the runtime you cannot have two versions of the same class. At the end of the day the VM will have to pick one, possibly in a non deterministic way. Silencing the check doesn't make the problem go away. We need to pick a single version explicitly and exclude the other one to resolve the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZacBlanco I looked at a problem a little bit more. As you mentioned it looks like okio-jvm
depends on a different version of the kotlin-stdlib-jdk8
and the kotlin-stdlib-common
libraries. okio-jvm
depends on version 1.9.10
while the okhttp
depends on 1.8.21
.
Consider excluding kotlin-stdlib-jdk8
, kotlin-stdlib-common
from the okio-jvm
in the dependency management section of the main pom:
<dependency>
<groupId>com.squareup.okio</groupId>
<artifactId>okio-jvm</artifactId>
<version>3.6.0</version>
<exclusions>
<exclusion>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-stdlib-jdk8</artifactId>
</exclusion>
<exclusion>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-stdlib-common</artifactId>
</exclusion>
</exclusions>
</dependency>
And including this dependency in the presto-client/pom.xml:
<dependency>
<groupId>com.squareup.okio</groupId>
<artifactId>okio-jvm</artifactId>
</dependency>
Unfortunately once you do this the dependency checker complains that the okio-jvm
is unused. However due to Kotlin quirks it has to stay in compile scope. To workaround that consider referencing a class from the okio-jvm
in a Dummy class:
import okio.ByteString;
public class Dummy
{
static {
okio.ByteString byteString = new ByteString(new byte[] {});
System.out.println(byteString);
}
}
This should hopefully resolve the conflict. This approach is better as it doesn't push the problem down to the presto-jdbc
consumers.
Additionally you need to shade kotlin dependencies (kotlin.) as well as Jetbrains annotations (org.intellij., org.jetbrains.*) here to avoid class name conflicts in presto-jdbc.
@@ -55,6 +55,12 @@ | |||
<dependency> | |||
<groupId>com.squareup.okhttp3</groupId> | |||
<artifactId>okhttp</artifactId> | |||
<exclusions> | |||
<exclusion> | |||
<groupId>org.jetbrains</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What issue does this dependency cause? Is this dependency needed in runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exclusion prevents a build failure due to scope conflicts. Without it, Maven reports scope issues that lead to build failures. This dependency isn't needed at runtime; it's only required to pass the build checks.
@ethanyzhang imported this issue into IBM GitHub Enterprise |
Description
This PR fixes the Okio security vulnerability (CVE-2023-3635) by upgrading from version 1.17.2 to 3.6.0. It also includes an update to the OkHttp jar from 3.9.0 to 4.12.0
Motivation and Context
A flaw was found in Okio’s GzipSource class that doesn’t handle exceptions properly, allowing potential Denial of Service (DoS) attacks with malformed files.
CVE-2023-3635: Details
Impact
Image Scan showed the vulnerability have been removed.
correlation-report-ibm-lh-presto-okie check.csv
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.