diff options
author | Jeff Davidson <jpd@google.com> | 2018-02-12 18:39:42 -0800 |
---|---|---|
committer | Jeff Davidson <jpd@google.com> | 2018-03-09 20:32:40 +0000 |
commit | ae1f9edc0c877c2ccc32ae8b30de974271def420 (patch) | |
tree | a738a01bd1cc6ab0b8cdabfe9c729a634515b163 | |
parent | ba7155f8367a25deec2b16069241731ff92a7257 (diff) | |
download | ContactsProvider-ae1f9edc0c877c2ccc32ae8b30de974271def420.tar.gz |
Allow carrier-privileged apps to access voicemail provider.
Bug: 70041899
Test: TreeHugger + tests in CL topic
Change-Id: Icd80caa6f755f549f2433ac4dcacb4a77c962077
Merged-In: Icd80caa6f755f549f2433ac4dcacb4a77c962077
(cherry picked from commit 9c57007fb2e6bbb3d0303320ce6938751240c1c7)
7 files changed, 183 insertions, 51 deletions
diff --git a/AndroidManifest.xml b/AndroidManifest.xml index ca4264b2..559e60f1 100644 --- a/AndroidManifest.xml +++ b/AndroidManifest.xml @@ -66,11 +66,13 @@ android:writePermission="android.permission.MANAGE_USERS"> </provider> + <!-- Note: While this provider does not declare a permission explicitly, it enforces that + the caller has either ADD_VOICEMAIL or carrier privileges at a minimum to access it. + Additional permission checks may be done depending on the operation. --> <provider android:name="VoicemailContentProvider" android:authorities="com.android.voicemail" android:syncable="false" android:multiprocess="false" - android:exported="true" - android:permission="com.android.voicemail.permission.ADD_VOICEMAIL"> + android:exported="true"> </provider> <provider android:name="ContactMetadataProvider" diff --git a/src/com/android/providers/contacts/VoicemailContentProvider.java b/src/com/android/providers/contacts/VoicemailContentProvider.java index 01c10481..1ced1be6 100644 --- a/src/com/android/providers/contacts/VoicemailContentProvider.java +++ b/src/com/android/providers/contacts/VoicemailContentProvider.java @@ -15,6 +15,7 @@ */ package com.android.providers.contacts; +import static android.app.AppOpsManager.MODE_ALLOWED; import static android.provider.VoicemailContract.SOURCE_PACKAGE_FIELD; import static com.android.providers.contacts.util.DbQueryUtils.concatenateClauses; @@ -23,13 +24,13 @@ import static com.android.providers.contacts.util.DbQueryUtils.getEqualityClause import android.annotation.NonNull; import android.app.AppOpsManager; import android.content.ContentProvider; -import android.content.ContentResolver; import android.content.ContentValues; import android.content.Context; import android.content.Intent; import android.database.Cursor; import android.net.Uri; import android.os.Binder; +import android.os.IBinder; import android.os.ParcelFileDescriptor; import android.provider.BaseColumns; import android.provider.VoicemailContract; @@ -50,7 +51,6 @@ import com.google.common.annotations.VisibleForTesting; import java.io.FileNotFoundException; import java.util.Arrays; import java.util.List; -import java.util.Set; /** * An implementation of the Voicemail content provider. This class in the entry point for both @@ -83,8 +83,15 @@ public class VoicemailContentProvider extends ContentProvider } Context context = context(); - // ADD_VOICEMAIL permission guards read and write. We do the same with app ops. - // The permission name doesn't reflect its function but we cannot rename it. + // Read and write permission requires ADD_VOICEMAIL or carrier privileges. We can't declare + // any permission entries in the manifest because carrier-privileged apps without + // ADD_VOICEMAIL would be blocked by the platform without even reaching our custom + // enforce{Read,Write}PermissionInner functions. These overrides are what allow carrier- + // privileged apps to bypass these runtime-configured permissions. + // TODO(b/74245334): See if these can be removed since individual operations perform their + // own checks. + setReadPermission(android.Manifest.permission.ADD_VOICEMAIL); + setWritePermission(android.Manifest.permission.ADD_VOICEMAIL); setAppOps(AppOpsManager.OP_ADD_VOICEMAIL, AppOpsManager.OP_ADD_VOICEMAIL); mVoicemailPermissions = new VoicemailPermissions(context); @@ -110,6 +117,27 @@ public class VoicemailContentProvider extends ContentProvider return true; } + @Override + protected int enforceReadPermissionInner(Uri uri, String callingPkg, IBinder callerToken) + throws SecurityException { + // Permit carrier-privileged apps regardless of ADD_VOICEMAIL permission state. + if (mVoicemailPermissions.callerHasCarrierPrivileges()) { + return MODE_ALLOWED; + } + return super.enforceReadPermissionInner(uri, callingPkg, callerToken); + } + + + @Override + protected int enforceWritePermissionInner(Uri uri, String callingPkg, IBinder callerToken) + throws SecurityException { + // Permit carrier-privileged apps regardless of ADD_VOICEMAIL permission state. + if (mVoicemailPermissions.callerHasCarrierPrivileges()) { + return MODE_ALLOWED; + } + return super.enforceWritePermissionInner(uri, callingPkg, callerToken); + } + @VisibleForTesting void scheduleScanStalePackages() { scheduleTask(BACKGROUND_TASK_SCAN_STALE_PACKAGES, null); diff --git a/src/com/android/providers/contacts/VoicemailNotifier.java b/src/com/android/providers/contacts/VoicemailNotifier.java index 04a9bd64..159cec73 100644 --- a/src/com/android/providers/contacts/VoicemailNotifier.java +++ b/src/com/android/providers/contacts/VoicemailNotifier.java @@ -1,8 +1,5 @@ package com.android.providers.contacts; -import static android.Manifest.permission.ADD_VOICEMAIL; -import static android.Manifest.permission.READ_VOICEMAIL; - import android.content.ComponentName; import android.content.Context; import android.content.Intent; @@ -15,6 +12,7 @@ import android.util.ArraySet; import android.util.Log; import com.google.android.collect.Lists; + import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -68,11 +66,16 @@ public class VoicemailNotifier { intentAction, uri)); for (ComponentName component : getBroadcastReceiverComponents(intentAction, uri)) { - // Ignore any package that is not affected by the change and don't have full access - // either. - if (!mModifiedPackages.contains(component.getPackageName()) && - !mVoicemailPermissions.packageHasReadAccess( - component.getPackageName())) { + boolean hasFullReadAccess = + mVoicemailPermissions.packageHasReadAccess(component.getPackageName()); + boolean hasOwnAccess = + mVoicemailPermissions.packageHasOwnVoicemailAccess( + component.getPackageName()); + // If we don't have full access, ignore the broadcast if the package isn't affected + // by the change or doesn't have access to its own messages. + if (!hasFullReadAccess + && (!mModifiedPackages.contains(component.getPackageName()) + || !hasOwnAccess)) { continue; } @@ -82,12 +85,10 @@ public class VoicemailNotifier { intent.putExtra(VoicemailContract.EXTRA_SELF_CHANGE, callingPackages.contains(component.getPackageName())); } - String permissionNeeded = mModifiedPackages.contains(component.getPackageName()) ? - ADD_VOICEMAIL : READ_VOICEMAIL; - mContext.sendBroadcast(intent, permissionNeeded); - Log.v(TAG, String.format("Sent intent. act:%s, url:%s, comp:%s, perm:%s," + + mContext.sendBroadcast(intent); + Log.v(TAG, String.format("Sent intent. act:%s, url:%s, comp:%s," + " self_change:%s", intent.getAction(), intent.getData(), - component.getClassName(), permissionNeeded, + component.getClassName(), intent.hasExtra(VoicemailContract.EXTRA_SELF_CHANGE) ? intent.getBooleanExtra(VoicemailContract.EXTRA_SELF_CHANGE, false) : null)); diff --git a/src/com/android/providers/contacts/VoicemailPermissions.java b/src/com/android/providers/contacts/VoicemailPermissions.java index 50f2447a..ed3815dc 100644 --- a/src/com/android/providers/contacts/VoicemailPermissions.java +++ b/src/com/android/providers/contacts/VoicemailPermissions.java @@ -16,10 +16,12 @@ package com.android.providers.contacts; -import com.android.providers.contacts.util.ContactsPermissions; - import android.content.Context; +import android.os.Binder; import android.telecom.DefaultDialerManager; +import android.telephony.TelephonyManager; + +import com.android.providers.contacts.util.ContactsPermissions; /** * Provides method related to check various voicemail permissions under the @@ -35,7 +37,8 @@ public class VoicemailPermissions { /** Determines if the calling process has access to its own voicemails. */ public boolean callerHasOwnVoicemailAccess() { - return callerHasPermission(android.Manifest.permission.ADD_VOICEMAIL); + return callerHasPermission(android.Manifest.permission.ADD_VOICEMAIL) + || callerHasCarrierPrivileges(); } /** Determine if the calling process has full read access to all voicemails. */ @@ -63,7 +66,7 @@ public class VoicemailPermissions { public void checkCallerHasOwnVoicemailAccess() { if (!callerHasOwnVoicemailAccess()) { throw new SecurityException("The caller must have permission: " + - android.Manifest.permission.ADD_VOICEMAIL); + android.Manifest.permission.ADD_VOICEMAIL + " or carrier privileges"); } } @@ -91,7 +94,8 @@ public class VoicemailPermissions { /** Determines if the given package has access to its own voicemails. */ public boolean packageHasOwnVoicemailAccess(String packageName) { return packageHasPermission(packageName, - android.Manifest.permission.ADD_VOICEMAIL); + android.Manifest.permission.ADD_VOICEMAIL) + || packageHasCarrierPrivileges(packageName); } /** Determines if the given package has read access. */ @@ -113,4 +117,25 @@ public class VoicemailPermissions { private boolean callerHasPermission(String permission) { return ContactsPermissions.hasCallerOrSelfPermission(mContext, permission); } + + /** Determines if the calling process has carrier privileges. */ + public boolean callerHasCarrierPrivileges() { + TelephonyManager tm = + (TelephonyManager) mContext.getSystemService(Context.TELEPHONY_SERVICE); + String[] packages = mContext.getPackageManager().getPackagesForUid(Binder.getCallingUid()); + for (String packageName : packages) { + if (tm.checkCarrierPrivilegesForPackageAnyPhone(packageName) + == TelephonyManager.CARRIER_PRIVILEGE_STATUS_HAS_ACCESS) { + return true; + } + } + return false; + } + + /** Determines if the given package has carrier privileges. */ + private boolean packageHasCarrierPrivileges(String packageName) { + TelephonyManager tm = + (TelephonyManager) mContext.getSystemService(Context.TELEPHONY_SERVICE); + return tm.getPackagesWithCarrierPrivileges().contains(packageName); + } } diff --git a/tests/src/com/android/providers/contacts/BaseVoicemailProviderTest.java b/tests/src/com/android/providers/contacts/BaseVoicemailProviderTest.java index 1020da89..665f68ba 100644 --- a/tests/src/com/android/providers/contacts/BaseVoicemailProviderTest.java +++ b/tests/src/com/android/providers/contacts/BaseVoicemailProviderTest.java @@ -70,6 +70,15 @@ public abstract class BaseVoicemailProviderTest extends BaseContactsProvider2Tes mActor.removePermissions(READ_VOICEMAIL_PERMISSION); mActor.removePermissions(WRITE_VOICEMAIL_PERMISSION); mActor.addPermissions(ADD_VOICEMAIL_PERMISSION); + mActor.revokeCarrierPrivileges(); + mUseSourceUri = true; + } + + protected void setUpForOwnPermissionViaCarrierPrivileges() { + mActor.removePermissions(READ_VOICEMAIL_PERMISSION); + mActor.removePermissions(WRITE_VOICEMAIL_PERMISSION); + mActor.removePermissions(ADD_VOICEMAIL_PERMISSION); + mActor.grantCarrierPrivileges(); mUseSourceUri = true; } @@ -77,6 +86,7 @@ public abstract class BaseVoicemailProviderTest extends BaseContactsProvider2Tes mActor.addPermissions(ADD_VOICEMAIL_PERMISSION); mActor.addPermissions(READ_VOICEMAIL_PERMISSION); mActor.addPermissions(WRITE_VOICEMAIL_PERMISSION); + mActor.revokeCarrierPrivileges(); mUseSourceUri = false; } @@ -84,6 +94,7 @@ public abstract class BaseVoicemailProviderTest extends BaseContactsProvider2Tes mActor.removePermissions(ADD_VOICEMAIL_PERMISSION); mActor.removePermissions(READ_VOICEMAIL_PERMISSION); mActor.removePermissions(WRITE_VOICEMAIL_PERMISSION); + mActor.revokeCarrierPrivileges(); mUseSourceUri = true; } diff --git a/tests/src/com/android/providers/contacts/ContactsActor.java b/tests/src/com/android/providers/contacts/ContactsActor.java index 11a13c27..470f64b7 100644 --- a/tests/src/com/android/providers/contacts/ContactsActor.java +++ b/tests/src/com/android/providers/contacts/ContactsActor.java @@ -57,9 +57,11 @@ import android.provider.ContactsContract.Contacts; import android.provider.ContactsContract.Data; import android.provider.ContactsContract.RawContacts; import android.provider.ContactsContract.StatusUpdates; +import android.telephony.TelephonyManager; import android.test.IsolatedContext; import android.test.mock.MockContentResolver; import android.test.mock.MockContext; +import android.text.TextUtils; import com.android.providers.contacts.util.ContactsPermissions; import com.android.providers.contacts.util.MockSharedPreferences; @@ -70,6 +72,7 @@ import java.io.File; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Set; @@ -98,6 +101,7 @@ public class ContactsActor { private Set<String> mGrantedPermissions = Sets.newHashSet(); private final Set<Uri> mGrantedUriPermissions = Sets.newHashSet(); + private boolean mHasCarrierPrivileges; private List<ContentProvider> mAllProviders = new ArrayList<>(); @@ -242,6 +246,31 @@ public class ContactsActor { } } + private MockTelephonyManager mMockTelephonyManager; + + private class MockTelephonyManager extends TelephonyManager { + public MockTelephonyManager(Context context) { + super(context); + } + + @Override + public int checkCarrierPrivilegesForPackageAnyPhone(String packageName) { + if (TextUtils.equals(packageName, ContactsActor.this.packageName) + && mHasCarrierPrivileges) { + return TelephonyManager.CARRIER_PRIVILEGE_STATUS_HAS_ACCESS; + } + return TelephonyManager.CARRIER_PRIVILEGE_STATUS_NO_ACCESS; + } + + @Override + public List<String> getPackagesWithCarrierPrivileges() { + if (!mHasCarrierPrivileges) { + return Collections.emptyList(); + } + return Collections.singletonList(packageName); + } + } + /** * A context wrapper that reports a different user id. * @@ -290,6 +319,9 @@ public class ContactsActor { if (Context.USER_SERVICE.equals(name)) { return mockUserManager; } + if (Context.TELEPHONY_SERVICE.equals(name)) { + return mMockTelephonyManager; + } // Use overallContext here; super.getSystemService() somehow won't return // DevicePolicyManager. return overallContext.getSystemService(name); @@ -334,6 +366,9 @@ public class ContactsActor { if (Context.USER_SERVICE.equals(name)) { return mockUserManager; } + if (Context.TELEPHONY_SERVICE.equals(name)) { + return mMockTelephonyManager; + } // Use overallContext here; super.getSystemService() somehow won't return // DevicePolicyManager. return overallContext.getSystemService(name); @@ -367,6 +402,7 @@ public class ContactsActor { mMockAccountManager = new MockAccountManager(mProviderContext); mockUserManager = new MockUserManager(mProviderContext); + mMockTelephonyManager = new MockTelephonyManager(mProviderContext); provider = addProvider(providerClass, authority); } @@ -427,6 +463,14 @@ public class ContactsActor { mGrantedUriPermissions.removeAll(Arrays.asList(uris)); } + public void grantCarrierPrivileges() { + mHasCarrierPrivileges = true; + } + + public void revokeCarrierPrivileges() { + mHasCarrierPrivileges = false; + } + /** * Mock {@link Context} that reports specific well-known values for testing * data protection. The creator can override the owner package name, and diff --git a/tests/src/com/android/providers/contacts/VoicemailProviderTest.java b/tests/src/com/android/providers/contacts/VoicemailProviderTest.java index d2bfa875..9f9ef002 100644 --- a/tests/src/com/android/providers/contacts/VoicemailProviderTest.java +++ b/tests/src/com/android/providers/contacts/VoicemailProviderTest.java @@ -35,13 +35,14 @@ import android.test.suitebuilder.annotation.SmallTest; import com.android.common.io.MoreCloseables; +import org.mockito.Mockito; + import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.Arrays; import java.util.List; -import org.mockito.Mockito; /** * Unit tests for {@link VoicemailContentProvider}. @@ -365,6 +366,12 @@ public class VoicemailProviderTest extends BaseVoicemailProviderTest { // no package URI specified). public void testMustUsePackageUriWithoutFullPermission() { setUpForOwnPermission(); + assertBaseUriThrowsSecurityExceptions(); + setUpForOwnPermissionViaCarrierPrivileges(); + assertBaseUriThrowsSecurityExceptions(); + } + + private void assertBaseUriThrowsSecurityExceptions() { EvenMoreAsserts.assertThrows(SecurityException.class, new Runnable() { @Override public void run() { @@ -403,24 +410,31 @@ public class VoicemailProviderTest extends BaseVoicemailProviderTest { // Now give away full permission and check that only 1 message is accessible. setUpForOwnPermission(); - assertEquals(1, getCount(voicemailUri(), null, null)); + assertOnlyOwnVoicemailsCanBeQueriedAndInserted(); + // Same as above, but with carrier privileges. + setUpForOwnPermissionViaCarrierPrivileges(); + assertOnlyOwnVoicemailsCanBeQueriedAndInserted(); - // Once again try to insert message for another package. This time - // it should fail. + setUpForNoPermission(); + mUseSourceUri = false; + // With the READ_ALL_VOICEMAIL permission, we should now be able to read all voicemails + mActor.addPermissions(READ_VOICEMAIL_PERMISSION); + assertEquals(2, getCount(voicemailUri(), null, null)); + + // An insert for another package should still fail EvenMoreAsserts.assertThrows(SecurityException.class, new Runnable() { @Override public void run() { insertVoicemailForSourcePackage("another-package"); } }); + } - setUpForNoPermission(); - mUseSourceUri = false; - // With the READ_ALL_VOICEMAIL permission, we should now be able to read all voicemails - mActor.addPermissions(READ_VOICEMAIL_PERMISSION); - assertEquals(2, getCount(voicemailUri(), null, null)); + private void assertOnlyOwnVoicemailsCanBeQueriedAndInserted() { + assertEquals(1, getCount(voicemailUri(), null, null)); - // An insert for another package should still fail + // Once again try to insert message for another package. This time + // it should fail. EvenMoreAsserts.assertThrows(SecurityException.class, new Runnable() { @Override public void run() { @@ -439,23 +453,9 @@ public class VoicemailProviderTest extends BaseVoicemailProviderTest { // Now give away full permission and check that we can update and delete only // the own voicemail. setUpForOwnPermission(); - mResolver.update(withSourcePackageParam(ownVoicemail), - getTestVoicemailValues(), null, null); - mResolver.delete(withSourcePackageParam(ownVoicemail), null, null); - - // However, attempting to update or delete another-package's voicemail should fail. - EvenMoreAsserts.assertThrows(SecurityException.class, new Runnable() { - @Override - public void run() { - mResolver.update(anotherVoicemail, null, null, null); - } - }); - EvenMoreAsserts.assertThrows(SecurityException.class, new Runnable() { - @Override - public void run() { - mResolver.delete(anotherVoicemail, null, null); - } - }); + assertOnlyOwnVoicemailsCanBeUpdatedAndDeleted(ownVoicemail, anotherVoicemail); + setUpForOwnPermissionViaCarrierPrivileges(); + assertOnlyOwnVoicemailsCanBeUpdatedAndDeleted(ownVoicemail, anotherVoicemail); // If we have the manage voicemail permission, we should be able to both update voicemails // from all packages. @@ -478,6 +478,27 @@ public class VoicemailProviderTest extends BaseVoicemailProviderTest { assertEquals(0, getCount(anotherVoicemail, null, null)); } + private void assertOnlyOwnVoicemailsCanBeUpdatedAndDeleted( + Uri ownVoicemail, Uri anotherVoicemail) { + mResolver.update(withSourcePackageParam(ownVoicemail), + getTestVoicemailValues(), null, null); + mResolver.delete(withSourcePackageParam(ownVoicemail), null, null); + + // However, attempting to update or delete another-package's voicemail should fail. + EvenMoreAsserts.assertThrows(SecurityException.class, new Runnable() { + @Override + public void run() { + mResolver.update(anotherVoicemail, null, null, null); + } + }); + EvenMoreAsserts.assertThrows(SecurityException.class, new Runnable() { + @Override + public void run() { + mResolver.delete(anotherVoicemail, null, null); + } + }); + } + private Uri withSourcePackageParam(Uri uri) { return uri.buildUpon() .appendQueryParameter(VoicemailContract.PARAM_KEY_SOURCE_PACKAGE, |