From 5ba093377c40a2ce976093d046f3723feae9c953 Mon Sep 17 00:00:00 2001 From: Janis Danisevskis Date: Thu, 17 Dec 2020 10:05:15 -0800 Subject: [PATCH] Revise keymint_tags.h * replace NullOr with std::optional. * Add mising tag. * Undefine helper macros so that keymint_tags.h can be used together with keymaster_tags.h * Check if KeyParameterValue variant matches KeyParameterTag in accessors. Test: VtsAidlKeyMintTargetTest Change-Id: I6c951071f30fd27c8c21a2e8cc86f421a3bc37d9 --- .../vts/functional/KeyMintAidlTestBase.cpp | 4 +- .../aidl/vts/functional/KeyMintTest.cpp | 31 +-- .../keymint/support/authorization_set.cpp | 5 +- .../keymint_support/authorization_set.h | 8 +- .../keymint_support/key_param_output.h | 2 +- .../include/keymint_support/keymint_tags.h | 189 +++++++++--------- 6 files changed, 122 insertions(+), 117 deletions(-) diff --git a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp index 9ba4099819..94bc199ca7 100644 --- a/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintAidlTestBase.cpp @@ -577,8 +577,8 @@ string KeyMintAidlTestBase::EncryptMessage(const string& message, BlockMode bloc string ciphertext = EncryptMessage(message, params, &out_params); EXPECT_EQ(1U, out_params.size()); auto ivVal = out_params.GetTagValue(TAG_NONCE); - EXPECT_TRUE(ivVal.isOk()); - if (ivVal.isOk()) *iv_out = ivVal.value(); + EXPECT_TRUE(ivVal); + if (ivVal) *iv_out = *ivVal; return ciphertext; } diff --git a/security/keymint/aidl/vts/functional/KeyMintTest.cpp b/security/keymint/aidl/vts/functional/KeyMintTest.cpp index 6e38539a36..30601538dd 100644 --- a/security/keymint/aidl/vts/functional/KeyMintTest.cpp +++ b/security/keymint/aidl/vts/functional/KeyMintTest.cpp @@ -80,7 +80,10 @@ namespace { template bool contains(vector& set, TypedTag ttag, ValueT expected_value) { auto it = std::find_if(set.begin(), set.end(), [&](const KeyParameter& param) { - return param.tag == tag && accessTagValue(ttag, param) == expected_value; + if (auto p = authorizationValue(ttag, param)) { + return *p == expected_value; + } + return false; }); return (it != set.end()); } @@ -251,10 +254,10 @@ class NewKeyGenerationTest : public KeyMintAidlTestBase { EXPECT_TRUE(auths.Contains(TAG_OS_VERSION, os_version())) << "OS version is " << os_version() << " key reported " - << auths.GetTagValue(TAG_OS_VERSION); + << auths.GetTagValue(TAG_OS_VERSION)->get(); EXPECT_TRUE(auths.Contains(TAG_OS_PATCHLEVEL, os_patch_level())) << "OS patch level is " << os_patch_level() << " key reported " - << auths.GetTagValue(TAG_OS_PATCHLEVEL); + << auths.GetTagValue(TAG_OS_PATCHLEVEL)->get(); } }; @@ -2333,8 +2336,8 @@ TEST_P(EncryptionOperationsTest, AesEcbPkcs7PaddingCorrupted) { vector CopyIv(const AuthorizationSet& set) { auto iv = set.GetTagValue(TAG_NONCE); - EXPECT_TRUE(iv.isOk()); - return iv.value(); + EXPECT_TRUE(iv); + return iv->get(); } /* @@ -2459,13 +2462,13 @@ TEST_P(EncryptionOperationsTest, AesIncremental) { case BlockMode::CBC: case BlockMode::GCM: case BlockMode::CTR: - ASSERT_TRUE(iv.isOk()) << "No IV for block mode " << block_mode; - EXPECT_EQ(block_mode == BlockMode::GCM ? 12U : 16U, iv.value().size()); - params.push_back(TAG_NONCE, iv.value()); + ASSERT_TRUE(iv) << "No IV for block mode " << block_mode; + EXPECT_EQ(block_mode == BlockMode::GCM ? 12U : 16U, iv->get().size()); + params.push_back(TAG_NONCE, iv->get()); break; case BlockMode::ECB: - EXPECT_FALSE(iv.isOk()) << "ECB mode should not generate IV"; + EXPECT_FALSE(iv) << "ECB mode should not generate IV"; break; } @@ -2649,9 +2652,9 @@ TEST_P(EncryptionOperationsTest, AesCallerNonce) { AuthorizationSet out_params; string ciphertext = EncryptMessage(message, params, &out_params); EXPECT_EQ(message.size(), ciphertext.size()); - EXPECT_EQ(16U, out_params.GetTagValue(TAG_NONCE).value().size()); + EXPECT_EQ(16U, out_params.GetTagValue(TAG_NONCE)->get().size()); - params.push_back(TAG_NONCE, out_params.GetTagValue(TAG_NONCE).value()); + params.push_back(TAG_NONCE, out_params.GetTagValue(TAG_NONCE)->get()); string plaintext = DecryptMessage(ciphertext, params); EXPECT_EQ(message, plaintext); @@ -2697,9 +2700,9 @@ TEST_P(EncryptionOperationsTest, AesCallerNonceProhibited) { AuthorizationSet out_params; string ciphertext = EncryptMessage(message, params, &out_params); EXPECT_EQ(message.size(), ciphertext.size()); - EXPECT_EQ(16U, out_params.GetTagValue(TAG_NONCE).value().size()); + EXPECT_EQ(16U, out_params.GetTagValue(TAG_NONCE)->get().size()); - params.push_back(TAG_NONCE, out_params.GetTagValue(TAG_NONCE).value()); + params.push_back(TAG_NONCE, out_params.GetTagValue(TAG_NONCE)->get()); string plaintext = DecryptMessage(ciphertext, params); EXPECT_EQ(message, plaintext); @@ -2893,7 +2896,7 @@ TEST_P(EncryptionOperationsTest, AesGcmTooShortTagOnDecrypt) { AuthorizationSet begin_out_params; EXPECT_EQ(ErrorCode::OK, Begin(KeyPurpose::ENCRYPT, params, &begin_out_params)); EXPECT_EQ(1U, begin_out_params.size()); - ASSERT_TRUE(begin_out_params.GetTagValue(TAG_NONCE).isOk()); + ASSERT_TRUE(begin_out_params.GetTagValue(TAG_NONCE)); AuthorizationSet finish_out_params; string ciphertext; diff --git a/security/keymint/support/authorization_set.cpp b/security/keymint/support/authorization_set.cpp index 37b6cd1ff3..f98851cfa2 100644 --- a/security/keymint/support/authorization_set.cpp +++ b/security/keymint/support/authorization_set.cpp @@ -106,10 +106,11 @@ bool AuthorizationSet::erase(int index) { return false; } -NullOr AuthorizationSet::GetEntry(Tag tag) const { +std::optional> AuthorizationSet::GetEntry( + Tag tag) const { int pos = find(tag); if (pos == -1) return {}; - return data_[pos]; + return std::reference_wrapper(data_[pos]); } AuthorizationSetBuilder& AuthorizationSetBuilder::RsaKey(uint32_t key_size, diff --git a/security/keymint/support/include/keymint_support/authorization_set.h b/security/keymint/support/include/keymint_support/authorization_set.h index c85f3050d9..4ff170526a 100644 --- a/security/keymint/support/include/keymint_support/authorization_set.h +++ b/security/keymint/support/include/keymint_support/authorization_set.h @@ -168,7 +168,7 @@ class AuthorizationSet { bool Contains(TypedTag ttag, const ValueT& value) const { for (const auto& param : data_) { auto entry = authorizationValue(ttag, param); - if (entry.isOk() && static_cast(entry.value()) == value) return true; + if (entry && static_cast(*entry) == value) return true; } return false; } @@ -178,9 +178,9 @@ class AuthorizationSet { size_t GetTagCount(Tag tag) const; template - inline NullOr::type&> GetTagValue(T tag) const { + inline auto GetTagValue(T tag) const -> decltype(authorizationValue(tag, KeyParameter())) { auto entry = GetEntry(tag); - if (entry.isOk()) return authorizationValue(tag, entry.value()); + if (entry) return authorizationValue(tag, *entry); return {}; } @@ -219,7 +219,7 @@ class AuthorizationSet { } private: - NullOr GetEntry(Tag tag) const; + std::optional> GetEntry(Tag tag) const; std::vector data_; }; diff --git a/security/keymint/support/include/keymint_support/key_param_output.h b/security/keymint/support/include/keymint_support/key_param_output.h index 6e0e35d9c0..5f004fe529 100644 --- a/security/keymint/support/include/keymint_support/key_param_output.h +++ b/security/keymint/support/include/keymint_support/key_param_output.h @@ -71,7 +71,7 @@ inline ::std::ostream& operator<<(::std::ostream& os, SecurityLevel value) { } template -::std::ostream& operator<<(::std::ostream& os, const NullOr& value) { +::std::ostream& operator<<(::std::ostream& os, const std::optional& value) { if (!value.isOk()) { os << "(value not present)"; } else { diff --git a/security/keymint/support/include/keymint_support/keymint_tags.h b/security/keymint/support/include/keymint_support/keymint_tags.h index c443c535ff..d932b40b49 100644 --- a/security/keymint/support/include/keymint_support/keymint_tags.h +++ b/security/keymint/support/include/keymint_support/keymint_tags.h @@ -58,6 +58,10 @@ struct Tag2TypedTag { typedef TypedTag type; }; +#ifdef DECLARE_TYPED_TAG +#undef DECLARE_TYPED_TAG +#endif + #define DECLARE_TYPED_TAG(name) \ typedef typename Tag2TypedTag::type TAG_##name##_t; \ static TAG_##name##_t TAG_##name; @@ -72,9 +76,12 @@ DECLARE_TYPED_TAG(ATTESTATION_APPLICATION_ID); DECLARE_TYPED_TAG(ATTESTATION_CHALLENGE); DECLARE_TYPED_TAG(ATTESTATION_ID_BRAND); DECLARE_TYPED_TAG(ATTESTATION_ID_DEVICE); -DECLARE_TYPED_TAG(ATTESTATION_ID_PRODUCT); +DECLARE_TYPED_TAG(ATTESTATION_ID_IMEI); DECLARE_TYPED_TAG(ATTESTATION_ID_MANUFACTURER); +DECLARE_TYPED_TAG(ATTESTATION_ID_MEID); +DECLARE_TYPED_TAG(ATTESTATION_ID_PRODUCT); DECLARE_TYPED_TAG(ATTESTATION_ID_MODEL); +DECLARE_TYPED_TAG(ATTESTATION_ID_SERIAL); DECLARE_TYPED_TAG(AUTH_TIMEOUT); DECLARE_TYPED_TAG(BLOCK_MODE); DECLARE_TYPED_TAG(BOOTLOADER_ONLY); @@ -118,6 +125,8 @@ DECLARE_TYPED_TAG(USER_ID); DECLARE_TYPED_TAG(USER_SECURE_ID); DECLARE_TYPED_TAG(VENDOR_PATCHLEVEL); +#undef DECLARE_TYPED_TAG + template struct MetaList {}; @@ -133,6 +142,7 @@ using all_tags_t = MetaList< TAG_OS_VERSION_t, TAG_OS_PATCHLEVEL_t, TAG_UNIQUE_ID_t, TAG_ATTESTATION_CHALLENGE_t, TAG_ATTESTATION_APPLICATION_ID_t, TAG_ATTESTATION_ID_BRAND_t, TAG_ATTESTATION_ID_DEVICE_t, TAG_ATTESTATION_ID_PRODUCT_t, TAG_ATTESTATION_ID_MANUFACTURER_t, TAG_ATTESTATION_ID_MODEL_t, + TAG_ATTESTATION_ID_SERIAL_t, TAG_ATTESTATION_ID_IMEI_t, TAG_ATTESTATION_ID_MEID_t, TAG_RESET_SINCE_ID_ROTATION_t, TAG_PURPOSE_t, TAG_ALGORITHM_t, TAG_BLOCK_MODE_t, TAG_DIGEST_t, TAG_PADDING_t, TAG_ORIGIN_t, TAG_USER_AUTH_TYPE_t, TAG_EC_CURVE_t, TAG_BOOT_PATCHLEVEL_t, TAG_VENDOR_PATCHLEVEL_t, TAG_TRUSTED_CONFIRMATION_REQUIRED_t, @@ -141,21 +151,39 @@ using all_tags_t = MetaList< template struct TypedTag2ValueType; -#define MAKE_TAG_VALUE_ACCESSOR(tag_type, field_name) \ - template \ - struct TypedTag2ValueType> { \ - using type = std::remove_reference(nullptr) \ - ->get())>::type; \ - static constexpr KeyParameterValue::Tag unionTag = KeyParameterValue::field_name; \ - }; \ - template \ - inline auto& accessTagValue(TypedTag, const KeyParameter& param) { \ - return param.value.get(); \ - } \ - template \ - inline auto& accessTagValue(TypedTag, KeyParameter& param) { \ - return param.value.get(); \ +#ifdef MAKE_TAG_VALUE_ACCESSOR +#undef MAKE_TAG_VALUE_ACCESSOR +#endif + +#define MAKE_TAG_VALUE_ACCESSOR(tag_type, field_name) \ + template \ + struct TypedTag2ValueType> { \ + using type = std::remove_reference< \ + decltype(static_cast(nullptr) \ + ->get())>::type; \ + static constexpr KeyParameterValue::Tag unionTag = KeyParameterValue::field_name; \ + }; \ + template \ + inline std::optional>::type>> \ + accessTagValue(TypedTag, const KeyParameter& param) { \ + if (param.value.getTag() == KeyParameterValue::field_name) { \ + return std::optional( \ + std::reference_wrapper(param.value.get())); \ + } else { \ + return std::nullopt; \ + } \ + } \ + template \ + inline std::optional< \ + std::reference_wrapper>::type>> \ + accessTagValue(TypedTag, KeyParameter& param) { \ + if (param.value.getTag() == KeyParameterValue::field_name) { \ + return std::optional( \ + std::reference_wrapper(param.value.get())); \ + } else { \ + return std::nullopt; \ + } \ } MAKE_TAG_VALUE_ACCESSOR(TagType::ULONG, longInteger) @@ -167,19 +195,39 @@ MAKE_TAG_VALUE_ACCESSOR(TagType::BOOL, boolValue) MAKE_TAG_VALUE_ACCESSOR(TagType::BYTES, blob) MAKE_TAG_VALUE_ACCESSOR(TagType::BIGNUM, blob) -#define MAKE_TAG_ENUM_VALUE_ACCESSOR(typed_tag, field_name) \ - template <> \ - struct TypedTag2ValueType { \ - using type = std::remove_reference(nullptr) \ - ->get())>::type; \ - static constexpr KeyParameterValue::Tag unionTag = KeyParameterValue::field_name; \ - }; \ - inline auto& accessTagValue(decltype(typed_tag), const KeyParameter& param) { \ - return param.value.get(); \ - } \ - inline auto& accessTagValue(decltype(typed_tag), KeyParameter& param) { \ - return param.value.get(); \ +#undef MAKE_TAG_VALUE_ACCESSOR + +#ifdef MAKE_TAG_ENUM_VALUE_ACCESSOR +#undef MAKE_TAG_ENUM_VALUE_ACCESSOR +#endif + +#define MAKE_TAG_ENUM_VALUE_ACCESSOR(typed_tag, field_name) \ + template <> \ + struct TypedTag2ValueType { \ + using type = std::remove_reference< \ + decltype(static_cast(nullptr) \ + ->get())>::type; \ + static constexpr KeyParameterValue::Tag unionTag = KeyParameterValue::field_name; \ + }; \ + inline std::optional< \ + std::reference_wrapper::type>> \ + accessTagValue(decltype(typed_tag), const KeyParameter& param) { \ + if (param.value.getTag() == KeyParameterValue::field_name) { \ + return std::optional( \ + std::reference_wrapper(param.value.get())); \ + } else { \ + return std::nullopt; \ + } \ + } \ + inline std::optional< \ + std::reference_wrapper::type>> \ + accessTagValue(decltype(typed_tag), KeyParameter& param) { \ + if (param.value.getTag() == KeyParameterValue::field_name) { \ + return std::optional( \ + std::reference_wrapper(param.value.get())); \ + } else { \ + return std::nullopt; \ + } \ } MAKE_TAG_ENUM_VALUE_ACCESSOR(TAG_ALGORITHM, algorithm) @@ -192,6 +240,8 @@ MAKE_TAG_ENUM_VALUE_ACCESSOR(TAG_PURPOSE, keyPurpose) MAKE_TAG_ENUM_VALUE_ACCESSOR(TAG_USER_AUTH_TYPE, hardwareAuthenticatorType) MAKE_TAG_ENUM_VALUE_ACCESSOR(TAG_HARDWARE_TYPE, securityLevel) +#undef MAKE_TAG_ENUM_VALUE_ACCESSOR + template inline KeyParameter makeKeyParameter(TypedTag ttag, ValueT&& value) { KeyParameter retval; @@ -210,6 +260,14 @@ inline KeyParameter makeKeyParameter(TypedTag) { return retval; } +// the invalid case +inline KeyParameter makeKeyParameter(TypedTag) { + KeyParameter retval; + retval.tag = Tag::INVALID; + retval.value = KeyParameterValue::make(0); + return retval; +} + template struct FirstOrNoneHelper; template @@ -240,88 +298,31 @@ inline KeyParameter Authorization(TypedTag ttag, Args&&... args) return makeKeyParameter(ttag, std::forward(args)...); } -/** - * This class wraps a (mostly return) value and stores whether or not the wrapped value is valid out - * of band. Note that if the wrapped value is a reference it is unsafe to access the value if - * !isOk(). If the wrapped type is a pointer or value and !isOk(), it is still safe to access the - * wrapped value. In this case the pointer will be NULL though, and the value will be default - * constructed. - * - * TODO(seleneh) replace this with std::optional. - */ -template -class NullOr { - using internal_t = std::conditional_t::value, - std::remove_reference_t*, ValueT>; - - struct pointer_initializer { - static std::nullptr_t init() { return nullptr; } - }; - struct value_initializer { - static ValueT init() { return ValueT(); } - }; - struct value_pointer_deref_t { - static ValueT& deref(ValueT& v) { return v; } - }; - struct reference_deref_t { - static auto& deref(internal_t v) { return *v; } - }; - using initializer_t = std::conditional_t::value || - std::is_pointer::value, - pointer_initializer, value_initializer>; - using deref_t = std::conditional_t::value, reference_deref_t, - value_pointer_deref_t>; - - public: - NullOr() : value_(initializer_t::init()), null_(true) {} - template - NullOr(T&& value, typename std::enable_if< - !std::is_lvalue_reference::value && - std::is_same, std::decay_t>::value, - int>::type = 0) - : value_(std::forward(value)), null_(false) {} - template - NullOr(T& value, typename std::enable_if< - std::is_lvalue_reference::value && - std::is_same, std::decay_t>::value, - int>::type = 0) - : value_(&value), null_(false) {} - - bool isOk() const { return !null_; } - - const ValueT& value() const& { return deref_t::deref(value_); } - ValueT& value() & { return deref_t::deref(value_); } - ValueT&& value() && { return std::move(deref_t::deref(value_)); } - - private: - internal_t value_; - bool null_; -}; - template std::remove_reference_t NullOrOr(T&& v) { - if (v.isOk()) return v; + if (v) return v; return {}; } template std::remove_reference_t NullOrOr(Head&& head, Tail&&... tail) { - if (head.isOk()) return head; + if (head) return head; return NullOrOr(std::forward(tail)...); } template -std::remove_reference_t defaultOr(NullOr&& optional, Default&& def) { +std::remove_reference_t defaultOr(std::optional&& optional, Default&& def) { static_assert(std::is_convertible, std::remove_reference_t>::value, - "Type of default value must match the type wrapped by NullOr"); - if (optional.isOk()) return optional.value(); + "Type of default value must match the type wrapped by std::optional"); + if (optional) return *optional; return def; } template -inline NullOr>::type&> authorizationValue( - TypedTag ttag, const KeyParameter& param) { +inline std::optional< + std::reference_wrapper>::type>> +authorizationValue(TypedTag ttag, const KeyParameter& param) { if (TypedTag2ValueType>::unionTag != param.value.getTag()) return {}; return accessTagValue(ttag, param); }