Expand fix for potential Privilege Escalation via Content Provider (CVE-2018-9492) #2482
Conversation
vestrel00
requested review from
adinauer,
romtsn,
stefanosiano and
markushi
as code owners
| * of the {@link ContentProvider}. Additionally, those functions are secure. | ||
| */ | ||
| @ApiStatus.Internal | ||
| abstract class EmptySecureContentProvider extends ContentProvider { |
There was a problem hiding this comment.
This PR comes with this additional benefit of making it clear to contributors and consumers that Sentry does not actually have any "real" ContentProviders =)
| @Nullable String s, | ||
| @Nullable String[] strings1, | ||
| @Nullable String s1) { | ||
| securityChecker.checkPrivilegeEscalation(this); |
There was a problem hiding this comment.
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)
| new ContentProviderSecurityChecker(); | ||
|
|
||
| @Override | ||
| public final @Nullable Cursor query( |
There was a problem hiding this comment.
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!
|
|
||
| @Override | ||
| public final @Nullable Uri insert(@NotNull Uri uri, @Nullable ContentValues contentValues) { | ||
| securityChecker.checkPrivilegeEscalation(this); |
There was a problem hiding this comment.
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)
|
|
||
| @Override | ||
| public final int delete(@NotNull Uri uri, @Nullable String s, @Nullable String[] strings) { | ||
| securityChecker.checkPrivilegeEscalation(this); |
There was a problem hiding this comment.
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)
| @Nullable ContentValues contentValues, | ||
| @Nullable String s, | ||
| @Nullable String[] strings) { | ||
| securityChecker.checkPrivilegeEscalation(this); |
There was a problem hiding this comment.
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)
| abstract class EmptySecureContentProvider extends ContentProvider { | ||
|
|
||
| private final ContentProviderSecurityChecker securityChecker = | ||
| new ContentProviderSecurityChecker(); |
There was a problem hiding this comment.
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.
| * <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. |
There was a problem hiding this comment.
Updated documentation.
Codecov ReportBase: 80.08% // Head: 80.08% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #2482 +/- ##
=========================================
Coverage 80.08% 80.08%
Complexity 3904 3904
=========================================
Files 320 320
Lines 14751 14751
Branches 1950 1950
=========================================
Hits 11813 11813
Misses 2168 2168
Partials 770 770 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. |
|
I will fix the merge conflicts tomorrow. Or feel free to do it if you plan to merge this now. |
| new ContentProviderSecurityChecker().checkPrivilegeEscalation(this); | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getType(@NotNull Uri uri) { | ||
| return null; |
There was a problem hiding this comment.
Does it make sense to validate the getType as well?
There was a problem hiding this comment.
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 🙏
|
I fixed the merge conflicts. Feel free to merge whenever 🙏 Thanks in advance! |
|
FYI: This fix will be part of the |
❗ Problem
The changes contained in #2466 that were merged and released in 6.12.0 were not enough to pass Data/Secure theorem security scans.
This is either due to...
insert,update, anddeleteContentProvider functionsqueryfunction.💡 Solution
In order to completely fix the potential security vulnerability described in #2460, we should also apply the security checks to the
insert,update, anddeletefunctions ofSentryInitProviderandSentryPerformanceProvider.This may not result in passing Data/Secure theorem security scans in case it is expecting copy-paste code of the suggest code. However, we will be 100% sure that we are protected no matter what scanners say 😎
Thanks to my colleague @artour-bakiev for being diligent in making sure that I do the right thing for everyone ❤️
💚 How did you test it?
I smoke tested the Android sample to make sure that it still works and is not just crashing (lol).
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.
📝 Checklist
🔮 Next steps
There are no next steps after this. If the next Sentry version that contains these changes still do no pass the Data/Secure theorem security scans, then we know that they are expecting copy-paste of their suggested code, which we are just not going to do 😁
Note that there is no urgency in getting this change out in a new Sentry version. We can wait a whole month. No rush. Whenever the next Sentry version is released is kewl!