Expand fix for potential Privilege Escalation via Content Provider (C…

…VE-2018-9492) (#2482)
getsentry · Jan 23, 2023 · 897ad85 · 897ad85
1 parent ffa66c8
commit 897ad85
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 91 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,10 @@
  - Distinguish between foreground and background ANRs
 - Improve possible date precision to 10 μs ([#2451](https://github.com/getsentry/sentry-java/pull/2451))
 
+### Fixes
+
+- Expand guard against CVE-2018-9492 "Privilege Escalation via Content Provider" ([#2482](https://github.com/getsentry/sentry-java/pull/2482))
+
 ## 6.12.1
 
 ### Fixes

diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api
@@ -204,24 +204,18 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr
  public fun setProfilingTracesIntervalMillis (I)V
 }
 
-public final class io/sentry/android/core/SentryInitProvider : android/content/ContentProvider {
+public final class io/sentry/android/core/SentryInitProvider {
  public fun <init> ()V
  public fun attachInfo (Landroid/content/Context;Landroid/content/pm/ProviderInfo;)V
- public fun delete (Landroid/net/Uri;Ljava/lang/String;[Ljava/lang/String;)I
  public fun getType (Landroid/net/Uri;)Ljava/lang/String;
- public fun insert (Landroid/net/Uri;Landroid/content/ContentValues;)Landroid/net/Uri;
  public fun onCreate ()Z
- public fun query (Landroid/net/Uri;[Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Ljava/lang/String;)Landroid/database/Cursor;
  public fun shutdown ()V
- public fun update (Landroid/net/Uri;Landroid/content/ContentValues;Ljava/lang/String;[Ljava/lang/String;)I
 }
 
-public final class io/sentry/android/core/SentryPerformanceProvider : android/content/ContentProvider, android/app/Application$ActivityLifecycleCallbacks {
+public final class io/sentry/android/core/SentryPerformanceProvider : android/app/Application$ActivityLifecycleCallbacks {
  public fun <init> ()V
  public fun attachInfo (Landroid/content/Context;Landroid/content/pm/ProviderInfo;)V
- public fun delete (Landroid/net/Uri;Ljava/lang/String;[Ljava/lang/String;)I
  public fun getType (Landroid/net/Uri;)Ljava/lang/String;
- public fun insert (Landroid/net/Uri;Landroid/content/ContentValues;)Landroid/net/Uri;
  public fun onActivityCreated (Landroid/app/Activity;Landroid/os/Bundle;)V
  public fun onActivityDestroyed (Landroid/app/Activity;)V
  public fun onActivityPaused (Landroid/app/Activity;)V
@@ -230,8 +224,6 @@ public final class io/sentry/android/core/SentryPerformanceProvider : android/co
  public fun onActivityStarted (Landroid/app/Activity;)V
  public fun onActivityStopped (Landroid/app/Activity;)V
  public fun onCreate ()Z
- public fun query (Landroid/net/Uri;[Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Ljava/lang/String;)Landroid/database/Cursor;
- public fun update (Landroid/net/Uri;Landroid/content/ContentValues;Ljava/lang/String;[Ljava/lang/String;)I
 }
 
 public final class io/sentry/android/core/SystemEventsBreadcrumbsIntegration : io/sentry/Integration, java/io/Closeable {

diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/EmptySecureContentProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/EmptySecureContentProvider.java
@@ -0,0 +1,56 @@
+package io.sentry.android.core;
+
+import android.content.ContentProvider;
+import android.content.ContentValues;
+import android.database.Cursor;
+import android.net.Uri;
+import io.sentry.android.core.internal.util.ContentProviderSecurityChecker;
+import org.jetbrains.annotations.ApiStatus;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A ContentProvider that does NOT store or provide any data for read or write operations.
+ *
+ * <p>This does not allow for overriding the abstract query, insert, update, and delete operations
+ * of the {@link ContentProvider}. Additionally, those functions are secure.
+ */
+@ApiStatus.Internal
+abstract class EmptySecureContentProvider extends ContentProvider {
+
+ private final ContentProviderSecurityChecker securityChecker =
+ new ContentProviderSecurityChecker();
+
+ @Override
+ public final @Nullable Cursor query(
+ @NotNull Uri uri,
+ @Nullable String[] strings,
+ @Nullable String s,
+ @Nullable String[] strings1,
+ @Nullable String s1) {
+ securityChecker.checkPrivilegeEscalation(this);
+ return null;
+ }
+
+ @Override
+ public final @Nullable Uri insert(@NotNull Uri uri, @Nullable ContentValues contentValues) {
+ securityChecker.checkPrivilegeEscalation(this);
+ return null;
+ }
+
+ @Override
+ public final int delete(@NotNull Uri uri, @Nullable String s, @Nullable String[] strings) {
+ securityChecker.checkPrivilegeEscalation(this);
+ return 0;
+ }
+
+ @Override
+ public final int update(
+ @NotNull Uri uri,
+ @Nullable ContentValues contentValues,
+ @Nullable String s,
+ @Nullable String[] strings) {
+ securityChecker.checkPrivilegeEscalation(this);
+ return 0;
+ }
+}
diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryInitProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryInitProvider.java
@@ -1,20 +1,16 @@
 package io.sentry.android.core;
 
-import android.content.ContentProvider;
-import android.content.ContentValues;
 import android.content.Context;
 import android.content.pm.ProviderInfo;
-import android.database.Cursor;
 import android.net.Uri;
 import io.sentry.Sentry;
 import io.sentry.SentryLevel;
-import io.sentry.android.core.internal.util.ContentProviderSecurityChecker;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 @ApiStatus.Internal
-public final class SentryInitProvider extends ContentProvider {
+public final class SentryInitProvider extends EmptySecureContentProvider {
 
  @Override
  public boolean onCreate() {
@@ -45,38 +41,8 @@ public void attachInfo(@NotNull Context context, @NotNull ProviderInfo info) {
  super.attachInfo(context, info);
  }
 
- @Override
- public @Nullable Cursor query(
- @NotNull Uri uri,
- @Nullable String[] strings,
- @Nullable String s,
- @Nullable String[] strings1,
- @Nullable String s1) {
- new ContentProviderSecurityChecker().checkPrivilegeEscalation(this);
- return null;
- }
-
  @Override
  public @Nullable String getType(@NotNull Uri uri) {
  return null;
  }
-
- @Override
- public @Nullable Uri insert(@NotNull Uri uri, @Nullable ContentValues contentValues) {
- return null;
- }
-
- @Override
- public int delete(@NotNull Uri uri, @Nullable String s, @Nullable String[] strings) {
- return 0;
- }
-
- @Override
- public int update(
- @NotNull Uri uri,
- @Nullable ContentValues contentValues,
- @Nullable String s,
- @Nullable String[] strings) {
- return 0;
- }
 }
diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java
@@ -2,16 +2,12 @@
 
 import android.app.Activity;
 import android.app.Application;
-import android.content.ContentProvider;
-import android.content.ContentValues;
 import android.content.Context;
 import android.content.pm.ProviderInfo;
-import android.database.Cursor;
 import android.net.Uri;
 import android.os.Bundle;
 import android.os.SystemClock;
 import io.sentry.SentryDate;
-import io.sentry.android.core.internal.util.ContentProviderSecurityChecker;
 import org.jetbrains.annotations.ApiStatus;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -24,7 +20,7 @@
  * AppComponentFactory but it depends on androidx.core.app.AppComponentFactory
  */
 @ApiStatus.Internal
-public final class SentryPerformanceProvider extends ContentProvider
+public final class SentryPerformanceProvider extends EmptySecureContentProvider
  implements Application.ActivityLifecycleCallbacks {
 
  // static to rely on Class load
@@ -70,45 +66,12 @@ public void attachInfo(Context context, ProviderInfo info) {
  super.attachInfo(context, info);
  }
 
- @Nullable
- @Override
- public Cursor query(
- @NotNull Uri uri,
- @Nullable String[] projection,
- @Nullable String selection,
- @Nullable String[] selectionArgs,
- @Nullable String sortOrder) {
- new ContentProviderSecurityChecker().checkPrivilegeEscalation(this);
- return null;
- }
-
  @Nullable
  @Override
  public String getType(@NotNull Uri uri) {
  return null;
  }
 
- @Nullable
- @Override
- public Uri insert(@NotNull Uri uri, @Nullable ContentValues values) {
- return null;
- }
-
- @Override
- public int delete(
- @NotNull Uri uri, @Nullable String selection, @Nullable String[] selectionArgs) {
- return 0;
- }
-
- @Override
- public int update(
- @NotNull Uri uri,
- @Nullable ContentValues values,
- @Nullable String selection,
- @Nullable String[] selectionArgs) {
- return 0;
- }
-
  @TestOnly
  static void setAppStartTime(
  final long appStartMillisLong, final @NotNull SentryDate appStartTimeDate) {

diff --git a/...re/src/main/java/io/sentry/android/core/internal/util/ContentProviderSecurityChecker.java b/...re/src/main/java/io/sentry/android/core/internal/util/ContentProviderSecurityChecker.java
@@ -2,7 +2,6 @@
 
 import android.annotation.SuppressLint;
 import android.content.ContentProvider;
-import android.net.Uri;
 import android.os.Build;
 import io.sentry.NoOpLogger;
 import io.sentry.android.core.BuildInfoProvider;
@@ -30,20 +29,21 @@ public ContentProviderSecurityChecker(final @NotNull BuildInfoProvider buildInfo
  * <p>See https://www.cvedetails.com/cve/CVE-2018-9492/ and
  * https://github.com/getsentry/sentry-java/issues/2460
  *
- * <p>Call this function in the {@link ContentProvider#query(Uri, String[], String, String[],
- * String)} function.
+ * <p>Call this function in the {@link ContentProvider}'s implementations of the abstract
+ * functions; query, insert, update, and delete.
  *
- * <p>This should be invoked regardless of whether there is data to query or not. The attack is
- * not contained to the specific provider but rather the entire system.
+ * <p>This should be invoked regardless of whether there is data to read/write or not. The attack
+ * is not contained to the specific provider but rather the entire system.
  *
- * <p>This blocks the attacker by only allowing the app itself (not other apps) to "query" the
- * provider.
+ * <p>This blocks the attacker by only allowing the app itself (not other apps) to interact with
+ * the ContentProvider. If the ContentProvider needs to be able to interact with other trusted
+ * apps, then this function or class should be refactored to accommodate that.
  *
  * <p>The vulnerability is specific to un-patched versions of Android 8 and 9 (API 26 to 28).
  * Therefore, this security check is limited to those versions to mitigate risk of regression.
  */
  @SuppressLint("NewApi")
- public void checkPrivilegeEscalation(ContentProvider contentProvider) {
+ public void checkPrivilegeEscalation(@NotNull ContentProvider contentProvider) {
  final int sdkVersion = buildInfoProvider.getSdkInfoVersion();
  if (sdkVersion >= Build.VERSION_CODES.O && sdkVersion <= Build.VERSION_CODES.P) {