commit e74f0bc9502a4012f41ac1097472b1ce66a9c678
author Max Bires <jbires@google.com> Wed Mar 16 13:58:27 2022 -0700
committer Max Bires <jbires@google.com> Mon Mar 21 05:19:15 2022 +0000
tree fd2b792707c69a0be07bc8b20696e5e04ca9821b
parent b66f1308ac19476466411cc4b49e001cac570d57

More canonicalization checks and canonicalize before signing

This change makes sure the DeviceInfo CBOR map is canonicalized before
the signature check instead of just separately checking the
canonicalization in a separate call. Additionally, some ASSERTs have
been changed to EXPECTs in validation of the DeviceInfo map more
generally, where it makes sense to avoid failing immediately.

Test: atest VtsHalRemotelyProvisionedComponentTargetTest
Change-Id: I69806c887656772ea6b5e2e3f0af50957e6b05e3

security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp

diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp
index e2d75ce..544393a 100644
--- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp
+++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp
@@ -421,10 +421,9 @@
         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);
-
         auto& signingKey = bccContents->back().pubKey;
+        deviceInfoMap->asMap()->canonicalize();
         auto macKey = verifyAndParseCoseSign1(signedMac->asArray(), signingKey,
                                               cppbor::Array()  // SignedMacAad
                                                       .add(challenge_)
@@ -456,10 +455,10 @@
         ASSERT_EQ(val->type(), majorType) << entryName << " has the wrong type.";
         switch (majorType) {
             case cppbor::TSTR:
-                ASSERT_GT(val->asTstr()->value().size(), 0);
+                EXPECT_GT(val->asTstr()->value().size(), 0);
                 break;
             case cppbor::BSTR:
-                ASSERT_GT(val->asBstr()->value().size(), 0);
+                EXPECT_GT(val->asBstr()->value().size(), 0);
                 break;
             default:
                 break;
@@ -467,6 +466,8 @@
     }
 
     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");
         ASSERT_TRUE(version);
         ASSERT_TRUE(version->asUint());
@@ -485,21 +486,21 @@
                 // TODO: Refactor the KeyMint code that validates these fields and include it here.
                 checkType(deviceInfo, cppbor::TSTR, "vb_state");
                 allowList = getAllowedVbStates();
-                ASSERT_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();
-                ASSERT_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");
-                ASSERT_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();
-                ASSERT_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") {
                     checkType(deviceInfo, cppbor::TSTR, "os_version");
@@ -508,20 +509,18 @@
             case 1:
                 checkType(deviceInfo, cppbor::TSTR, "security_level");
                 allowList = getAllowedSecurityLevels();
-                ASSERT_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();
-                    ASSERT_NE(allowList.find(deviceInfo->get("att_id_state")->asTstr()->value()),
+                    EXPECT_NE(allowList.find(deviceInfo->get("att_id_state")->asTstr()->value()),
                               allowList.end());
                 }
                 break;
             default:
                 FAIL() << "Unrecognized version: " << version->asUint()->value();
         }
-        ASSERT_EQ(deviceInfo->clone()->asMap()->canonicalize().encode(), deviceInfoBytes)
-                << "DeviceInfo ordering is non-canonical.";
     }
 
     bytevec eekId_;