Merge pull request #4251: Backport "Safeguard from potential attacks against OCB2"

OCB2 is known to be broken under certain conditions:
https://eprint.iacr.org/2019/311

To execute the universal attacks described in the paper, an attacker needs
access to an encryption oracle that allows it to perform encryption queries with
attacker-chosen nonce. Luckily in Mumble the encryption nonce is a fixed counter
which is far too restrictive for the universal attacks to be feasible against
Mumble.

The basic attacks do not require an attacker-chosen nonce and as such are more
applicable to Mumble. They are however of limited use and do require an en- and
a decryption oracle which Mumble seemingly does not provide at the same time.

To be on the safe side, this commit implements the counter-cryptanalysis
measure described in the paper in section 9 for the sender and receiver side.
This way if either server of client are patched, their communication is almost
certainly (merely lacking formal proof) not susceptible to the attacks described
in the paper.

Backported from #4227

5 files changed, 88 insertions, 17 deletions

diff --git a/src/CryptState.cpp b/src/CryptState.cpp
index c8ba5865b..8517f8bcd 100644
--- a/src/CryptState.cpp
+++ b/src/CryptState.cpp
@@ -59,7 +59,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.
@@ -67,12 +67,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) {
@@ -148,9 +151,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;
 	}
@@ -215,8 +218,9 @@ static void inline ZERO(keyblock &block) {
 #define AESencrypt(src,dst,key) AES_encrypt(reinterpret_cast<const unsigned char *>(src),reinterpret_cast<unsigned char *>(dst), key);
 #define AESdecrypt(src,dst,key) AES_decrypt(reinterpret_cast<const unsigned char *>(src),reinterpret_cast<unsigned char *>(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);
@@ -228,6 +232,18 @@ void CryptState::ocb_encrypt(const unsigned char *plain, unsigned char *encrypte
 		AESencrypt(tmp, tmp, &encrypt_key);
 		XOR(reinterpret_cast<subblock *>(encrypted), delta, tmp);
 		XOR(checksum, checksum, reinterpret_cast<const subblock *>(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;
@@ -247,10 +263,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);
@@ -278,7 +297,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 7e851d524..bf5576e6a 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 fcd0351c7..26d58f765 100644
--- a/src/mumble/ServerHandler.cpp
+++ b/src/mumble/ServerHandler.cpp
@@ -242,7 +242,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<const unsigned char *>(data), crypto, len);
+		if (!connection->csCrypt.encrypt(reinterpret_cast<const unsigned char *>(data), crypto, len)) {
+			return;
+		}
 		qusUdp->writeDatagram(reinterpret_cast<const char *>(crypto), len + 4, qhaRemote, usResolvedPort);
 	}
 }
@@ -978,4 +980,3 @@ QUrl ServerHandler::getServerURL(bool withPassword) const {
 	
 	return url;
 }
-
diff --git a/src/murmur/Server.cpp b/src/murmur/Server.cpp
index 1c7b2f329..dc5d3714f 100644
--- a/src/murmur/Server.cpp
+++ b/src/murmur/Server.cpp
@@ -934,8 +934,9 @@ void Server::sendMessage(ServerUser *u, const char *data, int len, QByteArray &c
 				return;
 			}
 
-			u->csCrypt.encrypt(reinterpret_cast<const unsigned char *>(data), reinterpret_cast<unsigned char *>(buffer),
-							   len);
+			if (!u->csCrypt.encrypt(reinterpret_cast<const unsigned char *>(data), reinterpret_cast<unsigned char *>(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 61179fc9d..33cd91599 100644
--- a/src/tests/TestCrypt/TestCrypt.cpp
+++ b/src/tests/TestCrypt/TestCrypt.cpp
@@ -20,6 +20,7 @@ class TestCrypt : public QObject {
 		void cleanupTestCase();
 		void testvectors();
 		void authcrypt();
+		void xexstarAttack();
 		void ivrecovery();
 		void reverserecovery();
 		void tamper();
@@ -150,7 +151,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<AES_BLOCK_SIZE;i++)
@@ -160,7 +161,7 @@ void TestCrypt::testvectors() {
 	unsigned char crypt[40];
 	for (int i=0;i<40;i++)
 		source[i]=i;
-	cs.ocb_encrypt(source, crypt, 40, rawkey, tag);
+	QVERIFY(cs.ocb_encrypt(source, crypt, 40, rawkey, tag));
 	const unsigned char longtag[AES_BLOCK_SIZE] = {0x9D,0xB0,0xCD,0xF8,0x80,0xF7,0x3E,0x3E,0x10,0xD4,0xEB,0x32,0x17,0x76,0x66,0x88};
 	const unsigned char crypted[40] = {0xF7,0x5D,0x6B,0xC8,0xB4,0xDC,0x8D,0x66,0xB8,0x36,0xA2,0xB0,0x8B,0x32,0xA6,0x36,0x9F,0x1C,0xD3,0xC5,0x22,0x8D,0x79,0xFD,
 	                                   0x6C,0x26,0x7F,0x5F,0x6A,0xA7,0xB2,0x31,0xC7,0xDF,0xB9,0xD5,0x99,0x51,0xAE,0x9C
@@ -189,8 +190,8 @@ void TestCrypt::authcrypt() {
 		STACKVAR(unsigned char, encrypted, len);
 		STACKVAR(unsigned char, decrypted, len);
 
-		cs.ocb_encrypt(src, encrypted, len, nonce, enctag);
-		cs.ocb_decrypt(encrypted, decrypted, len, nonce, dectag);
+		QVERIFY(cs.ocb_encrypt(src, encrypted, len, nonce, enctag));
+		QVERIFY(cs.ocb_decrypt(encrypted, decrypted, len, nonce, dectag));
 
 		for (int i=0;i<AES_BLOCK_SIZE;i++)
 			QCOMPARE(enctag[i], dectag[i]);
@@ -200,6 +201,44 @@ void TestCrypt::authcrypt() {
 	}
 }
 
+// Test prevention of the attack described in section 4.1 of https://eprint.iacr.org/2019/311
+void TestCrypt::xexstarAttack() {
+	const unsigned char rawkey[AES_BLOCK_SIZE] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f};
+	const unsigned char nonce[AES_BLOCK_SIZE] = {0xff, 0xee, 0xdd, 0xcc, 0xbb, 0xaa, 0x99, 0x88, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11, 0x00};
+	CryptState cs;
+	cs.setKey(rawkey, nonce, nonce);
+
+	STACKVAR(unsigned char, src, 2 * AES_BLOCK_SIZE);
+	// Set first block to `len(secondBlock)`
+	memset(src, 0, AES_BLOCK_SIZE);
+	src[AES_BLOCK_SIZE - 1] = AES_BLOCK_SIZE * 8;
+	// Set second block to arbitrary value
+	memset(src + AES_BLOCK_SIZE, 42, AES_BLOCK_SIZE);
+
+	unsigned char enctag[AES_BLOCK_SIZE];
+	unsigned char dectag[AES_BLOCK_SIZE];
+	STACKVAR(unsigned char, encrypted, 2 * AES_BLOCK_SIZE);
+	STACKVAR(unsigned char, decrypted, 1 * AES_BLOCK_SIZE);
+
+	const bool failed_encrypt = !cs.ocb_encrypt(src, encrypted, 2 * AES_BLOCK_SIZE, nonce, enctag);
+
+	// Perform the attack
+	encrypted[AES_BLOCK_SIZE - 1] ^= AES_BLOCK_SIZE * 8;
+	for (int i = 0; i < AES_BLOCK_SIZE; ++i)
+		enctag[i] = src[AES_BLOCK_SIZE + i] ^ encrypted[AES_BLOCK_SIZE + i];
+
+	const bool failed_decrypt = !cs.ocb_decrypt(encrypted, decrypted, 1 * AES_BLOCK_SIZE, nonce, dectag);
+
+	// Verify forged tag (should match if attack is properly implemented)
+	for (int i = 0; i < AES_BLOCK_SIZE; ++i) {
+		QCOMPARE(enctag[i], dectag[i]);
+	}
+
+	// Make sure we detected the attack
+	QVERIFY(failed_encrypt);
+	QVERIFY(failed_decrypt);
+}
+
 void TestCrypt::tamper() {
 	const unsigned char rawkey[AES_BLOCK_SIZE] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f};
 	const unsigned char nonce[AES_BLOCK_SIZE] = {0xff, 0xee, 0xdd, 0xcc, 0xbb, 0xaa, 0x99, 0x88, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11, 0x00};