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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 28, 2025
Merged

Security Fix: Okio CVE-2023-3635 + OkHttp Jar Update #23796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
<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.okhttp.version>3.9.0</dep.okhttp.version>
<dep.okhttp.version>4.12.0</dep.okhttp.version>
Mariamalmesfer marked this conversation as resolved.
Show resolved Hide resolved
<dep.jdbi3.version>3.4.0</dep.jdbi3.version>
<dep.oracle.version>19.3.0.0</dep.oracle.version>
<dep.drift.version>1.40</dep.drift.version>
Expand Down Expand Up @@ -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>
ZacBlanco marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

#24459

Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Member

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.

Copy link
Member

@arhimondr arhimondr Jan 30, 2025 •
edited
Loading

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.

</excludes>
</requireUpperBoundDeps>
</rules>
Expand Down
6 changes: 0 additions & 6 deletions presto-client/src/main/java/com/facebook/presto/client/JsonResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import javax.annotation.Nullable;

import java.io.IOException;
import java.io.InterruptedIOException;
import java.io.UncheckedIOException;

import static com.google.common.base.MoreObjects.toStringHelper;
Expand Down Expand Up @@ -146,11 +145,6 @@ public static <T> JsonResponse<T> execute(JsonCodec<T> codec, OkHttpClient clien
return new JsonResponse<>(response.code(), response.message(), response.headers(), body);
}
catch (IOException e) {
// OkHttp throws this after clearing the interrupt status
// TODO: remove after updating to Okio 1.15.0+
if ((e instanceof InterruptedIOException) && "thread interrupted".equals(e.getMessage())) {
Thread.currentThread().interrupt();
}
throw new UncheckedIOException(e);
}
}
Expand Down
2 changes: 2 additions & 0 deletions presto-client/src/main/java/com/facebook/presto/client/OkHttpUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.facebook.airlift.security.pem.PemReader;
import com.google.common.base.CharMatcher;
import com.google.common.net.HostAndPort;
import okhttp.internal.tls.LegacyHostnameVerifier;
import okhttp3.Call;
import okhttp3.Callback;
import okhttp3.Credentials;
Expand Down Expand Up @@ -237,6 +238,7 @@ public static void setupSsl(
sslContext.init(keyManagers, new TrustManager[] {trustManager}, null);

clientBuilder.sslSocketFactory(sslContext.getSocketFactory(), trustManager);
clientBuilder.hostnameVerifier(LegacyHostnameVerifier.INSTANCE);
aaneja marked this conversation as resolved.
Show resolved Hide resolved
}
catch (GeneralSecurityException | IOException e) {
throw new ClientException("Error setting up SSL: " + e.getMessage(), e);
Expand Down
Loading
Loading