From 59f362495a3ec932b16827cb8abc1ee274a3822c Mon Sep 17 00:00:00 2001
From: Greyson Parrelli <greyson@signal.org>
Date: Mon, 6 May 2019 12:25:53 -0700
Subject: [PATCH] Add additional checks around link preview domains.

We never make requests to non-whitelisted domains, but there were
situations where some links would redirect to non-whitelisted domains,
which would hit a final failsafe that resulted in a crash.

To prevent this, we detect bad redirects earlier and fail more
gracefully.

Fixes #8796
---
 .../glide/ChunkedImageUrlLoader.java          |  2 +
 .../linkpreview/LinkPreviewRepository.java    |  2 +
 .../linkpreview/LinkPreviewUtil.java          |  9 ++-
 .../net/ContentProxySafetyInterceptor.java    | 58 +++++++++++++++++++
 4 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 src/org/thoughtcrime/securesms/net/ContentProxySafetyInterceptor.java

diff --git a/src/org/thoughtcrime/securesms/glide/ChunkedImageUrlLoader.java b/src/org/thoughtcrime/securesms/glide/ChunkedImageUrlLoader.java
index fe0d765b8d..5ac02f33f6 100644
--- a/src/org/thoughtcrime/securesms/glide/ChunkedImageUrlLoader.java
+++ b/src/org/thoughtcrime/securesms/glide/ChunkedImageUrlLoader.java
@@ -8,6 +8,7 @@ import com.bumptech.glide.load.model.ModelLoaderFactory;
 import com.bumptech.glide.load.model.MultiModelLoaderFactory;
 
 import org.thoughtcrime.securesms.giph.model.ChunkedImageUrl;
+import org.thoughtcrime.securesms.net.ContentProxySafetyInterceptor;
 import org.thoughtcrime.securesms.net.ContentProxySelector;
 
 import java.io.InputStream;
@@ -41,6 +42,7 @@ public class ChunkedImageUrlLoader implements ModelLoader<ChunkedImageUrl, Input
       this.client  = new OkHttpClient.Builder()
                                      .proxySelector(new ContentProxySelector())
                                      .cache(null)
+                                     .addNetworkInterceptor(new ContentProxySafetyInterceptor())
                                      .addNetworkInterceptor(new PaddedHeadersInterceptor())
                                      .build();
     }
diff --git a/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewRepository.java b/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewRepository.java
index 4cb5e6f577..bda1461a4b 100644
--- a/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewRepository.java
+++ b/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewRepository.java
@@ -18,6 +18,7 @@ import org.thoughtcrime.securesms.logging.Log;
 import org.thoughtcrime.securesms.mms.GlideApp;
 import org.thoughtcrime.securesms.net.CallRequestController;
 import org.thoughtcrime.securesms.net.CompositeRequestController;
+import org.thoughtcrime.securesms.net.ContentProxySafetyInterceptor;
 import org.thoughtcrime.securesms.net.ContentProxySelector;
 import org.thoughtcrime.securesms.net.RequestController;
 import org.thoughtcrime.securesms.providers.BlobProvider;
@@ -49,6 +50,7 @@ public class LinkPreviewRepository {
   public LinkPreviewRepository() {
     this.client = new OkHttpClient.Builder()
                                   .proxySelector(new ContentProxySelector())
+                                  .addNetworkInterceptor(new ContentProxySafetyInterceptor())
                                   .cache(null)
                                   .build();
   }
diff --git a/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewUtil.java b/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewUtil.java
index cefa024664..81697c7cdd 100644
--- a/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewUtil.java
+++ b/src/org/thoughtcrime/securesms/linkpreview/LinkPreviewUtil.java
@@ -1,6 +1,7 @@
 package org.thoughtcrime.securesms.linkpreview;
 
 import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
 import android.text.SpannableString;
 import android.text.TextUtils;
 import android.text.style.URLSpan;
@@ -41,7 +42,9 @@ public final class LinkPreviewUtil {
   /**
    * @return True if the host is present in the link whitelist.
    */
-  public static boolean isWhitelistedLinkUrl(@NonNull String linkUrl) {
+  public static boolean isWhitelistedLinkUrl(@Nullable String linkUrl) {
+    if (linkUrl == null) return false;
+
     HttpUrl url = HttpUrl.parse(linkUrl);
     return url != null                                   &&
            !TextUtils.isEmpty(url.scheme())              &&
@@ -53,7 +56,9 @@ public final class LinkPreviewUtil {
   /**
    * @return True if the top-level domain is present in the media whitelist.
    */
-  public static boolean isWhitelistedMediaUrl(@NonNull String mediaUrl) {
+  public static boolean isWhitelistedMediaUrl(@Nullable String mediaUrl) {
+    if (mediaUrl == null) return false;
+
     HttpUrl url = HttpUrl.parse(mediaUrl);
     return url != null                                                &&
            !TextUtils.isEmpty(url.scheme())                           &&
diff --git a/src/org/thoughtcrime/securesms/net/ContentProxySafetyInterceptor.java b/src/org/thoughtcrime/securesms/net/ContentProxySafetyInterceptor.java
new file mode 100644
index 0000000000..51d32ba2ae
--- /dev/null
+++ b/src/org/thoughtcrime/securesms/net/ContentProxySafetyInterceptor.java
@@ -0,0 +1,58 @@
+package org.thoughtcrime.securesms.net;
+
+import android.support.annotation.NonNull;
+import android.support.annotation.Nullable;
+
+import org.thoughtcrime.securesms.linkpreview.LinkPreviewUtil;
+import org.thoughtcrime.securesms.logging.Log;
+
+import java.io.IOException;
+
+import okhttp3.HttpUrl;
+import okhttp3.Interceptor;
+import okhttp3.Response;
+
+/**
+ * Interceptor to do extra safety checks on requests through the {@link ContentProxySelector}
+ * to prevent non-whitelisted requests from getting to it. In particular, this guards against
+ * requests redirecting to non-whitelisted domains.
+ *
+ * Note that because of the way interceptors are ordered, OkHttp will hit the proxy with the
+ * bad-redirected-domain before we can intercept the request, so we have to "look ahead" by
+ * detecting a redirected response on the first pass.
+ */
+public class ContentProxySafetyInterceptor implements Interceptor {
+
+  private static final String TAG = Log.tag(ContentProxySafetyInterceptor.class);
+
+  @Override
+  public @NonNull Response intercept(@NonNull Chain chain) throws IOException {
+    if (isWhitelisted(chain.request().url())) {
+      Response response = chain.proceed(chain.request());
+
+      if (response.isRedirect()) {
+        if (isWhitelisted(response.header("Location"))) {
+          return response;
+        } else {
+          Log.w(TAG, "Tried to redirect to a non-whitelisted domain!");
+          chain.call().cancel();
+          throw new IOException("Tried to redirect to a non-whitelisted domain!");
+        }
+      } else {
+        return response;
+      }
+    } else {
+      Log.w(TAG, "Request was for a non-whitelisted domain!");
+      chain.call().cancel();
+      throw new IOException("Request was for a non-whitelisted domain!");
+    }
+  }
+
+  private static boolean isWhitelisted(@NonNull HttpUrl url) {
+    return isWhitelisted(url.toString());
+  }
+
+  private static boolean isWhitelisted(@Nullable String url) {
+    return LinkPreviewUtil.isWhitelistedLinkUrl(url) || LinkPreviewUtil.isWhitelistedMediaUrl(url);
+  }
+}