From f6fc5a63363d94e2ac2f3460ad852e0c737c9b1e Mon Sep 17 00:00:00 2001 From: David Drysdale Date: Wed, 31 Mar 2021 16:14:31 +0100 Subject: [PATCH 1/2] Fix DeviceInfo encoding and checks - Make the default implementation include the DeviceInfo as a map, not a bstr-holding-a-map, to match the spec. - Check the signature of the signed MAC even in test mode. - Include the DeviceInfo in the data that the signature covers. Test: VtsHalRemotelyProvisionedComponentTargetTest Change-Id: I9084343c1273c16a9cbd5a1156e7057a1c54a860 --- .../default/RemotelyProvisionedComponent.cpp | 25 ++++++++++++++++--- .../default/RemotelyProvisionedComponent.h | 2 +- .../VtsRemotelyProvisionedComponentTests.cpp | 17 ++++++++----- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/security/keymint/aidl/default/RemotelyProvisionedComponent.cpp b/security/keymint/aidl/default/RemotelyProvisionedComponent.cpp index ca06abc48e..5b027292fe 100644 --- a/security/keymint/aidl/default/RemotelyProvisionedComponent.cpp +++ b/security/keymint/aidl/default/RemotelyProvisionedComponent.cpp @@ -358,12 +358,13 @@ ScopedAStatus RemotelyProvisionedComponent::generateCertificateRequest( bcc = bcc_.clone(); } - deviceInfo->deviceInfo = createDeviceInfo(); + std::unique_ptr deviceInfoMap = createDeviceInfo(); + deviceInfo->deviceInfo = deviceInfoMap->encode(); auto signedMac = constructCoseSign1(devicePrivKey /* Signing key */, // ephemeralMacKey /* Payload */, cppbor::Array() /* AAD */ .add(challenge) - .add(deviceInfo->deviceInfo) + .add(std::move(deviceInfoMap)) .encode()); if (!signedMac) return Status(signedMac.moveMessage()); @@ -409,8 +410,24 @@ bytevec RemotelyProvisionedComponent::deriveBytesFromHbk(const string& context, return result; } -bytevec RemotelyProvisionedComponent::createDeviceInfo() const { - return cppbor::Map().encode(); +std::unique_ptr RemotelyProvisionedComponent::createDeviceInfo() const { + auto result = std::make_unique(cppbor::Map()); + + // The following placeholders show how the DeviceInfo map would be populated. + // result->add(cppbor::Tstr("brand"), cppbor::Tstr("Google")); + // result->add(cppbor::Tstr("manufacturer"), cppbor::Tstr("Google")); + // result->add(cppbor::Tstr("product"), cppbor::Tstr("Fake")); + // result->add(cppbor::Tstr("model"), cppbor::Tstr("Imaginary")); + // result->add(cppbor::Tstr("board"), cppbor::Tstr("Chess")); + // result->add(cppbor::Tstr("vb_state"), cppbor::Tstr("orange")); + // result->add(cppbor::Tstr("bootloader_state"), cppbor::Tstr("unlocked")); + // result->add(cppbor::Tstr("os_version"), cppbor::Tstr("SC")); + // result->add(cppbor::Tstr("system_patch_level"), cppbor::Uint(20210331)); + // result->add(cppbor::Tstr("boot_patch_level"), cppbor::Uint(20210331)); + // result->add(cppbor::Tstr("vendor_patch_level"), cppbor::Uint(20210331)); + + result->canonicalize(); + return result; } std::pair diff --git a/security/keymint/aidl/default/RemotelyProvisionedComponent.h b/security/keymint/aidl/default/RemotelyProvisionedComponent.h index 65b1bbc87e..8185e26e1f 100644 --- a/security/keymint/aidl/default/RemotelyProvisionedComponent.h +++ b/security/keymint/aidl/default/RemotelyProvisionedComponent.h @@ -45,7 +45,7 @@ class RemotelyProvisionedComponent : public BnRemotelyProvisionedComponent { private: // TODO(swillden): Move these into an appropriate Context class. std::vector deriveBytesFromHbk(const std::string& context, size_t numBytes) const; - std::vector createDeviceInfo() const; + std::unique_ptr createDeviceInfo() const; std::pair, cppbor::Array> generateBcc(); std::vector macKey_ = deriveBytesFromHbk("Key to MAC public keys", 32); diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp index e4c4a22243..516be3bdde 100644 --- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp +++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp @@ -370,7 +370,7 @@ class CertificateRequestTest : public VtsRemotelyProvisionedComponentTests { } } - void checkProtectedData(bool testMode, const cppbor::Array& keysToSign, + void checkProtectedData(const DeviceInfo& deviceInfo, const cppbor::Array& keysToSign, const bytevec& keysToSignMac, const ProtectedData& protectedData) { auto [parsedProtectedData, _, protDataErrMsg] = cppbor::parse(protectedData.protectedData); ASSERT_TRUE(parsedProtectedData) << protDataErrMsg; @@ -404,11 +404,16 @@ class CertificateRequestTest : public VtsRemotelyProvisionedComponentTests { ASSERT_TRUE(bccContents) << "\n" << bccContents.message() << "\n" << prettyPrint(bcc.get()); ASSERT_GT(bccContents->size(), 0U); + auto [deviceInfoMap, __2, deviceInfoErrMsg] = cppbor::parse(deviceInfo.deviceInfo); + ASSERT_TRUE(deviceInfoMap) << "Failed to parse deviceInfo: " << deviceInfoErrMsg; + ASSERT_TRUE(deviceInfoMap->asMap()); + auto& signingKey = bccContents->back().pubKey; - auto macKey = verifyAndParseCoseSign1(testMode, signedMac->asArray(), signingKey, - cppbor::Array() // DeviceInfo + auto macKey = verifyAndParseCoseSign1(/* ignore_signature = */ false, signedMac->asArray(), + signingKey, + cppbor::Array() // SignedMacAad .add(challenge_) - .add(cppbor::Map()) + .add(std::move(deviceInfoMap)) .encode()); ASSERT_TRUE(macKey) << macKey.message(); @@ -451,7 +456,7 @@ TEST_P(CertificateRequestTest, EmptyRequest_testMode) { &protectedData, &keysToSignMac); ASSERT_TRUE(status.isOk()) << status.getMessage(); - checkProtectedData(testMode, cppbor::Array(), keysToSignMac, protectedData); + checkProtectedData(deviceInfo, cppbor::Array(), keysToSignMac, protectedData); } } @@ -499,7 +504,7 @@ TEST_P(CertificateRequestTest, NonEmptyRequest_testMode) { &keysToSignMac); ASSERT_TRUE(status.isOk()) << status.getMessage(); - checkProtectedData(testMode, cborKeysToSign_, keysToSignMac, protectedData); + checkProtectedData(deviceInfo, cborKeysToSign_, keysToSignMac, protectedData); } } From 4d3c298c6605ef3629f613fb2850b882d5516223 Mon Sep 17 00:00:00 2001 From: David Drysdale Date: Wed, 31 Mar 2021 18:21:40 +0100 Subject: [PATCH 2/2] Use a heuristic to correlate devices Every KeyMint device should have a corresponding IRemotelyProvisionedComponent instance, but the converse is not true. So given an IRPC instance under test, look for a corresponding KeyMint device by comparing suffixes, but just skip the test if not found. Test: VtsHalRemotelyProvisionedComponentTargetTest Change-Id: I390aa7025eb77f75a3280e8d85dc453b784c23ee --- .../VtsRemotelyProvisionedComponentTests.cpp | 59 +++++++++++++++---- 1 file changed, 48 insertions(+), 11 deletions(-) diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp index 516be3bdde..57bc27a3f6 100644 --- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp +++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp @@ -239,6 +239,30 @@ ErrMsgOr corrupt_sig_chain(const EekChain& eek, int which) { return EekChain{corruptChain.encode(), eek.last_pubkey, eek.last_privkey}; } +string device_suffix(const string& name) { + size_t pos = name.find('/'); + if (pos == string::npos) { + return name; + } + return name.substr(pos + 1); +} + +bool matching_keymint_device(const string& rp_name, std::shared_ptr* keyMint) { + string rp_suffix = device_suffix(rp_name); + + vector km_names = ::android::getAidlHalInstanceNames(IKeyMintDevice::descriptor); + for (const string& km_name : km_names) { + // If the suffix of the KeyMint instance equals the suffix of the + // RemotelyProvisionedComponent instance, assume they match. + if (device_suffix(km_name) == rp_suffix && AServiceManager_isDeclared(km_name.c_str())) { + ::ndk::SpAIBinder binder(AServiceManager_waitForService(km_name.c_str())); + *keyMint = IKeyMintDevice::fromBinder(binder); + return true; + } + } + return false; +} + } // namespace class VtsRemotelyProvisionedComponentTests : public testing::TestWithParam { @@ -276,21 +300,34 @@ TEST_P(GenerateKeyTests, generateEcdsaP256Key_prodMode) { ASSERT_TRUE(status.isOk()); vector coseKeyData; check_maced_pubkey(macedPubKey, testMode, &coseKeyData); +} + +/** + * Generate and validate a production-mode key, then use it as a KeyMint attestation key. + */ +TEST_P(GenerateKeyTests, generateAndUseEcdsaP256Key_prodMode) { + // See if there is a matching IKeyMintDevice for this IRemotelyProvisionedComponent. + std::shared_ptr keyMint; + if (!matching_keymint_device(GetParam(), &keyMint)) { + // No matching IKeyMintDevice. + GTEST_SKIP() << "Skipping key use test as no matching KeyMint device found"; + return; + } + KeyMintHardwareInfo info; + ASSERT_TRUE(keyMint->getHardwareInfo(&info).isOk()); + + MacedPublicKey macedPubKey; + bytevec privateKeyBlob; + bool testMode = false; + auto status = provisionable_->generateEcdsaP256KeyPair(testMode, &macedPubKey, &privateKeyBlob); + ASSERT_TRUE(status.isOk()); + vector coseKeyData; + check_maced_pubkey(macedPubKey, testMode, &coseKeyData); + AttestationKey attestKey; attestKey.keyBlob = std::move(privateKeyBlob); attestKey.issuerSubjectName = make_name_from_str("Android Keystore Key"); - // Also talk to an IKeyMintDevice. - // TODO: if there were multiple instances of IRemotelyProvisionedComponent and IKeyMintDevice, - // what should the correlation between them be? - vector params = ::android::getAidlHalInstanceNames(IKeyMintDevice::descriptor); - ASSERT_GT(params.size(), 0U); - ASSERT_TRUE(AServiceManager_isDeclared(params[0].c_str())); - ::ndk::SpAIBinder binder(AServiceManager_waitForService(params[0].c_str())); - std::shared_ptr keyMint = IKeyMintDevice::fromBinder(binder); - KeyMintHardwareInfo info; - ASSERT_TRUE(keyMint->getHardwareInfo(&info).isOk()); - // Generate an ECDSA key that is attested by the generated P256 keypair. AuthorizationSet keyDesc = AuthorizationSetBuilder() .Authorization(TAG_NO_AUTH_REQUIRED)