From 675088a491e73b6fa9a91ba1512a21ba932f3017 Mon Sep 17 00:00:00 2001 From: Victor Hsieh Date: Tue, 16 Feb 2021 10:27:21 -0800 Subject: [PATCH] Require .dm to install with .fsv_sig if supported Previously, .fsv_sig for .dm is optional, and is only required if the base apk has it. This change makes it required regardless of (base) apk, when a system property is set to "true". fs-verity is required for devices first launced with R (can also be opted in). On older devices, .dm is still allowed to install without .fsv_sig. The change contains 3 conceptual changes: - mVerityFound -> mVerityFoundForApks: since it now focuses on only APKs. - Bail out if .dm does not come with .fsv_sig (on supported devices). - Update tests Bug: 180414192 Test: CtsAppSecurityHostTestCases:android.appsecurity.cts.ApkVerityInstallTest Test: CtsDexMetadataHostTestCases Test: ApkVerityTest Test: Also run the above on a device without fs-verity support, and with the requirement disable by system property. Change-Id: I1c93671b9b433a73b88ae23205eec2a57a147ae1 --- .../content/pm/dex/DexMetadataHelper.java | 16 +- .../server/pm/PackageInstallerSession.java | 78 +++++---- .../com/android/apkverity/ApkVerityTest.java | 155 +++++++++++++----- 3 files changed, 172 insertions(+), 77 deletions(-) diff --git a/core/java/android/content/pm/dex/DexMetadataHelper.java b/core/java/android/content/pm/dex/DexMetadataHelper.java index bc5d14a533e2..0d5b33cd8672 100644 --- a/core/java/android/content/pm/dex/DexMetadataHelper.java +++ b/core/java/android/content/pm/dex/DexMetadataHelper.java @@ -24,16 +24,16 @@ import android.content.pm.parsing.ApkLiteParseUtils; import android.content.pm.parsing.PackageLite; import android.os.SystemProperties; import android.util.ArrayMap; -import android.util.jar.StrictJarFile; import android.util.JsonReader; import android.util.Log; +import android.util.jar.StrictJarFile; import com.android.internal.annotations.VisibleForTesting; import java.io.File; +import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; -import java.io.IOException; import java.io.UnsupportedEncodingException; import java.nio.file.Files; import java.nio.file.Paths; @@ -53,7 +53,10 @@ public class DexMetadataHelper { /** $> adb shell 'setprop log.tag.DexMetadataHelper VERBOSE' */ public static final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG); /** $> adb shell 'setprop pm.dexopt.dm.require_manifest true' */ - private static String PROPERTY_DM_JSON_MANIFEST_REQUIRED = "pm.dexopt.dm.require_manifest"; + private static final String PROPERTY_DM_JSON_MANIFEST_REQUIRED = + "pm.dexopt.dm.require_manifest"; + /** $> adb shell 'setprop pm.dexopt.dm.require_fsverity true' */ + private static final String PROPERTY_DM_FSVERITY_REQUIRED = "pm.dexopt.dm.require_fsverity"; private static final String DEX_METADATA_FILE_EXTENSION = ".dm"; @@ -69,6 +72,13 @@ public class DexMetadataHelper { return path.endsWith(DEX_METADATA_FILE_EXTENSION); } + /** + * Returns whether fs-verity is required to install a dex metadata + */ + public static boolean isFsVerityRequired() { + return SystemProperties.getBoolean(PROPERTY_DM_FSVERITY_REQUIRED, false); + } + /** * Return the size (in bytes) of all dex metadata files associated with the given package. */ diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 0ce26739b51c..9c023fb25219 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -778,7 +778,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @GuardedBy("mLock") private File mInheritedFilesBase; @GuardedBy("mLock") - private boolean mVerityFound; + private boolean mVerityFoundForApks; /** * Both flags should be guarded with mLock whenever changes need to be in lockstep. @@ -2714,8 +2714,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new PackageManagerException(INSTALL_FAILED_INVALID_APK, "Missing existing base package"); } - // Default to require only if existing base has fs-verity. - mVerityFound = PackageManagerServiceUtils.isApkVerityEnabled() + // Default to require only if existing base apk has fs-verity. + mVerityFoundForApks = PackageManagerServiceUtils.isApkVerityEnabled() && params.mode == SessionParams.MODE_INHERIT_EXISTING && VerityUtils.hasFsverity(pkgInfo.applicationInfo.getBaseCodePath()); @@ -3010,34 +3010,18 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } @GuardedBy("mLock") - private void maybeStageFsveritySignatureLocked(File origFile, File targetFile) - throws PackageManagerException { + private void maybeStageFsveritySignatureLocked(File origFile, File targetFile, + boolean fsVerityRequired) throws PackageManagerException { final File originalSignature = new File( VerityUtils.getFsveritySignatureFilePath(origFile.getPath())); - // Make sure .fsv_sig exists when it should, then resolve and stage it. if (originalSignature.exists()) { - // mVerityFound can only change from false to true here during the staging loop. Since - // all or none of files should have .fsv_sig, this should only happen in the first time - // (or never), otherwise bail out. - if (!mVerityFound) { - mVerityFound = true; - if (mResolvedStagedFiles.size() > 1) { - throw new PackageManagerException(INSTALL_FAILED_BAD_SIGNATURE, - "Some file is missing fs-verity signature"); - } - } - } else { - if (!mVerityFound) { - return; - } + final File stagedSignature = new File( + VerityUtils.getFsveritySignatureFilePath(targetFile.getPath())); + stageFileLocked(originalSignature, stagedSignature); + } else if (fsVerityRequired) { throw new PackageManagerException(INSTALL_FAILED_BAD_SIGNATURE, "Missing corresponding fs-verity signature to " + origFile); } - - final File stagedSignature = new File( - VerityUtils.getFsveritySignatureFilePath(targetFile.getPath())); - - stageFileLocked(originalSignature, stagedSignature); } @GuardedBy("mLock") @@ -3056,7 +3040,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { DexMetadataHelper.buildDexMetadataPathForApk(targetFile.getName())); stageFileLocked(dexMetadataFile, targetDexMetadataFile); - maybeStageFsveritySignatureLocked(dexMetadataFile, targetDexMetadataFile); + + // Also stage .dm.fsv_sig. .dm may be required to install with fs-verity signature on + // supported on older devices. + maybeStageFsveritySignatureLocked(dexMetadataFile, targetDexMetadataFile, + VerityUtils.isFsVeritySupported() && DexMetadataHelper.isFsVerityRequired()); } private void storeBytesToInstallationFile(final String localPath, final String absolutePath, @@ -3117,14 +3105,46 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } + @GuardedBy("mLock") + private boolean isFsVerityRequiredForApk(File origFile, File targetFile) + throws PackageManagerException { + if (mVerityFoundForApks) { + return true; + } + + // We haven't seen .fsv_sig for any APKs. Treat it as not required until we see one. + final File originalSignature = new File( + VerityUtils.getFsveritySignatureFilePath(origFile.getPath())); + if (!originalSignature.exists()) { + return false; + } + mVerityFoundForApks = true; + + // When a signature is found, also check any previous staged APKs since they also need to + // have fs-verity signature consistently. + for (File file : mResolvedStagedFiles) { + if (!file.getName().endsWith(".apk")) { + continue; + } + // Ignore the current targeting file. + if (targetFile.getName().equals(file.getName())) { + continue; + } + throw new PackageManagerException(INSTALL_FAILED_BAD_SIGNATURE, + "Previously staged apk is missing fs-verity signature"); + } + return true; + } + @GuardedBy("mLock") private void resolveAndStageFileLocked(File origFile, File targetFile, String splitName) throws PackageManagerException { stageFileLocked(origFile, targetFile); - // Stage fsverity signature if present. - maybeStageFsveritySignatureLocked(origFile, targetFile); - // Stage dex metadata (.dm) if present. + // Stage APK's fs-verity signature if present. + maybeStageFsveritySignatureLocked(origFile, targetFile, + isFsVerityRequiredForApk(origFile, targetFile)); + // Stage dex metadata (.dm) and corresponding fs-verity signature if present. maybeStageDexMetadataLocked(origFile, targetFile); // Stage checksums (.digests) if present. maybeStageDigestsLocked(origFile, targetFile, splitName); diff --git a/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java b/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java index ab3572ba2173..d96005b8a71a 100644 --- a/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java +++ b/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java @@ -95,10 +95,13 @@ public class ApkVerityTest extends BaseHostJUnit4Test { new AddFsVerityCertRule(this, CERT_PATH); private ITestDevice mDevice; + private boolean mDmRequireFsVerity; @Before public void setUp() throws DeviceNotAvailableException { mDevice = getDevice(); + mDmRequireFsVerity = "true".equals( + mDevice.getProperty("pm.dexopt.dm.require_fsverity")); uninstallPackage(TARGET_PACKAGE); } @@ -124,7 +127,7 @@ public class ApkVerityTest extends BaseHostJUnit4Test { verifyInstalledFiles( INSTALLED_BASE_APK, INSTALLED_BASE_APK_FSV_SIG); - verifyInstalledFilesHaveFsverity(); + verifyInstalledFilesHaveFsverity(INSTALLED_BASE_APK); } @Test @@ -151,7 +154,9 @@ public class ApkVerityTest extends BaseHostJUnit4Test { INSTALLED_BASE_APK_FSV_SIG, INSTALLED_SPLIT_APK, INSTALLED_SPLIT_APK_FSV_SIG); - verifyInstalledFilesHaveFsverity(); + verifyInstalledFilesHaveFsverity( + INSTALLED_BASE_APK, + INSTALLED_SPLIT_APK); } @Test @@ -167,7 +172,9 @@ public class ApkVerityTest extends BaseHostJUnit4Test { INSTALLED_BASE_APK_FSV_SIG, INSTALLED_BASE_DM, INSTALLED_BASE_DM_FSV_SIG); - verifyInstalledFilesHaveFsverity(); + verifyInstalledFilesHaveFsverity( + INSTALLED_BASE_APK, + INSTALLED_BASE_DM); } @Test @@ -189,7 +196,11 @@ public class ApkVerityTest extends BaseHostJUnit4Test { INSTALLED_SPLIT_APK_FSV_SIG, INSTALLED_SPLIT_DM, INSTALLED_SPLIT_DM_FSV_SIG); - verifyInstalledFilesHaveFsverity(); + verifyInstalledFilesHaveFsverity( + INSTALLED_BASE_APK, + INSTALLED_BASE_DM, + INSTALLED_SPLIT_APK, + INSTALLED_SPLIT_DM); } @Test @@ -213,7 +224,9 @@ public class ApkVerityTest extends BaseHostJUnit4Test { INSTALLED_BASE_APK_FSV_SIG, INSTALLED_SPLIT_APK, INSTALLED_SPLIT_APK_FSV_SIG); - verifyInstalledFilesHaveFsverity(); + verifyInstalledFilesHaveFsverity( + INSTALLED_BASE_APK, + INSTALLED_SPLIT_APK); } @Test @@ -250,18 +263,6 @@ public class ApkVerityTest extends BaseHostJUnit4Test { INSTALLED_BASE_APK, INSTALLED_SPLIT_APK, INSTALLED_SPLIT_APK_FSV_SIG); - - } - - @Test - public void testInstallOnlyBaseHasFsvSig() - throws DeviceNotAvailableException, FileNotFoundException { - new InstallMultiple() - .addFileAndSignature(BASE_APK) - .addFile(BASE_APK_DM) - .addFile(SPLIT_APK) - .addFile(SPLIT_APK_DM) - .runExpectingFailure(); } @Test @@ -271,18 +272,83 @@ public class ApkVerityTest extends BaseHostJUnit4Test { .addFile(BASE_APK) .addFileAndSignature(BASE_APK_DM) .addFile(SPLIT_APK) - .addFile(SPLIT_APK_DM) + .addFileAndSignature(SPLIT_APK_DM) + .run(); + verifyInstalledFiles( + INSTALLED_BASE_APK, + INSTALLED_BASE_DM, + INSTALLED_BASE_DM_FSV_SIG, + INSTALLED_SPLIT_APK, + INSTALLED_SPLIT_DM, + INSTALLED_SPLIT_DM_FSV_SIG); + verifyInstalledFilesHaveFsverity( + INSTALLED_BASE_DM, + INSTALLED_SPLIT_DM); + } + + @Test + public void testInstallDmWithoutFsvSig_Base() + throws DeviceNotAvailableException, FileNotFoundException { + InstallMultiple installer = new InstallMultiple() + .addFile(BASE_APK) + .addFile(BASE_APK_DM) + .addFile(SPLIT_APK) + .addFileAndSignature(SPLIT_APK_DM); + if (mDmRequireFsVerity) { + installer.runExpectingFailure(); + } else { + installer.run(); + verifyInstalledFiles( + INSTALLED_BASE_APK, + INSTALLED_BASE_DM, + INSTALLED_SPLIT_APK, + INSTALLED_SPLIT_DM, + INSTALLED_SPLIT_DM_FSV_SIG); + verifyInstalledFilesHaveFsverity(INSTALLED_SPLIT_DM); + } + } + + @Test + public void testInstallDmWithoutFsvSig_Split() + throws DeviceNotAvailableException, FileNotFoundException { + InstallMultiple installer = new InstallMultiple() + .addFile(BASE_APK) + .addFileAndSignature(BASE_APK_DM) + .addFile(SPLIT_APK) + .addFile(SPLIT_APK_DM); + if (mDmRequireFsVerity) { + installer.runExpectingFailure(); + } else { + installer.run(); + verifyInstalledFiles( + INSTALLED_BASE_APK, + INSTALLED_BASE_DM, + INSTALLED_BASE_DM_FSV_SIG, + INSTALLED_SPLIT_APK, + INSTALLED_SPLIT_DM); + verifyInstalledFilesHaveFsverity(INSTALLED_BASE_DM); + } + } + + @Test + public void testInstallSomeApkIsMissingFsvSig_Base() + throws DeviceNotAvailableException, FileNotFoundException { + new InstallMultiple() + .addFileAndSignature(BASE_APK) + .addFileAndSignature(BASE_APK_DM) + .addFile(SPLIT_APK) + .addFileAndSignature(SPLIT_APK_DM) .runExpectingFailure(); } @Test - public void testInstallOnlySplitHasFsvSig() + public void testInstallSomeApkIsMissingFsvSig_Split() throws DeviceNotAvailableException, FileNotFoundException { new InstallMultiple() .addFile(BASE_APK) - .addFile(BASE_APK_DM) + .addFileAndSignature(BASE_APK_DM) .addFileAndSignature(SPLIT_APK) - .addFile(SPLIT_APK_DM) + .addFileAndSignature(SPLIT_APK_DM) .runExpectingFailure(); } @@ -383,37 +449,36 @@ public class ApkVerityTest extends BaseHostJUnit4Test { } } - private void verifyInstalledFilesHaveFsverity() throws DeviceNotAvailableException { + private void verifyInstalledFilesHaveFsverity(String... filenames) + throws DeviceNotAvailableException { // Verify that all files are protected by fs-verity String apkPath = getApkPath(TARGET_PACKAGE); String appDir = apkPath.substring(0, apkPath.lastIndexOf("/")); long kTargetOffset = 0; - for (String basename : expectRemoteCommandToSucceed("ls " + appDir).split("\n")) { - if (basename.endsWith(".apk") || basename.endsWith(".dm")) { - String path = appDir + "/" + basename; - damageFileAgainstBlockDevice(path, kTargetOffset); + for (String basename : filenames) { + String path = appDir + "/" + basename; + damageFileAgainstBlockDevice(path, kTargetOffset); - // Retry is sometimes needed to pass the test. Package manager may have FD leaks - // (see b/122744005 as example) that prevents the file in question to be evicted - // from filesystem cache. Forcing GC workarounds the problem. - int retry = 5; - for (; retry > 0; retry--) { - BlockDeviceWriter.dropCaches(mDevice); - if (!BlockDeviceWriter.canReadByte(mDevice, path, kTargetOffset)) { - break; - } - try { - CLog.d("lsof: " + expectRemoteCommandToSucceed("lsof " + apkPath)); - Thread.sleep(1000); - String pid = expectRemoteCommandToSucceed("pidof system_server"); - mDevice.executeShellV2Command("kill -10 " + pid); // force GC - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - return; - } + // Retry is sometimes needed to pass the test. Package manager may have FD leaks + // (see b/122744005 as example) that prevents the file in question to be evicted + // from filesystem cache. Forcing GC workarounds the problem. + int retry = 5; + for (; retry > 0; retry--) { + BlockDeviceWriter.dropCaches(mDevice); + if (!BlockDeviceWriter.canReadByte(mDevice, path, kTargetOffset)) { + break; + } + try { + CLog.d("lsof: " + expectRemoteCommandToSucceed("lsof " + apkPath)); + Thread.sleep(1000); + String pid = expectRemoteCommandToSucceed("pidof system_server"); + mDevice.executeShellV2Command("kill -10 " + pid); // force GC + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; } - assertTrue("Read from " + path + " should fail", retry > 0); } + assertTrue("Read from " + path + " should fail", retry > 0); } }