From af7e9cfb284b92e98299a7e9053505b145557686 Mon Sep 17 00:00:00 2001 From: David Zeuthen Date: Thu, 10 Jun 2021 18:34:24 -0400 Subject: [PATCH] identity: fix access control checks in libeic. Also add a new libeic_test binary which has a regression test for this vulnerability. Bug: 190757775 Test: atest libeic_test Test: atest VtsHalIdentityTargetTest Test: atest CtsIdentityTestCases Merged-In: I8344655c59930d6bf1baa4e0f8d0f60e4fc9e48d Change-Id: I8344655c59930d6bf1baa4e0f8d0f60e4fc9e48d --- identity/TEST_MAPPING | 3 + identity/aidl/default/Android.bp | 37 ++++++++ identity/aidl/default/EicTests.cpp | 85 +++++++++++++++++++ .../aidl/default/libeic/EicPresentation.c | 8 ++ .../aidl/default/libeic/EicPresentation.h | 4 + 5 files changed, 137 insertions(+) create mode 100644 identity/aidl/default/EicTests.cpp diff --git a/identity/TEST_MAPPING b/identity/TEST_MAPPING index f35f4b7ef9..85cf91f15c 100644 --- a/identity/TEST_MAPPING +++ b/identity/TEST_MAPPING @@ -8,6 +8,9 @@ }, { "name": "android.hardware.identity-support-lib-test" + }, + { + "name": "libeic_test" } ] } diff --git a/identity/aidl/default/Android.bp b/identity/aidl/default/Android.bp index 7c68aee26d..28c489309f 100644 --- a/identity/aidl/default/Android.bp +++ b/identity/aidl/default/Android.bp @@ -114,6 +114,43 @@ cc_binary { ], } +cc_test { + name: "libeic_test", + srcs: [ + "EicTests.cpp", + "FakeSecureHardwareProxy.cpp", + ], + cflags: [ + "-Wall", + "-Wextra", + "-g", + "-DEIC_DEBUG", + ], + local_include_dirs: [ + "common", + ], + shared_libs: [ + "liblog", + "libcrypto", + "libkeymaster_messages", + ], + static_libs: [ + "libbase", + "libcppbor_external", + "libcppcose_rkp", + "libutils", + "libsoft_attestation_cert", + "libkeymaster_portable", + "libsoft_attestation_cert", + "libpuresoftkeymasterdevice", + "android.hardware.identity-support-lib", + "android.hardware.identity-libeic-library", + ], + test_suites: [ + "general-tests", + ], +} + prebuilt_etc { name: "android.hardware.identity_credential.xml", sub_dir: "permissions", diff --git a/identity/aidl/default/EicTests.cpp b/identity/aidl/default/EicTests.cpp new file mode 100644 index 0000000000..a28080d009 --- /dev/null +++ b/identity/aidl/default/EicTests.cpp @@ -0,0 +1,85 @@ +/* + * Copyright (c) 2021, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include +#include + +#include "FakeSecureHardwareProxy.h" + +// Most of libeic is tested as part of VTS since there's almost a 1:1 mapping between +// the HAL and libeic interfaces. This test suite is mainly for the few things which +// doesn't map directly. +// + +using std::optional; +using std::string; +using std::vector; + +using android::hardware::identity::AccessCheckResult; +using android::hardware::identity::FakeSecureHardwarePresentationProxy; +using android::hardware::identity::FakeSecureHardwareProvisioningProxy; + +TEST(EicTest, AccessControlIsEnforced) { + // First provision the credential... + // + FakeSecureHardwareProvisioningProxy provisioningProxy; + bool isTestCredential = false; + provisioningProxy.initialize(isTestCredential); + optional> credKey = + provisioningProxy.createCredentialKey({0x01, 0x02}, {0x03, 0x04}); + ASSERT_TRUE(credKey.has_value()); + string docType = "org.iso.18013.5.1.mDL"; + ASSERT_TRUE(provisioningProxy.startPersonalization(0, {1}, docType, 125)); + + vector acpIds = {}; + string nameSpace = "org.iso.18013.5.1"; + string name = "NonAccessibleElement"; + vector content = {0x63, 0x46, 0x6f, 0x6f}; // "Foo" tstr + ASSERT_TRUE(provisioningProxy.beginAddEntry(acpIds, nameSpace, name, content.size())); + optional> encContent = + provisioningProxy.addEntryValue(acpIds, nameSpace, name, content); + ASSERT_TRUE(encContent.has_value()); + ASSERT_EQ(encContent->size(), content.size() + 28); + + optional> signatureOfToBeSigned = provisioningProxy.finishAddingEntries(); + ASSERT_TRUE(signatureOfToBeSigned.has_value()); + + optional> credData = provisioningProxy.finishGetCredentialData(docType); + ASSERT_TRUE(credData.has_value()); + ASSERT_TRUE(provisioningProxy.shutdown()); + + // Then present data from it... + // + FakeSecureHardwarePresentationProxy presentationProxy; + ASSERT_TRUE(presentationProxy.initialize(isTestCredential, docType, credData.value())); + AccessCheckResult res = + presentationProxy.startRetrieveEntryValue(nameSpace, name, 1, content.size(), acpIds); + ASSERT_EQ(res, AccessCheckResult::kNoAccessControlProfiles); + + // Ensure that we can't get the data out if startRetrieveEntryValue() returned + // something other than kOk... See b/190757775 for details. + // + optional> decContent = + presentationProxy.retrieveEntryValue(encContent.value(), nameSpace, name, acpIds); + ASSERT_FALSE(decContent.has_value()); +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/identity/aidl/default/libeic/EicPresentation.c b/identity/aidl/default/libeic/EicPresentation.c index 9e033b39fb..3d13766f1e 100644 --- a/identity/aidl/default/libeic/EicPresentation.c +++ b/identity/aidl/default/libeic/EicPresentation.c @@ -633,6 +633,8 @@ EicAccessCheckResult eicPresentationStartRetrieveEntryValue( // We'll need to calc and store a digest of additionalData to check that it's the same // additionalData being passed in for every eicPresentationRetrieveEntryValue() call... + // + ctx->accessCheckOk = false; if (!eicCborCalcEntryAdditionalData(accessControlProfileIds, numAccessControlProfileIds, nameSpace, name, additionalDataCbor, additionalDataCborBufSize, &additionalDataCborSize, @@ -680,6 +682,7 @@ EicAccessCheckResult eicPresentationStartRetrieveEntryValue( if (result == EIC_ACCESS_CHECK_RESULT_OK) { eicCborAppendString(&ctx->cbor, name); + ctx->accessCheckOk = true; } return result; } @@ -702,10 +705,15 @@ bool eicPresentationRetrieveEntryValue(EicPresentation* ctx, const uint8_t* encr calculatedSha256)) { return false; } + if (eicCryptoMemCmp(calculatedSha256, ctx->additionalDataSha256, EIC_SHA256_DIGEST_SIZE) != 0) { eicDebug("SHA-256 mismatch of additionalData"); return false; } + if (!ctx->accessCheckOk) { + eicDebug("Attempting to retrieve a value for which access is not granted"); + return false; + } if (!eicOpsDecryptAes128Gcm(ctx->storageKey, encryptedContent, encryptedContentSize, additionalDataCbor, additionalDataCborSize, content)) { diff --git a/identity/aidl/default/libeic/EicPresentation.h b/identity/aidl/default/libeic/EicPresentation.h index 7cad068772..c888049dbe 100644 --- a/identity/aidl/default/libeic/EicPresentation.h +++ b/identity/aidl/default/libeic/EicPresentation.h @@ -70,6 +70,10 @@ typedef struct { // Set to true initialized as a test credential. bool testCredential; + // Set to true if the evaluation of access control checks in + // eicPresentationStartRetrieveEntryValue() resulted EIC_ACCESS_CHECK_RESULT_OK + bool accessCheckOk; + // These are bitmasks indicating which of the possible 32 access control profiles are // authorized. They are built up by eicPresentationValidateAccessControlProfile(). //