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

Jump to bottom

Expand fix for potential Privilege Escalation via Content Provider (CVE-2018-9492) #2482

Merged
merged 4 commits into from
Jan 23, 2023
Merged

Expand fix for potential Privilege Escalation via Content Provider (CVE-2018-9492) #2482

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f4c9b9
Expand fix for potential Privilege Escalation via Content Provider (C…
vestrel00 Jan 18, 2023
57648df
Added unreleased notes to CHANGELOG
vestrel00 Jan 18, 2023
a56578c
Ran apiDump
vestrel00 Jan 18, 2023
61baa44
Merge branch 'main' of github.com:getsentry/sentry-java
vestrel00 Jan 20, 2023
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
56 changes: 56 additions & 0 deletions sentry-android-core/src/main/java/io/sentry/android/core/EmptySecureContentProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR comes with this additional benefit of making it clear to contributors and consumers that Sentry does not actually have any "real" ContentProviders =)


private final ContentProviderSecurityChecker securityChecker =
new ContentProviderSecurityChecker();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You will notice that I am not initializing this with a reference to this ContentProvider. If I did, the code would be a tiny bit shorter at function call-sites;

securityChecker.checkPrivilegeEscalation();

It looks nice and is okay because there is no harm in saving a hard reference to the ContentProvider as long as we don't leak it. However, this would come at the price of having to assign the value of this in onCreate, which would require it being annotated with @CallSuper, and this not being final...

EmptySecureContentProvider {

  private ContentProviderSecurityChecker securityChecker;

  @CallSuper
  @Override
  public boolean onCreate() {
    securityChecker = new ContentProviderSecurityChecker(this);
    return false;
  }
}

So.. I think that we don't need to go through all that trouble just so that we don't have to pas in a reference to the ContentProvider when calling securityChecker.checkPrivilegeEscalation.


@Override
public final @Nullable Cursor query(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the final modifier here to make sure that this cannot be overridden further by subclasses. This ensures that the contract described in the class name and class doc is upheld at the compiler level!

@NotNull Uri uri,
@Nullable String[] strings,
@Nullable String s,
@Nullable String[] strings1,
@Nullable String s1) {
securityChecker.checkPrivilegeEscalation(this);
Copy link
Contributor Author

@vestrel00 vestrel00 Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that there is no need to add this check to other overridable query functions because they all ultimately use this function.

See my comment in my previous PR; #2466 (comment)

return null;
}

@Override
public final @Nullable Uri insert(@NotNull Uri uri, @Nullable ContentValues contentValues) {
securityChecker.checkPrivilegeEscalation(this);
Copy link
Contributor Author

@vestrel00 vestrel00 Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that there is no need to add this check to other overridable insert functions because they all ultimately use this function.

See my similar comment in my previous PR; #2466 (comment)

return null;
}

@Override
public final int delete(@NotNull Uri uri, @Nullable String s, @Nullable String[] strings) {
securityChecker.checkPrivilegeEscalation(this);
Copy link
Contributor Author

@vestrel00 vestrel00 Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that there is no need to add this check to other overridable delete functions because they all ultimately use this function.

See my similar comment in my previous PR; #2466 (comment)

return 0;
}

@Override
public final int update(
@NotNull Uri uri,
@Nullable ContentValues contentValues,
@Nullable String s,
@Nullable String[] strings) {
securityChecker.checkPrivilegeEscalation(this);
Copy link
Contributor Author

@vestrel00 vestrel00 Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that there is no need to add this check to other overridable update functions because they all ultimately use this function.

See my similar comment in my previous PR; #2466 (comment)

return 0;
}
}
36 changes: 1 addition & 35 deletions sentry-android-core/src/main/java/io/sentry/android/core/SentryInitProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() {
Expand Down Expand Up @@ -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;
Copy link

Choose a reason for hiding this comment

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

Does it make sense to validate the getType as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. According to the ContentProvider.getType, documentation...

Note that there are no permissions needed for an application to access this information; if your content provider requires read and/or write permissions, or is not exported, all applications can still call this method regardless of their access permissions. This allows them to retrieve the MIME type for a URI when dispatching intents.

There is no "privilege escalation" exploit here because there is no privilege required to use this function at all. This is public function that is able to be used without any permissions even if not exported.

I would really like to keep my code changes minimal so as to minimize the risk of regression 🙏

}

@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;
}
}
39 changes: 1 addition & 38 deletions sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.DateUtils;
import io.sentry.android.core.internal.util.ContentProviderSecurityChecker;
import java.util.Date;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
Expand All @@ -25,7 +21,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
Expand Down Expand Up @@ -71,45 +67,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 Date appStartTimeDate) {
appStartMillis = appStartMillisLong;
Expand Down
16 changes: 8 additions & 8 deletions ...re/src/main/java/io/sentry/android/core/internal/util/ContentProviderSecurityChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated documentation.

*
* <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) {

Expand Down