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.

1 files changed, 36 insertions, 6 deletions

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<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);
@@ -237,6 +241,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;
@@ -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;
 }