From a8eb60f82c9503214522b31679de3006c66ca577 Mon Sep 17 00:00:00 2001
From: Atneya Nair <atneya@google.com>
Date: Tue, 5 Nov 2024 14:55:59 -0800
Subject: Reapply "appops: Finish started proxy op when chain fails"

This reverts commit f13df1570453a4dc5748b0372b5e807095f04f89.

The original commit had an issue where we attempted to clean up ops
for preflight checks. Since in this case, we use the raw check op,
we get MODE_FOREGROUND, which was incorrectly treated as a soft denial,
instead of an accept. This impacted cases where we compared the
preflight check to GRANTED directly (uncommon, hotword specific).
It also means we would finish ops unexpectedly on preflight.

Original commit message:

A more precise version of I92060d44e666fa6725411de5d714ac0d380f42ae

This fixes an issue where we finish the op which failed permission
checks... which causes refcount mismatches again.

Instead, ensure that we finish only the proxy ops which were
*successfully* started: acheiving this by pushing the cleanup into the
checkPerm loop which iterates through the attr chain.

Technically this should also be added for appop permissions, but focus
on runtime appops for now, since that is where the security issue is.

Test: CtsMediaAudioPermissionTestCases
Test: Manual assistant/now playing
Bug: 377407253
Bug: 293603271
Flag: EXEMPT security
Change-Id: I16f82c9438083f8f64f84ba710f97539960009f1
---
 .../pm/permission/PermissionManagerService.java    | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
index 5fc3e332b95c..05bc69a9f1f0 100644
--- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
+++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java
@@ -1015,7 +1015,8 @@ public class PermissionManagerService extends IPermissionManager.Stub {
                     permission, attributionSource, message, forDataDelivery, startDataDelivery,
                     fromDatasource, attributedOp);
             // Finish any started op if some step in the attribution chain failed.
-            if (startDataDelivery && result != PermissionChecker.PERMISSION_GRANTED) {
+            if (startDataDelivery && result != PermissionChecker.PERMISSION_GRANTED
+                    && result != PermissionChecker.PERMISSION_SOFT_DENIED) {
                 if (attributedOp == AppOpsManager.OP_NONE) {
                     finishDataDelivery(AppOpsManager.permissionToOpCode(permission),
                             attributionSource.asState(), fromDatasource);
@@ -1244,6 +1245,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
             final boolean hasChain = attributionChainId != ATTRIBUTION_CHAIN_ID_NONE;
             AttributionSource current = attributionSource;
             AttributionSource next = null;
+            AttributionSource prev = null;
             // We consider the chain trusted if the start node has UPDATE_APP_OPS_STATS, and
             // every attributionSource in the chain is registered with the system.
             final boolean isChainStartTrusted = !hasChain || checkPermission(context,
@@ -1310,6 +1312,22 @@ public class PermissionManagerService extends IPermissionManager.Stub {
                         selfAccess, singleReceiverFromDatasource, attributedOp,
                         proxyAttributionFlags, proxiedAttributionFlags, attributionChainId);
 
+                if (startDataDelivery && opMode != AppOpsManager.MODE_ALLOWED) {
+                    // Current failed the perm check, so if we are part-way through an attr chain,
+                    // we need to clean up the already started proxy op higher up the chain.  Note,
+                    // proxy ops are verified two by two, which means we have to clear the 2nd next
+                    // from the previous iteration (since it is actually curr.next which failed
+                    // to pass the perm check).
+                    if (prev != null) {
+                        final var cutAttrSourceState = prev.asState();
+                        if (cutAttrSourceState.next.length > 0) {
+                            cutAttrSourceState.next[0].next = new AttributionSourceState[0];
+                        }
+                        finishDataDelivery(context, attributedOp,
+                                cutAttrSourceState, fromDatasource);
+                    }
+                }
+
                 switch (opMode) {
                     case AppOpsManager.MODE_ERRORED: {
                         if (permission.equals(Manifest.permission.BLUETOOTH_CONNECT)) {
@@ -1335,6 +1353,8 @@ public class PermissionManagerService extends IPermissionManager.Stub {
                     return PermissionChecker.PERMISSION_GRANTED;
                 }
 
+                // an attribution we have already possibly started an op for
+                prev = current;
                 current = next;
             }
         }
-- 
cgit v1.2.3-59-g8ed1b