From df32f52a471c94b0f1ae1a16422dc56c77184075 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 16 Feb 2024 20:15:59 +0000 Subject: [PATCH] keymint: document deprecation of UNLOCKED_DEVICE_REQUIRED enforcement KeyMint enforcement of UNLOCKED_DEVICE_REQUIRED is broken, has never been used, and cannot be fixed. So, document that it does not need to be implemented. Also remove the VTS test for it, which was disabled. UNLOCKED_DEVICE_REQUIRED remains supported in Keystore. Bug: 321100166 Test: Build Change-Id: If4d47ee49c9d4a595820cfceb0f5f3027f99ee9f --- .../security/keymint/IKeyMintDevice.aidl | 55 +++++++++++-------- .../hardware/security/keymint/Tag.aidl | 9 +-- .../aidl/vts/functional/KeyMintTest.cpp | 34 ------------ 3 files changed, 36 insertions(+), 62 deletions(-) diff --git a/security/keymint/aidl/android/hardware/security/keymint/IKeyMintDevice.aidl b/security/keymint/aidl/android/hardware/security/keymint/IKeyMintDevice.aidl index aeb0163977..4ebafee126 100644 --- a/security/keymint/aidl/android/hardware/security/keymint/IKeyMintDevice.aidl +++ b/security/keymint/aidl/android/hardware/security/keymint/IKeyMintDevice.aidl @@ -794,33 +794,40 @@ interface IKeyMintDevice { in @nullable HardwareAuthToken authToken); /** - * Called by client to notify the IKeyMintDevice that the device is now locked, and keys with - * the UNLOCKED_DEVICE_REQUIRED tag should no longer be usable. When this function is called, - * the IKeyMintDevice should note the current timestamp, and attempts to use - * UNLOCKED_DEVICE_REQUIRED keys must be rejected with Error::DEVICE_LOCKED until an - * authentication token with a later timestamp is presented. If the `passwordOnly' argument is - * set to true the sufficiently-recent authentication token must indicate that the user - * authenticated with a password, not a biometric. + * This method is deprecated and has never been used. Implementations should return + * ErrorCode::UNIMPLEMENTED. * - * Note that the IKeyMintDevice UNLOCKED_DEVICE_REQUIRED semantics are slightly different from - * the UNLOCKED_DEVICE_REQUIRED semantics enforced by keystore. Keystore handles device locking - * on a per-user basis. Because auth tokens do not contain an Android user ID, it's not - * possible to replicate the keystore enforcement logic in IKeyMintDevice. So from the - * IKeyMintDevice perspective, any user unlock unlocks all UNLOCKED_DEVICE_REQUIRED keys. - * Keystore will continue enforcing the per-user device locking. + * This method was originally intended to be used to notify KeyMint that the device is now + * locked, and keys with the UNLOCKED_DEVICE_REQUIRED tag should no longer be usable until a + * later valid HardwareAuthToken is presented. However, Android has never called this method + * and it cannot start doing so, because KeyMint's enforcement of UNLOCKED_DEVICE_REQUIRED did + * not provide the correct semantics and therefore could never be enabled. Specifically, the + * following issues existed with the design of KeyMint's enforcement of + * UNLOCKED_DEVICE_REQUIRED: * - * @param passwordOnly specifies whether the device must be unlocked with a password, rather - * than a biometric, before UNLOCKED_DEVICE_REQUIRED keys can be used. + * o It assumed a global device lock state only.  Android actually has a separate lock state for + * each user. See the javadoc for KeyguardManager#isDeviceLocked(). + * o It assumed that unlocking the device involves a successful user authentication that + * generates a HardwareAuthToken. This is not necessarily the case, since Android supports + * weaker unlock methods including class 1 and 2 biometrics and trust agents. These unlock + * methods do not generate a HardwareAuthToken or interact with KeyMint in any way. Also, + * UNLOCKED_DEVICE_REQUIRED must work even for users who do not have a secure lock screen. + * o It would have made UNLOCKED_DEVICE_REQUIRED incompatible with requiring user + * authentication in some cases. These two key protections can each require a different + * HardwareAuthToken, but KeyMint only supports one HardwareAuthToken per operation. + * o It would have provided no security benefit over Keystore's enforcement of + * UNLOCKED_DEVICE_REQUIRED. This is because since Android 12, Keystore enforces + * UNLOCKED_DEVICE_REQUIRED not just logically, but it also cryptographically by + * superencrypting all such keys and wiping or re-encrypting the superencryption key when the + * device is locked (whenever possible). KeyMint is still used to support biometric unlocks, + * but this mechanism does not use KeyMint's direct enforcement of UNLOCKED_DEVICE_REQUIRED. * - * @param timestampToken is used by StrongBox implementations of IKeyMintDevice. It - * provides the StrongBox IKeyMintDevice with a fresh, MACed timestamp which it can use as the - * device-lock time, for future comparison against auth tokens when operations using - * UNLOCKED_DEVICE_REQUIRED keys are attempted. Unless the auth token timestamp is newer than - * the timestamp in the timestampToken, the device is still considered to be locked. - * Crucially, if a StrongBox IKeyMintDevice receives a deviceLocked() call with a timestampToken - * timestamp that is less than the timestamp in the last deviceLocked() call, it must ignore the - * new timestamp. TEE IKeyMintDevice implementations will receive an empty timestampToken (zero - * values and empty vectors) and should use their own clock as the device-lock time. + * Therefore, this method is not useful, and there is no reason for it be called. + * Implementations should return ErrorCode::UNIMPLEMENTED and should not include + * UNLOCKED_DEVICE_REQUIRED in the list of hardware-enforced key parameters. + * + * @param passwordOnly N/A due to the deprecation + * @param timestampToken N/A due to the deprecation */ void deviceLocked(in boolean passwordOnly, in @nullable TimeStampToken timestampToken); diff --git a/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl b/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl index be29f59e65..996e4e3479 100644 --- a/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl +++ b/security/keymint/aidl/android/hardware/security/keymint/Tag.aidl @@ -482,11 +482,12 @@ enum Tag { /** * Tag::UNLOCKED_DEVICE_REQUIRED specifies that the key may only be used when the device is - * unlocked, as reported to KeyMint via authToken operation parameter and the - * IKeyMintDevice::deviceLocked() method + * unlocked. * - * Must be hardware-enforced (but is also keystore-enforced on a per-user basis: see the - * deviceLocked() documentation). + * This tag was originally intended to be hardware-enforced. However, the support for hardware + * enforcement of this tag is now considered deprecated because it cannot work correctly, and + * even if implemented it does nothing because it was never enabled by Keystore. Refer to the + * documentation for the deprecated method IKeyMintDevice::deviceLocked(). */ UNLOCKED_DEVICE_REQUIRED = TagType.BOOL | 509, diff --git a/security/keymint/aidl/vts/functional/KeyMintTest.cpp b/security/keymint/aidl/vts/functional/KeyMintTest.cpp index 0b7627c5b2..a8f41c3d6a 100644 --- a/security/keymint/aidl/vts/functional/KeyMintTest.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintTest.cpp @@ -8760,40 +8760,6 @@ TEST_P(EarlyBootKeyTest, DISABLED_FullTest) { INSTANTIATE_KEYMINT_AIDL_TEST(EarlyBootKeyTest); -using UnlockedDeviceRequiredTest = KeyMintAidlTestBase; - -// This may be a problematic test. It can't be run repeatedly without unlocking the device in -// between runs... and on most test devices there are no enrolled credentials so it can't be -// unlocked at all, meaning the only way to get the test to pass again on a properly-functioning -// device is to reboot it. For that reason, this is disabled by default. It can be used as part of -// a manual test process, which includes unlocking between runs, which is why it's included here. -// Well, that and the fact that it's the only test we can do without also making calls into the -// Gatekeeper HAL. We haven't written any cross-HAL tests, and don't know what all of the -// implications might be, so that may or may not be a solution. -TEST_P(UnlockedDeviceRequiredTest, DISABLED_KeysBecomeUnusable) { - auto [aesKeyData, hmacKeyData, rsaKeyData, ecdsaKeyData] = - CreateTestKeys(TAG_UNLOCKED_DEVICE_REQUIRED, ErrorCode::OK); - KeyBlobDeleter aes_deleter(keymint_, aesKeyData.blob); - KeyBlobDeleter hmac_deleter(keymint_, hmacKeyData.blob); - KeyBlobDeleter rsa_deleter(keymint_, rsaKeyData.blob); - KeyBlobDeleter ecdsa_deleter(keymint_, ecdsaKeyData.blob); - - EXPECT_EQ(ErrorCode::OK, UseAesKey(aesKeyData.blob)); - EXPECT_EQ(ErrorCode::OK, UseHmacKey(hmacKeyData.blob)); - EXPECT_EQ(ErrorCode::OK, UseRsaKey(rsaKeyData.blob)); - EXPECT_EQ(ErrorCode::OK, UseEcdsaKey(ecdsaKeyData.blob)); - - ErrorCode rc = GetReturnErrorCode( - keyMint().deviceLocked(false /* passwordOnly */, {} /* timestampToken */)); - ASSERT_EQ(ErrorCode::OK, rc); - EXPECT_EQ(ErrorCode::DEVICE_LOCKED, UseAesKey(aesKeyData.blob)); - EXPECT_EQ(ErrorCode::DEVICE_LOCKED, UseHmacKey(hmacKeyData.blob)); - EXPECT_EQ(ErrorCode::DEVICE_LOCKED, UseRsaKey(rsaKeyData.blob)); - EXPECT_EQ(ErrorCode::DEVICE_LOCKED, UseEcdsaKey(ecdsaKeyData.blob)); -} - -INSTANTIATE_KEYMINT_AIDL_TEST(UnlockedDeviceRequiredTest); - using VsrRequirementTest = KeyMintAidlTestBase; // @VsrTest = VSR-3.10-008