From 929af52cff39457ec21451f66f6109f9a6d0a25b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 16 May 2023 17:19:22 +0000 Subject: [PATCH] Fix downgrades on device with FRP enabled Unfortunately, non-forwards-compatible changes to the PasswordData format break setting up an older version of Android on a device that had Factory Reset Protection (FRP) set up on a newer version. Commit 23070d87275c ("Added changes for storing PIN length in PasswordData") (http://ag/21983004) made a non-forwards-compatible change to the PasswordData format by reusing the first two bytes of the credentialType field as a version number. Therefore, undo that part of the change, and just use the data length to determine whether the new pinLength field is present or not. Bug: 276780938 Test: atest com.android.server.locksettings Test: Reproduced the bug by flashing udc-dev, adding Google account and PIN, then flashing latest public build (TQ2A.230505.002). Could not complete the setup wizard. Then did the same with udc-dev + this CL. Was able to complete setup wizard on public build. Also tested the same thing for udc-dev + this CL => udc-dev. Test: Manually tested upgrades: udc-dev => udc-dev + this CL, and latest public build (TQ2A.230505.002) => udc-dev + this CL. Specifically, tested that if a PIN was set before, the device can still be unlocked using the PIN afterwards. Test: Tested changing the PIN on udc-dev + this CL. Change-Id: Ibf3c91d14a0c6bd9af4403b080532f2739fde119 --- .../SyntheticPasswordManager.java | 47 ++++++------ .../locksettings/SyntheticPasswordTests.java | 71 +++++++++++++------ 2 files changed, 74 insertions(+), 44 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java index 8b8c5f6002555..65e7a00ad13f7 100644 --- a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java +++ b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java @@ -152,9 +152,6 @@ class SyntheticPasswordManager { // The security strength of the synthetic password, in bytes private static final int SYNTHETIC_PASSWORD_SECURITY_STRENGTH = 256 / 8; - public static final short PASSWORD_DATA_V1 = 1; - public static final short PASSWORD_DATA_V2 = 2; - private static final int PASSWORD_SCRYPT_LOG_N = 11; private static final int PASSWORD_SCRYPT_LOG_R = 3; private static final int PASSWORD_SCRYPT_LOG_P = 1; @@ -379,21 +376,18 @@ class SyntheticPasswordManager { buffer.put(data, 0, data.length); buffer.flip(); - /* - * Originally this file did not contain a version number. However, its first field was - * 'credentialType' as an 'int'. Since 'credentialType' could only be in the range - * [-1, 4] and this file uses big endian byte order, the first two bytes were redundant, - * and when interpreted as a 'short' could only contain -1 or 0. Therefore, we've now - * reclaimed these two bytes for a 'short' version number and shrunk 'credentialType' - * to a 'short'. - */ - short version = buffer.getShort(); - if (version == ((short) 0) || version == (short) -1) { - version = PASSWORD_DATA_V1; - } else if (version != PASSWORD_DATA_V2) { - throw new IllegalArgumentException("Unknown PasswordData version: " + version); - } - result.credentialType = buffer.getShort(); + /* + * The serialized PasswordData is supposed to begin with credentialType as an int. + * However, all credentialType values fit in a short and the byte order is big endian, + * so the first two bytes don't convey any non-redundant information. For this reason, + * temporarily during development of Android 14, the first two bytes were "stolen" from + * credentialType to use for a data format version number. + * + * However, this change was reverted as it was a non-forwards-compatible change. (See + * toBytes() for why this data format needs to be forwards-compatible.) Therefore, + * recover from this misstep by ignoring the first two bytes. + */ + result.credentialType = (short) buffer.getInt(); result.scryptLogN = buffer.get(); result.scryptLogR = buffer.get(); result.scryptLogP = buffer.get(); @@ -407,7 +401,7 @@ class SyntheticPasswordManager { } else { result.passwordHandle = null; } - if (version == PASSWORD_DATA_V2) { + if (buffer.remaining() >= Integer.BYTES) { result.pinLength = buffer.getInt(); } else { result.pinLength = PIN_LENGTH_UNAVAILABLE; @@ -415,16 +409,25 @@ class SyntheticPasswordManager { return result; } + /** + * Serializes this PasswordData into a byte array. + *

+ * Careful: all changes to the format of the serialized PasswordData must be forwards + * compatible. I.e., older versions of Android must still accept the latest PasswordData. + * This is because a serialized PasswordData is stored in the Factory Reset Protection (FRP) + * persistent data block. It's possible that a device has FRP set up on a newer version of + * Android, is factory reset, and then is set up with an older version of Android. + */ public byte[] toBytes() { - ByteBuffer buffer = ByteBuffer.allocate(2 * Short.BYTES + 3 * Byte.BYTES + ByteBuffer buffer = ByteBuffer.allocate(Integer.BYTES + 3 * Byte.BYTES + Integer.BYTES + salt.length + Integer.BYTES + (passwordHandle != null ? passwordHandle.length : 0) + Integer.BYTES); + // credentialType must fit in a short. For an explanation, see fromBytes(). if (credentialType < Short.MIN_VALUE || credentialType > Short.MAX_VALUE) { throw new IllegalArgumentException("Unknown credential type: " + credentialType); } - buffer.putShort(PASSWORD_DATA_V2); - buffer.putShort((short) credentialType); + buffer.putInt(credentialType); buffer.put(scryptLogN); buffer.put(scryptLogR); buffer.put(scryptLogP); diff --git a/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java b/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java index bfb6b0f1b6c77..067feae4d36ae 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/SyntheticPasswordTests.java @@ -23,6 +23,7 @@ import static android.content.pm.UserInfo.FLAG_PRIMARY; import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_NONE; import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_PASSWORD; import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_PASSWORD_OR_PIN; +import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_PIN; import static com.android.internal.widget.LockPatternUtils.PIN_LENGTH_UNAVAILABLE; import static org.junit.Assert.assertEquals; @@ -625,11 +626,9 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { } @Test - public void testPasswordDataV2VersionCredentialTypePin_deserialize() { - // Test that we can deserialize existing PasswordData and don't inadvertently change the - // wire format. + public void testDeserializePasswordData_forPinWithLengthAvailable() { byte[] serialized = new byte[] { - 0, 2, 0, 2, /* CREDENTIAL_TYPE_PASSWORD_OR_PIN */ + 0, 0, 0, 3, /* CREDENTIAL_TYPE_PIN */ 11, /* scryptLogN */ 22, /* scryptLogR */ 33, /* scryptLogP */ @@ -637,25 +636,23 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { 1, 2, -1, -2, 55, /* salt */ 0, 0, 0, 6, /* passwordHandle.length */ 2, 3, -2, -3, 44, 1, /* passwordHandle */ - 0, 0, 0, 5, /* pinLength */ + 0, 0, 0, 6, /* pinLength */ }; PasswordData deserialized = PasswordData.fromBytes(serialized); assertEquals(11, deserialized.scryptLogN); assertEquals(22, deserialized.scryptLogR); assertEquals(33, deserialized.scryptLogP); - assertEquals(5, deserialized.pinLength); - assertEquals(CREDENTIAL_TYPE_PASSWORD_OR_PIN, deserialized.credentialType); + assertEquals(CREDENTIAL_TYPE_PIN, deserialized.credentialType); assertArrayEquals(PAYLOAD, deserialized.salt); assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); + assertEquals(6, deserialized.pinLength); } @Test - public void testPasswordDataV2VersionNegativePinLengthNoCredential_deserialize() { - // Test that we can deserialize existing PasswordData and don't inadvertently change the - // wire format. + public void testDeserializePasswordData_forPinWithLengthExplicitlyUnavailable() { byte[] serialized = new byte[] { - 0, 2, -1, -1, /* CREDENTIAL_TYPE_NONE */ + 0, 0, 0, 3, /* CREDENTIAL_TYPE_PIN */ 11, /* scryptLogN */ 22, /* scryptLogR */ 33, /* scryptLogP */ @@ -663,23 +660,52 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { 1, 2, -1, -2, 55, /* salt */ 0, 0, 0, 6, /* passwordHandle.length */ 2, 3, -2, -3, 44, 1, /* passwordHandle */ - -1, -1, -1, -2, /* pinLength */ + -1, -1, -1, -1, /* pinLength */ }; PasswordData deserialized = PasswordData.fromBytes(serialized); assertEquals(11, deserialized.scryptLogN); assertEquals(22, deserialized.scryptLogR); assertEquals(33, deserialized.scryptLogP); - assertEquals(-2, deserialized.pinLength); - assertEquals(CREDENTIAL_TYPE_NONE, deserialized.credentialType); + assertEquals(CREDENTIAL_TYPE_PIN, deserialized.credentialType); assertArrayEquals(PAYLOAD, deserialized.salt); assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); + assertEquals(PIN_LENGTH_UNAVAILABLE, deserialized.pinLength); } @Test - public void testPasswordDataV1VersionNoCredential_deserialize() { - // Test that we can deserialize existing PasswordData and don't inadvertently change the - // wire format. + public void testDeserializePasswordData_forPinWithVersionNumber() { + // Test deserializing a PasswordData that has a version number in the first two bytes. + // Files like this were created by some Android 14 beta versions. This version number was a + // mistake and should be ignored by the deserializer. + byte[] serialized = new byte[] { + 0, 2, /* version 2 */ + 0, 3, /* CREDENTIAL_TYPE_PIN */ + 11, /* scryptLogN */ + 22, /* scryptLogR */ + 33, /* scryptLogP */ + 0, 0, 0, 5, /* salt.length */ + 1, 2, -1, -2, 55, /* salt */ + 0, 0, 0, 6, /* passwordHandle.length */ + 2, 3, -2, -3, 44, 1, /* passwordHandle */ + 0, 0, 0, 6, /* pinLength */ + }; + PasswordData deserialized = PasswordData.fromBytes(serialized); + + assertEquals(11, deserialized.scryptLogN); + assertEquals(22, deserialized.scryptLogR); + assertEquals(33, deserialized.scryptLogP); + assertEquals(CREDENTIAL_TYPE_PIN, deserialized.credentialType); + assertArrayEquals(PAYLOAD, deserialized.salt); + assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); + assertEquals(6, deserialized.pinLength); + } + + @Test + public void testDeserializePasswordData_forNoneCred() { + // Test that a PasswordData that uses CREDENTIAL_TYPE_NONE and lacks the PIN length field + // can be deserialized. Files like this were created by Android 13 and earlier. Android 14 + // and later no longer create PasswordData for CREDENTIAL_TYPE_NONE. byte[] serialized = new byte[] { -1, -1, -1, -1, /* CREDENTIAL_TYPE_NONE */ 11, /* scryptLogN */ @@ -695,16 +721,17 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { assertEquals(11, deserialized.scryptLogN); assertEquals(22, deserialized.scryptLogR); assertEquals(33, deserialized.scryptLogP); - assertEquals(PIN_LENGTH_UNAVAILABLE, deserialized.pinLength); assertEquals(CREDENTIAL_TYPE_NONE, deserialized.credentialType); assertArrayEquals(PAYLOAD, deserialized.salt); assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); + assertEquals(PIN_LENGTH_UNAVAILABLE, deserialized.pinLength); } @Test - public void testPasswordDataV1VersionCredentialTypePin_deserialize() { - // Test that we can deserialize existing PasswordData and don't inadvertently change the - // wire format. + public void testDeserializePasswordData_forPasswordOrPin() { + // Test that a PasswordData that uses CREDENTIAL_TYPE_PASSWORD_OR_PIN and lacks the PIN + // length field can be deserialized. Files like this were created by Android 10 and + // earlier. Android 11 eliminated CREDENTIAL_TYPE_PASSWORD_OR_PIN. byte[] serialized = new byte[] { 0, 0, 0, 2, /* CREDENTIAL_TYPE_PASSWORD_OR_PIN */ 11, /* scryptLogN */ @@ -720,10 +747,10 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { assertEquals(11, deserialized.scryptLogN); assertEquals(22, deserialized.scryptLogR); assertEquals(33, deserialized.scryptLogP); - assertEquals(PIN_LENGTH_UNAVAILABLE, deserialized.pinLength); assertEquals(CREDENTIAL_TYPE_PASSWORD_OR_PIN, deserialized.credentialType); assertArrayEquals(PAYLOAD, deserialized.salt); assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); + assertEquals(PIN_LENGTH_UNAVAILABLE, deserialized.pinLength); } @Test