Skip to content

Commit

Permalink
[pm] fix potential NPE during permission check
Browse files Browse the repository at this point in the history 
Removing redundant copy of sourcePackageSetting from BasePermission.

BUG: 154337853
Test: cts-tf > run cts -m CtsAppSecurityHostTestCases -t android.appsecurity.cts.AppSecurityTests
Change-Id: I9d235b66d18f8fcbe88afc9aa8aa0dca2d715deb
  • Loading branch information
@schfan-1
schfan-1 committed Apr 23, 2020
1 parent 4f9bf27 commit e5c0afe
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 44 deletions.
2 changes: 0 additions & 2 deletions services/art-profile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32290,7 +32290,6 @@ HPLcom/android/server/pm/permission/BasePermission;->generatePermissionInfo(Ljav
HSPLcom/android/server/pm/permission/BasePermission;->getName()Ljava/lang/String;
HSPLcom/android/server/pm/permission/BasePermission;->getProtectionLevel()I
HSPLcom/android/server/pm/permission/BasePermission;->getSourcePackageName()Ljava/lang/String;
HSPLcom/android/server/pm/permission/BasePermission;->getSourcePackageSetting()Lcom/android/server/pm/PackageSettingBase;
PLcom/android/server/pm/permission/BasePermission;->getUid()I
HSPLcom/android/server/pm/permission/BasePermission;->isAppOp()Z
HSPLcom/android/server/pm/permission/BasePermission;->isAppPredictor()Z
Expand Down Expand Up @@ -32329,7 +32328,6 @@ HSPLcom/android/server/pm/permission/BasePermission;->readLPw(Ljava/util/Map;Lor
HSPLcom/android/server/pm/permission/BasePermission;->setGids([IZ)V
HSPLcom/android/server/pm/permission/BasePermission;->setPermission(Landroid/content/pm/parsing/ComponentParseUtils$ParsedPermission;)V
PLcom/android/server/pm/permission/BasePermission;->setPermission(Landroid/content/pm/parsing/component/ParsedPermission;)V
HSPLcom/android/server/pm/permission/BasePermission;->setSourcePackageSetting(Lcom/android/server/pm/PackageSettingBase;)V
HSPLcom/android/server/pm/permission/BasePermission;->updateDynamicPermission(Ljava/util/Collection;)V
HSPLcom/android/server/pm/permission/BasePermission;->writeLPr(Lorg/xmlpull/v1/XmlSerializer;)V
HSPLcom/android/server/pm/permission/DefaultPermissionGrantPolicy$1;-><init>(Lcom/android/server/pm/permission/DefaultPermissionGrantPolicy;Landroid/os/Looper;)V
Expand Down
1 change: 0 additions & 1 deletion services/art-profile-boot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ Lcom/android/server/pm/PackageUsage;->readToken(Ljava/io/InputStream;Ljava/lang/
Lcom/android/server/pm/PackageUsage;->readVersion1LP(Ljava/util/Map;Ljava/io/InputStream;Ljava/lang/StringBuffer;)V
Lcom/android/server/pm/PackageUsage;->parseAsLong(Ljava/lang/String;)J
Lcom/android/server/pm/CompilerStats;->read(Ljava/io/Reader;)Z
Lcom/android/server/pm/permission/BasePermission;->getSourcePackageSetting()Lcom/android/server/pm/PackageSettingBase;
Lcom/android/server/pm/permission/PermissionManagerService;->updatePermissionSourcePackage(Ljava/lang/String;Landroid/content/pm/PackageParser$Package;)Z
Lcom/android/server/pm/permission/BasePermission;->isDynamic()Z
Lcom/android/server/pm/permission/PermissionManagerService;->cacheBackgroundToForegoundPermissionMapping()V
Expand Down
18 changes: 12 additions & 6 deletions services/core/java/com/android/server/pm/PackageManagerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17122,7 +17122,12 @@ private PrepareResult preparePackageLI(InstallArgs args, PackageInstalledInfo re
// "updating same package" could also involve key-rotation.
final boolean sigsOk;
final String sourcePackageName = bp.getSourcePackageName();
final PackageSettingBase sourcePackageSetting = bp.getSourcePackageSetting();
final PackageSetting sourcePackageSetting;
synchronized (mLock) {
sourcePackageSetting = mSettings.getPackageLPr(sourcePackageName);
}
final SigningDetails sourceSigningDetails = (sourcePackageSetting == null
? SigningDetails.UNKNOWN : sourcePackageSetting.getSigningDetails());
final KeySetManagerService ksms = mSettings.mKeySetManagerService;
if (sourcePackageName.equals(parsedPackage.getPackageName())
&& (ksms.shouldCheckUpgradeKeySetLocked(
Expand All @@ -17133,18 +17138,19 @@ private PrepareResult preparePackageLI(InstallArgs args, PackageInstalledInfo re
// in the event of signing certificate rotation, we need to see if the
// package's certificate has rotated from the current one, or if it is an
// older certificate with which the current is ok with sharing permissions
if (sourcePackageSetting.signatures.mSigningDetails.checkCapability(
if (sourceSigningDetails.checkCapability(
parsedPackage.getSigningDetails(),
PackageParser.SigningDetails.CertCapabilities.PERMISSION)) {
sigsOk = true;
} else if (parsedPackage.getSigningDetails().checkCapability(
sourcePackageSetting.signatures.mSigningDetails,
sourceSigningDetails,
PackageParser.SigningDetails.CertCapabilities.PERMISSION)) {

// the scanned package checks out, has signing certificate rotation
// history, and is newer; bring it over
sourcePackageSetting.signatures.mSigningDetails =
parsedPackage.getSigningDetails();
synchronized (mLock) {
sourcePackageSetting.signatures.mSigningDetails =
parsedPackage.getSigningDetails();
}
sigsOk = true;
} else {
sigsOk = false;
Expand Down
22 changes: 1 addition & 21 deletions services/core/java/com/android/server/pm/permission/BasePermission.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import android.annotation.Nullable;
import android.content.pm.PackageManagerInternal;
import android.content.pm.PermissionInfo;
import android.content.pm.Signature;
import android.content.pm.parsing.component.ParsedPermission;
import android.os.UserHandle;
import android.util.Log;
Expand Down Expand Up @@ -86,9 +85,6 @@ public final class BasePermission {

String sourcePackageName;

// TODO: Can we get rid of this? Seems we only use some signature info from the setting
PackageSettingBase sourcePackageSetting;

int protectionLevel;

ParsedPermission perm;
Expand Down Expand Up @@ -130,12 +126,6 @@ public int getProtectionLevel() {
public String getSourcePackageName() {
return sourcePackageName;
}
public PackageSettingBase getSourcePackageSetting() {
return sourcePackageSetting;
}
public Signature[] getSourceSignatures() {
return sourcePackageSetting.getSignatures();
}
public int getType() {
return type;
}
Expand All @@ -149,9 +139,6 @@ public void setGids(int[] gids, boolean perUser) {
public void setPermission(@Nullable ParsedPermission perm) {
this.perm = perm;
}
public void setSourcePackageSetting(PackageSettingBase sourcePackageSetting) {
this.sourcePackageSetting = sourcePackageSetting;
}

public int[] computeGids(int userId) {
if (perUser) {
Expand Down Expand Up @@ -290,7 +277,6 @@ public void transfer(@NonNull String origPackageName, @NonNull String newPackage
return;
}
sourcePackageName = newPackageName;
sourcePackageSetting = null;
perm = null;
if (pendingPermissionInfo != null) {
pendingPermissionInfo.packageName = newPackageName;
Expand Down Expand Up @@ -319,10 +305,9 @@ public void updateDynamicPermission(Collection<BasePermission> permissionTrees)
if (PackageManagerService.DEBUG_SETTINGS) Log.v(TAG, "Dynamic permission: name="
+ getName() + " pkg=" + getSourcePackageName()
+ " info=" + pendingPermissionInfo);
if (sourcePackageSetting == null && pendingPermissionInfo != null) {
if (pendingPermissionInfo != null) {
final BasePermission tree = findPermissionTree(permissionTrees, name);
if (tree != null && tree.perm != null) {
sourcePackageSetting = tree.sourcePackageSetting;
perm = new ParsedPermission(tree.perm, pendingPermissionInfo,
tree.perm.getPackageName(), name);
uid = tree.uid;
Expand Down Expand Up @@ -355,7 +340,6 @@ static BasePermission createOrUpdate(PackageManagerInternal packageManagerIntern
if (bp.type == BasePermission.TYPE_BUILTIN && bp.perm == null) {
// It's a built-in permission and no owner, take ownership now
p.setFlags(p.getFlags() | PermissionInfo.FLAG_INSTALLED);
bp.sourcePackageSetting = pkgSetting;
bp.perm = p;
bp.uid = pkg.getUid();
bp.sourcePackageName = p.getPackageName();
Expand All @@ -378,7 +362,6 @@ static BasePermission createOrUpdate(PackageManagerInternal packageManagerIntern
if (tree == null
|| tree.sourcePackageName.equals(p.getPackageName())) {
p.setFlags(p.getFlags() | PermissionInfo.FLAG_INSTALLED);
bp.sourcePackageSetting = pkgSetting;
bp.perm = p;
bp.uid = pkg.getUid();
bp.sourcePackageName = p.getPackageName();
Expand Down Expand Up @@ -639,9 +622,6 @@ public boolean dumpPermissionsLPr(@NonNull PrintWriter pw, @NonNull String packa
pw.print(" flags=0x"); pw.println(Integer.toHexString(perm.getFlags()));
}
}
if (sourcePackageSetting != null) {
pw.print(" packageSetting="); pw.println(sourcePackageSetting);
}
if (READ_EXTERNAL_STORAGE.equals(name)) {
pw.print(" enforced=");
pw.println(readEnforced);
Expand Down
43 changes: 29 additions & 14 deletions services/core/java/com/android/server/pm/permission/PermissionManagerService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2526,7 +2526,7 @@ private void restorePermissionState(@NonNull AndroidPackage pkg, boolean replace
+ " checking " + permName + ": " + bp);
}

if (bp == null || bp.getSourcePackageSetting() == null) {
if (bp == null || getSourcePackageSetting(bp) == null) {
if (packageOfInterest == null || packageOfInterest.equals(
pkg.getPackageName())) {
if (DEBUG_PERMISSIONS) {
Expand Down Expand Up @@ -3403,10 +3403,11 @@ private boolean grantSignaturePermission(String perm, AndroidPackage pkg,
// - or its signing certificate is a previous signing certificate of the defining
// package, and the defining package still trusts the old certificate for permissions
// - or it shares the above relationships with the system package
final PackageParser.SigningDetails sourceSigningDetails =
getSourcePackageSigningDetails(bp);
boolean allowed =
pkg.getSigningDetails().hasAncestorOrSelf(
bp.getSourcePackageSetting().getSigningDetails())
|| bp.getSourcePackageSetting().getSigningDetails().checkCapability(
pkg.getSigningDetails().hasAncestorOrSelf(sourceSigningDetails)
|| sourceSigningDetails.checkCapability(
pkg.getSigningDetails(),
PackageParser.SigningDetails.CertCapabilities.PERMISSION)
|| pkg.getSigningDetails().hasAncestorOrSelf(systemPackage.getSigningDetails())
Expand Down Expand Up @@ -3576,6 +3577,22 @@ && canGrantOemPermission(disabledPs, perm)))) {
return allowed;
}

@NonNull
private PackageParser.SigningDetails getSourcePackageSigningDetails(
@NonNull BasePermission bp) {
final PackageSetting ps = getSourcePackageSetting(bp);
if (ps == null) {
return PackageParser.SigningDetails.UNKNOWN;
}
return ps.getSigningDetails();
}

@Nullable
private PackageSetting getSourcePackageSetting(@NonNull BasePermission bp) {
final String sourcePackageName = bp.getSourcePackageName();
return mPackageManagerInt.getPackageSetting(sourcePackageName);
}

private static boolean isProfileOwner(int uid) {
DevicePolicyManagerInternal dpmInternal =
LocalServices.getService(DevicePolicyManagerInternal.class);
Expand Down Expand Up @@ -4084,8 +4101,10 @@ private boolean updatePermissionSourcePackage(@Nullable String packageName,
if (bp.isDynamic()) {
bp.updateDynamicPermission(mSettings.mPermissionTrees.values());
}
if (bp.getSourcePackageSetting() == null
|| !packageName.equals(bp.getSourcePackageName())) {
if (!packageName.equals(bp.getSourcePackageName())) {
// Not checking sourcePackageSetting because it can be null when
// the permission source package is the target package and the target package is
// being uninstalled,
continue;
}
// The target package is the source of the current permission
Expand Down Expand Up @@ -4123,9 +4142,6 @@ private boolean updatePermissionSourcePackage(@Nullable String packageName,
bp.getSourcePackageName());
synchronized (mLock) {
if (sourcePkg != null && sourcePs != null) {
if (bp.getSourcePackageSetting() == null) {
bp.setSourcePackageSetting(sourcePs);
}
continue;
}
Slog.w(TAG, "Removing dangling permission: " + bp.getName()
Expand Down Expand Up @@ -4200,8 +4216,10 @@ private boolean updatePermissionTreeSourcePackage(@Nullable String packageName,
final Iterator<BasePermission> it = mSettings.mPermissionTrees.values().iterator();
while (it.hasNext()) {
final BasePermission bp = it.next();
if (bp.getSourcePackageSetting() == null
|| !packageName.equals(bp.getSourcePackageName())) {
if (!packageName.equals(bp.getSourcePackageName())) {
// Not checking sourcePackageSetting because it can be null when
// the permission source package is the target package and the target package is
// being uninstalled,
continue;
}
// The target package is the source of the current permission tree
Expand All @@ -4227,9 +4245,6 @@ private boolean updatePermissionTreeSourcePackage(@Nullable String packageName,
bp.getSourcePackageName());
synchronized (mLock) {
if (sourcePkg != null && sourcePs != null) {
if (bp.getSourcePackageSetting() == null) {
bp.setSourcePackageSetting(sourcePs);
}
continue;
}
Slog.w(TAG, "Removing dangling permission tree: " + bp.getName()
Expand Down

0 comments on commit e5c0afe

Please sign in to comment.