From 63b2ea86d94412adb25efa6f7359a82a539c8cc9 Mon Sep 17 00:00:00 2001 From: "John W. Bruce" Date: Wed, 21 Aug 2019 22:35:34 -0700 Subject: [PATCH] Allow CE CDM to Create Sessions Without a Service Certificate (This is a merge of http://go/wvgerrit/84510) When the CE CDM 3.5 behavior around service certificates was originally implemented, it allowed sessions to be created if a service certificate had not yet been installed, in keeping with the EME spec. However, the service certificate in use at session creation time was cached, and so there was a bug where any sessions open before a service certificate was installed would never be updated with any future service certificates. The code also caused problems for Android. When it was merged to master, it was fixed to simply not allow session creation on CE CDM without a service certificate. However, this created an impedance mismatch between the CE CDM and EME that has caused pain for Shaka Player Embedded, Chrome, Chromecast, Fuchsia, and likely every partner that is trying to implement a fully-compliant EME stack on top of CE CDM. Removing the code that blocks session creation without a service certificate is easy. Fixing the bug that motivated it is not. Removing the caching is not possible because Android needs it for certain behavior on its end. So instead, the CE CDM will have to iterate over all open sessions and update their service certificates if the installed service certificate changes. Test: CE CDM Unit Tests Test: Android Unit Tests Bug: 111766009 Change-Id: I1bd70553e2209b823a6acdc221c0497a5f3181b2 --- libwvdrmengine/cdm/core/include/cdm_engine.h | 4 +++ libwvdrmengine/cdm/core/include/cdm_session.h | 25 +++++++++++++------ libwvdrmengine/cdm/core/include/license.h | 5 ++++ .../cdm/core/include/wv_cdm_types.h | 6 ++++- libwvdrmengine/cdm/core/src/cdm_engine.cpp | 13 ++++++++++ libwvdrmengine/cdm/core/src/cdm_session.cpp | 5 ++++ libwvdrmengine/cdm/core/src/license.cpp | 18 ++++++------- .../cdm/core/test/cdm_engine_test.cpp | 23 +++++++++++++++++ .../cdm/core/test/license_unittest.cpp | 7 +++--- .../cdm/core/test/test_printers.cpp | 3 +++ libwvdrmengine/include/mapErrors-inl.h | 1 + libwvdrmengine/include_hidl/mapErrors-inl.h | 1 + 12 files changed, 88 insertions(+), 23 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/cdm_engine.h b/libwvdrmengine/cdm/core/include/cdm_engine.h index b9fd762a..a6a0fc78 100644 --- a/libwvdrmengine/cdm/core/include/cdm_engine.h +++ b/libwvdrmengine/cdm/core/include/cdm_engine.h @@ -137,6 +137,10 @@ class CdmEngine { virtual CdmResponseType RenewKey(const CdmSessionId& session_id, const CdmKeyResponse& key_data); + // Change the service certificate installed in a given session. + virtual CdmResponseType SetSessionServiceCertificate( + const CdmSessionId& session_id, const std::string& service_certificate); + // Query system information virtual CdmResponseType QueryStatus(SecurityLevel security_level, const std::string& query_token, diff --git a/libwvdrmengine/cdm/core/include/cdm_session.h b/libwvdrmengine/cdm/core/include/cdm_session.h index 5ed5fe2b..a5c585ac 100644 --- a/libwvdrmengine/cdm/core/include/cdm_session.h +++ b/libwvdrmengine/cdm/core/include/cdm_session.h @@ -43,19 +43,23 @@ class CdmSession { bool IsClosed() { return closed_; } // Initializes this instance of CdmSession with the given property set. - // |cdm_client_property_set| MAY be null, is owned by the caller, - // and must remain in scope throughout the scope of this session. + // + // |cdm_client_property_set| is caller owned, may be null, but must be in + // scope as long as the session is in scope. The service certificate field is + // cached at the time Init() is called. virtual CdmResponseType Init(CdmClientPropertySet* cdm_client_property_set); // Initializes this instance of CdmSession with the given parmeters. // All parameters are owned by the caller. - // |service_certificate| is caller owned, cannot be null, and must be in - // scope as long as the session is in scope. - // |cdm_client_property_set| is caller owned, may be null, but must be - // in scope as long as the session is in scope. + // + // |cdm_client_property_set| is caller owned, may be null, but must be in + // scope as long as the session is in scope. The service certificate field is + // cached at the time Init() is called. + // // |forced_session_id| is caller owned and may be null. - // |event_listener| is caller owned, may be null, but must be in scope - // as long as the session is in scope. + // + // |event_listener| is caller owned, may be null, but must be in scope as long + // as the session is in scope. virtual CdmResponseType Init(CdmClientPropertySet* cdm_client_property_set, const CdmSessionId* forced_session_id, WvCdmEventListener* event_listener); @@ -84,6 +88,11 @@ class CdmSession { // AddKey() - Accept license response and extract key info. virtual CdmResponseType AddKey(const CdmKeyResponse& key_response); + // Override the currently-installed service certificate with a new service + // certificate. + virtual CdmResponseType SetServiceCertificate( + const std::string& service_certificate); + // Query session status virtual CdmResponseType QueryStatus(CdmQueryMap* query_response); diff --git a/libwvdrmengine/cdm/core/include/license.h b/libwvdrmengine/cdm/core/include/license.h index 25e5c2a6..c96e69a8 100644 --- a/libwvdrmengine/cdm/core/include/license.h +++ b/libwvdrmengine/cdm/core/include/license.h @@ -44,6 +44,11 @@ class CdmLicense { const std::string& signed_service_certificate, CryptoSession* session, PolicyEngine* policy_engine); + // Override the currently-installed service certificate with a new service + // certificate. + virtual CdmResponseType SetServiceCertificate( + const std::string& signed_service_certificate); + virtual CdmResponseType PrepareKeyRequest( const InitializationData& init_data, CdmLicenseType license_type, const CdmAppParameterMap& app_parameters, CdmKeyMessage* signed_request, diff --git a/libwvdrmengine/cdm/core/include/wv_cdm_types.h b/libwvdrmengine/cdm/core/include/wv_cdm_types.h index 4e563e3c..626d6845 100644 --- a/libwvdrmengine/cdm/core/include/wv_cdm_types.h +++ b/libwvdrmengine/cdm/core/include/wv_cdm_types.h @@ -399,7 +399,11 @@ enum CdmResponseType { REWRAP_DEVICE_RSA_KEY_30_ERROR = 345, INVALID_SRM_LIST = 346, KEYSET_ID_NOT_FOUND_4 = 347, - // Don't forget to add new values to ../test/test_printers.cpp. + SESSION_NOT_FOUND_22 = 348, + // Don't forget to add new values to + // * core/test/test_printers.cpp. + // * android/include/mapErrors-inl.h + // * android/include_hidl/mapErrors-inl.h }; enum CdmKeyStatus { diff --git a/libwvdrmengine/cdm/core/src/cdm_engine.cpp b/libwvdrmengine/cdm/core/src/cdm_engine.cpp index aebd5db8..717c8c12 100644 --- a/libwvdrmengine/cdm/core/src/cdm_engine.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_engine.cpp @@ -507,6 +507,19 @@ CdmResponseType CdmEngine::RenewKey(const CdmSessionId& session_id, return KEY_ADDED; } +CdmResponseType CdmEngine::SetSessionServiceCertificate( + const CdmSessionId& session_id, const std::string& service_certificate) { + LOGI("Setting service certificate: session_id = %s", session_id.c_str()); + + std::shared_ptr session; + if (!session_map_.FindSession(session_id, &session)) { + LOGE("Session ID not found: %s", session_id.c_str()); + return SESSION_NOT_FOUND_22; + } + + return session->SetServiceCertificate(service_certificate); +} + CdmResponseType CdmEngine::QueryStatus(SecurityLevel security_level, const std::string& query_token, std::string* query_response) { diff --git a/libwvdrmengine/cdm/core/src/cdm_session.cpp b/libwvdrmengine/cdm/core/src/cdm_session.cpp index 71fd431a..4b6d997a 100644 --- a/libwvdrmengine/cdm/core/src/cdm_session.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_session.cpp @@ -586,6 +586,11 @@ CdmResponseType CdmSession::QueryStatus(CdmQueryMap* query_response) { return NO_ERROR; } +CdmResponseType CdmSession::SetServiceCertificate( + const std::string& service_certificate) { + return license_parser_->SetServiceCertificate(service_certificate); +} + CdmResponseType CdmSession::QueryKeyStatus(CdmQueryMap* query_response) { return policy_engine_->Query(query_response); } diff --git a/libwvdrmengine/cdm/core/src/license.cpp b/libwvdrmengine/cdm/core/src/license.cpp index 36d8c01a..98ac86d0 100644 --- a/libwvdrmengine/cdm/core/src/license.cpp +++ b/libwvdrmengine/cdm/core/src/license.cpp @@ -226,16 +226,9 @@ bool CdmLicense::Init(const std::string& client_token, return false; } - if (use_privacy_mode) { - if (!signed_service_certificate.empty()) { - if (service_certificate_.Init(signed_service_certificate) != NO_ERROR) - return false; - } - if (!service_certificate_.has_certificate() && - !Properties::allow_service_certificate_requests()) { - LOGE("Required service certificate not provided"); - return false; - } + if (use_privacy_mode && !signed_service_certificate.empty() && + service_certificate_.Init(signed_service_certificate) != NO_ERROR) { + return false; } client_token_ = client_token; @@ -248,6 +241,11 @@ bool CdmLicense::Init(const std::string& client_token, return true; } +CdmResponseType CdmLicense::SetServiceCertificate( + const std::string& signed_service_certificate) { + return service_certificate_.Init(signed_service_certificate); +} + CdmResponseType CdmLicense::PrepareKeyRequest( const InitializationData& init_data, CdmLicenseType license_type, const CdmAppParameterMap& app_parameters, CdmKeyMessage* signed_request, diff --git a/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp b/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp index 4a2423d2..5dd9bc31 100644 --- a/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp +++ b/libwvdrmengine/cdm/core/test/cdm_engine_test.cpp @@ -44,6 +44,8 @@ const std::string kWebmMimeType = "video/webm"; const std::string kEmptyString; const std::string kComma = ","; +const std::string kFakeSessionId = "TotallyARealSession123456789"; + } // namespace class WvCdmEnginePreProvTest : public WvCdmTestBase { @@ -317,6 +319,27 @@ TEST_F(WvCdmEnginePreProvTestUat, ProvisioningServiceCertificateInvalidTest) { ASSERT_NE(cdm_engine_.ValidateServiceCertificate(certificate), NO_ERROR); }; +TEST_F(WvCdmEngineTest, SetLicensingServiceValidCertificate) { + ASSERT_EQ(cdm_engine_.SetSessionServiceCertificate( + session_id_, config_.license_service_certificate()), + NO_ERROR); +}; + +TEST_F(WvCdmEngineTest, SetLicensingServiceCertificateUnknownSession) { + ASSERT_EQ(cdm_engine_.SetSessionServiceCertificate( + kFakeSessionId, config_.license_service_certificate()), + SESSION_NOT_FOUND_22); +}; + +TEST_F(WvCdmEngineTest, SetLicensingServiceInvalidCertificate) { + std::string certificate = config_.license_service_certificate(); + // Add four nulls to the beginning of the cert to invalidate it + certificate.insert(0, 4, '\0'); + + ASSERT_NE(cdm_engine_.SetSessionServiceCertificate(session_id_, certificate), + NO_ERROR); +}; + TEST_F(WvCdmEnginePreProvTestStaging, ProvisioningTest) { Provision(); } TEST_F(WvCdmEnginePreProvTestUatBinary, ProvisioningTest) { diff --git a/libwvdrmengine/cdm/core/test/license_unittest.cpp b/libwvdrmengine/cdm/core/test/license_unittest.cpp index ba671cb1..b780c0e6 100644 --- a/libwvdrmengine/cdm/core/test/license_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/license_unittest.cpp @@ -282,10 +282,9 @@ TEST_F(CdmLicenseTest, InitWithEmptyServiceCert) { EXPECT_CALL(*crypto_session_, IsOpen()).WillOnce(Return(true)); CreateCdmLicense(); - EXPECT_EQ(cdm_license_->Init(kToken, kClientTokenDrmCert, "", true, - kEmptyServiceCertificate, crypto_session_, - policy_engine_), - Properties::allow_service_certificate_requests()); + EXPECT_TRUE(cdm_license_->Init(kToken, kClientTokenDrmCert, "", true, + kEmptyServiceCertificate, crypto_session_, + policy_engine_)); } TEST_F(CdmLicenseTest, InitWithInvalidServiceCert) { diff --git a/libwvdrmengine/cdm/core/test/test_printers.cpp b/libwvdrmengine/cdm/core/test/test_printers.cpp index c7af0f8c..e312531b 100644 --- a/libwvdrmengine/cdm/core/test/test_printers.cpp +++ b/libwvdrmengine/cdm/core/test/test_printers.cpp @@ -781,6 +781,9 @@ void PrintTo(const enum CdmResponseType& value, ::std::ostream* os) { case SESSION_NOT_FOUND_21: *os << "SESSION_NOT_FOUND_21"; break; + case SESSION_NOT_FOUND_22: + *os << "SESSION_NOT_FOUND_22"; + break; case INVALID_DECRYPT_HASH_FORMAT: *os << "INVALID_DECRYPT_HASH_FORMAT"; break; diff --git a/libwvdrmengine/include/mapErrors-inl.h b/libwvdrmengine/include/mapErrors-inl.h index bc3070ec..02a8d458 100644 --- a/libwvdrmengine/include/mapErrors-inl.h +++ b/libwvdrmengine/include/mapErrors-inl.h @@ -231,6 +231,7 @@ static android::status_t mapCdmResponseType(wvcdm::CdmResponseType res) { case wvcdm::SESSION_NOT_FOUND_19: case wvcdm::SESSION_NOT_FOUND_20: case wvcdm::SESSION_NOT_FOUND_21: + case wvcdm::SESSION_NOT_FOUND_22: return android::ERROR_DRM_SESSION_NOT_OPENED; case wvcdm::SESSION_KEYS_NOT_FOUND: return kSessionKeysNotFound; diff --git a/libwvdrmengine/include_hidl/mapErrors-inl.h b/libwvdrmengine/include_hidl/mapErrors-inl.h index 8ff6b44f..2e3a8678 100644 --- a/libwvdrmengine/include_hidl/mapErrors-inl.h +++ b/libwvdrmengine/include_hidl/mapErrors-inl.h @@ -65,6 +65,7 @@ static Status mapCdmResponseType(wvcdm::CdmResponseType res) { case wvcdm::SESSION_NOT_FOUND_19: case wvcdm::SESSION_NOT_FOUND_20: case wvcdm::SESSION_NOT_FOUND_21: + case wvcdm::SESSION_NOT_FOUND_22: return Status::ERROR_DRM_SESSION_NOT_OPENED; case wvcdm::DECRYPT_ERROR: