From 4bdb6e5ef09eb8e060359eab3c824cc036d2c105 Mon Sep 17 00:00:00 2001 From: Victor Hsieh Date: Wed, 9 Nov 2022 15:34:01 -0800 Subject: [PATCH] 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); }