From ce852ddcecefc6b3e1265462f5c8670bd0e4ace1 Mon Sep 17 00:00:00 2001 From: Svetoslav Date: Tue, 8 Sep 2015 14:36:35 -0700 Subject: Add get_accounts app op For each runtime permission we have an app op to toggle the permission for legacy apps as they cannot handle permission revocations. We were lacking an app op for get_accounts which prevented the user from controlling access to accounts regardelss that they change the state of the permission toggle in the UI. Even worse the permission UI is written with the assumption that every runtime permission has an app op and as a result revoking the contacts group (if the app requests the get_accounts permission) is reset back to allowed in the UI. bug:23854618 Change-Id: I9e3f9bfeb320bed561d718db99ee285915d5701b --- .../server/accounts/AccountManagerService.java | 86 ++++++++++++---------- .../com/android/server/content/SyncManager.java | 6 +- .../server/accounts/AccountManagerServiceTest.java | 6 +- 3 files changed, 54 insertions(+), 44 deletions(-) (limited to 'services') diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java index 8b0e6f2..d590d7a 100644 --- a/services/core/java/com/android/server/accounts/AccountManagerService.java +++ b/services/core/java/com/android/server/accounts/AccountManagerService.java @@ -32,6 +32,7 @@ import android.accounts.IAccountManagerResponse; import android.app.ActivityManager; import android.app.ActivityManagerNative; import android.app.AppGlobals; +import android.app.AppOpsManager; import android.app.Notification; import android.app.NotificationManager; import android.app.PendingIntent; @@ -122,6 +123,7 @@ public class AccountManagerService private final Context mContext; private final PackageManager mPackageManager; + private final AppOpsManager mAppOpsManager; private UserManager mUserManager; private final MessageHandler mMessageHandler; @@ -266,6 +268,7 @@ public class AccountManagerService IAccountAuthenticatorCache authenticatorCache) { mContext = context; mPackageManager = packageManager; + mAppOpsManager = mContext.getSystemService(AppOpsManager.class); mMessageHandler = new MessageHandler(FgThread.get().getLooper()); @@ -510,7 +513,7 @@ public class AccountManagerService // Check if there's a shared account that needs to be created as an account Account[] sharedAccounts = getSharedAccountsAsUser(userId); if (sharedAccounts == null || sharedAccounts.length == 0) return; - Account[] accounts = getAccountsAsUser(null, userId); + Account[] accounts = getAccountsAsUser(null, userId, mContext.getOpPackageName()); for (Account sa : sharedAccounts) { if (ArrayUtils.contains(accounts, sa)) continue; // Account doesn't exist. Copy it now. @@ -868,7 +871,8 @@ public class AccountManagerService // Confirm that the owner's account still exists before this step. UserAccounts owner = getUserAccounts(UserHandle.USER_OWNER); synchronized (owner.cacheLock) { - for (Account acc : getAccounts(UserHandle.USER_OWNER)) { + for (Account acc : getAccounts(UserHandle.USER_OWNER, + mContext.getOpPackageName())) { if (acc.equals(account)) { mAuthenticator.addAccountFromCredentials( this, account, accountCredentials); @@ -988,7 +992,7 @@ public class AccountManagerService @Override public void hasFeatures(IAccountManagerResponse response, - Account account, String[] features) { + Account account, String[] features, String opPackageName) { int callingUid = Binder.getCallingUid(); if (Log.isLoggable(TAG, Log.VERBOSE)) { Log.v(TAG, "hasFeatures: " + account @@ -1001,7 +1005,8 @@ public class AccountManagerService if (account == null) throw new IllegalArgumentException("account is null"); if (features == null) throw new IllegalArgumentException("features is null"); int userId = UserHandle.getCallingUserId(); - checkReadAccountsPermitted(callingUid, account.type, userId); + checkReadAccountsPermitted(callingUid, account.type, userId, + opPackageName); long identityToken = clearCallingIdentity(); try { @@ -2507,9 +2512,10 @@ public class AccountManagerService * Returns the accounts visible to the client within the context of a specific user * @hide */ - public Account[] getAccounts(int userId) { + public Account[] getAccounts(int userId, String opPackageName) { int callingUid = Binder.getCallingUid(); - List visibleAccountTypes = getTypesVisibleToCaller(callingUid, userId); + List visibleAccountTypes = getTypesVisibleToCaller(callingUid, userId, + opPackageName); if (visibleAccountTypes.isEmpty()) { return new Account[0]; } @@ -2571,15 +2577,16 @@ public class AccountManagerService } @Override - public Account[] getAccountsAsUser(String type, int userId) { - return getAccountsAsUser(type, userId, null, -1); + public Account[] getAccountsAsUser(String type, int userId, String opPackageName) { + return getAccountsAsUser(type, userId, null, -1, opPackageName); } private Account[] getAccountsAsUser( String type, int userId, String callingPackage, - int packageUid) { + int packageUid, + String opPackageName) { int callingUid = Binder.getCallingUid(); // Only allow the system process to read accounts of other users if (userId != UserHandle.getCallingUserId() @@ -2602,7 +2609,8 @@ public class AccountManagerService callingUid = packageUid; } - List visibleAccountTypes = getTypesVisibleToCaller(callingUid, userId); + List visibleAccountTypes = getTypesVisibleToCaller(callingUid, userId, + opPackageName); if (visibleAccountTypes.isEmpty() || (type != null && !visibleAccountTypes.contains(type))) { return new Account[0]; @@ -2741,22 +2749,24 @@ public class AccountManagerService } @Override - public Account[] getAccounts(String type) { - return getAccountsAsUser(type, UserHandle.getCallingUserId()); + public Account[] getAccounts(String type, String opPackageName) { + return getAccountsAsUser(type, UserHandle.getCallingUserId(), opPackageName); } @Override - public Account[] getAccountsForPackage(String packageName, int uid) { + public Account[] getAccountsForPackage(String packageName, int uid, String opPackageName) { int callingUid = Binder.getCallingUid(); if (!UserHandle.isSameApp(callingUid, Process.myUid())) { throw new SecurityException("getAccountsForPackage() called from unauthorized uid " + callingUid + " with uid=" + uid); } - return getAccountsAsUser(null, UserHandle.getCallingUserId(), packageName, uid); + return getAccountsAsUser(null, UserHandle.getCallingUserId(), packageName, uid, + opPackageName); } @Override - public Account[] getAccountsByTypeForPackage(String type, String packageName) { + public Account[] getAccountsByTypeForPackage(String type, String packageName, + String opPackageName) { int packageUid = -1; try { packageUid = AppGlobals.getPackageManager().getPackageUid( @@ -2765,14 +2775,16 @@ public class AccountManagerService Slog.e(TAG, "Couldn't determine the packageUid for " + packageName + re); return new Account[0]; } - return getAccountsAsUser(type, UserHandle.getCallingUserId(), packageName, packageUid); + return getAccountsAsUser(type, UserHandle.getCallingUserId(), packageName, + packageUid, opPackageName); } @Override public void getAccountsByFeatures( IAccountManagerResponse response, String type, - String[] features) { + String[] features, + String opPackageName) { int callingUid = Binder.getCallingUid(); if (Log.isLoggable(TAG, Log.VERBOSE)) { Log.v(TAG, "getAccounts: accountType " + type @@ -2785,7 +2797,8 @@ public class AccountManagerService if (type == null) throw new IllegalArgumentException("accountType is null"); int userId = UserHandle.getCallingUserId(); - List visibleAccountTypes = getTypesVisibleToCaller(callingUid, userId); + List visibleAccountTypes = getTypesVisibleToCaller(callingUid, userId, + opPackageName); if (!visibleAccountTypes.contains(type)) { Bundle result = new Bundle(); // Need to return just the accounts that are from matching signatures. @@ -3685,31 +3698,22 @@ public class AccountManagerService } } - private boolean isPermitted(int callingUid, String... permissions) { + private boolean isPermitted(String opPackageName, int callingUid, String... permissions) { for (String perm : permissions) { if (mContext.checkCallingOrSelfPermission(perm) == PackageManager.PERMISSION_GRANTED) { if (Log.isLoggable(TAG, Log.VERBOSE)) { Log.v(TAG, " caller uid " + callingUid + " has " + perm); } - return true; + final int opCode = AppOpsManager.permissionToOpCode(perm); + if (opCode == AppOpsManager.OP_NONE || mAppOpsManager.noteOp( + opCode, callingUid, opPackageName) == AppOpsManager.MODE_ALLOWED) { + return true; + } } } return false; } - /** Succeeds if any of the specified permissions are granted. */ - private void checkBinderPermission(String... permissions) { - final int callingUid = Binder.getCallingUid(); - if (isPermitted(callingUid, permissions)) { - String msg = String.format( - "caller uid %s lacks any of %s", - callingUid, - TextUtils.join(",", permissions)); - Log.w(TAG, " " + msg); - throw new SecurityException(msg); - } - } - private int handleIncomingUser(int userId) { try { return ActivityManagerNative.getDefault().handleIncomingUser( @@ -3763,11 +3767,13 @@ public class AccountManagerService return fromAuthenticator || hasExplicitGrants || isPrivileged; } - private boolean isAccountVisibleToCaller(String accountType, int callingUid, int userId) { + private boolean isAccountVisibleToCaller(String accountType, int callingUid, int userId, + String opPackageName) { if (accountType == null) { return false; } else { - return getTypesVisibleToCaller(callingUid, userId).contains(accountType); + return getTypesVisibleToCaller(callingUid, userId, + opPackageName).contains(accountType); } } @@ -3779,9 +3785,10 @@ public class AccountManagerService } } - private List getTypesVisibleToCaller(int callingUid, int userId) { + private List getTypesVisibleToCaller(int callingUid, int userId, + String opPackageName) { boolean isPermitted = - isPermitted(callingUid, Manifest.permission.GET_ACCOUNTS, + isPermitted(opPackageName, callingUid, Manifest.permission.GET_ACCOUNTS, Manifest.permission.GET_ACCOUNTS_PRIVILEGED); Log.i(TAG, String.format("getTypesVisibleToCaller: isPermitted? %s", isPermitted)); return getTypesForCaller(callingUid, userId, isPermitted); @@ -3877,8 +3884,9 @@ public class AccountManagerService private void checkReadAccountsPermitted( int callingUid, String accountType, - int userId) { - if (!isAccountVisibleToCaller(accountType, callingUid, userId)) { + int userId, + String opPackageName) { + if (!isAccountVisibleToCaller(accountType, callingUid, userId, opPackageName)) { String msg = String.format( "caller uid %s cannot access %s accounts", callingUid, diff --git a/services/core/java/com/android/server/content/SyncManager.java b/services/core/java/com/android/server/content/SyncManager.java index c998c2c..334bc18 100644 --- a/services/core/java/com/android/server/content/SyncManager.java +++ b/services/core/java/com/android/server/content/SyncManager.java @@ -340,7 +340,8 @@ public class SyncManager { for (UserInfo user : mUserManager.getUsers(true)) { // Skip any partially created/removed users if (user.partial) continue; - Account[] accountsForUser = AccountManagerService.getSingleton().getAccounts(user.id); + Account[] accountsForUser = AccountManagerService.getSingleton().getAccounts( + user.id, mContext.getOpPackageName()); mSyncStorageEngine.doDatabaseCleanup(accountsForUser, user.id); } } @@ -1232,7 +1233,8 @@ public class SyncManager { } // Schedule sync for any accounts under started user - final Account[] accounts = AccountManagerService.getSingleton().getAccounts(userId); + final Account[] accounts = AccountManagerService.getSingleton().getAccounts(userId, + mContext.getOpPackageName()); for (Account account : accounts) { scheduleSync(account, userId, SyncOperation.REASON_USER_START, null, null, 0 /* no delay */, 0 /* No flex */, diff --git a/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java index 0f20dde..f9aa124 100644 --- a/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/accounts/AccountManagerServiceTest.java @@ -82,7 +82,7 @@ public class AccountManagerServiceTest extends AndroidTestCase { mAms.addAccountExplicitly(a31, "p31", null); mAms.addAccountExplicitly(a32, "p32", null); - Account[] accounts = mAms.getAccounts(null); + Account[] accounts = mAms.getAccounts(null, mContext.getOpPackageName()); Arrays.sort(accounts, new AccountSorter()); assertEquals(6, accounts.length); assertEquals(a11, accounts[0]); @@ -92,7 +92,7 @@ public class AccountManagerServiceTest extends AndroidTestCase { assertEquals(a22, accounts[4]); assertEquals(a32, accounts[5]); - accounts = mAms.getAccounts("type1" ); + accounts = mAms.getAccounts("type1", mContext.getOpPackageName()); Arrays.sort(accounts, new AccountSorter()); assertEquals(3, accounts.length); assertEquals(a11, accounts[0]); @@ -101,7 +101,7 @@ public class AccountManagerServiceTest extends AndroidTestCase { mAms.removeAccountInternal(a21); - accounts = mAms.getAccounts("type1" ); + accounts = mAms.getAccounts("type1", mContext.getOpPackageName()); Arrays.sort(accounts, new AccountSorter()); assertEquals(2, accounts.length); assertEquals(a11, accounts[0]); -- cgit v1.1