From 18a1915f049e8f114ccebe217fffef677bfb30a7 Mon Sep 17 00:00:00 2001 From: Andrew Scull Date: Mon, 24 Apr 2023 19:09:09 +0000 Subject: [PATCH] Strictly deprecate IRPC test mode key generation It's already documented that IRPC v3 doesn't make use of test mode keys however VTS still required support for their generation. Fix this and simplify implementation of the v3 HAL by expecting an error in all cases that the deprecated test mode keys are seen. IRPC v3 also fully deprecated the EEK meaning a v3 implementation must unconditionally report CURVE_NONE for supportedEekCurve. The VTS tests are enhanced with contextual version constants rather than reusing constants with seemingly unrelated names. Bug: 278013975 Test: atest VtsHalRemotelyProvisionedComponentTargetTest (cherry picked from https://android-review.googlesource.com/q/commit:f2ae193680d6f02a2394423f805aadd13a7d152b) Merged-In: I5709a0b1cd77eb28e677f64bb781fad58d91570a Change-Id: I5709a0b1cd77eb28e677f64bb781fad58d91570a --- .../IRemotelyProvisionedComponent.aidl | 4 ++ .../VtsRemotelyProvisionedComponentTests.cpp | 59 ++++++++++--------- 2 files changed, 35 insertions(+), 28 deletions(-) diff --git a/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl b/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl index 461357d4e3..2a4cba1f0e 100644 --- a/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl +++ b/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl @@ -134,6 +134,10 @@ interface IRemotelyProvisionedComponent { * are marked (see the definition of PublicKey in the MacedPublicKey structure) to * prevent them from being confused with production keys. * + * This parameter has been deprecated since version 3 of the HAL and will always be + * false. From v3, if this parameter is true, the method must raise a + * ServiceSpecificException with an error of code of STATUS_REMOVED. + * * @param out MacedPublicKey macedPublicKey contains the public key of the generated key pair, * MACed so that generateCertificateRequest can easily verify, without the * privateKeyHandle, that the contained public key is for remote certification. diff --git a/security/rkp/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/rkp/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp index 94bfbb48de..8906543e7f 100644 --- a/security/rkp/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp +++ b/security/rkp/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp @@ -47,7 +47,11 @@ using ::std::vector; namespace { constexpr int32_t VERSION_WITH_UNIQUE_ID_SUPPORT = 2; + +constexpr int32_t VERSION_WITHOUT_EEK = 3; constexpr int32_t VERSION_WITHOUT_TEST_MODE = 3; +constexpr int32_t VERSION_WITH_CERTIFICATE_REQUEST_V2 = 3; +constexpr int32_t VERSION_WITH_SUPPORTED_NUM_KEYS_IN_CSR = 3; constexpr uint8_t MIN_CHALLENGE_SIZE = 0; constexpr uint8_t MAX_CHALLENGE_SIZE = 64; @@ -226,21 +230,13 @@ TEST_P(GetHardwareInfoTests, supportsValidCurve) { RpcHardwareInfo hwInfo; ASSERT_TRUE(provisionable_->getHardwareInfo(&hwInfo).isOk()); - const std::set validCurves = {RpcHardwareInfo::CURVE_P256, RpcHardwareInfo::CURVE_25519}; - // First check for the implementations that supports only IRPC V3+. - if (rpcHardwareInfo.versionNumber >= VERSION_WITHOUT_TEST_MODE) { - bytevec keysToSignMac; - DeviceInfo deviceInfo; - ProtectedData protectedData; - auto status = provisionable_->generateCertificateRequest(false, {}, {}, {}, &deviceInfo, - &protectedData, &keysToSignMac); - if (!status.isOk() && - (status.getServiceSpecificError() == BnRemotelyProvisionedComponent::STATUS_REMOVED)) { - ASSERT_EQ(hwInfo.supportedEekCurve, RpcHardwareInfo::CURVE_NONE) - << "Invalid curve: " << hwInfo.supportedEekCurve; - return; - } + if (rpcHardwareInfo.versionNumber >= VERSION_WITHOUT_EEK) { + ASSERT_EQ(hwInfo.supportedEekCurve, RpcHardwareInfo::CURVE_NONE) + << "Invalid curve: " << hwInfo.supportedEekCurve; + return; } + + const std::set validCurves = {RpcHardwareInfo::CURVE_P256, RpcHardwareInfo::CURVE_25519}; ASSERT_EQ(validCurves.count(hwInfo.supportedEekCurve), 1) << "Invalid curve: " << hwInfo.supportedEekCurve; } @@ -264,7 +260,7 @@ TEST_P(GetHardwareInfoTests, uniqueId) { * Verify implementation supports at least MIN_SUPPORTED_NUM_KEYS_IN_CSR keys in a CSR. */ TEST_P(GetHardwareInfoTests, supportedNumKeysInCsr) { - if (rpcHardwareInfo.versionNumber < VERSION_WITHOUT_TEST_MODE) { + if (rpcHardwareInfo.versionNumber < VERSION_WITH_SUPPORTED_NUM_KEYS_IN_CSR) { return; } @@ -365,6 +361,13 @@ TEST_P(GenerateKeyTests, generateEcdsaP256Key_testMode) { bytevec privateKeyBlob; bool testMode = true; auto status = provisionable_->generateEcdsaP256KeyPair(testMode, &macedPubKey, &privateKeyBlob); + + if (rpcHardwareInfo.versionNumber >= VERSION_WITHOUT_TEST_MODE) { + ASSERT_FALSE(status.isOk()); + EXPECT_EQ(status.getServiceSpecificError(), BnRemotelyProvisionedComponent::STATUS_REMOVED); + return; + } + ASSERT_TRUE(status.isOk()); check_maced_pubkey(macedPubKey, testMode, nullptr); } @@ -410,7 +413,7 @@ class CertificateRequestTest : public CertificateRequestTestBase { CertificateRequestTestBase::SetUp(); ASSERT_FALSE(HasFatalFailure()); - if (rpcHardwareInfo.versionNumber >= VERSION_WITHOUT_TEST_MODE) { + if (rpcHardwareInfo.versionNumber >= VERSION_WITH_CERTIFICATE_REQUEST_V2) { GTEST_SKIP() << "This test case only applies to RKP v1 and v2. " << "RKP version discovered: " << rpcHardwareInfo.versionNumber; } @@ -688,7 +691,7 @@ class CertificateRequestV2Test : public CertificateRequestTestBase { CertificateRequestTestBase::SetUp(); ASSERT_FALSE(HasFatalFailure()); - if (rpcHardwareInfo.versionNumber < VERSION_WITHOUT_TEST_MODE) { + if (rpcHardwareInfo.versionNumber < VERSION_WITH_CERTIFICATE_REQUEST_V2) { GTEST_SKIP() << "This test case only applies to RKP v3 and above. " << "RKP version discovered: " << rpcHardwareInfo.versionNumber; } @@ -802,23 +805,23 @@ TEST_P(CertificateRequestV2Test, NonEmptyRequestCorruptMac) { } /** - * Generate a non-empty certificate request in prod mode, with test keys. Must fail with - * STATUS_TEST_KEY_IN_PRODUCTION_REQUEST. + * Call generateCertificateRequest(). Make sure it's removed. */ -TEST_P(CertificateRequestV2Test, NonEmptyRequest_testKeyInProdCert) { - generateKeys(true /* testMode */, 1 /* numKeys */); - - bytevec csr; - auto status = provisionable_->generateCertificateRequestV2(keysToSign_, challenge_, &csr); +TEST_P(CertificateRequestV2Test, CertificateRequestV1Removed_prodMode) { + bytevec keysToSignMac; + DeviceInfo deviceInfo; + ProtectedData protectedData; + auto status = provisionable_->generateCertificateRequest( + false /* testMode */, {} /* keysToSign */, {} /* EEK chain */, challenge_, &deviceInfo, + &protectedData, &keysToSignMac); ASSERT_FALSE(status.isOk()) << status.getMessage(); - ASSERT_EQ(status.getServiceSpecificError(), - BnRemotelyProvisionedComponent::STATUS_TEST_KEY_IN_PRODUCTION_REQUEST); + EXPECT_EQ(status.getServiceSpecificError(), BnRemotelyProvisionedComponent::STATUS_REMOVED); } /** - * Call generateCertificateRequest(). Make sure it's removed. + * Call generateCertificateRequest() in test mode. Make sure it's removed. */ -TEST_P(CertificateRequestV2Test, CertificateRequestV1Removed) { +TEST_P(CertificateRequestV2Test, CertificateRequestV1Removed_testMode) { bytevec keysToSignMac; DeviceInfo deviceInfo; ProtectedData protectedData;