From 7f028d25c8487880ef8642abdc9112e730920320 Mon Sep 17 00:00:00 2001 From: "John W. Bruce" Date: Tue, 18 Aug 2020 18:02:36 -0700 Subject: [PATCH 1/2] Fix Undefined Behavior Around Enums (This is a cherry-pick of http://go/wvgerrit/104184.) UBSan has detected several places where our code tripped over what is technically Undefined Behavior when handling enums, although in practice any compiler would still generate safe code. Some of these were places a variable was not being initialized and thus was filled with garbage data. These have been fixed. Understanding the rest depends on a bit of C++ trivia I had certainly never heard before: An enum that doesn't specify its backing type will frequently have a gap between the range of values the compiler will let it take (which is limited only by the size of the backing type assigned by the C++ standard) and the range of values for which the C++ standard defines the behavior. (which is limited by the minimum number of bits needed to hold the largest valid enumeration entry) So, for example, an enum containing ten entries numbered 0 through 9 would be stored in memory as an int and could thus take any value in the range of an int. But it only takes 4 bits to represent the numbers 0 through 9. The largest number that can be represented in 4 bits is 15. So reading the value of a variable of this enum type when its stored value is outside the range 0 to 15 is undefined behavior. An enum that specifies its backing type is not subject to this because it is defined behavior to access any value representable in the backing type if one was explicitly specified. If you think this sounds a bit silly, you'll be happy to know it doesn't apply from C++17 onwards and most compilers generate code that handles the undefined behavior values correctly. Nonetheless, to appease UBSan and protect us from any compilers that actually rely on this undefined behavior for optimizations, I have defined backing types for all our enums. I have defaulted to the type the compiler was already using (int32) and have deviated only where an enum exists to be compared to or filled from a protobuf field and that field in the protobuf is unsigned, in which case I used a uint32. In the case of the CE CDM exported API, this also required changing our enums from C-style to C++-style. Bug: 163080356 Test: CE CDM Build & Unit Tests Pass even with UBSan Test: Android Build & Tests Change-Id: Id7e0064129e7c4d2827bb4a94825d144eeaacec8 --- .../cdm/core/include/wv_cdm_types.h | 45 ++++++++++--------- .../cdm_engine_metrics_decorator_unittest.cpp | 2 +- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/wv_cdm_types.h b/libwvdrmengine/cdm/core/include/wv_cdm_types.h index ac7ce25b..041baa72 100644 --- a/libwvdrmengine/cdm/core/include/wv_cdm_types.h +++ b/libwvdrmengine/cdm/core/include/wv_cdm_types.h @@ -34,7 +34,7 @@ using CdmProvisioningResponse = std::string; using CdmUsageTableHeader = std::string; using CdmUsageEntry = std::string; -enum CdmKeyRequestType { +enum CdmKeyRequestType : uint32_t { kKeyRequestTypeUnknown, kKeyRequestTypeInitial, kKeyRequestTypeRenewal, @@ -43,13 +43,13 @@ enum CdmKeyRequestType { kKeyRequestTypeUpdate, }; -enum CdmOfflineLicenseState { +enum CdmOfflineLicenseState : int32_t { kLicenseStateActive, kLicenseStateReleasing, kLicenseStateUnknown, }; -enum CdmResponseType { +enum CdmResponseType : int32_t { NO_ERROR = 0, UNKNOWN_ERROR = 1, KEY_ADDED = 2, @@ -421,7 +421,7 @@ enum CdmResponseType { // * android/include_hidl/mapErrors-inl.h }; -enum CdmKeyStatus { +enum CdmKeyStatus : int32_t { kKeyStatusKeyUnknown, kKeyStatusUsable, kKeyStatusExpired, @@ -432,7 +432,7 @@ enum CdmKeyStatus { }; using CdmKeyStatusMap = std::map; -enum CdmLicenseType { +enum CdmLicenseType : int32_t { kLicenseTypeOffline, kLicenseTypeStreaming, kLicenseTypeRelease, @@ -446,48 +446,51 @@ enum CdmLicenseType { // on code review comments from go/wvgerrit/41860 this license type should not // be added. This type can be removed once it is no longer needed by // GenerateKeyRequest. - kLicenseTypeEmbeddedKeyData + kLicenseTypeEmbeddedKeyData, }; -enum CdmLicenseKeyType { kLicenseKeyTypeContent, kLicenseKeyTypeEntitlement }; +enum CdmLicenseKeyType : int32_t { + kLicenseKeyTypeContent, + kLicenseKeyTypeEntitlement, +}; -enum SecurityLevel { kLevelDefault, kLevel3 }; +enum SecurityLevel : uint32_t { kLevelDefault, kLevel3 }; -enum CdmSecurityLevel { +enum CdmSecurityLevel : int32_t { kSecurityLevelUninitialized, kSecurityLevelL1, kSecurityLevelL2, kSecurityLevelL3, - kSecurityLevelUnknown + kSecurityLevelUnknown, }; -enum CdmCertificateType { +enum CdmCertificateType : int32_t { kCertificateWidevine, kCertificateX509, }; -enum CdmHlsMethod { +enum CdmHlsMethod : int32_t { kHlsMethodNone, kHlsMethodAes128, kHlsMethodSampleAes, }; -enum CdmCipherMode { +enum CdmCipherMode : int32_t { kCipherModeCtr, kCipherModeCbc, }; -enum CdmEncryptionAlgorithm { +enum CdmEncryptionAlgorithm : int32_t { kEncryptionAlgorithmUnknown, - kEncryptionAlgorithmAesCbc128 + kEncryptionAlgorithmAesCbc128, }; -enum CdmSigningAlgorithm { +enum CdmSigningAlgorithm : int32_t { kSigningAlgorithmUnknown, - kSigningAlgorithmHmacSha256 + kSigningAlgorithmHmacSha256, }; -enum CdmClientTokenType { +enum CdmClientTokenType : int32_t { kClientTokenKeybox, kClientTokenDrmCert, kClientTokenOemCert, @@ -500,13 +503,13 @@ enum CdmClientTokenType { // persisted in non-secure storage but are loaded and unloaded from // the TEE during use (OEMCrypto v13+) // kUnknownUsageSupport - usage support type is uninitialized or unavailable -enum CdmUsageSupportType { +enum CdmUsageSupportType : int32_t { kNonSecureUsageSupport, kUsageEntrySupport, kUnknownUsageSupport, }; -enum CdmUsageEntryStorageType { +enum CdmUsageEntryStorageType : int32_t { kStorageLicense, kStorageUsageInfo, kStorageTypeUnknown, @@ -546,7 +549,7 @@ struct CdmUsageEntryInfo { } }; -enum CdmKeySecurityLevel { +enum CdmKeySecurityLevel : int32_t { kKeySecurityLevelUnset, kSoftwareSecureCrypto, kSoftwareSecureDecode, diff --git a/libwvdrmengine/cdm/core/test/cdm_engine_metrics_decorator_unittest.cpp b/libwvdrmengine/cdm/core/test/cdm_engine_metrics_decorator_unittest.cpp index 4b9d2d4b..cf8476bb 100644 --- a/libwvdrmengine/cdm/core/test/cdm_engine_metrics_decorator_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/cdm_engine_metrics_decorator_unittest.cpp @@ -233,7 +233,7 @@ TEST_F(WvCdmEngineMetricsImplTest, GenerateKeyRequest) { } TEST_F(WvCdmEngineMetricsImplTest, AddKey) { - CdmLicenseType license_type; + CdmLicenseType license_type = {}; CdmKeySetId key_set_id; EXPECT_CALL(*test_cdm_metrics_engine_, From 7604158d6f627e29a4acef9b24913c3d5075f217 Mon Sep 17 00:00:00 2001 From: Fred Gylys-Colwell Date: Fri, 14 Aug 2020 12:42:30 -0700 Subject: [PATCH 2/2] Test renewal against same and different server Merge from Widevine repo of http://go/wvgerrit/102843 The test WvCdmEngineTest.LicenseRenewal is split into two tests. One test verifies that the renewal may be fetched from the server specified in the license. The second test verifies that the renewal may be fetched from the same server that the license was fetched from. These might be the same server, but when we run against an experimental server, a staging server, or UAT Nightly, these will be different. Test: ran the tests Bug: 141438127 Change-Id: Ia11441bd2ba0c6ddb264ee38bfcb5060b9ddb476 --- .../cdm/core/test/cdm_engine_test.cpp | 20 +++++++++++++++---- libwvdrmengine/cdm/core/test/url_request.cpp | 4 +++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp b/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp index 5d24e264..080748d5 100644 --- a/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp +++ b/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp @@ -402,14 +402,26 @@ TEST_F(WvCdmEngineTest, LoadKey) { holder.SignAndLoadLicense(); } -TEST_F(WvCdmEngineTest, LicenseRenewal) { +// This test generates a renewal and then requests the renewal using the server +// url specified in the license. This is what most apps would do, but you should +// skip this test when you want to set the license and renewal server on the +// command line. +TEST_F(WvCdmEngineTest, LicenseRenewalSpecifiedServer) { GenerateKeyRequest(binary_key_id(), kCencMimeType); VerifyNewKeyResponse(config_.license_server(), config_.client_auth()); GenerateRenewalRequest(); - VerifyRenewalKeyResponse( - server_url_.empty() ? config_.license_server() : server_url_, - config_.client_auth()); + VerifyRenewalKeyResponse(server_url_, config_.client_auth()); +} + +// This test generates a renewal and then requests the renewal from the same +// server from which we requested the original license. +TEST_F(WvCdmEngineTest, LicenseRenewalSameServer) { + GenerateKeyRequest(binary_key_id(), kCencMimeType); + VerifyNewKeyResponse(config_.license_server(), config_.client_auth()); + + GenerateRenewalRequest(); + VerifyRenewalKeyResponse(config_.license_server(), config_.client_auth()); } TEST_F(WvCdmEngineTest, ParseDecryptHashStringTest) { diff --git a/libwvdrmengine/cdm/core/test/url_request.cpp b/libwvdrmengine/cdm/core/test/url_request.cpp index e33aed8c..7978ef3d 100644 --- a/libwvdrmengine/cdm/core/test/url_request.cpp +++ b/libwvdrmengine/cdm/core/test/url_request.cpp @@ -117,7 +117,9 @@ bool UrlRequest::GetResponse(std::string* message) { } ConcatenateChunkedResponse(response, message); - LOGV("HTTP response: (%d): %s", message->size(), message->c_str()); + LOGV("HTTP response from %s://%s:%d%s: (%zd): %s", socket_.scheme().c_str(), + socket_.domain_name().c_str(), socket_.port(), + socket_.resource_path().c_str(), message->size(), message->c_str()); return true; }