From 4bdb6e5ef09eb8e060359eab3c824cc036d2c105 Mon Sep 17 00:00:00 2001 From: Victor Hsieh Date: Wed, 9 Nov 2022 15:34:01 -0800 Subject: [PATCH 1/3] Move fs-verity signature check from kernel to install This unblocks the deprecation of fs-verity kernel keyring. With an APK is installed with .fsv_sig, the signature was installed to the filesystem to be verified with a kernel keyring. Due to the current threat model of root or system server, checking the signature in kernel is not superior since the attacker can simply strip fs-verity protection from the file. The userspace check should be done anyway if the keyring contains multiple keys. Without regressing any security guarantee, this change moves the signature check to (only) install time. FileIntegrityService already holds the certificates and facilitates the signature check. In order to keep the test passing, FileIntegrityService now supports (debug only) cmd for adding and removing a debug cert. Bug: 258708453 Test: ApkVerityTest Test: CtsAppSecurityHostTestCases:android.appsecurity.cts.ApkVerityInstallTest Test: ChecksumsTest Change-Id: I737f058229928f1b242612631a13c62709b06d33 --- .../server/pm/InstallPackageHelper.java | 13 +- .../server/security/FileIntegrityService.java | 140 +++++++++++++++++- .../com/android/apkverity/ApkVerityTest.java | 8 +- 3 files changed, 143 insertions(+), 18 deletions(-) diff --git a/services/core/java/com/android/server/pm/InstallPackageHelper.java b/services/core/java/com/android/server/pm/InstallPackageHelper.java index 70bd24ce20a22..6a0225b7922db 100644 --- a/services/core/java/com/android/server/pm/InstallPackageHelper.java +++ b/services/core/java/com/android/server/pm/InstallPackageHelper.java @@ -174,6 +174,7 @@ import com.android.server.pm.pkg.component.ParsedPermission; import com.android.server.pm.pkg.component.ParsedPermissionGroup; import com.android.server.pm.pkg.parsing.ParsingPackageUtils; import com.android.server.rollback.RollbackManagerInternal; +import com.android.server.security.FileIntegrityService; import com.android.server.utils.WatchedArrayMap; import com.android.server.utils.WatchedLongSparseArray; @@ -1836,6 +1837,7 @@ final class InstallPackageHelper { } } + var fis = FileIntegrityService.getService(); for (Map.Entry entry : fsverityCandidates.entrySet()) { try { final String filePath = entry.getKey(); @@ -1843,13 +1845,16 @@ final class InstallPackageHelper { continue; } - // Set up fs-verity with optional signature. + VerityUtils.setUpFsverity(filePath, (byte[]) null); + + // Verify fs-verity signature if exists. final String signaturePath = entry.getValue(); - String optionalSignaturePath = null; if (new File(signaturePath).exists()) { - optionalSignaturePath = signaturePath; + if (!fis.verifyPkcs7DetachedSignature(signaturePath, filePath)) { + throw new PrepareFailure(PackageManager.INSTALL_FAILED_BAD_SIGNATURE, + "fs-verity signature does not verify against a known key"); + } } - VerityUtils.setUpFsverity(filePath, optionalSignaturePath); } catch (IOException e) { throw new PrepareFailure(PackageManager.INSTALL_FAILED_BAD_SIGNATURE, "Failed to enable fs-verity: " + e); diff --git a/services/core/java/com/android/server/security/FileIntegrityService.java b/services/core/java/com/android/server/security/FileIntegrityService.java index 466ac74a83225..5ae697315ed1b 100644 --- a/services/core/java/com/android/server/security/FileIntegrityService.java +++ b/services/core/java/com/android/server/security/FileIntegrityService.java @@ -23,27 +23,37 @@ import android.content.Context; import android.content.pm.PackageManager; import android.content.pm.PackageManagerInternal; import android.os.Binder; +import android.os.Build; import android.os.Environment; import android.os.IBinder; +import android.os.ParcelFileDescriptor; +import android.os.ResultReceiver; +import android.os.ShellCallback; +import android.os.ShellCommand; import android.os.UserHandle; import android.security.IFileIntegrityService; import android.util.Slog; +import com.android.internal.annotations.GuardedBy; import com.android.internal.security.VerityUtils; import com.android.server.LocalServices; import com.android.server.SystemService; import java.io.ByteArrayInputStream; import java.io.File; +import java.io.FileDescriptor; import java.io.IOException; +import java.io.InputStream; +import java.io.PrintWriter; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.security.cert.Certificate; +import java.security.cert.CertificateEncodingException; import java.security.cert.CertificateException; import java.security.cert.CertificateFactory; import java.security.cert.X509Certificate; import java.util.ArrayList; -import java.util.Collection; /** * A {@link SystemService} that provides file integrity related operations. @@ -52,9 +62,19 @@ import java.util.Collection; public class FileIntegrityService extends SystemService { private static final String TAG = "FileIntegrityService"; + /** The maximum size of signature file. This is just to avoid potential abuse. */ + private static final int MAX_SIGNATURE_FILE_SIZE_BYTES = 8192; + private static CertificateFactory sCertFactory; - private Collection mTrustedCertificates = new ArrayList(); + @GuardedBy("mTrustedCertificates") + private final ArrayList mTrustedCertificates = + new ArrayList(); + + /** Gets the instance of the service */ + public static FileIntegrityService getService() { + return LocalServices.getService(FileIntegrityService.class); + } private final IBinder mService = new IFileIntegrityService.Stub() { @Override @@ -75,13 +95,23 @@ public class FileIntegrityService extends SystemService { Slog.w(TAG, "Received a null certificate"); return false; } - return mTrustedCertificates.contains(toCertificate(certificateBytes)); + synchronized (mTrustedCertificates) { + return mTrustedCertificates.contains(toCertificate(certificateBytes)); + } } catch (CertificateException e) { Slog.e(TAG, "Failed to convert the certificate: " + e); return false; } } + @Override + public void onShellCommand(FileDescriptor in, FileDescriptor out, + FileDescriptor err, String[] args, ShellCallback callback, + ResultReceiver resultReceiver) { + new FileIntegrityServiceShellCommand() + .exec(this, in, out, err, args, callback, resultReceiver); + } + private void checkCallerPermission(String packageName) { final int callingUid = Binder.getCallingUid(); final int callingUserId = UserHandle.getUserId(callingUid); @@ -116,6 +146,7 @@ public class FileIntegrityService extends SystemService { } catch (CertificateException e) { Slog.wtf(TAG, "Cannot get an instance of X.509 certificate factory"); } + LocalServices.addService(FileIntegrityService.class, this); } @Override @@ -124,6 +155,34 @@ public class FileIntegrityService extends SystemService { publishBinderService(Context.FILE_INTEGRITY_SERVICE, mService); } + /** + * Returns whether the signature over the file's fs-verity digest can be verified by one of the + * known certiticates. + */ + public boolean verifyPkcs7DetachedSignature(String signaturePath, String filePath) + throws IOException { + if (Files.size(Paths.get(signaturePath)) > MAX_SIGNATURE_FILE_SIZE_BYTES) { + throw new SecurityException("Signature file is unexpectedly large: " + + signaturePath); + } + byte[] signatureBytes = Files.readAllBytes(Paths.get(signaturePath)); + byte[] digest = VerityUtils.getFsverityDigest(filePath); + synchronized (mTrustedCertificates) { + for (var cert : mTrustedCertificates) { + try { + byte[] derEncoded = cert.getEncoded(); + if (VerityUtils.verifyPkcs7DetachedSignature(signatureBytes, digest, + new ByteArrayInputStream(derEncoded))) { + return true; + } + } catch (CertificateEncodingException e) { + Slog.w(TAG, "Ignoring ill-formed certificate: " + e); + } + } + } + return false; + } + private void loadAllCertificates() { // A better alternative to load certificates would be to read from .fs-verity kernel // keyring, which fsverity_init loads to during earlier boot time from the same sources @@ -148,10 +207,6 @@ public class FileIntegrityService extends SystemService { for (File cert : files) { byte[] certificateBytes = Files.readAllBytes(cert.toPath()); - if (certificateBytes == null) { - Slog.w(TAG, "The certificate file is empty, ignoring " + cert); - continue; - } collectCertificate(certificateBytes); } } catch (IOException e) { @@ -165,7 +220,9 @@ public class FileIntegrityService extends SystemService { */ private void collectCertificate(@NonNull byte[] bytes) { try { - mTrustedCertificates.add(toCertificate(bytes)); + synchronized (mTrustedCertificates) { + mTrustedCertificates.add(toCertificate(bytes)); + } } catch (CertificateException e) { Slog.e(TAG, "Invalid certificate, ignored: " + e); } @@ -184,4 +241,71 @@ public class FileIntegrityService extends SystemService { } return (X509Certificate) certificate; } + + + private class FileIntegrityServiceShellCommand extends ShellCommand { + @Override + public int onCommand(String cmd) { + if (!Build.IS_DEBUGGABLE) { + return -1; + } + if (cmd == null) { + return handleDefaultCommands(cmd); + } + final PrintWriter pw = getOutPrintWriter(); + switch (cmd) { + case "append-cert": + String nextArg = getNextArg(); + if (nextArg == null) { + pw.println("Invalid argument"); + pw.println(""); + onHelp(); + return -1; + } + ParcelFileDescriptor pfd = openFileForSystem(nextArg, "r"); + if (pfd == null) { + pw.println("Cannot open the file"); + return -1; + } + InputStream is = new ParcelFileDescriptor.AutoCloseInputStream(pfd); + try { + collectCertificate(is.readAllBytes()); + } catch (IOException e) { + pw.println("Failed to add certificate: " + e); + return -1; + } + pw.println("Certificate is added successfully"); + return 0; + + case "remove-last-cert": + synchronized (mTrustedCertificates) { + if (mTrustedCertificates.size() == 0) { + pw.println("Certificate list is already empty"); + return -1; + } + mTrustedCertificates.remove(mTrustedCertificates.size() - 1); + } + pw.println("Certificate is removed successfully"); + return 0; + default: + pw.println("Unknown action"); + pw.println(""); + onHelp(); + } + return -1; + } + + @Override + public void onHelp() { + final PrintWriter pw = getOutPrintWriter(); + pw.println("File integrity service commands:"); + pw.println(" help"); + pw.println(" Print this help text."); + pw.println(" append-cert path/to/cert.der"); + pw.println(" Add the DER-encoded certificate (only in debug builds)"); + pw.println(" remove-last-cert"); + pw.println(" Remove the last certificate in the key list (only in debug builds)"); + pw.println(""); + } + } } diff --git a/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java b/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java index d96005b8a71a2..16f005f288564 100644 --- a/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java +++ b/tests/ApkVerityTest/src/com/android/apkverity/ApkVerityTest.java @@ -25,7 +25,6 @@ import static org.junit.Assert.fail; import android.platform.test.annotations.RootPermissionTest; import com.android.blockdevicewriter.BlockDeviceWriter; -import com.android.fsverity.AddFsVerityCertRule; import com.android.tradefed.device.DeviceNotAvailableException; import com.android.tradefed.device.ITestDevice; import com.android.tradefed.log.LogUtil.CLog; @@ -36,7 +35,6 @@ import com.android.tradefed.util.CommandStatus; import org.junit.After; import org.junit.Before; -import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -90,10 +88,6 @@ public class ApkVerityTest extends BaseHostJUnit4Test { /** Only 4K page is supported by fs-verity currently. */ private static final int FSVERITY_PAGE_SIZE = 4096; - @Rule - public final AddFsVerityCertRule mAddFsVerityCertRule = - new AddFsVerityCertRule(this, CERT_PATH); - private ITestDevice mDevice; private boolean mDmRequireFsVerity; @@ -103,11 +97,13 @@ public class ApkVerityTest extends BaseHostJUnit4Test { mDmRequireFsVerity = "true".equals( mDevice.getProperty("pm.dexopt.dm.require_fsverity")); + expectRemoteCommandToSucceed("cmd file_integrity append-cert " + CERT_PATH); uninstallPackage(TARGET_PACKAGE); } @After public void tearDown() throws DeviceNotAvailableException { + expectRemoteCommandToSucceed("cmd file_integrity remove-last-cert"); uninstallPackage(TARGET_PACKAGE); } From 2d8e9a3874f73899a7dbac553e5a20f9a4f32f8d Mon Sep 17 00:00:00 2001 From: Victor Hsieh Date: Thu, 17 Nov 2022 13:54:46 -0800 Subject: [PATCH 2/3] Enable fs-verity asynchronously on package install Bug: 259179666 Test: atest ChecksumsTest Test: atest CtsAppSecurityHostTestCases:android.appsecurity.cts.ApkVerityInstallTest Change-Id: Ie87efba347bd2c9bf08928191b2c724a47abd18b --- .../server/pm/InstallPackageHelper.java | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/pm/InstallPackageHelper.java b/services/core/java/com/android/server/pm/InstallPackageHelper.java index 6a0225b7922db..aedebbaa565ba 100644 --- a/services/core/java/com/android/server/pm/InstallPackageHelper.java +++ b/services/core/java/com/android/server/pm/InstallPackageHelper.java @@ -1845,15 +1845,30 @@ final class InstallPackageHelper { continue; } - VerityUtils.setUpFsverity(filePath, (byte[]) null); - - // Verify fs-verity signature if exists. final String signaturePath = entry.getValue(); if (new File(signaturePath).exists()) { + // If signature is provided, enable fs-verity first so that the file can be + // measured for signature check below. + VerityUtils.setUpFsverity(filePath, (byte[]) null); + if (!fis.verifyPkcs7DetachedSignature(signaturePath, filePath)) { throw new PrepareFailure(PackageManager.INSTALL_FAILED_BAD_SIGNATURE, "fs-verity signature does not verify against a known key"); } + } else { + // Without signature, we don't need to access the digest right away and can + // enable fs-verity in background (since this is a blocking call). + new Thread("fsverity-setup") { + @Override public void run() { + try { + VerityUtils.setUpFsverity(filePath, (byte[]) null); + } catch (IOException e) { + // There's nothing we can do if the setup failed. Since fs-verity is + // optional, just ignore the error for now. + Slog.e(TAG, "Failed to enable fs-verity to " + filePath); + } + } + }.start(); } } catch (IOException e) { throw new PrepareFailure(PackageManager.INSTALL_FAILED_BAD_SIGNATURE, From 6319ea152814e501f50156db143188ed98c73a0e Mon Sep 17 00:00:00 2001 From: Victor Hsieh Date: Mon, 21 Nov 2022 11:48:50 -0800 Subject: [PATCH 3/3] Delete dead code The only user was deleted in ag/20445927. Bug: N/A Test: TH Change-Id: I695409f2e925ba003b946809976ef534b5b36194 --- .../android/fsverity/AddFsVerityCertRule.java | 77 ------------------- 1 file changed, 77 deletions(-) delete mode 100644 tests/utils/hostutils/src/com/android/fsverity/AddFsVerityCertRule.java diff --git a/tests/utils/hostutils/src/com/android/fsverity/AddFsVerityCertRule.java b/tests/utils/hostutils/src/com/android/fsverity/AddFsVerityCertRule.java deleted file mode 100644 index 5ab4dc60e2ac3..0000000000000 --- a/tests/utils/hostutils/src/com/android/fsverity/AddFsVerityCertRule.java +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Copyright (C) 2021 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.fsverity; - -import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth.assertWithMessage; - -import static org.junit.Assume.assumeTrue; - -import com.android.tradefed.device.DeviceNotAvailableException; -import com.android.tradefed.device.ITestDevice; -import com.android.tradefed.log.LogUtil; -import com.android.tradefed.testtype.junit4.BaseHostJUnit4Test; -import com.android.tradefed.util.CommandResult; -import com.android.tradefed.util.CommandStatus; - -import org.junit.rules.ExternalResource; - -public final class AddFsVerityCertRule extends ExternalResource { - - private static final String APK_VERITY_STANDARD_MODE = "2"; - - private final BaseHostJUnit4Test mHost; - private final String mCertPath; - private String mKeyId; - - public AddFsVerityCertRule(BaseHostJUnit4Test host, String certPath) { - mHost = host; - mCertPath = certPath; - } - - @Override - protected void before() throws Throwable { - ITestDevice device = mHost.getDevice(); - String apkVerityMode = device.getProperty("ro.apk_verity.mode"); - assumeTrue(device.getLaunchApiLevel() >= 30 - || APK_VERITY_STANDARD_MODE.equals(apkVerityMode)); - - String keyId = executeCommand( - "mini-keyctl padd asymmetric fsv_test .fs-verity < " + mCertPath).trim(); - assertThat(keyId).matches("^\\d+$"); - mKeyId = keyId; - } - - @Override - protected void after() { - if (mKeyId == null) return; - try { - executeCommand("mini-keyctl unlink " + mKeyId + " .fs-verity"); - } catch (DeviceNotAvailableException e) { - LogUtil.CLog.e(e); - } - mKeyId = null; - } - - private String executeCommand(String cmd) throws DeviceNotAvailableException { - CommandResult result = mHost.getDevice().executeShellV2Command(cmd); - assertWithMessage("`" + cmd + "` failed: " + result.getStderr()) - .that(result.getStatus()) - .isEqualTo(CommandStatus.SUCCESS); - return result.getStdout(); - } -}