From 69ea909ff54c52ad8cfd289ccd1f518332c12e3f Mon Sep 17 00:00:00 2001 From: Aaron Vaage Date: Fri, 21 Aug 2020 17:18:28 -0700 Subject: [PATCH] Multiple Renewal Keys and Logging In this code update we add a test to ensure that the White-box API implementation handle seeing multiple renewal keys correctly. Since there should be no more than one renewal key in a license response, upon seeing a second renewal key, the implementation should return a WB_RESULT_INVALID_PARAMETER code. Due to changes in how Chrome manages CHECKS and DCHECKS, this code has been updated to use the new headers. --- api/aead_whitebox_benchmark.cc | 1 - api/license_whitebox_decrypt_benchmark.cc | 1 - api/license_whitebox_masked_decrypt_test.cc | 1 - ...ebox_process_license_response_benchmark.cc | 1 - ..._whitebox_process_license_response_test.cc | 20 +++++++ api/license_whitebox_sign_benchmark.cc | 1 - api/license_whitebox_verify_benchmark.cc | 1 - api/test_license_builder.cc | 3 +- chromium_deps/base/BUILD | 6 +- chromium_deps/base/check.h | 13 +++++ chromium_deps/base/check_op.h | 13 +++++ crypto_utils/aes_cbc_decryptor.cc | 1 + crypto_utils/aes_cbc_encryptor.cc | 1 + crypto_utils/aes_ctr_encryptor.cc | 1 + crypto_utils/crypto_util.cc | 2 +- crypto_utils/random_util.cc | 2 +- crypto_utils/rsa_key.cc | 1 + crypto_utils/rsa_util.cc | 1 + impl/reference/aead_whitebox_impl.cc | 1 + impl/reference/license_whitebox_impl.cc | 58 +++++++++++-------- 20 files changed, 95 insertions(+), 34 deletions(-) create mode 100644 chromium_deps/base/check.h create mode 100644 chromium_deps/base/check_op.h diff --git a/api/aead_whitebox_benchmark.cc b/api/aead_whitebox_benchmark.cc index c78e2e7..2ac2403 100644 --- a/api/aead_whitebox_benchmark.cc +++ b/api/aead_whitebox_benchmark.cc @@ -7,7 +7,6 @@ #include "api/aead_whitebox.h" #include "api/test_data.h" -#include "base/logging.h" #include "benchmarking/data_source.h" #include "benchmarking/measurements.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/api/license_whitebox_decrypt_benchmark.cc b/api/license_whitebox_decrypt_benchmark.cc index 9140f17..e93629a 100644 --- a/api/license_whitebox_decrypt_benchmark.cc +++ b/api/license_whitebox_decrypt_benchmark.cc @@ -11,7 +11,6 @@ #include "api/result.h" #include "api/test_data.h" #include "api/test_license_builder.h" -#include "base/logging.h" #include "benchmarking/data_source.h" #include "benchmarking/measurements.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/api/license_whitebox_masked_decrypt_test.cc b/api/license_whitebox_masked_decrypt_test.cc index 39edf6c..252b4cc 100644 --- a/api/license_whitebox_masked_decrypt_test.cc +++ b/api/license_whitebox_masked_decrypt_test.cc @@ -11,7 +11,6 @@ #include "api/license_whitebox_test_base.h" #include "api/test_data.h" #include "api/test_license_builder.h" -#include "base/logging.h" #include "crypto_utils/crypto_util.h" #include "crypto_utils/rsa_key.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/api/license_whitebox_process_license_response_benchmark.cc b/api/license_whitebox_process_license_response_benchmark.cc index df88c1d..5b21b7e 100644 --- a/api/license_whitebox_process_license_response_benchmark.cc +++ b/api/license_whitebox_process_license_response_benchmark.cc @@ -8,7 +8,6 @@ #include "api/result.h" #include "api/test_data.h" #include "api/test_license_builder.h" -#include "base/logging.h" #include "benchmarking/measurements.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/api/license_whitebox_process_license_response_test.cc b/api/license_whitebox_process_license_response_test.cc index 2fe34cd..bdbb20f 100644 --- a/api/license_whitebox_process_license_response_test.cc +++ b/api/license_whitebox_process_license_response_test.cc @@ -74,6 +74,26 @@ TEST_F(LicenseWhiteboxProcessLicenseResponseTest, WB_RESULT_OK); } +TEST_F(LicenseWhiteboxProcessLicenseResponseTest, + InvalidParameterWithMultipleSigningKeys) { + TestLicenseBuilder builder; + builder.AddSigningKey(TestLicenseBuilder::DefaultSigningKey(), + TestLicenseBuilder::PKSC8Padding()); + builder.AddSigningKey(TestLicenseBuilder::DefaultSigningKey(), + TestLicenseBuilder::PKSC8Padding()); + builder.AddStubbedContentKey(); + builder.Build(*public_key_, &license_); + + ASSERT_EQ( + WB_License_ProcessLicenseResponse( + whitebox_, license_.core_message.data(), license_.core_message.size(), + license_.message.data(), license_.message.size(), + license_.signature.data(), license_.signature.size(), + license_.session_key.data(), license_.session_key.size(), + license_.request.data(), license_.request.size()), + WB_RESULT_INVALID_PARAMETER); +} + class LicenseWhiteboxProcessLicenseResponseErrorTest : public LicenseWhiteboxProcessLicenseResponseTest { protected: diff --git a/api/license_whitebox_sign_benchmark.cc b/api/license_whitebox_sign_benchmark.cc index d9697b2..5b81190 100644 --- a/api/license_whitebox_sign_benchmark.cc +++ b/api/license_whitebox_sign_benchmark.cc @@ -11,7 +11,6 @@ #include "api/result.h" #include "api/test_data.h" #include "api/test_license_builder.h" -#include "base/logging.h" #include "benchmarking/data_source.h" #include "benchmarking/measurements.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/api/license_whitebox_verify_benchmark.cc b/api/license_whitebox_verify_benchmark.cc index 9c01013..6dd7a9f 100644 --- a/api/license_whitebox_verify_benchmark.cc +++ b/api/license_whitebox_verify_benchmark.cc @@ -11,7 +11,6 @@ #include "api/result.h" #include "api/test_data.h" #include "api/test_license_builder.h" -#include "base/logging.h" #include "benchmarking/data_source.h" #include "benchmarking/measurements.h" #include "testing/gtest/include/gtest/gtest.h" diff --git a/api/test_license_builder.cc b/api/test_license_builder.cc index 9eb5472..f5c3c14 100644 --- a/api/test_license_builder.cc +++ b/api/test_license_builder.cc @@ -4,7 +4,8 @@ #include -#include "base/logging.h" +#include "base/check.h" +#include "base/check_op.h" #include "cdm/keys/certs.h" #include "crypto_utils/aes_cbc_encryptor.h" #include "crypto_utils/crypto_util.h" diff --git a/chromium_deps/base/BUILD b/chromium_deps/base/BUILD index 8c6f6f5..d934f0b 100644 --- a/chromium_deps/base/BUILD +++ b/chromium_deps/base/BUILD @@ -2,7 +2,11 @@ cc_library( name = "glog", - hdrs = ["logging.h"], + hdrs = [ + "check.h", + "check_op.h", + "logging.h" + ], strip_include_prefix = "//chromium_deps", visibility = ["//visibility:public"], deps = [ diff --git a/chromium_deps/base/check.h b/chromium_deps/base/check.h new file mode 100644 index 0000000..3b565bd --- /dev/null +++ b/chromium_deps/base/check.h @@ -0,0 +1,13 @@ +// Copyright 2020 Google LLC. All Rights Reserved. + +#ifndef BASE_CHECK_H_ +#define BASE_CHECK_H_ + +// Chromium has split CHECK/DCHECK out of logging.h into it's own header. +// However, it's all still together in the GLOG header, so just include +// it. This may cause problems when importing into Alcatraz as code using +// anyone of these headers gets all the functions, not just the ones +// represented by this header. +#include "base/logging.h" + +#endif // BASE_CHECK_H_ diff --git a/chromium_deps/base/check_op.h b/chromium_deps/base/check_op.h new file mode 100644 index 0000000..9f1d204 --- /dev/null +++ b/chromium_deps/base/check_op.h @@ -0,0 +1,13 @@ +// Copyright 2020 Google LLC. All Rights Reserved. + +#ifndef BASE_CHECK_OP_H_ +#define BASE_CHECK_OP_H_ + +// Chromium has split CHECK_EQ/etc. out of logging.h into it's own header. +// However, it's all still together in the GLOG header, so just include +// it. This may cause problems when importing into Alcatraz as code using +// anyone of these headers gets all the functions, not just the ones +// represented by this header. +#include "base/logging.h" + +#endif // BASE_CHECK_OP_H_ diff --git a/crypto_utils/aes_cbc_decryptor.cc b/crypto_utils/aes_cbc_decryptor.cc index ce5b8b9..6eac61c 100644 --- a/crypto_utils/aes_cbc_decryptor.cc +++ b/crypto_utils/aes_cbc_decryptor.cc @@ -6,6 +6,7 @@ #include #include +#include "base/check.h" #include "base/logging.h" namespace widevine { diff --git a/crypto_utils/aes_cbc_encryptor.cc b/crypto_utils/aes_cbc_encryptor.cc index 2965bcd..91cc9a3 100644 --- a/crypto_utils/aes_cbc_encryptor.cc +++ b/crypto_utils/aes_cbc_encryptor.cc @@ -6,6 +6,7 @@ #include #include +#include "base/check.h" #include "base/logging.h" namespace widevine { diff --git a/crypto_utils/aes_ctr_encryptor.cc b/crypto_utils/aes_ctr_encryptor.cc index 87863f7..a943d0d 100644 --- a/crypto_utils/aes_ctr_encryptor.cc +++ b/crypto_utils/aes_ctr_encryptor.cc @@ -6,6 +6,7 @@ #include #include +#include "base/check.h" #include "base/logging.h" namespace widevine { diff --git a/crypto_utils/crypto_util.cc b/crypto_utils/crypto_util.cc index 6b4cd81..9a08e27 100644 --- a/crypto_utils/crypto_util.cc +++ b/crypto_utils/crypto_util.cc @@ -10,7 +10,7 @@ #include "crypto_utils/crypto_util.h" -#include "base/logging.h" +#include "base/check.h" #include "third_party/boringssl/src/include/openssl/aes.h" #include "third_party/boringssl/src/include/openssl/cmac.h" #include "third_party/boringssl/src/include/openssl/evp.h" diff --git a/crypto_utils/random_util.cc b/crypto_utils/random_util.cc index 297d696..3e5574e 100644 --- a/crypto_utils/random_util.cc +++ b/crypto_utils/random_util.cc @@ -8,7 +8,7 @@ #include "crypto_utils/random_util.h" -#include "base/logging.h" +#include "base/check.h" #include "third_party/boringssl/src/include/openssl/rand.h" namespace widevine { diff --git a/crypto_utils/rsa_key.cc b/crypto_utils/rsa_key.cc index eacfe58..47e50c7 100644 --- a/crypto_utils/rsa_key.cc +++ b/crypto_utils/rsa_key.cc @@ -25,6 +25,7 @@ #include "crypto_utils/rsa_key.h" +#include "base/check.h" #include "base/logging.h" #include "crypto_utils/rsa_util.h" #include "crypto_utils/sha_util.h" diff --git a/crypto_utils/rsa_util.cc b/crypto_utils/rsa_util.cc index 24de338..e30ff7d 100644 --- a/crypto_utils/rsa_util.cc +++ b/crypto_utils/rsa_util.cc @@ -17,6 +17,7 @@ #include #include +#include "base/check.h" #include "base/logging.h" #include "crypto_utils/private_key_util.h" #include "third_party/boringssl/src/include/openssl/pem.h" diff --git a/impl/reference/aead_whitebox_impl.cc b/impl/reference/aead_whitebox_impl.cc index cffa098..c3d5e95 100644 --- a/impl/reference/aead_whitebox_impl.cc +++ b/impl/reference/aead_whitebox_impl.cc @@ -5,6 +5,7 @@ #include #include +#include "base/check_op.h" #include "base/logging.h" #include "crypto_utils/crypto_util.h" #include "impl/reference/memory_util.h" diff --git a/impl/reference/license_whitebox_impl.cc b/impl/reference/license_whitebox_impl.cc index 225953e..d370f90 100644 --- a/impl/reference/license_whitebox_impl.cc +++ b/impl/reference/license_whitebox_impl.cc @@ -8,6 +8,8 @@ #include #include +#include "base/check.h" +#include "base/check_op.h" #include "base/logging.h" #include "cdm/protos/license_protocol.pb.h" #include "crypto_utils/aes_cbc_decryptor.h" @@ -50,12 +52,12 @@ struct ContentKey { bool Decrypt(AesCbcDecryptor& decryptor, const std::string& iv, const std::string& encrypted, - std::vector* decrypted) { + std::string* decrypted) { DCHECK_GE(decrypted->size(), encrypted.size()); - return decryptor.Decrypt(reinterpret_cast(iv.data()), - iv.size(), - reinterpret_cast(encrypted.data()), - encrypted.size(), decrypted->data()); + return decryptor.Decrypt( + reinterpret_cast(iv.data()), iv.size(), + reinterpret_cast(encrypted.data()), encrypted.size(), + reinterpret_cast(&decrypted->front())); } bool IsOdkVersionSupported(uint16_t major_version, uint16_t minor_version) { @@ -124,7 +126,7 @@ video_widevine::License_KeyContainer_SecurityLevel ExtractLevel( ContentKey CreateContentKey( video_widevine::License_KeyContainer_SecurityLevel level, bool is_hw_verified, - const std::vector& key) { + const std::string& key) { ContentKey content_key; switch (level) { @@ -149,7 +151,7 @@ ContentKey CreateContentKey( // it will only risk exposing it. We only have an entry for it so we can // handle errors correctly. if (content_key.allow_decrypt || content_key.allow_masked_decrypt) { - content_key.key = key; + content_key.key.assign(key.begin(), key.end()); } return content_key; @@ -515,9 +517,9 @@ WB_Result WB_License_ProcessLicenseResponse(WB_License_Whitebox* whitebox, decryptor.SetKey(reinterpret_cast(decryption_key.data()), decryption_key.size())); - std::string server_renewal_key; - std::string client_renewal_key; std::map content_keys; + std::vector server_renewal_keys; + std::vector client_renewal_keys; // Even if |core_message| is provided, only ODK v16.5 and later support the // fields needed. If an older API is used, ignore it and use the protobuf @@ -529,7 +531,8 @@ WB_Result WB_License_ProcessLicenseResponse(WB_License_Whitebox* whitebox, const std::string signing_key_iv = ExtractItem(parsed_license.enc_mac_keys_iv, message_str); if (!signing_key_encrypted.empty() && !signing_key_iv.empty()) { - std::vector unwrapped_signing_key(signing_key_encrypted.size()); + std::string unwrapped_signing_key(signing_key_encrypted); + if (!Decrypt(decryptor, signing_key_iv, signing_key_encrypted, &unwrapped_signing_key)) { DVLOG(1) << "Invalid parameter: Invalid enc_mac_keys."; @@ -541,11 +544,10 @@ WB_Result WB_License_ProcessLicenseResponse(WB_License_Whitebox* whitebox, return WB_RESULT_INVALID_PARAMETER; } - const std::string signing_key(unwrapped_signing_key.begin(), - unwrapped_signing_key.end()); - server_renewal_key = signing_key.substr(0, kSigningKeySizeBytes); - client_renewal_key = - signing_key.substr(kSigningKeySizeBytes, kSigningKeySizeBytes); + server_renewal_keys.push_back( + unwrapped_signing_key.substr(0, kSigningKeySizeBytes)); + client_renewal_keys.push_back(unwrapped_signing_key.substr( + kSigningKeySizeBytes, kSigningKeySizeBytes)); } // Now extract all the content keys. @@ -555,7 +557,7 @@ WB_Result WB_License_ProcessLicenseResponse(WB_License_Whitebox* whitebox, const std::string iv = ExtractItem(key.key_data_iv, message_str); const std::string wrapped_key = ExtractItem(key.key_data, message_str); - std::vector unwrapped_key(wrapped_key.size()); + std::string unwrapped_key(wrapped_key); if (!Decrypt(decryptor, iv, wrapped_key, &unwrapped_key)) { DVLOG(1) << "Invalid parameter: Invalid key.key_data."; @@ -604,7 +606,7 @@ WB_Result WB_License_ProcessLicenseResponse(WB_License_Whitebox* whitebox, } const std::string wrapped_key = key.key(); - std::vector unwrapped_key(wrapped_key.size()); + std::string unwrapped_key(wrapped_key); if (!Decrypt(decryptor, key.iv(), wrapped_key, &unwrapped_key)) { // The input has to be a specific length, so if it is not, it means that @@ -619,11 +621,10 @@ WB_Result WB_License_ProcessLicenseResponse(WB_License_Whitebox* whitebox, return WB_RESULT_INVALID_PARAMETER; } - const std::string signing_key(unwrapped_key.begin(), - unwrapped_key.end()); - server_renewal_key = signing_key.substr(0, kSigningKeySizeBytes); - client_renewal_key = - signing_key.substr(kSigningKeySizeBytes, kSigningKeySizeBytes); + server_renewal_keys.push_back( + unwrapped_key.substr(0, kSigningKeySizeBytes)); + client_renewal_keys.push_back( + unwrapped_key.substr(kSigningKeySizeBytes, kSigningKeySizeBytes)); } else if (key.type() == KeyContainer::CONTENT) { constexpr size_t kContentKeySizeBytes = 16; @@ -642,10 +643,19 @@ WB_Result WB_License_ProcessLicenseResponse(WB_License_Whitebox* whitebox, } } + DCHECK_EQ(server_renewal_keys.size(), client_renewal_keys.size()); + if (server_renewal_keys.size() > 1) { + return WB_RESULT_INVALID_PARAMETER; + } + // Add an empty string so that we can always reference a valid string. By + // adding it last, a valid key will get priority over this fallback value. + server_renewal_keys.push_back(""); + client_renewal_keys.push_back(""); + // Copy the loaded state over to the white-box instance now that we know we // have a valid state. - whitebox->server_signing_key.swap(server_renewal_key); - whitebox->client_signing_key.swap(client_renewal_key); + whitebox->server_signing_key.swap(server_renewal_keys[0]); + whitebox->client_signing_key.swap(client_renewal_keys[0]); whitebox->content_keys.swap(content_keys); return WB_RESULT_OK;