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

Commit 8a59b76d authored by Shawn Willden's avatar Shawn Willden
Browse files

Fix some bugs in FRP hardening 

ebiggers@ identified some problems:

1. The formatting of the persistent data block was incorrect, it
   shortened the FRP challenge section by the size of the FRP
   secret and magic, but then failed to advance the channel write
   position past them, causing subsequent sections to be written
   at the wrong locations.  The data being written to incorrect
   locations was all zeros and it seems unlikely that this could
   have caused any problems, but this fixes it.
2. `writeFrpMagicAndDefaultSecret()` failed to recalculate and
   write the digest, meaning that the PDB would subsequently be
   viewed as corrupted and reformatted on the next boot in some
   obscure cases.
3. `setFactoryResetProtectionSecret()` failed to check that the
   caller has permission to configure FRP.  This isn't strictly
   necessary (nor is the similar check on
   `deactivateFactoryResetProtection()`, but it's also not the
   case that anyone without the permission needs to be calling
   these methods.
4. There was a bug in the FRP secret file display in the status
   command of the persistent_data_block shell command.
5. `deactivateFrpWithFileSecret()` logged a warning if the secret
   file was not found, but this could happen in correct
   operation, so it should be an informational log.  (In general
   FRP hardening makes PDBService too noisy, but this is good in
   the short term; logging will be reduced in the future).

Test: atest PersistentDataBlockServiceTest
Bug: 290312729
Change-Id: Ief505a634d355a97bb6b99efc52cef68649a025f
parent bf9418b0

services/core/java/com/android/server/pdb/PersistentDataBlockService.java

+30 −19
Original line number Diff line number Diff line
@@ -143,7 +143,8 @@ public class PersistentDataBlockService extends SystemService { 
    // Magic number to mark block device as adhering to the format consumed by this service

    private static final int PARTITION_TYPE_MARKER = 0x19901873;

    /** Size of the block reserved for FRP credential, including 4 bytes for the size header. */

    private static final int FRP_CREDENTIAL_RESERVED_SIZE = 1000;

    @VisibleForTesting

    static final int FRP_CREDENTIAL_RESERVED_SIZE = 1000;

    /** Maximum size of the FRP credential handle that can be stored. */

    @VisibleForTesting

    static final int MAX_FRP_CREDENTIAL_HANDLE_SIZE = FRP_CREDENTIAL_RESERVED_SIZE - 4;
@@ -158,7 +159,8 @@ public class PersistentDataBlockService extends SystemService { 
    /**

     * Size of the block reserved for Test Harness Mode data, including 4 bytes for the size header.

     */

    private static final int TEST_MODE_RESERVED_SIZE = 10000;

    @VisibleForTesting

    static final int TEST_MODE_RESERVED_SIZE = 10000;

    /** Maximum size of the Test Harness Mode data that can be stored. */

    @VisibleForTesting

    static final int MAX_TEST_MODE_DATA_SIZE = TEST_MODE_RESERVED_SIZE - 4;
@@ -393,7 +395,8 @@ public class PersistentDataBlockService extends SystemService { 
        return totalDataSize;

    }



    private long getBlockDeviceSize() {

    @VisibleForTesting

    long getBlockDeviceSize() {

        synchronized (mLock) {

            if (mBlockDeviceSize == -1) {

                if (mIsFileBacked) {
@@ -553,26 +556,33 @@ public class PersistentDataBlockService extends SystemService { 
            channel.write(buf);

            channel.force(true);



            // 3. skip the test mode data and leave it unformatted.

            // 3. Write the default FRP secret (all zeros).

            if (mFrpEnforced) {

                Slog.i(TAG, "Writing FRP secret magic");

                channel.write(ByteBuffer.wrap(FRP_SECRET_MAGIC));



                Slog.i(TAG, "Writing default FRP secret");

                channel.write(ByteBuffer.allocate(FRP_SECRET_SIZE));

                channel.force(true);



                mFrpActive = false;

            }



            // 4. skip the test mode data and leave it unformatted.

            //    This is for a feature that enables testing.

            channel.position(channel.position() + TEST_MODE_RESERVED_SIZE);



            // 4. wipe the FRP_CREDENTIAL explicitly

            // 5. wipe the FRP_CREDENTIAL explicitly

            buf = ByteBuffer.allocate(FRP_CREDENTIAL_RESERVED_SIZE);

            channel.write(buf);

            channel.force(true);



            // 5. set unlock = 0 because it's a formatPartitionLocked

            // 6. set unlock = 0 because it's a formatPartitionLocked

            buf = ByteBuffer.allocate(FRP_CREDENTIAL_RESERVED_SIZE);

            buf.put((byte)0);

            buf.flip();

            channel.write(buf);

            channel.force(true);



            // 6. Write the default FRP secret (all zeros).

            if (mFrpEnforced) {

                writeFrpMagicAndDefaultSecret();

            }

        } catch (IOException e) {

            Slog.e(TAG, "failed to format block", e);

            return;
@@ -616,7 +626,7 @@ public class PersistentDataBlockService extends SystemService { 
            // version.  If so, we deactivate FRP and set the secret to the default value.

            if (isUpgradingFromPreVRelease()) {

                Slog.w(TAG, "Upgrading from Android 14 or lower, defaulting FRP secret");

                writeFrpMagicAndDefaultSecret();

                writeFrpMagicAndDefaultSecretLocked();

                mFrpActive = false;

                return true;

            }
@@ -630,7 +640,7 @@ public class PersistentDataBlockService extends SystemService { 
        try {

            return deactivateFrp(Files.readAllBytes(Paths.get(frpSecretFile)));

        } catch (IOException e) {

            Slog.w(TAG, "Failed to read FRP secret file: " + frpSecretFile + " "

            Slog.i(TAG, "Failed to read FRP secret file: " + frpSecretFile + " "

                    + e.getClass().getSimpleName());

            return false;

        }
@@ -653,7 +663,8 @@ public class PersistentDataBlockService extends SystemService { 
    }



    /**

     * Write the provided secret to the FRP secret file in /data and to the /persist partition.

     * Write the provided secret to the FRP secret file in /data and to the persistent data block

     * partition.

     *

     * Writing is a three-step process, to ensure that we can recover from a crash at any point.

     */
@@ -713,7 +724,7 @@ public class PersistentDataBlockService extends SystemService { 
        synchronized (mLock) {

            if (!hasFrpSecretMagic()) {

                Slog.i(TAG, "No FRP secret magic, system must have been upgraded.");

                writeFrpMagicAndDefaultSecret();

                writeFrpMagicAndDefaultSecretLocked();

            }

        }
@@ -735,11 +746,9 @@ public class PersistentDataBlockService extends SystemService { 
        }

    }



    private void writeFrpMagicAndDefaultSecret() {

    private void writeFrpMagicAndDefaultSecretLocked() {

        try (FileChannel channel = getBlockOutputChannelIgnoringFrp()) {

            synchronized (mLock) {

                // Write secret first in case we crash between the writes, causing the first write

                // to be synced but the second to be lost.

                Slog.i(TAG, "Writing default FRP secret");

                channel.position(getFrpSecretDataOffset());

                channel.write(ByteBuffer.allocate(FRP_SECRET_SIZE));
@@ -755,6 +764,7 @@ public class PersistentDataBlockService extends SystemService { 
        } catch (IOException e) {

            Slog.e(TAG, "Failed to write FRP magic and default secret", e);

        }

        computeAndWriteDigestLocked();

    }



    @VisibleForTesting
@@ -879,7 +889,7 @@ public class PersistentDataBlockService extends SystemService { 
                if (printSecret) {

                    try {

                        pw.println("FRP secret in " + frpSecretFile + ": " + HexFormat.of()

                                .formatHex(Files.readAllBytes(Paths.get(mFrpSecretFile))));

                                .formatHex(Files.readAllBytes(Paths.get(frpSecretFile))));

                    } catch (IOException e) {

                        Slog.e(TAG, "Failed to read " + frpSecretFile, e);

                    }
@@ -1230,6 +1240,7 @@ public class PersistentDataBlockService extends SystemService { 


        @Override

        public boolean setFactoryResetProtectionSecret(byte[] secret) {

            enforceConfigureFrpPermission();

            enforceUid(Binder.getCallingUid());

            if (secret == null || secret.length != FRP_SECRET_SIZE) {

                throw new IllegalArgumentException(

services/tests/servicestests/src/com/android/server/pdb/PersistentDataBlockServiceTest.java

+85 −0
Original line number Diff line number Diff line
@@ -17,12 +17,17 @@ 
package com.android.server.pdb;



import static com.android.server.pdb.PersistentDataBlockService.DIGEST_SIZE_BYTES;

import static com.android.server.pdb.PersistentDataBlockService.FRP_CREDENTIAL_RESERVED_SIZE;

import static com.android.server.pdb.PersistentDataBlockService.FRP_SECRET_MAGIC;

import static com.android.server.pdb.PersistentDataBlockService.FRP_SECRET_SIZE;

import static com.android.server.pdb.PersistentDataBlockService.HEADER_SIZE;

import static com.android.server.pdb.PersistentDataBlockService.MAX_DATA_BLOCK_SIZE;

import static com.android.server.pdb.PersistentDataBlockService.MAX_FRP_CREDENTIAL_HANDLE_SIZE;

import static com.android.server.pdb.PersistentDataBlockService.MAX_TEST_MODE_DATA_SIZE;

import static com.android.server.pdb.PersistentDataBlockService.TEST_MODE_RESERVED_SIZE;



import static com.google.common.truth.Truth.assertThat;

import static com.google.common.truth.Truth.assertWithMessage;



import static org.junit.Assert.assertThrows;

import static org.mockito.ArgumentMatchers.anyInt;
@@ -36,6 +41,7 @@ import static org.mockito.Mockito.when; 
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;



import android.Manifest;

import android.annotation.NonNull;

import android.content.Context;

import android.content.pm.PackageManager;

import android.os.Binder;
@@ -63,6 +69,7 @@ import java.nio.file.Files; 
import java.nio.file.StandardOpenOption;

import java.security.MessageDigest;

import java.security.NoSuchAlgorithmException;

import java.util.Arrays;



@RunWith(JUnitParamsRunner.class)

public class PersistentDataBlockServiceTest {
@@ -395,6 +402,68 @@ public class PersistentDataBlockServiceTest { 
        assertThat(mInterface.getDataBlockSize()).isEqualTo(0);

    }



    @Test

    @Parameters({"false", "true"})

    public void testPartitionFormat(boolean frpEnabled) throws Exception {

        setUp(frpEnabled);



        /*

         * 1. Fill the PDB with a specific value, so we can check regions that weren't touched

         *    by formatting

         */

        FileChannel channel = FileChannel.open(mDataBlockFile.toPath(), StandardOpenOption.WRITE,

                StandardOpenOption.TRUNCATE_EXISTING);

        byte[] bufArray = new byte[(int) mPdbService.getBlockDeviceSize()];

        Arrays.fill(bufArray, (byte) 0x7f);

        ByteBuffer buf = ByteBuffer.wrap(bufArray);

        channel.write(buf);

        channel.close();



        /*

         * 2. Format it.

         */

        mPdbService.formatPartitionLocked(true);



        /*

         * 3. Check it.

         */

        channel = FileChannel.open(mDataBlockFile.toPath(), StandardOpenOption.READ);



        // 3a. Skip the digest and header

        channel.position(channel.position() + DIGEST_SIZE_BYTES + HEADER_SIZE);



        // 3b. Check the FRP data segment

        assertContains("FRP data", readData(channel, mPdbService.getMaximumFrpDataSize()).array(),

                (byte) 0);



        if (frpEnabled) {

            // 3c. The FRP secret magic & value

            assertThat(mPdbService.getFrpSecretMagicOffset()).isEqualTo(channel.position());

            assertThat(readData(channel, FRP_SECRET_MAGIC.length).array()).isEqualTo(

                    FRP_SECRET_MAGIC);



            assertThat(mPdbService.getFrpSecretDataOffset()).isEqualTo(channel.position());

            assertContains("FRP secret", readData(channel, FRP_SECRET_SIZE).array(), (byte) 0);

        }



        // 3d. The test mode data (unmodified by formatPartitionLocked()).

        assertThat(mPdbService.getTestHarnessModeDataOffset()).isEqualTo(channel.position());

        assertContains("Test data", readData(channel, TEST_MODE_RESERVED_SIZE).array(),

                (byte) 0x7f);



        // 3e. The FRP credential segment

        assertThat(mPdbService.getFrpCredentialDataOffset()).isEqualTo(channel.position());

        assertContains("FRP credential", readData(channel, FRP_CREDENTIAL_RESERVED_SIZE).array(),

                (byte) 0);



        // 3f. OEM unlock byte.

        assertThat(mPdbService.getOemUnlockDataOffset()).isEqualTo(channel.position());

        assertThat(new byte[]{1}).isEqualTo(readData(channel, 1).array());



        // 3g. EOF

        assertThat(channel.position()).isEqualTo(channel.size());

    }



    @Test

    @Parameters({"false", "true"})

    public void wipePermissionCheck(boolean frpEnabled) throws Exception {
@@ -987,4 +1056,20 @@ public class PersistentDataBlockServiceTest { 
            return buffer;

        }

    }



    @NonNull

    private static ByteBuffer readData(FileChannel channel, int length) throws IOException {

        ByteBuffer buf = ByteBuffer.allocate(length);

        assertThat(channel.read(buf)).isEqualTo(length);

        buf.flip();

        assertThat(buf.limit()).isEqualTo(length);

        return buf;

    }



    private static void assertContains(String sectionName, byte[] buf, byte expected) {

        for (int i = 0; i < buf.length; i++) {

            assertWithMessage(sectionName + " is incorrect at offset " + i)

                    .that(buf[i]).isEqualTo(expected);

        }

    }

}