Merge branch 'APPLINK-8165-SDL_assert_on_SSLBroken' into develop

No subject for review

5 files changed, 85 insertions, 11 deletions

diff --git a/src/components/connection_handler/src/connection_handler_impl.cc b/src/components/connection_handler/src/connection_handler_impl.cc
index 30d06b4cef..1e4d66aad3 100644
--- a/src/components/connection_handler/src/connection_handler_impl.cc
+++ b/src/components/connection_handler/src/connection_handler_impl.cc
@@ -347,7 +347,7 @@ uint32_t ConnectionHandlerImpl::OnSessionEndedCallback(
   sync_primitives::AutoLock lock(connection_list_lock_);
   ConnectionList::iterator it = connection_list_.find(connection_handle);
   if (connection_list_.end() == it) {
-    LOG4CXX_ERROR(logger_, "Unknown connection!");
+    LOG4CXX_WARN(logger_, "Unknown connection!");
     return 0;
   }
   Connection *connection = it->second;
@@ -356,14 +356,14 @@ uint32_t ConnectionHandlerImpl::OnSessionEndedCallback(
     LOG4CXX_INFO(logger_, "Session "  << static_cast<uint32_t>(session_id)
                  << " to be removed");
     if (!connection->RemoveSession(session_id)) {
-      LOG4CXX_ERROR(logger_, "Not possible to remove session!");
+      LOG4CXX_WARN(logger_, "Not possible to remove session!");
       return 0;
     }
   } else {
     LOG4CXX_INFO(logger_, "Service "  << static_cast<uint32_t>(service_type)
                  << " to be removed");
     if (!connection->RemoveService(session_id, service_type)) {
-      LOG4CXX_ERROR(logger_, "Not possible to remove service!");
+      LOG4CXX_WARN(logger_, "Not possible to remove service!");
       return 0;
     }
   }
diff --git a/src/components/protocol_handler/src/protocol_handler_impl.cc b/src/components/protocol_handler/src/protocol_handler_impl.cc
index e02dcc2aa5..e109804d15 100644
--- a/src/components/protocol_handler/src/protocol_handler_impl.cc
+++ b/src/components/protocol_handler/src/protocol_handler_impl.cc
@@ -481,7 +481,6 @@ void ProtocolHandlerImpl::OnTMMessageReceiveFailed(
 }
 
 void ProtocolHandlerImpl::NotifySubscribers(const RawMessagePtr message) {
-  LOG4CXX_TRACE(logger_, "ProtocolHandlerImpl::NotifySubscribers");
   sync_primitives::AutoLock lock(protocol_observers_lock_);
   for (ProtocolObservers::iterator it = protocol_observers_.begin();
       protocol_observers_.end() != it; ++it) {
@@ -750,6 +749,7 @@ RESULT_CODE ProtocolHandlerImpl::HandleSingleFrameMessage(
       }
 #endif
 
+  // TODO(EZamakhov): check service in session
   NotifySubscribers(rawMessage);
   LOG4CXX_TRACE_EXIT(logger_);
   return RESULT_OK;
@@ -838,6 +838,7 @@ RESULT_CODE ProtocolHandlerImpl::HandleMultiFrameMessage(
         metric_observer_->EndMessageProcess(metric);
       }
 #endif  // TIME_TESTER
+      // TODO(EZamakhov): check service in session
       NotifySubscribers(rawMessage);
 
       incomplete_multi_frame_messages_.erase(it);
@@ -1162,12 +1163,17 @@ RESULT_CODE ProtocolHandlerImpl::EncryptFrame(ProtocolFramePtr packet) {
     LOG4CXX_ERROR(logger_, "Enryption failed: " << error_text);
     security_manager_->SendInternalError(connection_key,
           security_manager::SecurityManager::ERROR_ENCRYPTION_FAILED, error_text);
+    // Close session to prevent usage unprotected service/session
+    session_observer_->OnSessionEndedCallback(
+          packet->connection_id(), packet->session_id(),
+          packet->message_id(),    kRpc);
     return RESULT_OK;
   };
   LOG4CXX_DEBUG(logger_, "Encrypted " << packet->data_size() << " bytes to "
                 << out_data_size << " bytes");
   DCHECK(out_data);
   DCHECK(out_data_size);
+  DCHECK(out_data_size <= MAXIMUM_FRAME_DATA_SIZE);
   packet->set_protection_flag(true);
   packet->set_data(out_data, out_data_size);
   return RESULT_OK;
@@ -1208,6 +1214,10 @@ RESULT_CODE ProtocolHandlerImpl::DecryptFrame(ProtocolFramePtr packet) {
     LOG4CXX_ERROR(logger_, "Decryption failed: " << error_text);
     security_manager_->SendInternalError(connection_key,
           security_manager::SecurityManager::ERROR_DECRYPTION_FAILED, error_text);
+    // Close session to prevent usage unprotected service/session
+    session_observer_->OnSessionEndedCallback(
+          packet->connection_id(), packet->session_id(),
+          packet->message_id(),    kRpc);
     return RESULT_ENCRYPTION_FAILED;
   };
   LOG4CXX_DEBUG(logger_, "Decrypted " << packet->data_size() << " bytes to "
diff --git a/src/components/security_manager/include/security_manager/crypto_manager_impl.h b/src/components/security_manager/include/security_manager/crypto_manager_impl.h
index a5b4037cf3..43bb63ef67 100644
--- a/src/components/security_manager/include/security_manager/crypto_manager_impl.h
+++ b/src/components/security_manager/include/security_manager/crypto_manager_impl.h
@@ -43,6 +43,7 @@
 #include "security_manager/crypto_manager.h"
 #include "security_manager/ssl_context.h"
 #include "utils/macro.h"
+#include "utils/lock.h"
 
 namespace security_manager {
 class CryptoManagerImpl : public CryptoManager {
@@ -73,6 +74,7 @@ class CryptoManagerImpl : public CryptoManager {
     BIO *bioIn_;
     BIO *bioOut_;
     BIO *bioFilter_;
+    mutable sync_primitives::Lock bio_locker;
     size_t buffer_size_;
     uint8_t *buffer_;
     bool is_handshake_pending_;
diff --git a/src/components/security_manager/src/ssl_context_impl.cc b/src/components/security_manager/src/ssl_context_impl.cc
index 737c53fd03..94f67f8885 100644
--- a/src/components/security_manager/src/ssl_context_impl.cc
+++ b/src/components/security_manager/src/ssl_context_impl.cc
@@ -65,6 +65,7 @@ std::string CryptoManagerImpl::SSLContextImpl::LastError() const {
 }
 
 bool CryptoManagerImpl::SSLContextImpl::IsInitCompleted() const {
+  sync_primitives::AutoLock locker(bio_locker);
   return SSL_is_init_finished(connection_);
 }
 
@@ -132,16 +133,18 @@ CryptoManagerImpl::SSLContextImpl::max_block_sizes =
 
 SSLContext::HandshakeResult CryptoManagerImpl::SSLContextImpl::
 DoHandshakeStep(const uint8_t*  const in_data,  size_t in_data_size,
-                const uint8_t** const out_data, size_t *out_data_size) {
+                const uint8_t** const out_data, size_t* out_data_size) {
+  DCHECK(out_data);
+  DCHECK(out_data_size);
   // TODO(Ezamakhov): add test - hanshake fail -> restart StartHandshake
-  *out_data = NULL;
-  if (IsInitCompleted()) {
+  sync_primitives::AutoLock locker(bio_locker);
+  if (SSL_is_init_finished(connection_)) {
     is_handshake_pending_ = false;
     return SSLContext::Handshake_Result_Success;
   }
 
   if (in_data && in_data_size) {
-    int ret = BIO_write(bioIn_, in_data, in_data_size);
+    const int ret = BIO_write(bioIn_, in_data, in_data_size);
     if (ret <= 0) {
       is_handshake_pending_ = false;
       SSL_clear(connection_);
@@ -174,7 +177,7 @@ DoHandshakeStep(const uint8_t*  const in_data,  size_t in_data_size,
     EnsureBufferSizeEnough(pend);
 
     const int read_count = BIO_read(bioOut_, buffer_, pend);
-    if (read_count  == pend) {
+    if (read_count  == static_cast<int>(pend)) {
       *out_data_size = read_count;
       *out_data =  buffer_;
     } else {
@@ -191,6 +194,7 @@ bool CryptoManagerImpl::SSLContextImpl::Encrypt(
     const uint8_t *  const in_data,  size_t in_data_size,
     const uint8_t ** const out_data, size_t *out_data_size) {
 
+  sync_primitives::AutoLock locker(bio_locker);
   if (!SSL_is_init_finished(connection_) ||
       !in_data ||
       !in_data_size) {
@@ -203,7 +207,9 @@ bool CryptoManagerImpl::SSLContextImpl::Encrypt(
   EnsureBufferSizeEnough(len);
   const int read_size = BIO_read(bioOut_, buffer_, len);
   DCHECK(len == read_size);
-  if (read_size < 0) {
+  if (read_size <= 0) {
+    // Reset filter and connection deinitilization instead
+    BIO_reset(bioFilter_);
     return false;
   }
   *out_data_size = read_size;
@@ -216,6 +222,7 @@ bool CryptoManagerImpl::SSLContextImpl::Decrypt(
     const uint8_t *  const in_data,  size_t in_data_size,
     const uint8_t ** const out_data, size_t *out_data_size) {
 
+  sync_primitives::AutoLock locker(bio_locker);
   if (!SSL_is_init_finished(connection_)) {
     return false;
   }
@@ -231,8 +238,12 @@ bool CryptoManagerImpl::SSLContextImpl::Decrypt(
   while (len) {
     EnsureBufferSizeEnough(len + offset);
     len = BIO_read(bioFilter_, buffer_ + offset, len);
-    if (len < 0)
+    // TODO(EZamakhov): investigate BIO_read return 0, -1 and -2 meanings
+    if (len <= 0) {
+      // Reset filter and connection deinitilization instead
+      BIO_reset(bioFilter_);
       return false;
+    }
     *out_data_size += len;
     offset += len;
     len = BIO_ctrl_pending(bioFilter_);
diff --git a/test/components/security_manager/include/security_manager/crypto_manager_impl_test.h b/test/components/security_manager/include/security_manager/crypto_manager_impl_test.h
index 0f4b323a34..8d6d2af90c 100644
--- a/test/components/security_manager/include/security_manager/crypto_manager_impl_test.h
+++ b/test/components/security_manager/include/security_manager/crypto_manager_impl_test.h
@@ -256,6 +256,57 @@ TEST_F(SSLTest, Positive) {
                     4), 0);
 }
 
+TEST_F(SSLTest, EcncryptionFail) {
+
+  const uint8_t *server_buf;
+  const uint8_t *client_buf;
+  size_t server_buf_len;
+  size_t client_buf_len;
+  ASSERT_EQ(client_ctx->StartHandshake(&client_buf,
+                                       &client_buf_len),
+            security_manager::SSLContext::Handshake_Result_Success);
+
+  while (!server_ctx->IsInitCompleted()) {
+    ASSERT_FALSE(client_buf == NULL);
+    ASSERT_GT(client_buf_len, 0u);
+    ASSERT_EQ(server_ctx->DoHandshakeStep(client_buf, client_buf_len,
+                                          &server_buf, &server_buf_len),
+              security_manager::SSLContext::Handshake_Result_Success);
+    ASSERT_FALSE(server_buf == NULL);
+    ASSERT_GT(server_buf_len, 0u);
+
+    ASSERT_EQ(client_ctx->DoHandshakeStep(server_buf, server_buf_len,
+                                          &client_buf, &client_buf_len),
+              security_manager::SSLContext::Handshake_Result_Success);
+  }
+
+  EXPECT_TRUE(client_ctx->IsInitCompleted());
+  EXPECT_TRUE(server_ctx->IsInitCompleted());
+
+  // Encrypt text on client side
+  const uint8_t *text = reinterpret_cast<const uint8_t*>("abra");
+  const uint8_t *encrypted_text = 0;
+  size_t text_len = 4;
+  size_t encrypted_text_len;
+  EXPECT_TRUE(client_ctx->Encrypt(text, text_len, &encrypted_text, &encrypted_text_len));
+  ASSERT_NE(encrypted_text, (void*)NULL);
+  ASSERT_GT(encrypted_text_len, 0u);
+
+  std::vector<uint8_t> broken(encrypted_text, encrypted_text + encrypted_text_len);
+  // Broke message
+  broken[encrypted_text_len / 2] ^= 0xFF;
+
+  const uint8_t *out_text;
+  size_t out_text_size;
+  // Decrypt broken text on server side
+  EXPECT_FALSE(server_ctx->Decrypt(&broken[0], broken.size(), &out_text, &out_text_size));
+
+  // Check after broken message that server encryption and decryption fail
+  // Encrypte message on server side
+  EXPECT_FALSE(server_ctx->Decrypt(encrypted_text, encrypted_text_len, &out_text, &out_text_size));
+  EXPECT_FALSE(server_ctx->Encrypt(text, text_len, &encrypted_text, &encrypted_text_len));
+}
+
 /*
 TEST_F(SSLTest, DISABLED_BadData) {
   using security_manager::LastError;