Checks package name belongs to called UID on some content capture methods.

Also refactored how the SecurityException is thrown back to the caller app.

Bug: 122959591
Test: manual verification
Test: atest CtsContentCaptureServiceTestCases # sanity check (minus usual flakiness)

Change-Id: I4d2a68e61dc1c801d80734a30f4bbe6fdae8555d

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;