From e8ed977a8f54e20f526e6fcf0bd6529b122b163a Mon Sep 17 00:00:00 2001 From: Nick Bolton Date: Wed, 1 May 2013 15:53:22 +0000 Subject: [PATCH] fixed: issue 3565 - encryption fails with heavy network traffic. changed encrypt and decrypt to be asymmetrical (iv change now applies only in one direction). --- src/lib/client/CClient.cpp | 4 +- src/lib/client/CClient.h | 4 +- src/lib/client/CServerProxy.cpp | 2 +- src/lib/server/CClientProxy1_4.cpp | 2 +- src/lib/synergy/CCryptoStream.cpp | 15 ++- src/lib/synergy/CCryptoStream.h | 7 +- src/test/unittests/client/CMockClient.h | 2 +- .../unittests/server/CClientProxyTests.cpp | 118 ++++++++++++------ .../unittests/synergy/CCryptoStreamTests.cpp | 21 ++-- 9 files changed, 111 insertions(+), 64 deletions(-) diff --git a/src/lib/client/CClient.cpp b/src/lib/client/CClient.cpp index f2243b7d..8c6a862c 100644 --- a/src/lib/client/CClient.cpp +++ b/src/lib/client/CClient.cpp @@ -194,10 +194,10 @@ CClient::handshakeComplete() } void -CClient::setCryptoIv(const UInt8* iv) +CClient::setDecryptIv(const UInt8* iv) { if (m_cryptoStream != NULL) { - m_cryptoStream->setIv(iv); + m_cryptoStream->setDecryptIv(iv); } } diff --git a/src/lib/client/CClient.h b/src/lib/client/CClient.h index 86d40cab..06507d3b 100644 --- a/src/lib/client/CClient.h +++ b/src/lib/client/CClient.h @@ -88,8 +88,8 @@ public: */ virtual void handshakeComplete(); - //! Set crypto IV - virtual void setCryptoIv(const UInt8* iv); + //! Set crypto IV for decryption + virtual void setDecryptIv(const UInt8* iv); //@} //! @name accessors diff --git a/src/lib/client/CServerProxy.cpp b/src/lib/client/CServerProxy.cpp index 682f286a..31508ce6 100644 --- a/src/lib/client/CServerProxy.cpp +++ b/src/lib/client/CServerProxy.cpp @@ -838,7 +838,7 @@ CServerProxy::cryptoIv() LOG((CLOG_DEBUG2 "recv crypto iv size=%i", s.size())); // forward - m_client->setCryptoIv(reinterpret_cast(s.c_str())); + m_client->setDecryptIv(reinterpret_cast(s.c_str())); } void diff --git a/src/lib/server/CClientProxy1_4.cpp b/src/lib/server/CClientProxy1_4.cpp index 73473356..a77deb23 100644 --- a/src/lib/server/CClientProxy1_4.cpp +++ b/src/lib/server/CClientProxy1_4.cpp @@ -106,7 +106,7 @@ CClientProxy1_4::cryptoIv() // change IV only after we've sent the current IV, otherwise // the client won't be able to decrypt the new IV. - cryptoStream->setIv(iv); + cryptoStream->setEncryptIv(iv); } bool diff --git a/src/lib/synergy/CCryptoStream.cpp b/src/lib/synergy/CCryptoStream.cpp index e7824247..758277af 100644 --- a/src/lib/synergy/CCryptoStream.cpp +++ b/src/lib/synergy/CCryptoStream.cpp @@ -43,7 +43,8 @@ CCryptoStream::CCryptoStream( byte iv[CRYPTO_IV_SIZE]; createKey(iv, options.m_pass, CRYPTO_IV_SIZE, static_cast(options.m_pass.length()) * 2); - setIv(iv); + setEncryptIv(iv); + setDecryptIv(iv); } } @@ -111,12 +112,18 @@ CCryptoStream::createKey(byte* out, const CString& password, UInt8 keyLength, UI } void -CCryptoStream::setIv(const byte* iv) +CCryptoStream::setEncryptIv(const byte* iv) { assert(m_key != NULL); - logBuffer("iv", iv, CRYPTO_IV_SIZE); - + logBuffer("encrypt iv", iv, CRYPTO_IV_SIZE); m_encryption.setKeyWithIv(m_key, kKeyLength, iv); +} + +void +CCryptoStream::setDecryptIv(const byte* iv) +{ + assert(m_key != NULL); + logBuffer("decrypt iv", iv, CRYPTO_IV_SIZE); m_decryption.setKeyWithIv(m_key, kKeyLength, iv); } diff --git a/src/lib/synergy/CCryptoStream.h b/src/lib/synergy/CCryptoStream.h index a19ed29e..104b1f61 100644 --- a/src/lib/synergy/CCryptoStream.h +++ b/src/lib/synergy/CCryptoStream.h @@ -52,8 +52,11 @@ public: */ virtual void write(const void* in, UInt32 n); - //! Set the IV - void setIv(const byte* iv); + //! Set the IV for encryption + void setEncryptIv(const byte* iv); + + //! Set the IV for decryption + void setDecryptIv(const byte* iv); //! Get a new IV /*! diff --git a/src/test/unittests/client/CMockClient.h b/src/test/unittests/client/CMockClient.h index c83f4be0..ea73f423 100644 --- a/src/test/unittests/client/CMockClient.h +++ b/src/test/unittests/client/CMockClient.h @@ -32,5 +32,5 @@ public: MOCK_METHOD2(mouseMove, void(SInt32, SInt32)); MOCK_METHOD1(setOptions, void(const COptionsList&)); MOCK_METHOD0(handshakeComplete, void()); - MOCK_METHOD1(setCryptoIv, void(const UInt8*)); + MOCK_METHOD1(setDecryptIv, void(const UInt8*)); }; diff --git a/src/test/unittests/server/CClientProxyTests.cpp b/src/test/unittests/server/CClientProxyTests.cpp index 0dea12af..e3f8b584 100644 --- a/src/test/unittests/server/CClientProxyTests.cpp +++ b/src/test/unittests/server/CClientProxyTests.cpp @@ -26,69 +26,105 @@ using ::testing::_; using ::testing::NiceMock; using ::testing::Invoke; -const UInt8 cryptoIvWrite_bufferLen = 200; -UInt8 cryptoIvWrite_buffer[cryptoIvWrite_bufferLen]; -UInt32 cryptoIvWrite_bufferIndex; +const UInt8 g_cryptoIvWrite_bufferLen = 200; +UInt8 g_cryptoIvWrite_buffer[g_cryptoIvWrite_bufferLen]; +UInt32 g_cryptoIvWrite_writeBufferIndex; +UInt32 g_cryptoIvWrite_readBufferIndex; -void -cryptoIv_mockWrite(const void* in, UInt32 n); +void cryptoIv_mockWrite(const void* in, UInt32 n); +UInt8 cryptoIv_mockRead(void* out, UInt32 n); TEST(CClientProxyTests, cryptoIvWrite) { - cryptoIvWrite_bufferIndex = 0; + g_cryptoIvWrite_writeBufferIndex = 0; + g_cryptoIvWrite_readBufferIndex = 0; NiceMock eventQueue; NiceMock innerStream; NiceMock server; - NiceMock* stream = new NiceMock(&eventQueue, &innerStream); + CCryptoOptions options("ctr", "mock"); - ON_CALL(*stream, write(_, _)).WillByDefault(Invoke(cryptoIv_mockWrite)); + CCryptoStream* serverStream = new CCryptoStream(&eventQueue, &innerStream, options, false); + CCryptoStream* clientStream = new CCryptoStream(&eventQueue, &innerStream, options, false); - CClientProxy1_4 clientProxy("stub", stream, &server, &eventQueue); + byte iv[CRYPTO_IV_SIZE]; + serverStream->newIv(iv); + serverStream->setEncryptIv(iv); + clientStream->setDecryptIv(iv); + + ON_CALL(innerStream, write(_, _)).WillByDefault(Invoke(cryptoIv_mockWrite)); + ON_CALL(innerStream, read(_, _)).WillByDefault(Invoke(cryptoIv_mockRead)); + + CClientProxy1_4 clientProxy("stub", serverStream, &server, &eventQueue); + + UInt8 buffer[100]; + clientStream->read(buffer, 4); + + g_cryptoIvWrite_writeBufferIndex = 0; + g_cryptoIvWrite_readBufferIndex = 0; // DCIV, then DKDN. - cryptoIvWrite_bufferIndex = 0; clientProxy.keyDown(1, 2, 3); - EXPECT_EQ('D', cryptoIvWrite_buffer[0]); - EXPECT_EQ('C', cryptoIvWrite_buffer[1]); - EXPECT_EQ('I', cryptoIvWrite_buffer[2]); - EXPECT_EQ('V', cryptoIvWrite_buffer[3]); - EXPECT_EQ('D', cryptoIvWrite_buffer[24]); - EXPECT_EQ('K', cryptoIvWrite_buffer[25]); - EXPECT_EQ('D', cryptoIvWrite_buffer[26]); - EXPECT_EQ('N', cryptoIvWrite_buffer[27]); + clientStream->read(buffer, 24); + EXPECT_EQ('D', buffer[0]); + EXPECT_EQ('C', buffer[1]); + EXPECT_EQ('I', buffer[2]); + EXPECT_EQ('V', buffer[3]); + clientStream->setDecryptIv(&buffer[8]); + clientStream->read(buffer, 10); + EXPECT_EQ('D', buffer[0]); + EXPECT_EQ('K', buffer[1]); + EXPECT_EQ('D', buffer[2]); + EXPECT_EQ('N', buffer[3]); + g_cryptoIvWrite_writeBufferIndex = 0; + g_cryptoIvWrite_readBufferIndex = 0; + // DCIV, then DKUP. - cryptoIvWrite_bufferIndex = 0; clientProxy.keyUp(1, 2, 3); - EXPECT_EQ('D', cryptoIvWrite_buffer[0]); - EXPECT_EQ('C', cryptoIvWrite_buffer[1]); - EXPECT_EQ('I', cryptoIvWrite_buffer[2]); - EXPECT_EQ('V', cryptoIvWrite_buffer[3]); - EXPECT_EQ('D', cryptoIvWrite_buffer[24]); - EXPECT_EQ('K', cryptoIvWrite_buffer[25]); - EXPECT_EQ('U', cryptoIvWrite_buffer[26]); - EXPECT_EQ('P', cryptoIvWrite_buffer[27]); + clientStream->read(buffer, 24); + EXPECT_EQ('D', buffer[0]); + EXPECT_EQ('C', buffer[1]); + EXPECT_EQ('I', buffer[2]); + EXPECT_EQ('V', buffer[3]); + clientStream->setDecryptIv(&buffer[8]); + clientStream->read(buffer, 10); + EXPECT_EQ('D', buffer[0]); + EXPECT_EQ('K', buffer[1]); + EXPECT_EQ('U', buffer[2]); + EXPECT_EQ('P', buffer[3]); + + g_cryptoIvWrite_writeBufferIndex = 0; + g_cryptoIvWrite_readBufferIndex = 0; // DCIV, then DKRP. - cryptoIvWrite_bufferIndex = 0; clientProxy.keyRepeat(1, 2, 4, 4); - EXPECT_EQ('D', cryptoIvWrite_buffer[0]); - EXPECT_EQ('C', cryptoIvWrite_buffer[1]); - EXPECT_EQ('I', cryptoIvWrite_buffer[2]); - EXPECT_EQ('V', cryptoIvWrite_buffer[3]); - EXPECT_EQ('D', cryptoIvWrite_buffer[24]); - EXPECT_EQ('K', cryptoIvWrite_buffer[25]); - EXPECT_EQ('R', cryptoIvWrite_buffer[26]); - EXPECT_EQ('P', cryptoIvWrite_buffer[27]); + clientStream->read(buffer, 24); + EXPECT_EQ('D', buffer[0]); + EXPECT_EQ('C', buffer[1]); + EXPECT_EQ('I', buffer[2]); + EXPECT_EQ('V', buffer[3]); + clientStream->setDecryptIv(&buffer[8]); + clientStream->read(buffer, 12); + EXPECT_EQ('D', buffer[0]); + EXPECT_EQ('K', buffer[1]); + EXPECT_EQ('R', buffer[2]); + EXPECT_EQ('P', buffer[3]); } void cryptoIv_mockWrite(const void* in, UInt32 n) { - if (cryptoIvWrite_bufferIndex >= cryptoIvWrite_bufferLen) { - return; - } - memcpy(&cryptoIvWrite_buffer[cryptoIvWrite_bufferIndex], in, n); - cryptoIvWrite_bufferIndex += n; + assert(g_cryptoIvWrite_writeBufferIndex <= sizeof(g_cryptoIvWrite_buffer)); + memcpy(&g_cryptoIvWrite_buffer[g_cryptoIvWrite_writeBufferIndex], in, n); + g_cryptoIvWrite_writeBufferIndex += n; +} + +UInt8 +cryptoIv_mockRead(void* out, UInt32 n) +{ + assert(g_cryptoIvWrite_readBufferIndex <= sizeof(g_cryptoIvWrite_buffer)); + memcpy(out, &g_cryptoIvWrite_buffer[g_cryptoIvWrite_readBufferIndex], n); + g_cryptoIvWrite_readBufferIndex += n; + return n; } diff --git a/src/test/unittests/synergy/CCryptoStreamTests.cpp b/src/test/unittests/synergy/CCryptoStreamTests.cpp index 94470f7a..b4817106 100644 --- a/src/test/unittests/synergy/CCryptoStreamTests.cpp +++ b/src/test/unittests/synergy/CCryptoStreamTests.cpp @@ -75,7 +75,7 @@ TEST(CCryptoTests, write) ON_CALL(innerStream, write(_, _)).WillByDefault(Invoke(write_mockWrite)); CCryptoStream cs(&eventQueue, &innerStream, options, false); - cs.setIv(kIv); + cs.setEncryptIv(kIv); cs.write(buffer, size); EXPECT_EQ(95, g_write_buffer[0]); @@ -93,7 +93,8 @@ TEST(CCryptoTests, read) ON_CALL(innerStream, read(_, _)).WillByDefault(Invoke(read_mockRead)); CCryptoStream cs(&eventQueue, &innerStream, options, false); - cs.setIv(kIv); + cs.setEncryptIv(kIv); + cs.setDecryptIv(kIv); g_read_buffer[0] = 95; g_read_buffer[1] = 107; @@ -122,7 +123,7 @@ TEST(CCryptoTests, write4Read1) ON_CALL(innerStream, read(_, _)).WillByDefault(Invoke(write4Read1_mockRead)); CCryptoStream cs1(&eventQueue, &innerStream, options, false); - cs1.setIv(kIv); + cs1.setEncryptIv(kIv); cs1.write("a", 1); cs1.write("b", 1); @@ -130,7 +131,7 @@ TEST(CCryptoTests, write4Read1) cs1.write("d", 1); CCryptoStream cs2(&eventQueue, &innerStream, options, false); - cs2.setIv(kIv); + cs2.setDecryptIv(kIv); UInt8 buffer[4]; cs2.read(buffer, 4); @@ -153,7 +154,7 @@ TEST(CCryptoTests, write1Read4) ON_CALL(innerStream, read(_, _)).WillByDefault(Invoke(write1Read4_mockRead)); CCryptoStream cs1(&eventQueue, &innerStream, options, false); - cs1.setIv(kIv); + cs1.setEncryptIv(kIv); UInt8 bufferIn[4]; bufferIn[0] = 'a'; @@ -163,7 +164,7 @@ TEST(CCryptoTests, write1Read4) cs1.write(bufferIn, 4); CCryptoStream cs2(&eventQueue, &innerStream, options, false); - cs2.setIv(kIv); + cs2.setDecryptIv(kIv); UInt8 bufferOut[4]; cs2.read(&bufferOut[0], 1); @@ -194,7 +195,7 @@ TEST(CCryptoTests, readWriteIvChanged) const byte iv2[] = "bbbbbbbbbbbbbbbb"; CCryptoStream cs1(&eventQueue, &innerStream, options, false); - cs1.setIv(iv1); + cs1.setEncryptIv(iv1); UInt8 bufferIn[4]; bufferIn[0] = 'a'; @@ -204,7 +205,7 @@ TEST(CCryptoTests, readWriteIvChanged) cs1.write(bufferIn, 4); CCryptoStream cs2(&eventQueue, &innerStream, options, false); - cs1.setIv(iv2); + cs1.setDecryptIv(iv2); UInt8 bufferOut[4]; cs2.read(bufferOut, 4); @@ -220,8 +221,8 @@ TEST(CCryptoTests, readWriteIvChanged) // ensure that the new IV is used. byte iv[CRYPTO_IV_SIZE]; cs1.newIv(iv); - cs1.setIv(iv); - cs2.setIv(iv); + cs1.setEncryptIv(iv); + cs2.setDecryptIv(iv); cs1.write(bufferIn, 4); cs2.read(bufferOut, 4);