From b8e13cec2dd594f50a539b0f1b990dcf4d467d01 Mon Sep 17 00:00:00 2001 From: "John W. Bruce" Date: Tue, 26 May 2020 18:04:42 -0700 Subject: [PATCH 1/2] OEMCrypto Unit Test Fix: Do Not Derive Keys Immediately (This is a merge of http://go/wvgerrit/100053.) The OEMCrypto Unit Tests were previously deriving keys from the session key as part of loading the test RSA key. This creates an invalid function call order, since the OEMCrypto session will likely next be used for actions that need to be done *before* deriving these keys. With ODKiTEE, which is more strict about this order, all OEMCrypto tests were failing. Bug: 156655072 Test: OEMCrypto Unit Tests Change-Id: Ibfede587da30cfff4a44a5e0687e4199b1430372 --- libwvdrmengine/oemcrypto/test/oec_session_util.cpp | 9 ++++----- .../oemcrypto/test/oemcrypto_session_tests_helper.cpp | 2 +- libwvdrmengine/oemcrypto/test/oemcrypto_test.cpp | 3 +++ 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/libwvdrmengine/oemcrypto/test/oec_session_util.cpp b/libwvdrmengine/oemcrypto/test/oec_session_util.cpp index c64f979b..769aec75 100644 --- a/libwvdrmengine/oemcrypto/test/oec_session_util.cpp +++ b/libwvdrmengine/oemcrypto/test/oec_session_util.cpp @@ -1014,7 +1014,7 @@ void Session::GenerateDerivedKeysFromSessionKey() { // Uses test certificate. vector session_key; vector enc_session_key; - if (public_rsa_ == nullptr) PreparePublicKey(); + ASSERT_NE(public_rsa_, nullptr) << "No public RSA key loaded in test code."; // A failure here probably indicates that there is something wrong with the // test program and its dependency on BoringSSL. ASSERT_TRUE(GenerateRSASessionKey(&session_key, &enc_session_key)); @@ -1303,12 +1303,11 @@ void Session::VerifyRSASignature(const vector& message, const uint8_t* signature, size_t signature_length, RSA_Padding_Scheme padding_scheme) { - EXPECT_TRUE(nullptr != public_rsa_) - << "No public RSA key loaded in test code.\n"; + ASSERT_NE(public_rsa_, nullptr) << "No public RSA key loaded in test code."; - EXPECT_EQ(static_cast(RSA_size(public_rsa_)), signature_length) + ASSERT_EQ(static_cast(RSA_size(public_rsa_)), signature_length) << "Signature size is wrong. " << signature_length << ", should be " - << RSA_size(public_rsa_) << "\n"; + << RSA_size(public_rsa_); if (padding_scheme == kSign_RSASSA_PSS) { boringssl_ptr pkey(EVP_PKEY_new()); diff --git a/libwvdrmengine/oemcrypto/test/oemcrypto_session_tests_helper.cpp b/libwvdrmengine/oemcrypto/test/oemcrypto_session_tests_helper.cpp index 9e2bc7e7..da8d5ec2 100644 --- a/libwvdrmengine/oemcrypto/test/oemcrypto_session_tests_helper.cpp +++ b/libwvdrmengine/oemcrypto/test/oemcrypto_session_tests_helper.cpp @@ -81,6 +81,6 @@ void SessionUtil::InstallTestRSAKey(Session* s) { ASSERT_NO_FATAL_FAILURE(s->InstallRSASessionTestKey(wrapped_rsa_key_)); } // Test RSA key should be loaded. - ASSERT_NO_FATAL_FAILURE(s->GenerateDerivedKeysFromSessionKey()); + ASSERT_NO_FATAL_FAILURE(s->PreparePublicKey()); } } // namespace wvoec diff --git a/libwvdrmengine/oemcrypto/test/oemcrypto_test.cpp b/libwvdrmengine/oemcrypto/test/oemcrypto_test.cpp index 3526d57f..e584e20f 100644 --- a/libwvdrmengine/oemcrypto/test/oemcrypto_test.cpp +++ b/libwvdrmengine/oemcrypto/test/oemcrypto_test.cpp @@ -688,6 +688,7 @@ TEST_F(OEMCryptoProv30Test, GetCertOnlyAPI16) { ASSERT_NO_FATAL_FAILURE(s.open()); // Install the DRM Cert's RSA key. ASSERT_NO_FATAL_FAILURE(s.InstallRSASessionTestKey(wrapped_rsa_key_)); + ASSERT_NO_FATAL_FAILURE(s.PreparePublicKey()); // Request the OEM Cert. -- This should NOT load the OEM Private key. vector public_cert; size_t public_cert_length = 0; @@ -4885,6 +4886,7 @@ class LicenseWithUsageEntry { ASSERT_NO_FATAL_FAILURE(session_.open()); ASSERT_NO_FATAL_FAILURE(session_.ReloadUsageEntry()); ASSERT_NO_FATAL_FAILURE(util->InstallTestRSAKey(&session_)); + ASSERT_NO_FATAL_FAILURE(session_.GenerateDerivedKeysFromSessionKey()); ASSERT_EQ(OEMCrypto_SUCCESS, license_messages_.LoadResponse()); } @@ -6008,6 +6010,7 @@ TEST_P(OEMCryptoUsageTableTest, ReloadUsageTableWithSkew) { old_usage_entry_1.data(), old_usage_entry_1.size())); ASSERT_NO_FATAL_FAILURE(InstallTestRSAKey(&s)); + ASSERT_NO_FATAL_FAILURE(s.GenerateDerivedKeysFromSessionKey()); ASSERT_EQ(OEMCrypto_SUCCESS, entry.license_messages().LoadResponse()); } From 69e7e21882882dfb204b39b45a2641c39a38bf43 Mon Sep 17 00:00:00 2001 From: Cong Lin Date: Fri, 29 May 2020 10:07:08 -0700 Subject: [PATCH 2/2] Fix implicit type conversion issue in ODK Merging CL https://widevine-internal-review.googlesource.com/c/cdm/+/100924 Fix implicit type conversion issue in ODK 1. Implicit cast is reported as error when compiling ODK with Level3 2. Override odk_add_overflow_xxx function with the built in functions can cause redefinition issue when compiling Level3; Let's use odk customized overflow functions. Bug: b/157510403 Test: ODK unittests and CDM unittests passed. Change-Id: Ieef8ccfb41d08007ec72f4a061f92968e55539cb --- libwvdrmengine/oemcrypto/odk/src/odk_overflow.h | 11 ----------- libwvdrmengine/oemcrypto/odk/src/odk_serialize.c | 4 ++-- libwvdrmengine/oemcrypto/odk/src/odk_util.c | 4 ++-- 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/libwvdrmengine/oemcrypto/odk/src/odk_overflow.h b/libwvdrmengine/oemcrypto/odk/src/odk_overflow.h index 3fcb32cc..b1e03ee5 100644 --- a/libwvdrmengine/oemcrypto/odk/src/odk_overflow.h +++ b/libwvdrmengine/oemcrypto/odk/src/odk_overflow.h @@ -12,20 +12,9 @@ extern "C" { #endif -#ifndef __has_builtin -# define __has_builtin(x) 0 -#endif - -#if (defined(__GNUC__) && __GNUC__ >= 5) || \ - __has_builtin(__builtin_add_overflow) -# define odk_sub_overflow_u64 __builtin_sub_overflow -# define odk_add_overflow_u64 __builtin_add_overflow -# define odk_add_overflow_ux __builtin_add_overflow -#else int odk_sub_overflow_u64(uint64_t a, uint64_t b, uint64_t* c); int odk_add_overflow_u64(uint64_t a, uint64_t b, uint64_t* c); int odk_add_overflow_ux(size_t a, size_t b, size_t* c); -#endif #ifdef __cplusplus } diff --git a/libwvdrmengine/oemcrypto/odk/src/odk_serialize.c b/libwvdrmengine/oemcrypto/odk/src/odk_serialize.c index e0500505..efd94755 100644 --- a/libwvdrmengine/oemcrypto/odk/src/odk_serialize.c +++ b/libwvdrmengine/oemcrypto/odk/src/odk_serialize.c @@ -151,7 +151,7 @@ static void Unpack_ODK_ParsedLicense(Message* msg, ODK_ParsedLicense* obj) { Unpack_OEMCrypto_Substring(msg, &obj->enc_mac_keys); Unpack_OEMCrypto_Substring(msg, &obj->pst); Unpack_OEMCrypto_Substring(msg, &obj->srm_restriction_data); - obj->license_type = Unpack_enum(msg); + obj->license_type = (OEMCrypto_LicenseType)Unpack_enum(msg); Unpack_bool(msg, &obj->nonce_required); Unpack_ODK_TimerLimits(msg, &obj->timer_limits); Unpack_uint32_t(msg, &obj->key_array_length); @@ -167,7 +167,7 @@ static void Unpack_ODK_ParsedLicense(Message* msg, ODK_ParsedLicense* obj) { static void Unpack_ODK_ParsedProvisioning(Message* msg, ODK_ParsedProvisioning* obj) { - obj->key_type = Unpack_enum(msg); + obj->key_type = (OEMCrypto_PrivateKeyType)Unpack_enum(msg); Unpack_OEMCrypto_Substring(msg, &obj->enc_private_key); Unpack_OEMCrypto_Substring(msg, &obj->enc_private_key_iv); Unpack_OEMCrypto_Substring(msg, &obj->encrypted_message_key); diff --git a/libwvdrmengine/oemcrypto/odk/src/odk_util.c b/libwvdrmengine/oemcrypto/odk/src/odk_util.c index d1024262..ff85a9c2 100644 --- a/libwvdrmengine/oemcrypto/odk/src/odk_util.c +++ b/libwvdrmengine/oemcrypto/odk/src/odk_util.c @@ -14,8 +14,8 @@ int crypto_memcmp(const void* in_a, const void* in_b, size_t len) { return -1; } - const uint8_t* a = in_a; - const uint8_t* b = in_b; + const uint8_t* a = (const uint8_t*)in_a; + const uint8_t* b = (const uint8_t*)in_b; uint8_t x = 0; for (size_t i = 0; i < len; i++) {