From c1ceb5968c9609adccc30dec1ca57b20a4087cb9 Mon Sep 17 00:00:00 2001 From: Jonas Herzig Date: Tue, 26 Jan 2021 14:40:45 +0100 Subject: [PATCH] FIX(ocb2): Work around packet loss due to OCB2 XEX* mitigation The mitigation for vulnerabilities discovered in OCB2 (https://eprint.iacr.org/2019/311, called XEX* attack, or XEXStarAttack, in code) introduced in be97594 (#4227) willingly allowed for some packets with specific characteristics to be dropped during encryption to prevent the vulnerability from being exploited. It was assumed that the chance of such packets was sufficiently small (given we are dealing with compressed audio) that such loss was acceptable. It was however discovered that digital silence (as produced by e.g. a noise gate) will cause Opus to emit almost exclusively such packets, leading to strong artifacts on the receiving end. See #4385. This commit tries to work around the issue by modifying such packets in a way which will no longer require them to be dropped, and yet produce the expected output on the receiver side. As far as I understand [Opus] (specifically section 4.1, 4.3.0 and 4.3.3), the 0s are simply unused bits and are only there because we running Opus in constant bitrate mode. So, flipping one of them should have no effect on the resulting audio. [Opus]: https://tools.ietf.org/html/rfc6716 Fixes #4719 --- src/CryptState.cpp | 35 ++++++++++++++++++++++++------- src/CryptState.h | 3 ++- src/tests/TestCrypt/TestCrypt.cpp | 20 ++++++++++++++++-- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/src/CryptState.cpp b/src/CryptState.cpp index 8517f8bcdb9..fbbeb3e0db1 100644 --- a/src/CryptState.cpp +++ b/src/CryptState.cpp @@ -218,7 +218,8 @@ 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); -bool 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, bool modifyPlainOnXEXStarAttack) { keyblock checksum, delta, tmp, pad; bool success = true; @@ -227,21 +228,39 @@ bool CryptState::ocb_encrypt(const unsigned char *plain, unsigned char *encrypte ZERO(checksum); while (len > AES_BLOCK_SIZE) { - S2(delta); - XOR(tmp, delta, reinterpret_cast(plain)); - 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). + bool flipABit = false; // *plain is const, so we can't directly modify it 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; + if (sum == 0) { + if (modifyPlainOnXEXStarAttack) { + // The assumption that critical packets do not turn up by pure chance turned out to be incorrect + // since digital silence appears to produce them in mass. + // So instead we now modify the packet in a way which should not affect the audio but will + // prevent the attack. + flipABit = true; + } else { + // This option still exists but only to allow us to test ocb_decrypt's detection. + success = false; + } + } + } + + S2(delta); + XOR(tmp, delta, reinterpret_cast< const subblock * >(plain)); + if (flipABit) { + *reinterpret_cast< unsigned char * >(tmp) ^= 1; + } + AESencrypt(tmp, tmp, &encrypt_key); + XOR(reinterpret_cast< subblock * >(encrypted), delta, tmp); + XOR(checksum, checksum, reinterpret_cast< const subblock * >(plain)); + if (flipABit) { + *reinterpret_cast< unsigned char * >(checksum) ^= 1; } len -= AES_BLOCK_SIZE; diff --git a/src/CryptState.h b/src/CryptState.h index bf5576e6abf..8005cc4c883 100644 --- a/src/CryptState.h +++ b/src/CryptState.h @@ -44,7 +44,8 @@ class CryptState { void setKey(const unsigned char *rkey, const unsigned char *eiv, const unsigned char *div); void setDecryptIV(const unsigned char *iv); - bool ocb_encrypt(const unsigned char *plain, unsigned char *encrypted, 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 modifyPlainOnXEXStarAttack = true); 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); diff --git a/src/tests/TestCrypt/TestCrypt.cpp b/src/tests/TestCrypt/TestCrypt.cpp index 33cd915994a..918f0617a3e 100644 --- a/src/tests/TestCrypt/TestCrypt.cpp +++ b/src/tests/TestCrypt/TestCrypt.cpp @@ -218,9 +218,9 @@ void TestCrypt::xexstarAttack() { 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); + STACKVAR(unsigned char, decrypted, 2 * AES_BLOCK_SIZE); - const bool failed_encrypt = !cs.ocb_encrypt(src, encrypted, 2 * AES_BLOCK_SIZE, nonce, enctag); + const bool failed_encrypt = !cs.ocb_encrypt(src, encrypted, 2 * AES_BLOCK_SIZE, nonce, enctag, false); // Perform the attack encrypted[AES_BLOCK_SIZE - 1] ^= AES_BLOCK_SIZE * 8; @@ -237,6 +237,22 @@ void TestCrypt::xexstarAttack() { // Make sure we detected the attack QVERIFY(failed_encrypt); QVERIFY(failed_decrypt); + + // The assumption that critical packets do not turn up by pure chance turned out to be incorrect + // since digital silence appears to produce them in mass. + // So instead we now modify the packet in a way which should not affect the audio but will + // prevent the attack. + QVERIFY(cs.ocb_encrypt(src, encrypted, 2 * AES_BLOCK_SIZE, nonce, enctag)); + QVERIFY(cs.ocb_decrypt(encrypted, decrypted, 2 * AES_BLOCK_SIZE, nonce, dectag)); + + // Tags should match + for (int i = 0; i < AES_BLOCK_SIZE; ++i) { + QCOMPARE(enctag[i], dectag[i]); + } + + // Actual content should have been changed such that the critical block is no longer all 0. + QCOMPARE(src[0], static_cast(0)); + QCOMPARE(decrypted[0], static_cast(1)); } void TestCrypt::tamper() {