Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f82f7a77 authored by Hai Zhang's avatar Hai Zhang
Browse files

Fix updated privileged app with shared UID loses privileged permission

when request in manifest.

If two privileged apps share the same UID, one of them requested and
got a privileged permission granted while another didn't, both will be
granted the privileged permission. However when an update is installed
for the factory version of the system app that didn't request the
permission, and that updated app starts to request the privileged
permission, that request itself can't grant the permission because we
don't allow updated system apps to get new privileged permissions and
we only trust the factory version. However, the original permission
request from the other factory app is still valid. As a result of
this, in the refactored logic in S the permission has a chance to stop
being granted to both apps depending on the order of the two apps
being examined for permission grants.

The proper fix to this issue is to reconcile permission not by
packages, but by UIDs. However, given where we are in the S timeline,
such change will require a lot of refactoring to this critical piece
of logic, and may lead to performance regression even if it's okay
security-wise. So we should defer the proper refactoring to the next
release, either in this existing implementation, or in a new
permission subsystem.

The pre-S implementation had the logic that if a privileged permission
was granted to the factory version of a privileged app, it will
unconditionally keep the permission granted. And in the case of shared
UID, where the permission state of the factory app is the shared user
permission state, this helped resolving this issue. That piece of
logic was deemed unnecessary and simplified during S refactoring, but
this specific case shows that we actually still need it.

So we should bring back that logic in S. However due to the split of
permission and package locks, we can not call into package when we are
holding the permission lock. This is exactly the case in this piece of
logic because it needs both the disabled system package (guarded by
package lock), as well as the existing permission state (guarded by
permission lock). So we have to split the logic into two parts, that
is to store the information from package first, and then after
grabbing the permission lock, get the existing permissions state and
combine the two to reach a conclusion.

Another issue that existed before S was that the logic above was
broken by privapp permission allowlisting in O, which returns directly
without giving the logic handling updated system apps a chance to
run. So we need to fix this as well.

Test: Manual
Test: Install an updated system app that requests a privileged
      permission not in the allowlist but granted due to another
      shared UID app, and make sure that dumpsys package shows that
      the updated app has the permission both requested and granted
Fixes: 177926928
Change-Id: I667a9c2e1b87a15c22046e839b248ec6d4e55ca0
parent ee57f57f
Loading
Loading
Loading
Loading
+41 −18
Original line number Diff line number Diff line
@@ -2593,6 +2593,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
        ArraySet<String> isPrivilegedPermissionAllowlisted = null;
        ArraySet<String> shouldGrantSignaturePermission = null;
        ArraySet<String> shouldGrantInternalPermission = null;
        ArraySet<String> shouldGrantPrivilegedPermissionIfWasGranted = new ArraySet<>();
        final List<String> requestedPermissions = pkg.getRequestedPermissions();
        final int requestedPermissionsSize = requestedPermissions.size();
        for (int i = 0; i < requestedPermissionsSize; i++) {
@@ -2613,14 +2614,16 @@ public class PermissionManagerService extends IPermissionManager.Stub {
                isPrivilegedPermissionAllowlisted.add(permissionName);
            }
            if (permission.isSignature() && (shouldGrantPermissionBySignature(pkg, permission)
                    || shouldGrantPermissionByProtectionFlags(pkg, ps, permission))) {
                    || shouldGrantPermissionByProtectionFlags(pkg, ps, permission,
                            shouldGrantPrivilegedPermissionIfWasGranted))) {
                if (shouldGrantSignaturePermission == null) {
                    shouldGrantSignaturePermission = new ArraySet<>();
                }
                shouldGrantSignaturePermission.add(permissionName);
            }
            if (permission.isInternal()
                    && shouldGrantPermissionByProtectionFlags(pkg, ps, permission)) {
                    && shouldGrantPermissionByProtectionFlags(pkg, ps, permission,
                            shouldGrantPrivilegedPermissionIfWasGranted)) {
                if (shouldGrantInternalPermission == null) {
                    shouldGrantInternalPermission = new ArraySet<>();
                }
@@ -2842,14 +2845,18 @@ public class PermissionManagerService extends IPermissionManager.Stub {
                                            isPrivilegedPermissionAllowlisted, permName))
                                    && (CollectionUtils.contains(shouldGrantSignaturePermission,
                                            permName)
                                            || ((bp.isDevelopment() || bp.isRole())
                                            || (((bp.isPrivileged() && CollectionUtils.contains(
                                                    shouldGrantPrivilegedPermissionIfWasGranted,
                                                    permName)) || bp.isDevelopment() || bp.isRole())
                                                    && origState.isPermissionGranted(permName))))
                            || (bp.isInternal()
                                    && (!bp.isPrivileged() || CollectionUtils.contains(
                                            isPrivilegedPermissionAllowlisted, permName))
                                    && (CollectionUtils.contains(shouldGrantInternalPermission,
                                            permName)
                                            || ((bp.isDevelopment() || bp.isRole())
                                            || (((bp.isPrivileged() && CollectionUtils.contains(
                                                    shouldGrantPrivilegedPermissionIfWasGranted,
                                                    permName)) || bp.isDevelopment() || bp.isRole())
                                                    && origState.isPermissionGranted(permName))))) {
                        // Grant an install permission.
                        if (uidState.grantPermission(bp)) {
@@ -3374,10 +3381,23 @@ public class PermissionManagerService extends IPermissionManager.Stub {
        if (isInSystemConfigPrivAppPermissions(pkg, permissionName)) {
            return true;
        }
        // Only enforce the allowlist on boot
        if (!mSystemReady
        if (isInSystemConfigPrivAppDenyPermissions(pkg, permissionName)) {
            return false;
        }
        // Updated system apps do not need to be allowlisted
                && !packageSetting.getPkgState().isUpdatedSystemApp()) {
        if (packageSetting.getPkgState().isUpdatedSystemApp()) {
            // Let shouldGrantPermissionByProtectionFlags() decide whether the privileged permission
            // can be granted, because an updated system app may be in a shared UID, and in case a
            // new privileged permission is requested by the updated system app but not the factory
            // app, although this app and permission combination isn't in the allowlist and can't
            // get the permission this way, other apps in the shared UID may still get it. A proper
            // fix for this would be to perform the reconciliation by UID, but for now let's keep
            // the old workaround working, which is to keep granted privileged permissions still
            // granted.
            return true;
        }
        // Only enforce the allowlist on boot
        if (!mSystemReady) {
            final ApexManager apexManager = ApexManager.getInstance();
            final String containingApexPackageName =
                    apexManager.getActiveApexPackageNameContainingPackage(packageName);
@@ -3386,11 +3406,6 @@ public class PermissionManagerService extends IPermissionManager.Stub {
                    MATCH_ACTIVE_PACKAGE));
            // Apps that are in updated apexs' do not need to be allowlisted
            if (!isInUpdatedApex) {
                // it's only a reportable violation if the permission isn't explicitly
                // denied
                if (isInSystemConfigPrivAppDenyPermissions(pkg, permissionName)) {
                    return false;
                }
                Slog.w(TAG, "Privileged permission " + permissionName + " for package "
                        + packageName + " (" + pkg.getPath()
                        + ") not in privapp-permissions allowlist");
@@ -3468,7 +3483,8 @@ public class PermissionManagerService extends IPermissionManager.Stub {
    }

    private boolean shouldGrantPermissionByProtectionFlags(@NonNull AndroidPackage pkg,
            @NonNull PackageSetting pkgSetting, @NonNull Permission bp) {
            @NonNull PackageSetting pkgSetting, @NonNull Permission bp,
            @NonNull ArraySet<String> shouldGrantPrivilegedPermissionIfWasGranted) {
        boolean allowed = false;
        final boolean isPrivilegedPermission = bp.isPrivileged();
        final boolean isOemPermission = bp.isOem();
@@ -3480,11 +3496,18 @@ public class PermissionManagerService extends IPermissionManager.Stub {
                final PackageSetting disabledPs = mPackageManagerInt
                        .getDisabledSystemPackage(pkg.getPackageName());
                final AndroidPackage disabledPkg = disabledPs == null ? null : disabledPs.pkg;
                if (disabledPkg != null && disabledPkg.getRequestedPermissions().contains(
                        permissionName)) {
                    allowed = (isPrivilegedPermission && disabledPkg.isPrivileged())
                if (disabledPkg != null
                        && ((isPrivilegedPermission && disabledPkg.isPrivileged())
                        || (isOemPermission && canGrantOemPermission(disabledPkg,
                            permissionName));
                                permissionName)))) {
                    if (disabledPkg.getRequestedPermissions().contains(permissionName)) {
                        allowed = true;
                    } else {
                        // If the original was granted this permission, we take
                        // that grant decision as read and propagate it to the
                        // update.
                        shouldGrantPrivilegedPermissionIfWasGranted.add(permissionName);
                    }
                }
            } else {
                allowed = (isPrivilegedPermission && pkg.isPrivileged())