From de58a410a5842f0c8873feb6de16ca06832077ee Mon Sep 17 00:00:00 2001 From: Jacob Keeler Date: Tue, 14 Sep 2021 16:20:52 -0400 Subject: Add missing internal error codes to security manager (#3773) * Add missing internal error codes to security manager * Add internal error notification to SSL handshake failure * Remove unused handshake result * Add unit tests for new error codes Co-authored-by: JackLivio --- .../src/application_manager_impl.cc | 1 - .../include/security_manager/security_manager.h | 13 ++- .../include/security_manager/ssl_context.h | 1 - .../protocol_handler/src/handshake_handler.cc | 2 - .../security_manager/security_manager_impl.h | 4 +- .../security_manager/src/security_manager_impl.cc | 24 +++++ .../security_manager/test/security_manager_test.cc | 110 ++++++++++++++++++++- 7 files changed, 139 insertions(+), 16 deletions(-) diff --git a/src/components/application_manager/src/application_manager_impl.cc b/src/components/application_manager/src/application_manager_impl.cc index c5089872d4..f350c9fc5a 100644 --- a/src/components/application_manager/src/application_manager_impl.cc +++ b/src/components/application_manager/src/application_manager_impl.cc @@ -2447,7 +2447,6 @@ bool ApplicationManagerImpl::OnHandshakeDone( SSLContext::Handshake_Result_CertExpired, SSLContext::Handshake_Result_CertNotSigned, SSLContext::Handshake_Result_AppIDMismatch, - SSLContext::Handshake_Result_AppNameMismatch, SSLContext::Handshake_Result_NotYetValid)) { app->usage_report().RecordTLSError(); } diff --git a/src/components/include/security_manager/security_manager.h b/src/components/include/security_manager/security_manager.h index 305ade2b47..b231005010 100644 --- a/src/components/include/security_manager/security_manager.h +++ b/src/components/include/security_manager/security_manager.h @@ -69,6 +69,9 @@ class SecurityManager : public protocol_handler::ProtocolObserver, ERROR_DECRYPTION_FAILED = 0x06, ERROR_ENCRYPTION_FAILED = 0x07, ERROR_SSL_INVALID_DATA = 0x08, + ERROR_HANDSHAKE_FAILED = 0x09, // Handshake failed + ERROR_INVALID_CERT = 0x0A, // Handshake failed because cert is invalid + ERROR_EXPIRED_CERT = 0x0B, // Handshake failed because cert is expired ERROR_INTERNAL = 0xFF, ERROR_UNKNOWN_INTERNAL_ERROR = 0xFE // error value for testing }; @@ -99,24 +102,24 @@ class SecurityManager : public protocol_handler::ProtocolObserver, * \param connection_key Unique key used by other components as session * identifier * \param error_id unique error identifier - * \param erorr_text SSL impelmentation error text + * \param error_text SSL impelmentation error text * \param seq_number received from Mobile Application */ virtual void SendInternalError(const uint32_t connection_key, const uint8_t& error_id, - const std::string& erorr_text, + const std::string& error_text, const uint32_t seq_number) = 0; /** * \brief Sends InternalError override methode for sending without seq_number * \param connection_key Unique key used by other components as session * identifier * \param error_id unique error identifier - * \param erorr_text SSL impelmentation error text + * \param error_text SSL impelmentation error text */ void SendInternalError(const uint32_t connection_key, const uint8_t& error_id, - const std::string& erorr_text) { - SendInternalError(connection_key, error_id, erorr_text, 0); + const std::string& error_text) { + SendInternalError(connection_key, error_id, error_text, 0); } /** diff --git a/src/components/include/security_manager/ssl_context.h b/src/components/include/security_manager/ssl_context.h index 397cf89347..4f2caf530c 100644 --- a/src/components/include/security_manager/ssl_context.h +++ b/src/components/include/security_manager/ssl_context.h @@ -73,7 +73,6 @@ class SSLContext { Handshake_Result_NotYetValid, Handshake_Result_CertNotSigned, Handshake_Result_AppIDMismatch, - Handshake_Result_AppNameMismatch, }; struct HandshakeContext { diff --git a/src/components/protocol_handler/src/handshake_handler.cc b/src/components/protocol_handler/src/handshake_handler.cc index 78f324e0ae..87369c0793 100644 --- a/src/components/protocol_handler/src/handshake_handler.cc +++ b/src/components/protocol_handler/src/handshake_handler.cc @@ -149,8 +149,6 @@ bool HandshakeHandler::OnHandshakeDone( return "Certificate is not signed"; case security_manager::SSLContext::Handshake_Result_AppIDMismatch: return "Trying to run handshake with wrong app id"; - case security_manager::SSLContext::Handshake_Result_AppNameMismatch: - return "Trying to run handshake with wrong app name"; case security_manager::SSLContext::Handshake_Result_AbnormalFail: return "Error occurred during handshake"; case security_manager::SSLContext::Handshake_Result_Fail: diff --git a/src/components/security_manager/include/security_manager/security_manager_impl.h b/src/components/security_manager/include/security_manager/security_manager_impl.h index c4a752173e..437fede683 100644 --- a/src/components/security_manager/include/security_manager/security_manager_impl.h +++ b/src/components/security_manager/include/security_manager/security_manager_impl.h @@ -121,12 +121,12 @@ class SecurityManagerImpl : public SecurityManager, * \param connection_key Unique key used by other components as session * identifier * \param error_id unique error identifier - * \param erorr_text SSL impelmentation error text + * \param error_text SSL impelmentation error text * \param seq_number received from Mobile Application */ void SendInternalError(const uint32_t connection_key, const uint8_t& error_id, - const std::string& erorr_text, + const std::string& error_text, const uint32_t seq_number) OVERRIDE; using SecurityManager::SendInternalError; diff --git a/src/components/security_manager/src/security_manager_impl.cc b/src/components/security_manager/src/security_manager_impl.cc index f78f859c04..75cc104d95 100644 --- a/src/components/security_manager/src/security_manager_impl.cc +++ b/src/components/security_manager/src/security_manager_impl.cc @@ -557,6 +557,7 @@ bool SecurityManagerImpl::ProcessHandshakeData( // no handshake data to send return false; } + if (sslContext->IsInitCompleted()) { // On handshake success SDL_LOG_DEBUG("SSL initialization finished success."); @@ -565,6 +566,29 @@ bool SecurityManagerImpl::ProcessHandshakeData( } else if (handshake_result != SSLContext::Handshake_Result_Success) { // On handshake fail SDL_LOG_WARN("SSL initialization finished with fail."); + int32_t error_code = ERROR_HANDSHAKE_FAILED; + std::string error_text = "Handshake failed"; + switch (handshake_result) { + case SSLContext::Handshake_Result_CertExpired: + error_code = ERROR_EXPIRED_CERT; + error_text = "Certificate is expired"; + break; + case SSLContext::Handshake_Result_NotYetValid: + error_code = ERROR_INVALID_CERT; + error_text = "Certificate is not yet valid"; + break; + case SSLContext::Handshake_Result_CertNotSigned: + error_code = ERROR_INVALID_CERT; + error_text = "Certificate is not signed"; + break; + case SSLContext::Handshake_Result_AppIDMismatch: + error_code = ERROR_INVALID_CERT; + error_text = "App ID does not match certificate"; + break; + default: + break; + } + SendInternalError(connection_key, error_code, error_text); NotifyListenersOnHandshakeDone(connection_key, handshake_result); } diff --git a/src/components/security_manager/test/security_manager_test.cc b/src/components/security_manager/test/security_manager_test.cc index 7e0454e840..c2b0cda373 100644 --- a/src/components/security_manager/test/security_manager_test.cc +++ b/src/components/security_manager/test/security_manager_test.cc @@ -730,12 +730,13 @@ TEST_F(SecurityManagerTest, ProcessHandshakeData_InvalidData) { } /* * Shall send HandshakeData on getting SEND_HANDSHAKE_DATA from mobile side - * with correct handshake data Check Fail and sussecc states + * with correct handshake data Check Fail and success states */ TEST_F(SecurityManagerTest, ProcessHandshakeData_Answer) { SetMockCryptoManager(); // Count handshake calls const int handshake_emulates = 2; + const int internal_error_count = 1; uint32_t connection_id = 0; uint8_t session_id = 0; @@ -743,14 +744,14 @@ TEST_F(SecurityManagerTest, ProcessHandshakeData_Answer) { auto waiter = TestAsyncWaiter::createInstance(); uint32_t times = 0; EXPECT_CALL(mock_session_observer, PairFromKey(kKey, _, _)) - .Times(handshake_emulates) + .Times(handshake_emulates + internal_error_count) .WillRepeatedly(NotifyTestAsyncWaiter(waiter)); times += handshake_emulates; EXPECT_CALL(mock_session_observer, ProtocolVersionUsed(connection_id, session_id, An())) - .Times(handshake_emulates) + .Times(handshake_emulates + internal_error_count) .WillRepeatedly(DoAll(NotifyTestAsyncWaiter(waiter), Return(true))); - times += handshake_emulates; + times += handshake_emulates + internal_error_count; // Get size of raw message after const size_t raw_message_size = 15; @@ -759,7 +760,15 @@ TEST_F(SecurityManagerTest, ProcessHandshakeData_Answer) { RawMessageEqSize(raw_message_size), false, kIsFinal)) .Times(handshake_emulates) .WillRepeatedly(NotifyTestAsyncWaiter(waiter)); - times += handshake_emulates; + EXPECT_CALL( + mock_protocol_handler, + SendMessageToMobileApp( + InternalErrorWithErrId(SecurityManager::ERROR_HANDSHAKE_FAILED), + false, + kIsFinal)) + .Times(internal_error_count) + .WillRepeatedly(NotifyTestAsyncWaiter(waiter)); + times += handshake_emulates + internal_error_count; // Expect notifying listeners (unsuccess) EXPECT_CALL(*mock_sm_listener, @@ -803,6 +812,97 @@ TEST_F(SecurityManagerTest, ProcessHandshakeData_Answer) { // Listener was destroyed after OnHandshakeDone call mock_sm_listener.release(); } +/* + * Shall send HandshakeData on getting SEND_HANDSHAKE_DATA from mobile side + * with correct handshake data Check Fail and success states + */ +TEST_F(SecurityManagerTest, ProcessHandshakeData_Answer_Invalid_Cert) { + SetMockCryptoManager(); + // Count handshake calls + const int handshake_emulates = 4; + + uint32_t connection_id = 0; + uint8_t session_id = 0; + + auto waiter = TestAsyncWaiter::createInstance(); + uint32_t times = 0; + // Each of these calls is run twice, once for the next handshake data request, + // once for the the internal error notification + EXPECT_CALL(mock_session_observer, PairFromKey(kKey, _, _)) + .Times(handshake_emulates) + .WillRepeatedly(NotifyTestAsyncWaiter(waiter)); + times += handshake_emulates; + EXPECT_CALL(mock_session_observer, + ProtocolVersionUsed(connection_id, session_id, An())) + .Times(handshake_emulates) + .WillRepeatedly(DoAll(NotifyTestAsyncWaiter(waiter), Return(true))); + times += handshake_emulates; + + EXPECT_CALL(mock_protocol_handler, + SendMessageToMobileApp( + InternalErrorWithErrId(SecurityManager::ERROR_EXPIRED_CERT), + false, + kIsFinal)) + .WillOnce(NotifyTestAsyncWaiter(waiter)); + EXPECT_CALL(mock_protocol_handler, + SendMessageToMobileApp( + InternalErrorWithErrId(SecurityManager::ERROR_INVALID_CERT), + false, + kIsFinal)) + .Times(3) + .WillRepeatedly(NotifyTestAsyncWaiter(waiter)); + times += 4; + + // Listener is erased after first call + EXPECT_CALL(*mock_sm_listener, + OnHandshakeDone(kKey, SSLContext::Handshake_Result_CertExpired)) + .WillRepeatedly(DoAll(NotifyTestAsyncWaiter(waiter), Return(true))); + times++; + + // Emulate SessionObserver and CryptoManager result + EXPECT_CALL(mock_ssl_context_exists, IsInitCompleted()) + .Times(handshake_emulates) + .WillRepeatedly(DoAll(NotifyTestAsyncWaiter(waiter), Return(false))); + times += handshake_emulates; + EXPECT_CALL(mock_session_observer, GetSSLContext(kKey, kControl)) + .Times(handshake_emulates) + .WillRepeatedly(DoAll(NotifyTestAsyncWaiter(waiter), + Return(&mock_ssl_context_exists))); + times += handshake_emulates; + + // Emulate DoHandshakeStep correct logics + EXPECT_CALL( + mock_ssl_context_exists, + DoHandshakeStep(HandshakeStepEq(handshake_data, handshake_data_size), + handshake_data_size, + _, + _)) + .WillOnce(DoAll(SetArgPointee<2>((uint8_t*)NULL), + SetArgPointee<3>(0), + NotifyTestAsyncWaiter(waiter), + Return(SSLContext::Handshake_Result_CertExpired))) + .WillOnce(DoAll(SetArgPointee<2>((uint8_t*)NULL), + SetArgPointee<3>(0), + NotifyTestAsyncWaiter(waiter), + Return(SSLContext::Handshake_Result_NotYetValid))) + .WillOnce(DoAll(SetArgPointee<2>((uint8_t*)NULL), + SetArgPointee<3>(0), + NotifyTestAsyncWaiter(waiter), + Return(SSLContext::Handshake_Result_AppIDMismatch))) + .WillOnce(DoAll(SetArgPointee<2>((uint8_t*)NULL), + SetArgPointee<3>(0), + NotifyTestAsyncWaiter(waiter), + Return(SSLContext::Handshake_Result_CertNotSigned))); + times += 4; // matches to each single call above + + EmulateMobileMessageHandshake( + handshake_data, handshake_data_size, handshake_emulates); + + EXPECT_TRUE(waiter->WaitFor(times, kAsyncExpectationsTimeout)); + + // Listener was destroyed after OnHandshakeDone call + mock_sm_listener.release(); +} /* * Shall call all listeners on success end handshake * and return handshake data -- cgit v1.2.1