From 23070d87275c71748b5fad07ac867d5b94995d83 Mon Sep 17 00:00:00 2001 From: Bhavuk Jain Date: Mon, 13 Mar 2023 10:03:50 +0000 Subject: [PATCH] Added changes for storing PIN length in PasswordData This CL aims at adding changes for storing PIN length in PasswordData class. This would make it more secure and also help us fix any race conditions. Bug: b/267222046 Test: Tested by building and flashing on local Change-Id: I54023c36ab671baeb6c6e6020c78d1f86a00e75e --- .../internal/widget/ILockSettings.aidl | 1 + .../internal/widget/LockPatternChecker.java | 3 - .../internal/widget/LockPatternUtils.java | 33 +++---- .../locksettings/LockSettingsService.java | 36 ++++++-- .../SyntheticPasswordManager.java | 82 +++++++++++++++-- .../locksettings/SyntheticPasswordTests.java | 87 ++++++++++++++++++- 6 files changed, 207 insertions(+), 35 deletions(-) diff --git a/core/java/com/android/internal/widget/ILockSettings.aidl b/core/java/com/android/internal/widget/ILockSettings.aidl index a78145407b9f1..dfcde3dbee894 100644 --- a/core/java/com/android/internal/widget/ILockSettings.aidl +++ b/core/java/com/android/internal/widget/ILockSettings.aidl @@ -56,6 +56,7 @@ interface ILockSettings { VerifyCredentialResponse verifyGatekeeperPasswordHandle(long gatekeeperPasswordHandle, long challenge, int userId); void removeGatekeeperPasswordHandle(long gatekeeperPasswordHandle); int getCredentialType(int userId); + int getPinLength(int userId); byte[] getHashFactor(in LockscreenCredential currentCredential, int userId); void setSeparateProfileChallengeEnabled(int userId, boolean enabled, in LockscreenCredential managedUserPassword); boolean getSeparateProfileChallengeEnabled(int userId); diff --git a/core/java/com/android/internal/widget/LockPatternChecker.java b/core/java/com/android/internal/widget/LockPatternChecker.java index 5c3759f96764e..5adbc583140fe 100644 --- a/core/java/com/android/internal/widget/LockPatternChecker.java +++ b/core/java/com/android/internal/widget/LockPatternChecker.java @@ -117,9 +117,6 @@ public final class LockPatternChecker { @Override protected void onPostExecute(Boolean result) { callback.onChecked(result, mThrottleTimeout); - if (LockPatternUtils.isAutoPinConfirmFeatureAvailable()) { - utils.setPinLength(userId, credentialCopy.size()); - } credentialCopy.zeroize(); } diff --git a/core/java/com/android/internal/widget/LockPatternUtils.java b/core/java/com/android/internal/widget/LockPatternUtils.java index 4d9141052c49d..38632d15e857c 100644 --- a/core/java/com/android/internal/widget/LockPatternUtils.java +++ b/core/java/com/android/internal/widget/LockPatternUtils.java @@ -116,6 +116,12 @@ public class LockPatternUtils { public static final int CREDENTIAL_TYPE_PIN = 3; public static final int CREDENTIAL_TYPE_PASSWORD = 4; + // This is the value of pin length whenever pin length is not available + public static final int PIN_LENGTH_UNAVAILABLE = -1; + + // This is the minimum pin length at which auto confirmation is supported + public static final int MIN_AUTO_PIN_REQUIREMENT_LENGTH = 6; + /** * Header used for the encryption and decryption of the device credential for * remote device lockscreen validation. @@ -177,8 +183,6 @@ public class LockPatternUtils { @Deprecated public final static String LOCKSCREEN_WIDGETS_ENABLED = "lockscreen.widgets_enabled"; - public static final String PIN_LENGTH = "lockscreen.pin_length"; - public final static String PASSWORD_HISTORY_KEY = "lockscreen.passwordhistory"; private static final String LOCK_SCREEN_OWNER_INFO = Settings.Secure.LOCK_SCREEN_OWNER_INFO; @@ -193,7 +197,7 @@ public class LockPatternUtils { private static final String KNOWN_TRUST_AGENTS = "lockscreen.knowntrustagents"; private static final String IS_TRUST_USUALLY_MANAGED = "lockscreen.istrustusuallymanaged"; - private static final String AUTO_PIN_CONFIRM = "lockscreen.auto_pin_confirm"; + public static final String AUTO_PIN_CONFIRM = "lockscreen.auto_pin_confirm"; public static final String CURRENT_LSKF_BASED_PROTECTOR_ID_KEY = "sp-handle"; public static final String PASSWORD_HISTORY_DELIMITER = ","; @@ -603,24 +607,21 @@ public class LockPatternUtils { return getBoolean(PATTERN_EVER_CHOSEN_KEY, false, userId); } - /** - * Used for setting the length of the PIN set by a particular user. - * @param userId user id of the user whose pin length we save - * @param val value of length of pin - */ - public void setPinLength(int userId, long val) { - setLong(PIN_LENGTH, val, userId); - } - /** * Returns the length of the PIN set by a particular user. * @param userId user id of the user whose pin length we have to return - * @return the length of the pin set by user and -1 if nothing + * @return + * A. the length of the pin set by user if it is currently available + * B. PIN_LENGTH_UNAVAILABLE if it is not available or if an exception occurs */ - public long getPinLength(int userId) { - return getLong(PIN_LENGTH, -1, userId); + public int getPinLength(int userId) { + try { + return getLockSettings().getPinLength(userId); + } catch (RemoteException e) { + Log.e(TAG, "Could not fetch PIN length " + e); + return PIN_LENGTH_UNAVAILABLE; + } } - /** * Records that the user has chosen a pattern at some time, even if the pattern is * currently cleared. diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index 2b5f874156ee7..0da94ff6aa0fa 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -36,6 +36,7 @@ import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_PASSW import static com.android.internal.widget.LockPatternUtils.CREDENTIAL_TYPE_PIN; import static com.android.internal.widget.LockPatternUtils.CURRENT_LSKF_BASED_PROTECTOR_ID_KEY; import static com.android.internal.widget.LockPatternUtils.EscrowTokenStateChangeCallback; +import static com.android.internal.widget.LockPatternUtils.PIN_LENGTH_UNAVAILABLE; import static com.android.internal.widget.LockPatternUtils.StrongAuthTracker.STRONG_AUTH_REQUIRED_AFTER_LOCKOUT; import static com.android.internal.widget.LockPatternUtils.StrongAuthTracker.STRONG_AUTH_REQUIRED_FOR_UNATTENDED_UPDATE; import static com.android.internal.widget.LockPatternUtils.USER_FRP; @@ -1202,6 +1203,31 @@ public class LockSettingsService extends ILockSettings.Stub { DevicePolicyManager.PASSWORD_QUALITY_UNSPECIFIED, userId); } + /* + * Gets the PIN length for the given user if it is currently available. + * Can only be invoked by process/activity that have the right permission. + * Returns: + * A. Actual PIN length if credential type PIN and auto confirm feature is enabled + * for the user or user's PIN has been successfully verified since the device booted + * B. PIN_LENGTH_UNAVAILABLE if pin length is not stored/available + */ + @Override + public int getPinLength(int userId) { + checkPasswordHavePermission(); + PasswordMetrics passwordMetrics = getUserPasswordMetrics(userId); + if (passwordMetrics != null && passwordMetrics.credType == CREDENTIAL_TYPE_PIN) { + return passwordMetrics.length; + } + synchronized (mSpManager) { + final long protectorId = getCurrentLskfBasedProtectorId(userId); + if (protectorId == SyntheticPasswordManager.NULL_PROTECTOR_ID) { + // Only possible for new users during early boot (before onThirdPartyAppsStarted()) + return PIN_LENGTH_UNAVAILABLE; + } + return mSpManager.getPinLength(protectorId, userId); + } + } + /** * This API is cached; whenever the result would change, * {@link com.android.internal.widget.LockPatternUtils#invalidateCredentialTypeCache} @@ -1683,11 +1709,6 @@ public class LockSettingsService extends ILockSettings.Stub { if (newCredential.isPattern()) { setBoolean(LockPatternUtils.PATTERN_EVER_CHOSEN_KEY, true, userHandle); } - if (LockPatternUtils.isAutoPinConfirmFeatureAvailable()) { - if (newCredential.isPin()) { - setLong(LockPatternUtils.PIN_LENGTH, newCredential.size(), userHandle); - } - } updatePasswordHistory(newCredential, userHandle); mContext.getSystemService(TrustManager.class).reportEnabledTrustAgentsChanged(userHandle); @@ -2229,6 +2250,11 @@ public class LockSettingsService extends ILockSettings.Stub { } } + /** + * Returns the PasswordMetrics for the current user + * @param userHandle The id of the user for which we return the password metrics object + * @return passwordmetrics for the user or null if not available + */ @VisibleForTesting PasswordMetrics getUserPasswordMetrics(int userHandle) { if (!isUserSecure(userHandle)) { diff --git a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java index 1663b019d769a..a4dab729250d4 100644 --- a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java +++ b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java @@ -17,6 +17,7 @@ package com.android.server.locksettings; import static com.android.internal.widget.LockPatternUtils.EscrowTokenStateChangeCallback; +import static com.android.internal.widget.LockPatternUtils.PIN_LENGTH_UNAVAILABLE; import android.annotation.IntDef; import android.annotation.NonNull; @@ -150,6 +151,9 @@ 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; @@ -351,13 +355,19 @@ class SyntheticPasswordManager { // When Weaver is unavailable, this is the Gatekeeper password handle that resulted from // enrolling the stretched LSKF. public byte[] passwordHandle; + /** + * Pin length field, only stored in version 2 of the password data and when auto confirm + * flag is enabled, otherwise this field contains PIN_LENGTH_UNAVAILABLE + */ + public int pinLength; - public static PasswordData create(int credentialType) { + public static PasswordData create(int credentialType, int pinLength) { PasswordData result = new PasswordData(); result.scryptLogN = PASSWORD_SCRYPT_LOG_N; result.scryptLogR = PASSWORD_SCRYPT_LOG_R; result.scryptLogP = PASSWORD_SCRYPT_LOG_P; result.credentialType = credentialType; + result.pinLength = pinLength; result.salt = SecureRandomUtils.randomBytes(PASSWORD_SALT_LENGTH); return result; } @@ -367,7 +377,22 @@ class SyntheticPasswordManager { ByteBuffer buffer = ByteBuffer.allocate(data.length); buffer.put(data, 0, data.length); buffer.flip(); - result.credentialType = buffer.getInt(); + + /* + * 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(); result.scryptLogN = buffer.get(); result.scryptLogR = buffer.get(); result.scryptLogP = buffer.get(); @@ -381,15 +406,24 @@ class SyntheticPasswordManager { } else { result.passwordHandle = null; } + if (version == PASSWORD_DATA_V2) { + result.pinLength = buffer.getInt(); + } else { + result.pinLength = PIN_LENGTH_UNAVAILABLE; + } return result; } public byte[] toBytes() { - ByteBuffer buffer = ByteBuffer.allocate(Integer.BYTES + 3 * Byte.BYTES + ByteBuffer buffer = ByteBuffer.allocate(2 * Short.BYTES + 3 * Byte.BYTES + Integer.BYTES + salt.length + Integer.BYTES + - (passwordHandle != null ? passwordHandle.length : 0)); - buffer.putInt(credentialType); + (passwordHandle != null ? passwordHandle.length : 0) + Integer.BYTES); + 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.put(scryptLogN); buffer.put(scryptLogR); buffer.put(scryptLogP); @@ -401,6 +435,7 @@ class SyntheticPasswordManager { } else { buffer.putInt(0); } + buffer.putInt(pinLength); return buffer.array(); } } @@ -649,6 +684,14 @@ class SyntheticPasswordManager { } } + int getPinLength(long protectorId, int userId) { + byte[] passwordData = loadState(PASSWORD_DATA_NAME, protectorId, userId); + if (passwordData == null) { + return LockPatternUtils.PIN_LENGTH_UNAVAILABLE; + } + return PasswordData.fromBytes(passwordData).pinLength; + } + int getCredentialType(long protectorId, int userId) { byte[] passwordData = loadState(PASSWORD_DATA_NAME, protectorId, userId); if (passwordData == null) { @@ -857,8 +900,13 @@ class SyntheticPasswordManager { public long createLskfBasedProtector(IGateKeeperService gatekeeper, LockscreenCredential credential, SyntheticPassword sp, int userId) { long protectorId = generateProtectorId(); + int pinLength = PIN_LENGTH_UNAVAILABLE; + if (LockPatternUtils.isAutoPinConfirmFeatureAvailable()) { + pinLength = derivePinLength(credential, userId); + } // There's no need to store password data about an empty LSKF. - PasswordData pwd = credential.isNone() ? null : PasswordData.create(credential.getType()); + PasswordData pwd = credential.isNone() ? null : + PasswordData.create(credential.getType(), pinLength); byte[] stretchedLskf = stretchLskf(credential, pwd); long sid = GateKeeper.INVALID_SECURE_USER_ID; final byte[] protectorSecret; @@ -930,6 +978,15 @@ class SyntheticPasswordManager { return protectorId; } + private int derivePinLength(LockscreenCredential credential, int userId) { + if (!credential.isPin() + || !mStorage.getBoolean(LockPatternUtils.AUTO_PIN_CONFIRM, false, userId) + || credential.size() < LockPatternUtils.MIN_AUTO_PIN_REQUIREMENT_LENGTH) { + return PIN_LENGTH_UNAVAILABLE; + } + return credential.size(); + } + public VerifyCredentialResponse verifyFrpCredential(IGateKeeperService gatekeeper, LockscreenCredential userCredential, ICheckCredentialProgressCallback progressCallback) { @@ -1285,11 +1342,20 @@ class SyntheticPasswordManager { // Upgrade case: store the metrics if the device did not have stored metrics before, should // only happen once on old protectors. - if (result.syntheticPassword != null && !credential.isNone() && - !hasPasswordMetrics(protectorId, userId)) { + if (result.syntheticPassword != null && !credential.isNone() + && !hasPasswordMetrics(protectorId, userId)) { savePasswordMetrics(credential, result.syntheticPassword, protectorId, userId); syncState(userId); // Not strictly needed as the upgrade can be re-done, but be safe. } + if (LockPatternUtils.isAutoPinConfirmFeatureAvailable() + && result.syntheticPassword != null && pwd != null) { + int expectedPinLength = derivePinLength(credential, userId); + if (pwd.pinLength != expectedPinLength) { + pwd.pinLength = expectedPinLength; + saveState(PASSWORD_DATA_NAME, pwd.toBytes(), protectorId, userId); + syncState(userId); + } + } return result; } 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 62d8a76452366..bdc5be6d3714d 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.PIN_LENGTH_UNAVAILABLE; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -579,12 +580,13 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { data.scryptLogN = 11; data.scryptLogR = 3; data.scryptLogP = 1; + data.pinLength = 5; data.salt = "abcdefghijklmnop".getBytes(); return data; } @Test - public void testPasswordData_serializeDeserialize() { + public void testPasswordDataLatestVersion_serializeDeserialize() { PasswordData data = new PasswordData(); data.scryptLogN = 11; data.scryptLogR = 22; @@ -592,19 +594,97 @@ public class SyntheticPasswordTests extends BaseLockSettingsServiceTests { data.credentialType = CREDENTIAL_TYPE_PASSWORD; data.salt = PAYLOAD; data.passwordHandle = PAYLOAD2; + data.pinLength = 5; PasswordData deserialized = PasswordData.fromBytes(data.toBytes()); - assertEquals(11, deserialized.scryptLogN); assertEquals(22, deserialized.scryptLogR); assertEquals(33, deserialized.scryptLogP); + assertEquals(5, deserialized.pinLength); assertEquals(CREDENTIAL_TYPE_PASSWORD, deserialized.credentialType); assertArrayEquals(PAYLOAD, deserialized.salt); assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); } @Test - public void testPasswordData_deserialize() { + public void testPasswordDataV2VersionCredentialTypePin_deserialize() { + // Test that we can deserialize existing PasswordData and don't inadvertently change the + // wire format. + byte[] serialized = new byte[] { + 0, 2, 0, 2, /* CREDENTIAL_TYPE_PASSWORD_OR_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, 5, /* 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); + assertArrayEquals(PAYLOAD, deserialized.salt); + assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); + } + + @Test + public void testPasswordDataV2VersionNegativePinLengthNoCredential_deserialize() { + // Test that we can deserialize existing PasswordData and don't inadvertently change the + // wire format. + byte[] serialized = new byte[] { + 0, 2, -1, -1, /* CREDENTIAL_TYPE_NONE */ + 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 */ + -1, -1, -1, -2, /* 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); + assertArrayEquals(PAYLOAD, deserialized.salt); + assertArrayEquals(PAYLOAD2, deserialized.passwordHandle); + } + + @Test + public void testPasswordDataV1VersionNoCredential_deserialize() { + // Test that we can deserialize existing PasswordData and don't inadvertently change the + // wire format. + byte[] serialized = new byte[] { + -1, -1, -1, -1, /* CREDENTIAL_TYPE_NONE */ + 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 */ + }; + PasswordData deserialized = PasswordData.fromBytes(serialized); + + 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); + } + + @Test + public void testPasswordDataV1VersionCredentialTypePin_deserialize() { // Test that we can deserialize existing PasswordData and don't inadvertently change the // wire format. byte[] serialized = new byte[] { @@ -622,6 +702,7 @@ 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);