From 82e7d88ad3b72378ed093e37e2e2d13b35da09c9 Mon Sep 17 00:00:00 2001 From: Reema Bajwa Date: Sat, 18 Feb 2023 00:49:03 +0000 Subject: [PATCH] Guard setting of remote entry with a permission As part of API feedback review from internal alignment on allowing a single remote entry provider only, based on OEM config + permission. Currently the permission is being checked in CredentialProviderService, so as to throw the SecurityException in the provider process. This change also contains a second round of check for the permission and the OEM configured provider, in the system server. If the second layer of checls fail, the entry is silently ignored. Bug: 267665515 Test: built & deployed locally Change-Id: Ia6bd81da2f19226df469eb733054e46bdfa98e69 --- core/api/current.txt | 4 +- core/api/system-current.txt | 2 +- .../BeginCreateCredentialResponse.java | 12 ++++++ .../BeginGetCredentialResponse.java | 12 ++++++ .../CredentialProviderService.java | 20 ++++++++++ core/res/AndroidManifest.xml | 4 +- .../credentials/ProviderCreateSession.java | 23 ++++++----- .../credentials/ProviderGetSession.java | 39 ++++++++++++------- .../server/credentials/ProviderSession.java | 36 +++++++++++++++++ .../credentials/RemoteCredentialService.java | 31 ++------------- 10 files changed, 126 insertions(+), 57 deletions(-) diff --git a/core/api/current.txt b/core/api/current.txt index 259f1e9d4c538..133add1569108 100644 --- a/core/api/current.txt +++ b/core/api/current.txt @@ -40549,7 +40549,7 @@ package android.service.credentials { method @NonNull public android.service.credentials.BeginCreateCredentialResponse.Builder addCreateEntry(@NonNull android.service.credentials.CreateEntry); method @NonNull public android.service.credentials.BeginCreateCredentialResponse build(); method @NonNull public android.service.credentials.BeginCreateCredentialResponse.Builder setCreateEntries(@NonNull java.util.List); - method @NonNull public android.service.credentials.BeginCreateCredentialResponse.Builder setRemoteCreateEntry(@Nullable android.service.credentials.RemoteEntry); + method @NonNull @RequiresPermission("android.permission.PROVIDE_REMOTE_CREDENTIALS") public android.service.credentials.BeginCreateCredentialResponse.Builder setRemoteCreateEntry(@Nullable android.service.credentials.RemoteEntry); } public class BeginGetCredentialOption implements android.os.Parcelable { @@ -40598,7 +40598,7 @@ package android.service.credentials { method @NonNull public android.service.credentials.BeginGetCredentialResponse.Builder setActions(@NonNull java.util.List); method @NonNull public android.service.credentials.BeginGetCredentialResponse.Builder setAuthenticationActions(@NonNull java.util.List); method @NonNull public android.service.credentials.BeginGetCredentialResponse.Builder setCredentialEntries(@NonNull java.util.List); - method @NonNull public android.service.credentials.BeginGetCredentialResponse.Builder setRemoteCredentialEntry(@Nullable android.service.credentials.RemoteEntry); + method @NonNull @RequiresPermission("android.permission.PROVIDE_REMOTE_CREDENTIALS") public android.service.credentials.BeginGetCredentialResponse.Builder setRemoteCredentialEntry(@Nullable android.service.credentials.RemoteEntry); } public final class CallingAppInfo implements android.os.Parcelable { diff --git a/core/api/system-current.txt b/core/api/system-current.txt index 05f31a5a8a86a..019ba25e1c46f 100644 --- a/core/api/system-current.txt +++ b/core/api/system-current.txt @@ -254,7 +254,7 @@ package android { field public static final String PERFORM_SIM_ACTIVATION = "android.permission.PERFORM_SIM_ACTIVATION"; field public static final String POWER_SAVER = "android.permission.POWER_SAVER"; field public static final String PROVIDE_DEFAULT_ENABLED_CREDENTIAL_SERVICE = "android.permission.PROVIDE_DEFAULT_ENABLED_CREDENTIAL_SERVICE"; - field public static final String PROVIDE_HYBRID_CREDENTIAL_SERVICE = "android.permission.PROVIDE_HYBRID_CREDENTIAL_SERVICE"; + field public static final String PROVIDE_REMOTE_CREDENTIALS = "android.permission.PROVIDE_REMOTE_CREDENTIALS"; field public static final String PROVIDE_RESOLVER_RANKER_SERVICE = "android.permission.PROVIDE_RESOLVER_RANKER_SERVICE"; field public static final String PROVIDE_TRUST_AGENT = "android.permission.PROVIDE_TRUST_AGENT"; field public static final String PROVISION_DEMO_DEVICE = "android.permission.PROVISION_DEMO_DEVICE"; diff --git a/core/java/android/service/credentials/BeginCreateCredentialResponse.java b/core/java/android/service/credentials/BeginCreateCredentialResponse.java index eebd31c6fa968..cd53cb6afc71f 100644 --- a/core/java/android/service/credentials/BeginCreateCredentialResponse.java +++ b/core/java/android/service/credentials/BeginCreateCredentialResponse.java @@ -16,8 +16,10 @@ package android.service.credentials; +import android.Manifest; import android.annotation.NonNull; import android.annotation.Nullable; +import android.annotation.RequiresPermission; import android.os.Parcel; import android.os.Parcelable; @@ -137,7 +139,17 @@ public final class BeginCreateCredentialResponse implements Parcelable { * result should be set to {@link android.app.Activity#RESULT_OK} and an extra with the * {@link CredentialProviderService#EXTRA_CREATE_CREDENTIAL_RESPONSE} key should be populated * with a {@link android.credentials.CreateCredentialResponse} object. + * + *

Note that as a provider service you will only be able to set a remote entry if : + * - Provider service possesses the + * {@link Manifest.permission.PROVIDE_REMOTE_CREDENTIALS} permission. + * - Provider service is configured as the provider that can provide remote entries. + * + * If the above conditions are not met, setting back {@link BeginCreateCredentialResponse} + * on the callback from {@link CredentialProviderService#onBeginCreateCredential} + * will throw a {@link SecurityException}. */ + @RequiresPermission(Manifest.permission.PROVIDE_REMOTE_CREDENTIALS) public @NonNull Builder setRemoteCreateEntry(@Nullable RemoteEntry remoteCreateEntry) { mRemoteCreateEntry = remoteCreateEntry; return this; diff --git a/core/java/android/service/credentials/BeginGetCredentialResponse.java b/core/java/android/service/credentials/BeginGetCredentialResponse.java index 97f5079393a0d..e25b6869605dd 100644 --- a/core/java/android/service/credentials/BeginGetCredentialResponse.java +++ b/core/java/android/service/credentials/BeginGetCredentialResponse.java @@ -16,8 +16,10 @@ package android.service.credentials; +import android.Manifest; import android.annotation.NonNull; import android.annotation.Nullable; +import android.annotation.RequiresPermission; import android.os.Parcel; import android.os.Parcelable; @@ -154,7 +156,17 @@ public final class BeginGetCredentialResponse implements Parcelable { * result should be set to {@link android.app.Activity#RESULT_OK} and an extra with the * {@link CredentialProviderService#EXTRA_GET_CREDENTIAL_RESPONSE} key should be populated * with a {@link android.credentials.Credential} object. + * + *

Note that as a provider service you will only be able to set a remote entry if : + * - Provider service possesses the + * {@link Manifest.permission.PROVIDE_REMOTE_CREDENTIALS} permission. + * - Provider service is configured as the provider that can provide remote entries. + * + * If the above conditions are not met, setting back {@link BeginGetCredentialResponse} + * on the callback from {@link CredentialProviderService#onBeginGetCredential} will + * throw a {@link SecurityException}. */ + @RequiresPermission(Manifest.permission.PROVIDE_REMOTE_CREDENTIALS) public @NonNull Builder setRemoteCredentialEntry(@Nullable RemoteEntry remoteCredentialEntry) { mRemoteCredentialEntry = remoteCredentialEntry; diff --git a/core/java/android/service/credentials/CredentialProviderService.java b/core/java/android/service/credentials/CredentialProviderService.java index d737f6b6cdc8a..e88474d867988 100644 --- a/core/java/android/service/credentials/CredentialProviderService.java +++ b/core/java/android/service/credentials/CredentialProviderService.java @@ -18,6 +18,7 @@ package android.service.credentials; import static com.android.internal.util.function.pooled.PooledLambda.obtainMessage; +import android.Manifest; import android.annotation.CallSuper; import android.annotation.NonNull; import android.annotation.SdkConstant; @@ -218,6 +219,11 @@ public abstract class CredentialProviderService extends Service { GetCredentialException>() { @Override public void onResult(BeginGetCredentialResponse result) { + // If provider service does not possess the HYBRID permission, this + // check will throw an exception in the provider process. + if (result.getRemoteCredentialEntry() != null) { + enforceRemoteEntryPermission(); + } try { callback.onSuccess(result); } catch (RemoteException e) { @@ -236,6 +242,15 @@ public abstract class CredentialProviderService extends Service { )); return transport; } + private void enforceRemoteEntryPermission() { + String permission = + Manifest.permission.PROVIDE_REMOTE_CREDENTIALS; + getApplicationContext().enforceCallingOrSelfPermission( + permission, + String.format("Provider must have %s, in order to set a " + + "remote entry", permission) + ); + } @Override public ICancellationSignal onBeginCreateCredential(BeginCreateCredentialRequest request, @@ -253,6 +268,11 @@ public abstract class CredentialProviderService extends Service { BeginCreateCredentialResponse, CreateCredentialException>() { @Override public void onResult(BeginCreateCredentialResponse result) { + // If provider service does not possess the HYBRID permission, this + // check will throw an exception in the provider process. + if (result.getRemoteCreateEntry() != null) { + enforceRemoteEntryPermission(); + } try { callback.onSuccess(result); } catch (RemoteException e) { diff --git a/core/res/AndroidManifest.xml b/core/res/AndroidManifest.xml index b73623031dc1a..5a0ac0f30cb21 100644 --- a/core/res/AndroidManifest.xml +++ b/core/res/AndroidManifest.xml @@ -4482,8 +4482,8 @@ - + diff --git a/services/credentials/java/com/android/server/credentials/ProviderCreateSession.java b/services/credentials/java/com/android/server/credentials/ProviderCreateSession.java index 97b78116bbfac..370d21fe219e4 100644 --- a/services/credentials/java/com/android/server/credentials/ProviderCreateSession.java +++ b/services/credentials/java/com/android/server/credentials/ProviderCreateSession.java @@ -137,7 +137,8 @@ public final class ProviderCreateSession extends ProviderSession< remoteCredentialService); mCompleteRequest = completeCreateRequest; setStatus(Status.PENDING); - mProviderResponseDataHandler = new ProviderResponseDataHandler(hybridService); + mProviderResponseDataHandler = new ProviderResponseDataHandler( + ComponentName.unflattenFromString(hybridService)); } @Override @@ -297,21 +298,23 @@ public final class ProviderCreateSession extends ProviderSession< } private class ProviderResponseDataHandler { - private final ComponentName mExpectedRemoteEntryProviderService; + @Nullable private final ComponentName mExpectedRemoteEntryProviderService; @NonNull private final Map> mUiCreateEntries = new HashMap<>(); @Nullable private Pair> mUiRemoteEntry = null; - ProviderResponseDataHandler(String hybridService) { - mExpectedRemoteEntryProviderService = ComponentName.unflattenFromString(hybridService); + ProviderResponseDataHandler(@Nullable ComponentName expectedRemoteEntryProviderService) { + mExpectedRemoteEntryProviderService = expectedRemoteEntryProviderService; } public void addResponseContent(List createEntries, RemoteEntry remoteEntry) { createEntries.forEach(this::addCreateEntry); - setRemoteEntry(remoteEntry); + if (remoteEntry != null) { + setRemoteEntry(remoteEntry); + } } public void addCreateEntry(CreateEntry createEntry) { String id = generateUniqueId(); @@ -321,13 +324,13 @@ public final class ProviderCreateSession extends ProviderSession< } public void setRemoteEntry(@Nullable RemoteEntry remoteEntry) { - if (remoteEntry == null) { - mUiRemoteEntry = null; + if (!enforceRemoteEntryRestrictions(mExpectedRemoteEntryProviderService)) { + Log.i(TAG, "Remote entry being dropped as it does not meet the restriction" + + "checks."); return; } - if (!mComponentName.equals(mExpectedRemoteEntryProviderService)) { - Log.i(TAG, "Remote entry being dropped as it is not from the service " - + "configured by the OEM."); + if (remoteEntry == null) { + mUiRemoteEntry = null; return; } String id = generateUniqueId(); diff --git a/services/credentials/java/com/android/server/credentials/ProviderGetSession.java b/services/credentials/java/com/android/server/credentials/ProviderGetSession.java index ee813e90ddfed..80a61b917acc0 100644 --- a/services/credentials/java/com/android/server/credentials/ProviderGetSession.java +++ b/services/credentials/java/com/android/server/credentials/ProviderGetSession.java @@ -181,7 +181,6 @@ public final class ProviderGetSession extends ProviderSession> mUiCredentialEntries = new HashMap<>(); @@ -475,19 +476,27 @@ public final class ProviderGetSession extends ProviderSession> mUiRemoteEntry = null; - ProviderResponseDataHandler(ComponentName expectedRemoteEntryProviderService) { + ProviderResponseDataHandler(@Nullable ComponentName expectedRemoteEntryProviderService) { mExpectedRemoteEntryProviderService = expectedRemoteEntryProviderService; } public void addResponseContent(List credentialEntries, List actions, List authenticationActions, - RemoteEntry remoteEntry) { + RemoteEntry remoteEntry, boolean isInitialResponse) { credentialEntries.forEach(this::addCredentialEntry); actions.forEach(this::addAction); authenticationActions.forEach( authenticationAction -> addAuthenticationAction(authenticationAction, AuthenticationEntry.STATUS_LOCKED)); - setRemoteEntry(remoteEntry); + // In the query phase, it is likely most providers will return a null remote entry + // so no need to invoke the setter since it adds the overhead of checking for the + // hybrid permission, and then sets an already null value to null. + // If this is not the query phase, e.g. response after a locked entry is unlocked + // then it is valid for the provider to remove the remote entry, and so we allow + // them to set it to null. + if (remoteEntry != null || !isInitialResponse) { + setRemoteEntry(remoteEntry); + } } public void addCredentialEntry(CredentialEntry credentialEntry) { String id = generateUniqueId(); @@ -524,12 +533,13 @@ public final class ProviderGetSession extends ProviderSession(generateUniqueId(), new Pair<>(remoteEntry, entry)); } + + public GetCredentialProviderData toGetCredentialProviderData() { return new GetCredentialProviderData.Builder( mComponentName.flattenToString()).setActionChips(prepareActionEntries()) @@ -571,7 +583,6 @@ public final class ProviderGetSession extends ProviderSession return mProviderResponse; } + protected boolean enforceRemoteEntryRestrictions( + @Nullable ComponentName expectedRemoteEntryProviderService) { + // Check if the service is the one set by the OEM. If not silently reject this entry + if (!mComponentName.equals(expectedRemoteEntryProviderService)) { + Log.i(TAG, "Remote entry being dropped as it is not from the service " + + "configured by the OEM."); + return false; + } + // Check if the service has the hybrid permission .If not, silently reject this entry. + // This check is in addition to the permission check happening in the provider's process. + try { + ApplicationInfo appInfo = mContext.getPackageManager().getApplicationInfo( + mComponentName.getPackageName(), + PackageManager.ApplicationInfoFlags.of(PackageManager.MATCH_SYSTEM_ONLY)); + if (appInfo != null + && mContext.checkPermission( + Manifest.permission.PROVIDE_REMOTE_CREDENTIALS, + /*pId=*/-1, appInfo.uid) == PackageManager.PERMISSION_GRANTED) { + return true; + } + } catch (SecurityException e) { + Log.i(TAG, "Error getting info for " + + mComponentName.flattenToString() + ": " + e.getMessage()); + return false; + } catch (PackageManager.NameNotFoundException e) { + Log.i(TAG, "Error getting info for " + + mComponentName.flattenToString() + ": " + e.getMessage()); + return false; + } + Log.i(TAG, "In enforceRemoteEntryRestrictions - remote entry checks fail"); + return false; + } + /** Should be overridden to prepare, and stores state for {@link ProviderData} to be * shown on the UI. */ @Nullable protected abstract ProviderData prepareUiData(); diff --git a/services/credentials/java/com/android/server/credentials/RemoteCredentialService.java b/services/credentials/java/com/android/server/credentials/RemoteCredentialService.java index 702261ea43f53..ff4e3b6801318 100644 --- a/services/credentials/java/com/android/server/credentials/RemoteCredentialService.java +++ b/services/credentials/java/com/android/server/credentials/RemoteCredentialService.java @@ -121,8 +121,6 @@ public class RemoteCredentialService extends ServiceConnector.Impl callback) { Log.i(TAG, "In onGetCredentials in RemoteCredentialService"); AtomicReference cancellationSink = new AtomicReference<>(); - AtomicReference> futureRef = - new AtomicReference<>(); CompletableFuture connectThenExecute = postAsync(service -> { CompletableFuture getCredentials = @@ -134,7 +132,6 @@ public class RemoteCredentialService extends ServiceConnector.Impl future = futureRef.get(); - if (future != null && future.isCancelled()) { - dispatchCancellationSignal(cancellationSignal); - } else { - cancellationSink.set(cancellationSignal); - } + cancellationSink.set(cancellationSignal); return getCredentials; } finally { Binder.restoreCallingIdentity(originalCallingUidToken); } }).orTimeout(TIMEOUT_REQUEST_MILLIS, TimeUnit.MILLISECONDS); - futureRef.set(connectThenExecute); connectThenExecute.whenComplete((result, error) -> Handler.getMain().post(() -> handleExecutionResponse(result, error, cancellationSink, callback))); - return cancellationSink.get(); } @@ -178,8 +168,6 @@ public class RemoteCredentialService extends ServiceConnector.Impl callback) { Log.i(TAG, "In onCreateCredential in RemoteCredentialService"); AtomicReference cancellationSink = new AtomicReference<>(); - AtomicReference> futureRef = - new AtomicReference<>(); CompletableFuture connectThenExecute = postAsync(service -> { @@ -205,19 +193,13 @@ public class RemoteCredentialService extends ServiceConnector.Impl future = futureRef.get(); - if (future != null && future.isCancelled()) { - dispatchCancellationSignal(cancellationSignal); - } else { - cancellationSink.set(cancellationSignal); - } + cancellationSink.set(cancellationSignal); return createCredentialFuture; } finally { Binder.restoreCallingIdentity(originalCallingUidToken); } }).orTimeout(TIMEOUT_REQUEST_MILLIS, TimeUnit.MILLISECONDS); - futureRef.set(connectThenExecute); connectThenExecute.whenComplete((result, error) -> Handler.getMain().post(() -> handleExecutionResponse(result, error, cancellationSink, callback))); @@ -236,7 +218,6 @@ public class RemoteCredentialService extends ServiceConnector.Impl callback) { Log.i(TAG, "In onClearCredentialState in RemoteCredentialService"); AtomicReference cancellationSink = new AtomicReference<>(); - AtomicReference> futureRef = new AtomicReference<>(); CompletableFuture connectThenExecute = postAsync(service -> { @@ -263,19 +244,13 @@ public class RemoteCredentialService extends ServiceConnector.Impl future = futureRef.get(); - if (future != null && future.isCancelled()) { - dispatchCancellationSignal(cancellationSignal); - } else { - cancellationSink.set(cancellationSignal); - } + cancellationSink.set(cancellationSignal); return clearCredentialFuture; } finally { Binder.restoreCallingIdentity(originalCallingUidToken); } }).orTimeout(TIMEOUT_REQUEST_MILLIS, TimeUnit.MILLISECONDS); - futureRef.set(connectThenExecute); connectThenExecute.whenComplete((result, error) -> Handler.getMain().post(() -> handleExecutionResponse(result, error, cancellationSink, callback)));