diff options
4 files changed, 89 insertions, 49 deletions
diff --git a/core/java/android/view/contentcapture/ContentCaptureManager.java b/core/java/android/view/contentcapture/ContentCaptureManager.java index 35f802303fa1..253935680cb8 100644 --- a/core/java/android/view/contentcapture/ContentCaptureManager.java +++ b/core/java/android/view/contentcapture/ContentCaptureManager.java @@ -525,15 +525,12 @@ public final class ContentCaptureManager { // the service to fine tune how long-lived apps (like browsers) are whitelisted. if (!isContentCaptureEnabled() && !mOptions.lite) return null; - final SyncResultReceiver resultReceiver = new SyncResultReceiver(SYNC_CALLS_TIMEOUT_MS); - try { - mService.getContentCaptureConditions(mContext.getPackageName(), resultReceiver); - final ArrayList<ContentCaptureCondition> result = resultReceiver - .getParcelableListResult(); - return toSet(result); - } catch (RemoteException e) { - throw e.rethrowFromSystemServer(); - } + final SyncResultReceiver resultReceiver = syncRun( + (r) -> mService.getContentCaptureConditions(mContext.getPackageName(), r)); + + final ArrayList<ContentCaptureCondition> result = resultReceiver + .getParcelableListResult(); + return toSet(result); } /** @@ -566,21 +563,14 @@ public final class ContentCaptureManager { @SystemApi @TestApi public boolean isContentCaptureFeatureEnabled() { - final SyncResultReceiver resultReceiver = new SyncResultReceiver(SYNC_CALLS_TIMEOUT_MS); - final int resultCode; - try { - mService.isContentCaptureFeatureEnabled(resultReceiver); - resultCode = resultReceiver.getIntResult(); - } catch (RemoteException e) { - throw e.rethrowFromSystemServer(); - } + final SyncResultReceiver resultReceiver = syncRun( + (r) -> mService.isContentCaptureFeatureEnabled(r)); + final int resultCode = resultReceiver.getIntResult(); switch (resultCode) { case RESULT_CODE_TRUE: return true; case RESULT_CODE_FALSE: return false; - case RESULT_CODE_SECURITY_EXCEPTION: - throw new SecurityException("caller is not user's ContentCapture service"); default: Log.wtf(TAG, "received invalid result: " + resultCode); return false; @@ -603,6 +593,26 @@ public final class ContentCaptureManager { } } + /** + * Runs a sync method in the service, properly handling exceptions. + * + * @throws SecurityException if caller is not allowed to execute the method. + */ + @NonNull + private SyncResultReceiver syncRun(@NonNull MyRunnable r) { + final SyncResultReceiver resultReceiver = new SyncResultReceiver(SYNC_CALLS_TIMEOUT_MS); + try { + r.run(resultReceiver); + final int resultCode = resultReceiver.getIntResult(); + if (resultCode == RESULT_CODE_SECURITY_EXCEPTION) { + throw new SecurityException(resultReceiver.getStringResult()); + } + return resultReceiver; + } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } + } + /** @hide */ public void dump(String prefix, PrintWriter pw) { pw.print(prefix); pw.println("ContentCaptureManager"); @@ -626,4 +636,8 @@ public final class ContentCaptureManager { } } } + + private interface MyRunnable { + void run(@NonNull SyncResultReceiver receiver) throws RemoteException; + } } diff --git a/core/java/com/android/internal/util/SyncResultReceiver.java b/core/java/com/android/internal/util/SyncResultReceiver.java index 6fad84bb6c43..00e91017a9eb 100644 --- a/core/java/com/android/internal/util/SyncResultReceiver.java +++ b/core/java/com/android/internal/util/SyncResultReceiver.java @@ -19,7 +19,6 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.os.Bundle; import android.os.Parcelable; -import android.os.RemoteException; import com.android.internal.os.IResultReceiver; @@ -183,7 +182,7 @@ public final class SyncResultReceiver extends IResultReceiver.Stub { } /** @hide */ - public static final class TimeoutException extends RemoteException { + public static final class TimeoutException extends RuntimeException { private TimeoutException(String msg) { super(msg); } diff --git a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java index 2894662becac..9b02c4e72cfc 100644 --- a/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java +++ b/services/contentcapture/java/com/android/server/contentcapture/ContentCaptureManagerService.java @@ -450,23 +450,16 @@ public final class ContentCaptureManagerService extends } @GuardedBy("mLock") - private boolean assertCalledByServiceLocked(@NonNull String methodName, @UserIdInt int userId, - int callingUid, @NonNull IResultReceiver result) { - final boolean isService = isCalledByServiceLocked(methodName, userId, callingUid); - if (isService) return true; - - try { - result.send(RESULT_CODE_SECURITY_EXCEPTION, /* resultData= */ null); - } catch (RemoteException e) { - Slog.w(mTag, "Unable to send isContentCaptureFeatureEnabled(): " + e); + private void assertCalledByServiceLocked(@NonNull String methodName) { + if (!isCalledByServiceLocked(methodName)) { + throw new SecurityException("caller is not user's ContentCapture service"); } - return false; } @GuardedBy("mLock") - private boolean isCalledByServiceLocked(@NonNull String methodName, @UserIdInt int userId, - int callingUid) { - + private boolean isCalledByServiceLocked(@NonNull String methodName) { + final int userId = UserHandle.getCallingUserId(); + final int callingUid = Binder.getCallingUid(); final String serviceName = mServiceNameResolver.getServiceName(userId); if (serviceName == null) { Slog.e(mTag, methodName + ": called by UID " + callingUid @@ -499,6 +492,27 @@ public final class ContentCaptureManagerService extends return true; } + /** + * Executes the given {@code runnable} and if it throws a {@link SecurityException}, + * send it back to the receiver. + * + * @return whether the exception was thrown or not. + */ + private boolean throwsSecurityException(@NonNull IResultReceiver result, + @NonNull Runnable runable) { + try { + runable.run(); + return false; + } catch (SecurityException e) { + try { + result.send(RESULT_CODE_SECURITY_EXCEPTION, bundleFor(e.getMessage())); + } catch (RemoteException e2) { + Slog.w(mTag, "Unable to send security exception (" + e + "): ", e2); + } + } + return true; + } + @Override // from AbstractMasterSystemService protected void dumpLocked(String prefix, PrintWriter pw) { super.dumpLocked(prefix, pw); @@ -570,7 +584,7 @@ public final class ContentCaptureManagerService extends @Override public void removeUserData(@NonNull UserDataRemovalRequest request) { Preconditions.checkNotNull(request); - // TODO(b/122959591): check caller uid owns the package name + assertCalledByPackageOwner(request.getPackageName()); final int userId = UserHandle.getCallingUserId(); synchronized (mLock) { @@ -581,13 +595,14 @@ public final class ContentCaptureManagerService extends @Override public void isContentCaptureFeatureEnabled(@NonNull IResultReceiver result) { - final int userId = UserHandle.getCallingUserId(); boolean enabled; synchronized (mLock) { - final boolean isService = assertCalledByServiceLocked( - "isContentCaptureFeatureEnabled()", userId, Binder.getCallingUid(), result); - if (!isService) return; + if (throwsSecurityException(result, + () -> assertCalledByServiceLocked("isContentCaptureFeatureEnabled()"))) { + return; + } + final int userId = UserHandle.getCallingUserId(); enabled = !mDisabledByDeviceConfig && !isDisabledBySettingsLocked(userId); } try { @@ -599,15 +614,8 @@ public final class ContentCaptureManagerService extends @Override public void getServiceSettingsActivity(@NonNull IResultReceiver result) { - try { - enforceCallingPermissionForManagement(); - } catch (SecurityException e) { - try { - result.send(RESULT_CODE_SECURITY_EXCEPTION, bundleFor(e.getMessage())); - } catch (RemoteException e2) { - Slog.w(mTag, "Unable to send getServiceSettingsIntent() exception: " + e2); - return; - } + if (throwsSecurityException(result, () -> enforceCallingPermissionForManagement())) { + return; } final int userId = UserHandle.getCallingUserId(); @@ -627,7 +635,9 @@ public final class ContentCaptureManagerService extends @Override public void getContentCaptureConditions(@NonNull String packageName, @NonNull IResultReceiver result) { - // TODO(b/122959591): check caller uid owns the package name + if (throwsSecurityException(result, () -> assertCalledByPackageOwner(packageName))) { + return; + } final int userId = UserHandle.getCallingUserId(); final ArrayList<ContentCaptureCondition> conditions; diff --git a/services/core/java/com/android/server/infra/AbstractMasterSystemService.java b/services/core/java/com/android/server/infra/AbstractMasterSystemService.java index 39e93f5e44fe..098b0e9d4d39 100644 --- a/services/core/java/com/android/server/infra/AbstractMasterSystemService.java +++ b/services/core/java/com/android/server/infra/AbstractMasterSystemService.java @@ -616,6 +616,23 @@ public abstract class AbstractMasterSystemService<M extends AbstractMasterSystem mServicesCache.clear(); } + /** + * Asserts that the given package name is owned by the UID making this call. + * + * @throws SecurityException when it's not... + */ + protected final void assertCalledByPackageOwner(@NonNull String packageName) { + Preconditions.checkNotNull(packageName); + final int uid = Binder.getCallingUid(); + final String[] packages = getContext().getPackageManager().getPackagesForUid(uid); + if (packages != null) { + for (String candidate : packages) { + if (packageName.equals(candidate)) return; // Found it + } + } + throw new SecurityException("UID " + uid + " does not own " + packageName); + } + // TODO(b/117779333): support proto protected void dumpLocked(@NonNull String prefix, @NonNull PrintWriter pw) { boolean realDebug = debug; |