From 757ed42e2c2c9cbd6dceb01575ef802c3ab1e740 Mon Sep 17 00:00:00 2001 From: Max Bires Date: Wed, 7 Sep 2022 16:20:31 -0700 Subject: [PATCH] Clarify error message in DeviceInfo check. This change gives additional context to the device info checks while. Currently, an unprovisioned device will generate a massive spam of failures which may be WAI for an early hardware revision device that was not provisioned with attestation IDs. Test: atest VtsHalRemotelyProvisionedComponentTest Change-Id: I16069dba841a90aa55781148d3c268ced635e006 --- .../VtsRemotelyProvisionedComponentTests.cpp | 69 +++++++++++++------ 1 file changed, 47 insertions(+), 22 deletions(-) diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp index 7184613cfa..2e282e0925 100644 --- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp +++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -73,6 +74,10 @@ std::set getAllowedAttIdStates() { return {"locked", "open"}; } +std::set getAttestationIdEntrySet() { + return {"brand", "manufacturer", "product", "model", "device"}; +} + bytevec string_to_bytevec(const char* s) { const uint8_t* p = reinterpret_cast(s); return bytevec(p, p + strlen(s)); @@ -431,7 +436,7 @@ class CertificateRequestTest : public VtsRemotelyProvisionedComponentTests { auto [deviceInfoMap, __2, deviceInfoErrMsg] = cppbor::parse(deviceInfo.deviceInfo); ASSERT_TRUE(deviceInfoMap) << "Failed to parse deviceInfo: " << deviceInfoErrMsg; ASSERT_TRUE(deviceInfoMap->asMap()); - checkDeviceInfo(deviceInfoMap->asMap(), deviceInfo.deviceInfo); + checkDeviceInfo(*deviceInfoMap->asMap(), deviceInfo.deviceInfo); auto& signingKey = bccContents->back().pubKey; deviceInfoMap->asMap()->canonicalize(); auto macKey = verifyAndParseCoseSign1(signedMac->asArray(), signingKey, @@ -459,72 +464,92 @@ class CertificateRequestTest : public VtsRemotelyProvisionedComponentTests { } } - void checkType(const cppbor::Map* devInfo, uint8_t majorType, std::string entryName) { - const auto& val = devInfo->get(entryName); - ASSERT_TRUE(val) << entryName << " does not exist"; - ASSERT_EQ(val->type(), majorType) << entryName << " has the wrong type."; + std::optional assertAttribute(const cppbor::Map& devInfo, + cppbor::MajorType majorType, std::string entryName) { + const auto& val = devInfo.get(entryName); + if (!val) return entryName + " is missing.\n"; + if (val->type() != majorType) return entryName + " has the wrong type.\n"; switch (majorType) { case cppbor::TSTR: - EXPECT_GT(val->asTstr()->value().size(), 0); + if (val->asTstr()->value().size() <= 0) { + return entryName + " is present but the value is empty.\n"; + } break; case cppbor::BSTR: - EXPECT_GT(val->asBstr()->value().size(), 0); + if (val->asBstr()->value().size() <= 0) { + return entryName + " is present but the value is empty.\n"; + } break; default: break; } + return {}; } - void checkDeviceInfo(const cppbor::Map* deviceInfo, bytevec deviceInfoBytes) { - EXPECT_EQ(deviceInfo->clone()->asMap()->canonicalize().encode(), deviceInfoBytes) + void checkType(const cppbor::Map& devInfo, cppbor::MajorType majorType, std::string entryName) { + if (auto error = assertAttribute(devInfo, majorType, entryName)) { + FAIL() << *error; + } + } + + void checkDeviceInfo(const cppbor::Map& deviceInfo, bytevec deviceInfoBytes) { + EXPECT_EQ(deviceInfo.clone()->asMap()->canonicalize().encode(), deviceInfoBytes) << "DeviceInfo ordering is non-canonical."; - const auto& version = deviceInfo->get("version"); + const auto& version = deviceInfo.get("version"); ASSERT_TRUE(version); ASSERT_TRUE(version->asUint()); RpcHardwareInfo info; provisionable_->getHardwareInfo(&info); ASSERT_EQ(version->asUint()->value(), info.versionNumber); std::set allowList; + std::string problemEntries; switch (version->asUint()->value()) { // These fields became mandated in version 2. case 2: - checkType(deviceInfo, cppbor::TSTR, "brand"); - checkType(deviceInfo, cppbor::TSTR, "manufacturer"); - checkType(deviceInfo, cppbor::TSTR, "product"); - checkType(deviceInfo, cppbor::TSTR, "model"); - checkType(deviceInfo, cppbor::TSTR, "device"); + for (auto attId : getAttestationIdEntrySet()) { + if (auto errMsg = assertAttribute(deviceInfo, cppbor::TSTR, attId)) { + problemEntries += *errMsg; + } + } + EXPECT_EQ("", problemEntries) + << problemEntries + << "Attestation IDs are missing or malprovisioned. If this test is being " + "run against an early proto or EVT build, this error is probably WAI " + "and indicates that Device IDs were not provisioned in the factory. If " + "this error is returned on a DVT or later build revision, then " + "something is likely wrong with the factory provisioning process."; // TODO: Refactor the KeyMint code that validates these fields and include it here. checkType(deviceInfo, cppbor::TSTR, "vb_state"); allowList = getAllowedVbStates(); - EXPECT_NE(allowList.find(deviceInfo->get("vb_state")->asTstr()->value()), + EXPECT_NE(allowList.find(deviceInfo.get("vb_state")->asTstr()->value()), allowList.end()); checkType(deviceInfo, cppbor::TSTR, "bootloader_state"); allowList = getAllowedBootloaderStates(); - EXPECT_NE(allowList.find(deviceInfo->get("bootloader_state")->asTstr()->value()), + EXPECT_NE(allowList.find(deviceInfo.get("bootloader_state")->asTstr()->value()), allowList.end()); checkType(deviceInfo, cppbor::BSTR, "vbmeta_digest"); checkType(deviceInfo, cppbor::UINT, "system_patch_level"); checkType(deviceInfo, cppbor::UINT, "boot_patch_level"); checkType(deviceInfo, cppbor::UINT, "vendor_patch_level"); checkType(deviceInfo, cppbor::UINT, "fused"); - EXPECT_LT(deviceInfo->get("fused")->asUint()->value(), 2); // Must be 0 or 1. + EXPECT_LT(deviceInfo.get("fused")->asUint()->value(), 2); // Must be 0 or 1. checkType(deviceInfo, cppbor::TSTR, "security_level"); allowList = getAllowedSecurityLevels(); - EXPECT_NE(allowList.find(deviceInfo->get("security_level")->asTstr()->value()), + EXPECT_NE(allowList.find(deviceInfo.get("security_level")->asTstr()->value()), allowList.end()); - if (deviceInfo->get("security_level")->asTstr()->value() == "tee") { + if (deviceInfo.get("security_level")->asTstr()->value() == "tee") { checkType(deviceInfo, cppbor::TSTR, "os_version"); } break; case 1: checkType(deviceInfo, cppbor::TSTR, "security_level"); allowList = getAllowedSecurityLevels(); - EXPECT_NE(allowList.find(deviceInfo->get("security_level")->asTstr()->value()), + EXPECT_NE(allowList.find(deviceInfo.get("security_level")->asTstr()->value()), allowList.end()); if (version->asUint()->value() == 1) { checkType(deviceInfo, cppbor::TSTR, "att_id_state"); allowList = getAllowedAttIdStates(); - EXPECT_NE(allowList.find(deviceInfo->get("att_id_state")->asTstr()->value()), + EXPECT_NE(allowList.find(deviceInfo.get("att_id_state")->asTstr()->value()), allowList.end()); } break;