Fix potential Privilege Escalation via Content Provider (CVE-2018-9492) by vestrel00 · Pull Request #2466 · getsentry/sentry-java

vestrel00 · 2023-01-11T21:18:05Z

❗ Problem

Apps that use Secure theorem to scan for security vulnerabilities are being flagged for "Privilege Escalation via Content Provider" (CVE-2018-9492). Devices on Android API 26, 27, and 28 are susceptible to attackers.

The most alarming part of this vulnerability is that it allows attackers to use content providers (even those that do not provide any actual data or handling) to break open the entire system (within or outside the bounds of the application)...

The affected, insecure code are in the query functions in the following content providers...

The above content providers do not provide any content but may still be used by an attacker.

Users of Sentry Android can work around this issue by removing the content providers and using manual init. However, this would render apps using auto-init vulnerable.

For more info, initial discussion is at #2460

💡 Solution

Apply the suggested "secure code" from secure theorem;

class TestProvider extends android.content.ContentProvider {

    private void securityCheck(Uri uri)  {
        String callingPackage = this.getCallingPackage();
        if(callingPackage != null && (callingPackage.equals("com.trusted.app"))) {
            return;
        }
       SecurityException exception = new SecurityException("Provider does not allow granting of Uri permissions");
       throw exception;
    }

    @Nullable
    @Override
    public Cursor query(@NonNull Uri uri, @Nullable String[] projection, @Nullable String selection, @Nullable String[] selectionArgs, @Nullable String sortOrder) {
        securityCheck(uri);
        return null;
    }
}

💚 Testing

I added unit tests for the security checker SRP class.

I do not actually know how to perform the attack, so I'm not able to verify that this works against attackers. However, it is safe to assume that it will work given that the attacker will have a different package than the app package.

I also played around with the Android sample to make sure that it still works and not just crashing lol.

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

🔮 Next steps

Either merge and include this in the next Sentry release OR create a one-off-tag/snapshot that contains these changes (not released to the general public).
- Note that Secure Theorem scans our apps from the Google Play Store, which requires signed builds that we can only generate in our remote CI pipelines. Therefore, I will need an actual Sentry release as I am unable to use a locally-built Sentry binary. Otherwise, I would have to spend countless hours trying to figure out how to get the app binaries scanned by Secure Theorem, which I just don't have the time to do.
Once a Sentry tag is available with these changes, I will upgrade our apps to that Sentry version and check if Secure Theorem considers the vulnerability to be fixed.
- Even if the Secure Theorem scanner still considers the vulnerability to be open (maybe because it expects 100% copy-paste of their suggested code), we know for sure that the vulnerability is fixed and we can close Security Vulnerability: Privilege Escalation via Content Provider (CVE-2018-9492) #2460 as 100% "false positive".
If a snapshot was used in step 1, then we should make sure to include these changes in a Sentry release that is available to the general public.

…er (CVE-2018-9492)

...ava/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderCheckerTest.kt

vestrel00 · 2023-01-11T21:23:33Z

.../java/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderChecker.java

+ * and https://github.com/getsentry/sentry-java/issues/2460
+ */
+@ApiStatus.Internal
+public final class PrivilegeEscalationViaContentProviderChecker {


I created this SRP class to make it testable. Hopefully, Secure Theorem will recognize that the vulnerability is fixed even though I did not 100% copy-paste their suggested code.

Does it make sense to fetch a snapshot into the product and run the analyser against it?

@artour-bakiev, I think this is what I'm suggesting in the Next steps section of the PR description. Or do you mean something else?

I thought about running the datatheorem check before landing the PR into main branch. But perhaps it was intended, sorry if it was :)

artour-bakiev · 2023-01-13T00:19:18Z

...ava/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderCheckerTest.kt

+    private val fixture = Fixture()
+
+    @Test
+    fun `When sdk version is less than vulnerable versions, security check is not performed`() {


Minor stuff, it appears the Sentry team is utilising lowercase symbols in their test methods.

I think they use both upper and lowercase then. I was just following the style they had been using in RootCheckerTest.kt.

I copy-pasted-and-modified their existing RootCheckterTest.kt file for this; https://github.com/getsentry/sentry-java/blob/6.11.0/sentry-android-core/src/test/java/io/sentry/android/core/internal/util/RootCheckerTest.kt#L48

All tests in that file are in the format "When ..., ...."

artour-bakiev · 2023-01-13T01:08:29Z

.../java/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderChecker.java

+  }
+
+  public PrivilegeEscalationViaContentProviderChecker(
+    final @NotNull BuildInfoProvider buildInfoProvider


Minor stuff, final specified for a parameter.

I was just following the style they had been using in RootChecker.kt.

I copy-pasted-and-modified their existing RootCheckter.kt file for this; https://github.com/getsentry/sentry-java/blob/6.6.0/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/RootChecker.java#L36

Parameters in that file are all final.

Please keep in mind that this code is in Java where parameters are not final and can be reassigned.

In Kotlin, parameters are implicitly final (val) and cannot be reassigned.

I'm guessing that this is why the Sentry team is prepending parameters in Java with final. It is good practice to make sure something is immutable when it does not need to be mutable. Makes everything safer. Kotlin just does these things for us and sometimes we forget that we have to do it ourselves in Java 😁

All good then, mate. A final param seems to be a bit confusing but it's perhaps just for me.

artour-bakiev · 2023-01-13T04:20:09Z

sentry-android-core/src/main/java/io/sentry/android/core/SentryInitProvider.java

      @Nullable String s,
      @Nullable String[] strings1,
      @Nullable String s1) {
+    new PrivilegeEscalationViaContentProviderChecker().securityCheck(this);


It seems we should guard every public (ContentProvider) method.

I think that the vulnerability is specific to read operations though right? This means we only need to have this in the query method as suggested by Secure Theorem.

I'd like to avoid unnecessary code additions / changes.

That's a good point! Could you share which methods are affected by this?
I see that ContentProvider.query method has multiple overloads (1, 2, 3) so I guess at least all of those should be overridden and checked.

@markushi, it's not necessary to override the other can-be-overloaded query functions because they all ultimately just call this abstract function subclass implementation.

I think that the vulnerability is specific to read operations though right?

Mmmm, I'm not completely sure since I still can't understand the attack. But according to https://www.cvedetails.com/cve/CVE-2018-9492/ the issue happens because:

In checkGrantUriPermissionLocked of ActivityManagerService.java, there is a possible permissions bypass

It may mean that any public ContentProvider method potentially has the vulnerability (because the checkGrantUriPermissionLocked should happen every time the user addresses a ContentProvider).

@artour-bakiev, sorry I don't mean to ignore your warnings. From my experience, Data/Secure Theorem typically provides suggested "secure code" samples that are complete. So I'm thinking that guarding query should be enough because that's what they suggested.

No worries, mate. Let's have a look at the dt scan with v 6.12.0.

If the DT scan still flags the issue, I will update the Sentry implementation to guard insert, update, and delete functions too.

We might have to wait 2-4 weeks for the next Sentry release but I don't think there is a rush at this point 😄

Okay, these changes did not pass the Secure/Data Theorem scan...

However, this does not mean that the vulnerability was not actually fixed. Anyways, as agreed upon, I will create another PR to apply the security checks in insert, update, and delete functions.

If it still does not pass the scan, then there is nothing else we can do. It just means that the scanner wants an exact copy-paste of their suggested code 🤷

Created another PR; #2482

artour-bakiev · 2023-01-13T04:38:55Z

.../java/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderChecker.java

+   * 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.
+   */
+  @TargetApi(Build.VERSION_CODES.KITKAT) // Required for ContentProvider#getCallingPackage()


Minor, but it seems a bit confusing. It looks like we can safely remove @TargetApi annotation since getCallingPackage() is only called if the device's SDK version is >= 0.

No because I am using buildInfoProvider.getSdkInfoVersion() to retrieve the sdkVersion. The compiler or lint will complain because we are not using Build.VERSION.SDK_INT directly here.

You can try removing @TargetApi and you will see that you will get a compile or lint error.

I am using buildInfoProvider.getSdkInfoVersion() because it seems to be used throughout the codebase AND it allows me to mock the sdkVersion in the unit tests.

Since the method is going to be called, you can ignore the linter with the Supress annotation.

Good point!!! This method can be called even on prior versions to KitKat. Proof is that I'm not doing any version checks at the call sites.

This is embarrassing 😓 I'll suppress instead!

Nice catch, thanks!

Done! d8272e2

marandaneto · 2023-01-13T07:51:45Z

...ava/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderCheckerTest.kt

+        var securityException: SecurityException? = null
+        try {
+            fixture.getSut().securityCheck(contentProvider)
+        } catch (se: SecurityException) {
+            securityException = se
+        }
+
+        assertNotNull(securityException)


Kotlin test has an assertThrows<T> {} function.

Yep! @markushi pointed out the same thing https://github.com/getsentry/sentry-java/pull/2466/files#r1069057680

markushi

Looks good to me, thanks for your contribution!
Please have a look at my comments.
Regarding testability: I think we should be able to provide you with a public snapshot once the code is merged, so you can create builds in your CI and have them verified.

markushi · 2023-01-13T07:58:01Z

sentry-android-core/src/main/java/io/sentry/android/core/SentryInitProvider.java

      @Nullable String s,
      @Nullable String[] strings1,
      @Nullable String s1) {
+    new PrivilegeEscalationViaContentProviderChecker().securityCheck(this);


That's a good point! Could you share which methods are affected by this?
I see that ContentProvider.query method has multiple overloads (1, 2, 3) so I guess at least all of those should be overridden and checked.

markushi · 2023-01-13T08:02:26Z

...ava/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderCheckerTest.kt

+
+        contentProvider.mockPackages(null)
+
+        var securityException: SecurityException? = null


I think you can simplify this by using assertThrows

Suggested change

var securityException: SecurityException? = null

assertThrows<SecurityException> { fixture.getSut().securityCheck(contentProvider) }

Great callout! I'm an idiot 🤣 I'll change 🔥

Hmm... Wait a sec... I know I initially tried assertThrows but no code suggestion was popping up for it.

Looking for all instances in java or Kotlin files...

Seems like it requires a dependency currently only used in sentry-spring and sentry-spring-jakarta.

I'd like to avoid changing any build configurations in this PR because I'm not familiar with the current build setup and dependencies. Since I'm only using this try-catch approach in two places in this one file, maybe we can let this slide? If you feel strongly about using assertThrows, you can make changes after the PR has been merged 🙏

markushi · 2023-01-13T08:09:04Z

.../java/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderChecker.java

+ * and https://github.com/getsentry/sentry-java/issues/2460
+ */
+@ApiStatus.Internal
+public final class PrivilegeEscalationViaContentProviderChecker {


It could make sense to give this utility class a more generic name, so we could extends it's functionality in case there will be future CVEs.

Suggested change

public final class PrivilegeEscalationViaContentProviderChecker {

public final class ContentProviderSecurityChecker {

I would typically quote my good ol' friend YAGNI. I don't see future CVEs popping up specifically for ContentProviders when we've been scanning our apps with Secure Theorem for the past 2+ years and this is the only complaint we got so far. Even if something does pop up in the future, at that point we can easily make a refactor if we decide that it belongs in the same file.

However, I do like the generic name you suggested more in terms of length and how less intimidating it is 😃

So, sure! I'll rename to ContentProviderSecurityChecker and also rename the function within to checkPrivilegeEscalation and update the javadocs accordingly 🔥

Done! 1752102

…erSecurityChecker

vestrel00 · 2023-01-13T17:38:39Z

@markushi @marandaneto, I finished addressing all comments. Please let me know what else I need to do 🙏

markushi

Thanks again for iterating on this PR! Regarding the exception handling we're already using assertFailsWith<>() from the kotlin test library, which is already a dependency and should work out of the box (as opposed to assertThrows()).

As a last step please run make format in a terminal on the project root folder to ensure all code is formatted.

markushi · 2023-01-16T06:39:08Z

...ore/src/test/java/io/sentry/android/core/internal/util/ContentProviderSecurityCheckerTest.kt

+import org.mockito.kotlin.mock
+import org.mockito.kotlin.verifyNoInteractions
+import org.mockito.kotlin.whenever
+import kotlin.test.Test


Suggested change

import kotlin.test.Test

import kotlin.test.Test

import kotlin.test.assertFailsWith

@markushi, ahh! This is what I've been looking for! Thanks!

I will use assertFailsWith 🔥

Done! b7a40d7

markushi · 2023-01-16T06:42:11Z

...ore/src/test/java/io/sentry/android/core/internal/util/ContentProviderSecurityCheckerTest.kt

+        var securityException: SecurityException? = null
+        try {
+            fixture.getSut().checkPrivilegeEscalation(contentProvider)
+        } catch (se: SecurityException) {
+            securityException = se
+        }
+
+        assertNotNull(securityException)


Suggested change

var securityException: SecurityException? = null

try {

fixture.getSut().checkPrivilegeEscalation(contentProvider)

} catch (se: SecurityException) {

securityException = se

}

assertNotNull(securityException)

assertFailsWith<SecurityException> {

fixture.getSut().checkPrivilegeEscalation(contentProvider)

}

Done! b7a40d7

markushi · 2023-01-16T06:42:32Z

...ore/src/test/java/io/sentry/android/core/internal/util/ContentProviderSecurityCheckerTest.kt

+        var securityException: SecurityException? = null
+        try {
+            fixture.getSut().checkPrivilegeEscalation(contentProvider)
+        } catch (se: SecurityException) {
+            securityException = se
+        }
+
+        assertNotNull(securityException)


Suggested change

var securityException: SecurityException? = null

try {

fixture.getSut().checkPrivilegeEscalation(contentProvider)

} catch (se: SecurityException) {

securityException = se

}

assertNotNull(securityException)

assertFailsWith<SecurityException> {

fixture.getSut().checkPrivilegeEscalation(contentProvider)

}

Done! b7a40d7

vestrel00 · 2023-01-16T07:41:05Z

Okay, I addressed all PR comments again. I also ran make format so that spotless can format my code properly. Plus, added the PR to the CHANGELOG because I saw a build job was failing because of it.

Please review again and hopefully merge if all builds pass 🤞 🙏

codecov · 2023-01-16T08:04:31Z

Codecov Report

Base: 80.14% // Head: 80.14% // No change to project coverage 👍

Coverage data is based on head (1ba566b) compared to base (068ace3).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #2466   +/-   ##
=========================================
  Coverage     80.14%   80.14%           
  Complexity     3872     3872           
=========================================
  Files           312      312           
  Lines         14669    14669           
  Branches       1941     1941           
=========================================
  Hits          11756    11756           
  Misses         2153     2153           
  Partials        760      760

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

romtsn · 2023-01-16T08:08:40Z

sentry-android-core/src/main/java/io/sentry/android/core/SentryInitProvider.java

      @Nullable String s,
      @Nullable String[] strings1,
      @Nullable String s1) {
+    new ContentProviderSecurityChecker().checkPrivilegeEscalation(this);


nit: could we just create an instance and keep it as a class member instead of creating a new instance of the SecurityChecker every time the method is called? Same for SentryPerformanceProvider.

We could. It's what I would have done if the call-sites were using dependency injection or if we had to use an instance of this class in at least one other function. It's not used anywhere else, nor is this function really invoked (it's only invoke if an attacker tries the exploit) so performance shouldn't be an issue.

I was originally writing a static function but ended up with an SRP class instead for testability and future-proofing in case it needs to be mocked at call-sites.

I'll ignore this since this is a "nit" 😀

Fixes Security Vulnerability: Privilege Escalation via Content Provid…

ac51a14

…er (CVE-2018-9492)

vestrel00 requested review from adinauer, markushi, romtsn and stefanosiano as code owners January 11, 2023 21:18

vestrel00 mentioned this pull request Jan 11, 2023

Security Vulnerability: Privilege Escalation via Content Provider (CVE-2018-9492) #2460

Closed

vestrel00 commented Jan 11, 2023

View reviewed changes

...ava/io/sentry/android/core/internal/util/PrivilegeEscalationViaContentProviderCheckerTest.kt Outdated Show resolved Hide resolved

vestrel00 commented Jan 11, 2023

View reviewed changes

artour-bakiev reviewed Jan 13, 2023

View reviewed changes

marandaneto reviewed Jan 13, 2023

View reviewed changes

markushi reviewed Jan 13, 2023

View reviewed changes

vestrel00 added 3 commits January 13, 2023 07:06

Renamed PrivilegeEscalationViaContentProviderChecker to ContentProvid…

1752102

…erSecurityChecker

Suppress lint instead of using misleading TargetApi

d8272e2

Merge branch 'main' of github.com:getsentry/sentry-java

f90d855

markushi reviewed Jan 16, 2023

View reviewed changes

vestrel00 added 3 commits January 15, 2023 21:29

Use assertFailsWith instead of try-catch

b7a40d7

Updated CHANGELOG

5931c51

Ran make format

116ad0f

romtsn reviewed Jan 16, 2023

View reviewed changes

romtsn approved these changes Jan 16, 2023

View reviewed changes

Merge branch 'main' into main

1ba566b

markushi approved these changes Jan 16, 2023

View reviewed changes

markushi changed the title ~~Fixes #2460 Security Vulnerability: Privilege Escalation via Content Provider (CVE-2018-9492)~~ Fix potential Privilege Escalation via Content Provider (CVE-2018-9492) Jan 16, 2023

markushi merged commit 2374884 into getsentry:main Jan 16, 2023

vestrel00 mentioned this pull request Jan 18, 2023

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

Merged

4 tasks


		contentProvider.mockPackages(null)

		var securityException: SecurityException? = null

	var securityException: SecurityException? = null
	assertThrows<SecurityException> { fixture.getSut().securityCheck(contentProvider) }

	public final class PrivilegeEscalationViaContentProviderChecker {
	public final class ContentProviderSecurityChecker {

	import kotlin.test.Test
	import kotlin.test.Test
	import kotlin.test.assertFailsWith

Uh oh!

Conversation

vestrel00 commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Problem

💡 Solution

💚 Testing

📝 Checklist

🔮 Next steps

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vestrel00 Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vestrel00 Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vestrel00 Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vestrel00 Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vestrel00 commented Jan 11, 2023 •

edited

Loading

vestrel00 Jan 13, 2023 •

edited

Loading

vestrel00 Jan 13, 2023 •

edited

Loading

vestrel00 Jan 13, 2023 •

edited

Loading

vestrel00 Jan 13, 2023 •

edited

Loading

vestrel00 Jan 13, 2023 •

edited

Loading

vestrel00 Jan 13, 2023 •

edited

Loading