From ccad719d2e935306e601b0f6af5ff2acb7cd272e Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 11 Jan 2023 07:51:01 -0800 Subject: [PATCH] QCryptographicHash: add a mutex to writing to the results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QCryptographicHash::result() and resultView() are const, therefore two threads can call them on the same object. Given that the finalization of the hash is not a trivial operation but doesn't modify the state, let's do it without a locked mutex, onto a temporary stack buffer. Then we lock the mutex to copy said result to our cached value. Pick-to: 6.5 6.4 6.2 5.15 Fixes: QTBUG-110058 Change-Id: Ide4dbd0777a44ed0870efffd17394bf72785eabb Reviewed-by: Giuseppe D'Angelo Reviewed-by: Mårten Nordheim --- src/corelib/tools/qcryptographichash.cpp | 89 +++++++++++++----------- 1 file changed, 50 insertions(+), 39 deletions(-) diff --git a/src/corelib/tools/qcryptographichash.cpp b/src/corelib/tools/qcryptographichash.cpp index 88fdb6281ff..ef7f43a38c0 100644 --- a/src/corelib/tools/qcryptographichash.cpp +++ b/src/corelib/tools/qcryptographichash.cpp @@ -4,6 +4,7 @@ #include #include +#include #include @@ -255,16 +256,7 @@ public: blake2s_state blake2sContext; #endif }; -#ifndef QT_CRYPTOGRAPHICHASH_ONLY_SHA1 -#ifndef USING_OPENSSL30 - enum class Sha3Variant - { - Sha3, - Keccak - }; - void sha3Finish(int bitCount, Sha3Variant sha3Variant); -#endif -#endif + class SmallByteArray { std::array m_data; static_assert(MaxHashLength <= std::numeric_limits::max()); @@ -282,12 +274,25 @@ public: QByteArrayView toByteArrayView() const noexcept { return QByteArrayView{m_data.data(), size()}; } }; + + // Only write to the result under this mutex + QBasicMutex finalizeMutex; SmallByteArray result; + +#if !defined(QT_CRYPTOGRAPHICHASH_ONLY_SHA1) && !defined(USING_OPENSSL30) + enum class Sha3Variant + { + Sha3, + Keccak + }; + void sha3Finish(SmallByteArray *tmpresult, int bitCount, Sha3Variant sha3Variant); +#endif }; #ifndef QT_CRYPTOGRAPHICHASH_ONLY_SHA1 #ifndef USING_OPENSSL30 -void QCryptographicHashPrivate::sha3Finish(int bitCount, Sha3Variant sha3Variant) +void QCryptographicHashPrivate::sha3Finish(SmallByteArray *tmpresult, int bitCount, + Sha3Variant sha3Variant) { /* FIPS 202 §6.1 defines SHA-3 in terms of calculating the Keccak function @@ -311,7 +316,7 @@ void QCryptographicHashPrivate::sha3Finish(int bitCount, Sha3Variant sha3Variant */ static const unsigned char sha3FinalSuffix = 0x80; - result.resizeForOverwrite(bitCount / 8); + tmpresult->resizeForOverwrite(bitCount / 8); SHA3Context copy = sha3Context; @@ -323,7 +328,7 @@ void QCryptographicHashPrivate::sha3Finish(int bitCount, Sha3Variant sha3Variant break; } - sha3Final(©, reinterpret_cast(result.data())); + sha3Final(©, tmpresult->data()); } #endif // !QT_CONFIG(opensslv30) #endif @@ -730,33 +735,34 @@ void QCryptographicHashPrivate::finalize() noexcept if (!result.isEmpty()) return; + SmallByteArray tmpresult; #ifdef USING_OPENSSL30 if (method == QCryptographicHash::Blake2b_160 || method == QCryptographicHash::Blake2b_256 || method == QCryptographicHash::Blake2b_384) { const auto length = hashLengthInternal(method); blake2b_state copy = blake2bContext; - result.resizeForOverwrite(length); - blake2b_final(©, reinterpret_cast(result.data()), length); + tmpresult.resizeForOverwrite(length); + blake2b_final(©, tmpresult.data(), length); } else if (method == QCryptographicHash::Blake2s_128 || method == QCryptographicHash::Blake2s_160 || method == QCryptographicHash::Blake2s_224) { const auto length = hashLengthInternal(method); blake2s_state copy = blake2sContext; - result.resizeForOverwrite(length); - blake2s_final(©, reinterpret_cast(result.data()), length); + tmpresult.resizeForOverwrite(length); + blake2s_final(©, tmpresult.data(), length); } else if (!initializationFailed) { - result.resizeForOverwrite(EVP_MD_get_size(algorithm.get())); - const int ret = EVP_DigestFinal_ex(context.get(), (unsigned char *)result.data(), nullptr); + tmpresult.resizeForOverwrite(EVP_MD_get_size(algorithm.get())); + const int ret = EVP_DigestFinal_ex(context.get(), tmpresult.data(), nullptr); Q_UNUSED(ret); } #else switch (method) { case QCryptographicHash::Sha1: { Sha1State copy = sha1Context; - result.resizeForOverwrite(20); + tmpresult.resizeForOverwrite(20); sha1FinalizeState(©); - sha1ToHash(©, (unsigned char *)result.data()); + sha1ToHash(©, tmpresult.data()); break; } #ifdef QT_CRYPTOGRAPHICHASH_ONLY_SHA1 @@ -767,52 +773,52 @@ void QCryptographicHashPrivate::finalize() noexcept #else case QCryptographicHash::Md4: { md4_context copy = md4Context; - result.resizeForOverwrite(MD4_RESULTLEN); - md4_final(©, (unsigned char *)result.data()); + tmpresult.resizeForOverwrite(MD4_RESULTLEN); + md4_final(©, tmpresult.data()); break; } case QCryptographicHash::Md5: { MD5Context copy = md5Context; - result.resizeForOverwrite(16); - MD5Final(©, (unsigned char *)result.data()); + tmpresult.resizeForOverwrite(16); + MD5Final(©, tmpresult.data()); break; } case QCryptographicHash::Sha224: { SHA224Context copy = sha224Context; - result.resizeForOverwrite(SHA224HashSize); - SHA224Result(©, reinterpret_cast(result.data())); + tmpresult.resizeForOverwrite(SHA224HashSize); + SHA224Result(©, tmpresult.data()); break; } case QCryptographicHash::Sha256: { SHA256Context copy = sha256Context; - result.resizeForOverwrite(SHA256HashSize); - SHA256Result(©, reinterpret_cast(result.data())); + tmpresult.resizeForOverwrite(SHA256HashSize); + SHA256Result(©, tmpresult.data()); break; } case QCryptographicHash::Sha384: { SHA384Context copy = sha384Context; - result.resizeForOverwrite(SHA384HashSize); - SHA384Result(©, reinterpret_cast(result.data())); + tmpresult.resizeForOverwrite(SHA384HashSize); + SHA384Result(©, tmpresult.data()); break; } case QCryptographicHash::Sha512: { SHA512Context copy = sha512Context; - result.resizeForOverwrite(SHA512HashSize); - SHA512Result(©, reinterpret_cast(result.data())); + tmpresult.resizeForOverwrite(SHA512HashSize); + SHA512Result(©, tmpresult.data()); break; } case QCryptographicHash::RealSha3_224: case QCryptographicHash::RealSha3_256: case QCryptographicHash::RealSha3_384: case QCryptographicHash::RealSha3_512: { - sha3Finish(8 * hashLengthInternal(method), Sha3Variant::Sha3); + sha3Finish(&tmpresult, 8 * hashLengthInternal(method), Sha3Variant::Sha3); break; } case QCryptographicHash::Keccak_224: case QCryptographicHash::Keccak_256: case QCryptographicHash::Keccak_384: case QCryptographicHash::Keccak_512: { - sha3Finish(8 * hashLengthInternal(method), Sha3Variant::Keccak); + sha3Finish(&tmpresult, 8 * hashLengthInternal(method), Sha3Variant::Keccak); break; } case QCryptographicHash::Blake2b_160: @@ -821,8 +827,8 @@ void QCryptographicHashPrivate::finalize() noexcept case QCryptographicHash::Blake2b_512: { const auto length = hashLengthInternal(method); blake2b_state copy = blake2bContext; - result.resizeForOverwrite(length); - blake2b_final(©, reinterpret_cast(result.data()), length); + tmpresult.resizeForOverwrite(length); + blake2b_final(©, tmpresult.data(), length); break; } case QCryptographicHash::Blake2s_128: @@ -831,13 +837,18 @@ void QCryptographicHashPrivate::finalize() noexcept case QCryptographicHash::Blake2s_256: { const auto length = hashLengthInternal(method); blake2s_state copy = blake2sContext; - result.resizeForOverwrite(length); - blake2s_final(©, reinterpret_cast(result.data()), length); + tmpresult.resizeForOverwrite(length); + blake2s_final(©, tmpresult.data(), length); break; } #endif } #endif // !QT_CONFIG(opensslv30) + + // we're called from a const function, so only write to this->result under + // a mutex + QMutexLocker locker(&finalizeMutex); + result = std::move(tmpresult); } /*!