diff --git a/src/CryptState.cpp b/src/CryptState.cpp index daeda9737..519688fd7 100644 --- a/src/CryptState.cpp +++ b/src/CryptState.cpp @@ -68,7 +68,7 @@ void CryptState::setDecryptIV(const unsigned char *iv) { memcpy(decrypt_iv, iv, AES_BLOCK_SIZE); } -void CryptState::encrypt(const unsigned char *source, unsigned char *dst, unsigned int plain_length) { +bool CryptState::encrypt(const unsigned char *source, unsigned char *dst, unsigned int plain_length) { unsigned char tag[AES_BLOCK_SIZE]; // First, increase our IV. @@ -76,12 +76,15 @@ void CryptState::encrypt(const unsigned char *source, unsigned char *dst, unsign if (++encrypt_iv[i]) break; - ocb_encrypt(source, dst+4, plain_length, encrypt_iv, tag); + if (!ocb_encrypt(source, dst+4, plain_length, encrypt_iv, tag)) { + return false; + } dst[0] = encrypt_iv[0]; dst[1] = tag[0]; dst[2] = tag[1]; dst[3] = tag[2]; + return true; } bool CryptState::decrypt(const unsigned char *source, unsigned char *dst, unsigned int crypted_length) { @@ -157,9 +160,9 @@ bool CryptState::decrypt(const unsigned char *source, unsigned char *dst, unsign } } - ocb_decrypt(source+4, dst, plain_length, decrypt_iv, tag); + bool ocb_success = ocb_decrypt(source+4, dst, plain_length, decrypt_iv, tag); - if (memcmp(tag, source+1, 3) != 0) { + if (!ocb_success || memcmp(tag, source+1, 3) != 0) { memcpy(decrypt_iv, saveiv, AES_BLOCK_SIZE); return false; } @@ -224,8 +227,9 @@ static void inline ZERO(keyblock &block) { #define AESencrypt(src,dst,key) AES_encrypt(reinterpret_cast(src),reinterpret_cast(dst), key); #define AESdecrypt(src,dst,key) AES_decrypt(reinterpret_cast(src),reinterpret_cast(dst), key); -void CryptState::ocb_encrypt(const unsigned char *plain, unsigned char *encrypted, unsigned int len, const unsigned char *nonce, unsigned char *tag) { +bool CryptState::ocb_encrypt(const unsigned char *plain, unsigned char *encrypted, unsigned int len, const unsigned char *nonce, unsigned char *tag) { keyblock checksum, delta, tmp, pad; + bool success = true; // Initialize AESencrypt(nonce, delta, &encrypt_key); @@ -237,6 +241,18 @@ void CryptState::ocb_encrypt(const unsigned char *plain, unsigned char *encrypte AESencrypt(tmp, tmp, &encrypt_key); XOR(reinterpret_cast(encrypted), delta, tmp); XOR(checksum, checksum, reinterpret_cast(plain)); + + // Counter-cryptanalysis described in section 9 of https://eprint.iacr.org/2019/311 + // For an attack, the second to last block (i.e. the last iteration of this loop) + // must be all 0 except for the last byte (which may be 0 - 128). + if (len - AES_BLOCK_SIZE <= AES_BLOCK_SIZE) { + unsigned char sum = 0; + for (int i = 0; i < AES_BLOCK_SIZE - 1; ++i) { + sum |= plain[i]; + } + success &= sum != 0; + } + len -= AES_BLOCK_SIZE; plain += AES_BLOCK_SIZE; encrypted += AES_BLOCK_SIZE; @@ -256,10 +272,13 @@ void CryptState::ocb_encrypt(const unsigned char *plain, unsigned char *encrypte S3(delta); XOR(tmp, delta, checksum); AESencrypt(tmp, tag, &encrypt_key); + + return success; } -void CryptState::ocb_decrypt(const unsigned char *encrypted, unsigned char *plain, unsigned int len, const unsigned char *nonce, unsigned char *tag) { +bool CryptState::ocb_decrypt(const unsigned char *encrypted, unsigned char *plain, unsigned int len, const unsigned char *nonce, unsigned char *tag) { keyblock checksum, delta, tmp, pad; + bool success = true; // Initialize AESencrypt(nonce, delta, &encrypt_key); @@ -287,7 +306,18 @@ void CryptState::ocb_decrypt(const unsigned char *encrypted, unsigned char *plai XOR(checksum, checksum, tmp); memcpy(plain, tmp, len); + // Counter-cryptanalysis described in section 9 of https://eprint.iacr.org/2019/311 + // In an attack, the decrypted last block would need to equal `delta ^ len(128)`. + // With a bit of luck (or many packets), smaller values than 128 (i.e. non-full blocks) are also + // feasible, so we check `tmp` instead of `plain`. + // Since our `len` only ever modifies the last byte, we simply check all remaining ones. + if (memcmp(tmp, delta, AES_BLOCK_SIZE - 1) == 0) { + success = false; + } + S3(delta); XOR(tmp, delta, checksum); AESencrypt(tmp, tag, &encrypt_key); + + return success; } diff --git a/src/CryptState.h b/src/CryptState.h index 9b5d6e408..acfbe5c74 100644 --- a/src/CryptState.h +++ b/src/CryptState.h @@ -44,11 +44,11 @@ class CryptState { void setKey(const unsigned char *rkey, const unsigned char *eiv, const unsigned char *div); void setDecryptIV(const unsigned char *iv); - void ocb_encrypt(const unsigned char *plain, unsigned char *encrypted, unsigned int len, const unsigned char *nonce, unsigned char *tag); - void ocb_decrypt(const unsigned char *encrypted, unsigned char *plain, unsigned int len, const unsigned char *nonce, unsigned char *tag); + bool ocb_encrypt(const unsigned char *plain, unsigned char *encrypted, unsigned int len, const unsigned char *nonce, unsigned char *tag); + bool ocb_decrypt(const unsigned char *encrypted, unsigned char *plain, unsigned int len, const unsigned char *nonce, unsigned char *tag); bool decrypt(const unsigned char *source, unsigned char *dst, unsigned int crypted_length); - void encrypt(const unsigned char *source, unsigned char *dst, unsigned int plain_length); + bool encrypt(const unsigned char *source, unsigned char *dst, unsigned int plain_length); }; #endif diff --git a/src/mumble/ServerHandler.cpp b/src/mumble/ServerHandler.cpp index c9000d3f0..a5bd943de 100644 --- a/src/mumble/ServerHandler.cpp +++ b/src/mumble/ServerHandler.cpp @@ -275,7 +275,9 @@ void ServerHandler::sendMessage(const char *data, int len, bool force) { QApplication::postEvent(this, new ServerHandlerMessageEvent(qba, MessageHandler::UDPTunnel, true)); } else { - connection->csCrypt.encrypt(reinterpret_cast(data), crypto, len); + if (!connection->csCrypt.encrypt(reinterpret_cast(data), crypto, len)) { + return; + } qusUdp->writeDatagram(reinterpret_cast(crypto), len + 4, qhaRemote, usResolvedPort); } } @@ -1065,4 +1067,3 @@ QUrl ServerHandler::getServerURL(bool withPassword) const { return url; } - diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp index b845e5594..6b6bf8948 100644 --- a/src/murmur/Server.cpp +++ b/src/murmur/Server.cpp @@ -952,8 +952,9 @@ void Server::sendMessage(ServerUser *u, const char *data, int len, QByteArray &c return; } - u->csCrypt.encrypt(reinterpret_cast(data), reinterpret_cast(buffer), - len); + if (!u->csCrypt.encrypt(reinterpret_cast(data), reinterpret_cast(buffer), len)) { + return; + } } #ifdef Q_OS_WIN DWORD dwFlow = 0; diff --git a/src/tests/TestCrypt/TestCrypt.cpp b/src/tests/TestCrypt/TestCrypt.cpp index 3a904a132..1b621967f 100644 --- a/src/tests/TestCrypt/TestCrypt.cpp +++ b/src/tests/TestCrypt/TestCrypt.cpp @@ -18,6 +18,7 @@ class TestCrypt : public QObject { void cleanupTestCase(); void testvectors(); void authcrypt(); + void xexstarAttack(); void ivrecovery(); void reverserecovery(); void tamper(); @@ -148,7 +149,7 @@ void TestCrypt::testvectors() { cs.setKey(rawkey, rawkey, rawkey); unsigned char tag[16]; - cs.ocb_encrypt(NULL, NULL, 0, rawkey, tag); + QVERIFY(cs.ocb_encrypt(NULL, NULL, 0, rawkey, tag)); const unsigned char blanktag[AES_BLOCK_SIZE] = {0xBF,0x31,0x08,0x13,0x07,0x73,0xAD,0x5E,0xC7,0x0E,0xC6,0x9E,0x78,0x75,0xA7,0xB0}; for (int i=0;i