From 4ab5af1eb569a46f2117377382aac7dc6535cfc5 Mon Sep 17 00:00:00 2001 From: Tobias Thierer Date: Mon, 19 Oct 2020 19:37:30 +0100 Subject: [PATCH 1/3] Enforce BACKUP permission on Service end. BackupManager runs in the client process, whereas BackupManagerService runs in the system server process. Therefore, apps permissions need should be enforced on the service side. Bug: 158482162 Test: Manually checked that a sample app encounters SecurityException after but not before this CL, when running code similar to the sample in http://b/158482162#comment1 to figure out whether isBackupServiceActive() for its own uid. Change-Id: I59693819542a80a065a9c88373393b0ba0dbef65 Merged-In: I59693819542a80a065a9c88373393b0ba0dbef65 (cherry picked from commit b2ab2c414fcac5f68edd8d790d3773f1f9330e3b) --- core/java/android/app/backup/BackupManager.java | 2 -- .../java/com/android/server/backup/BackupManagerService.java | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/java/android/app/backup/BackupManager.java b/core/java/android/app/backup/BackupManager.java index 3bc043ee09126..56e9c36fe4b4c 100644 --- a/core/java/android/app/backup/BackupManager.java +++ b/core/java/android/app/backup/BackupManager.java @@ -401,8 +401,6 @@ public class BackupManager { @SystemApi @RequiresPermission(android.Manifest.permission.BACKUP) public boolean isBackupServiceActive(UserHandle user) { - mContext.enforceCallingOrSelfPermission(android.Manifest.permission.BACKUP, - "isBackupServiceActive"); checkServiceBinder(); if (sService != null) { try { diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java index b13bef2de1512..19ada753f33d7 100644 --- a/services/backup/java/com/android/server/backup/BackupManagerService.java +++ b/services/backup/java/com/android/server/backup/BackupManagerService.java @@ -506,6 +506,8 @@ public class BackupManagerService extends IBackupManager.Stub { */ @Override public boolean isBackupServiceActive(int userId) { + mContext.enforceCallingPermission(android.Manifest.permission.BACKUP, + "isBackupServiceActive"); synchronized (mStateLock) { return !mGlobalDisable && isBackupActivatedForUser(userId); } From b50cdde5f009ca1e8a548605c8b364d860110b67 Mon Sep 17 00:00:00 2001 From: Tobias Thierer Date: Mon, 19 Oct 2020 19:37:30 +0100 Subject: [PATCH 2/3] BackupManagerService: Make new behavior conditional on ChangeId. Base CL ag/12885739 introduced unconditional enforcement of the BACKUP permission for callers of BackupManagerService.isBackupServiceActive() in the service, but dropped the enforcement on the app process side (BackupManager). This CL makes the behavior change conditional on a compat ChangeId. Bug: 158482162 Test: Manually checked that an app similar to the code sample from http://b/158482162#comment1 can reproduce the behavior. This is true both before the base CL and after this CL, when the app targets an old SDK version (26). Test: Checked that both (a) before this CL, (b) after this CL where the change is manually enabled for the app via the below commands, the app runs into a SecurityException instead: $ adb shell am compat enable 158482162 com.example.tester $ adb shell dumpsys platform_compat | grep 158482162 ChangeId(158482162; name=IS_BACKUP_SERVICE_ACTIVE_ENFORCE_PERMISSION_IN_SERVICE; enableSinceTargetSdk=31; packageOverrides={com.example.tester=true}) Change-Id: I58e5d2a0b438296137fd76720636c8fdce740ded Merged-In: I58e5d2a0b438296137fd76720636c8fdce740ded (cherry picked from commit 7671f0d95c7e92a2342ed05a053a38d8aa16a17f) --- .../android/app/backup/BackupManager.java | 20 +++++++++++++++++++ .../server/backup/BackupManagerService.java | 9 +++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/core/java/android/app/backup/BackupManager.java b/core/java/android/app/backup/BackupManager.java index 56e9c36fe4b4c..acaea5fdd6098 100644 --- a/core/java/android/app/backup/BackupManager.java +++ b/core/java/android/app/backup/BackupManager.java @@ -21,10 +21,14 @@ import android.annotation.Nullable; import android.annotation.RequiresPermission; import android.annotation.SystemApi; import android.annotation.TestApi; +import android.app.compat.CompatChanges; +import android.compat.annotation.ChangeId; +import android.compat.annotation.EnabledAfter; import android.compat.annotation.UnsupportedAppUsage; import android.content.ComponentName; import android.content.Context; import android.content.Intent; +import android.os.Build; import android.os.Bundle; import android.os.Handler; import android.os.Message; @@ -391,6 +395,17 @@ public class BackupManager { return false; } + + /** + * If this change is enabled, the {@code BACKUP} permission needed for + * {@code isBackupServiceActive()} will be enforced on the service end + * rather than client-side in {@link BackupManager}. + * @hide + */ + @ChangeId + @EnabledAfter(targetSdkVersion = Build.VERSION_CODES.R) + public static final long IS_BACKUP_SERVICE_ACTIVE_ENFORCE_PERMISSION_IN_SERVICE = 158482162; + /** * Report whether the backup mechanism is currently active. * When it is inactive, the device will not perform any backup operations, nor will it @@ -401,6 +416,11 @@ public class BackupManager { @SystemApi @RequiresPermission(android.Manifest.permission.BACKUP) public boolean isBackupServiceActive(UserHandle user) { + if (!CompatChanges.isChangeEnabled( + IS_BACKUP_SERVICE_ACTIVE_ENFORCE_PERMISSION_IN_SERVICE)) { + mContext.enforceCallingOrSelfPermission(android.Manifest.permission.BACKUP, + "isBackupServiceActive"); + } checkServiceBinder(); if (sService != null) { try { diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java index 19ada753f33d7..445297a839ba6 100644 --- a/services/backup/java/com/android/server/backup/BackupManagerService.java +++ b/services/backup/java/com/android/server/backup/BackupManagerService.java @@ -30,6 +30,7 @@ import android.app.backup.IBackupObserver; import android.app.backup.IFullBackupRestoreObserver; import android.app.backup.IRestoreSession; import android.app.backup.ISelectBackupTransportCallback; +import android.app.compat.CompatChanges; import android.app.job.JobParameters; import android.app.job.JobScheduler; import android.app.job.JobService; @@ -506,8 +507,12 @@ public class BackupManagerService extends IBackupManager.Stub { */ @Override public boolean isBackupServiceActive(int userId) { - mContext.enforceCallingPermission(android.Manifest.permission.BACKUP, - "isBackupServiceActive"); + int callingUid = Binder.getCallingUid(); + if (CompatChanges.isChangeEnabled( + BackupManager.IS_BACKUP_SERVICE_ACTIVE_ENFORCE_PERMISSION_IN_SERVICE, callingUid)) { + mContext.enforceCallingPermission(android.Manifest.permission.BACKUP, + "isBackupServiceActive"); + } synchronized (mStateLock) { return !mGlobalDisable && isBackupActivatedForUser(userId); } From d8b61b515938cecd441fae16fc001f1e49274780 Mon Sep 17 00:00:00 2001 From: Tobias Thierer Date: Thu, 10 Dec 2020 13:46:08 +0000 Subject: [PATCH 3/3] Fix DevicePolicyManager.isBackupServiceEnabled() breakage. http://ag/12885739 introduced a enforceCallingPermission(BACKUP) check but callers of this API do not hold that permission. This CL fixes this by changing the check to enforceCallingOrSelfPermission(BACKUP), and clearing the binder identity in DevicePolicyManagerService, which makes the system server process the owner of that call. Bug: 158482162 Bug: 172466964 Test: atest com.android.cts.devicepolicy.{Device,Profile}OwnerTest#testBackupServiceEnabling Change-Id: I11d863229c4d62a058aaf37446a694b9c73ae5b8 Merged-In: I11d863229c4d62a058aaf37446a694b9c73ae5b8 (cherry picked from commit 630dec9eb496bc70db44c4f0b4196ea91e7df6cb) --- .../server/backup/BackupManagerService.java | 2 +- .../DevicePolicyManagerService.java | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java index 445297a839ba6..89a6eca8fdaad 100644 --- a/services/backup/java/com/android/server/backup/BackupManagerService.java +++ b/services/backup/java/com/android/server/backup/BackupManagerService.java @@ -510,7 +510,7 @@ public class BackupManagerService extends IBackupManager.Stub { int callingUid = Binder.getCallingUid(); if (CompatChanges.isChangeEnabled( BackupManager.IS_BACKUP_SERVICE_ACTIVE_ENFORCE_PERMISSION_IN_SERVICE, callingUid)) { - mContext.enforceCallingPermission(android.Manifest.permission.BACKUP, + mContext.enforceCallingOrSelfPermission(android.Manifest.permission.BACKUP, "isBackupServiceActive"); } synchronized (mStateLock) { diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index ce3cdeab232a5..33a346d1ac9b4 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -14438,15 +14438,17 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } enforceProfileOrDeviceOwner(admin); - synchronized (getLockObject()) { - try { - IBackupManager ibm = mInjector.getIBackupManager(); - return ibm != null && ibm.isBackupServiceActive( - mInjector.userHandleGetCallingUserId()); - } catch (RemoteException e) { - throw new IllegalStateException("Failed requesting backup service state.", e); + final int userId = mInjector.userHandleGetCallingUserId(); + return mInjector.binderWithCleanCallingIdentity(() -> { + synchronized (getLockObject()) { + try { + IBackupManager ibm = mInjector.getIBackupManager(); + return ibm != null && ibm.isBackupServiceActive(userId); + } catch (RemoteException e) { + throw new IllegalStateException("Failed requesting backup service state.", e); + } } - } + }); } @Override