1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QCryptographicHash: fix UB (data race on concurrent result()) [2nd try]

Browse Source 
The previous attempt at fixing QTBUG-110058,
ccad719d2e935306e601b0f6af5ff2acb7cd272e, was incomplete:

- the if (result.isEmpty()) check at the beginning of finalize() was
  not protected, so it raced against the assignment at the end of
  finalize(), which was protected

- because the mutex was not locked during the finalization of the hash
  algorithm, two threads could perform this operation simultaneously,
  which isn't such a bad idea in principle, as it can reduce latency,
  but for that to work, the losing thread needs to throw away its own
  work and adopt the work of the other thread, but that wasn't done:
  both threads would write their result to 'result', just one after
  the other, but that's still a data race, since the eventual _reader_
  of the result cannot be protected (is outside the class). Besides,
  we don't even know whether the algorithm-specific finalization
  functions are ok with being called from separate threads on the same
  context object

- in addition, the mutex wasn't necessary when finalize() was called
  from the static hash() function, as no sharing could possibly take
  place there (the state is function-local)

Fix all of the above by largely reverting the first attempt, dragging
the result.isEmpty() check out of finalize() and into resultView() and
instead simply holding the mutex over these two calls in
resultView(). To see why this is sufficient, consider that
resultView() is now idempotent again: the result is written once, the
next thread waits and then finds the work done. All following accesses
to the result are then reads, which happen-after the write at the end
of finalize().

The accesses to 'result' from reset() need no protection, as reset()
is a mutable function, and calling a mutable function on a shared
QCryptographicHash object is already UB. Only two const functions may
be called that way.

Manual conflict resolutions:
 - missing OpenSSLv3 code in 6.4
 - different order of Private member fields

Fixes: QTBUG-110058
Change-Id: Ia8ac095b785519682090801c1012e9dded6d60b2
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit b904de43a5acfc4067fc9e4146babd45c6ac1138)
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
This commit is contained in:
Marc Mutz 2023-01-18 21:46:42 +01:00
parent 7fe0f175d4
commit c73833ab52
1 changed files with 55 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

104
src/corelib/tools/qcryptographichash.cpp
View File

@ -5,6 +5,7 @@
#include <qcryptographichash.h>
#include <qiodevice.h>
#include <qmutex.h>
#include <private/qlocking_p.h>
#include "../../3rdparty/sha1/sha1.cpp"
@ -165,7 +166,10 @@ public:
void reset() noexcept;
void addData(QByteArrayView bytes) noexcept;
void finalize() noexcept;
// when not called from the static hash() function, this function needs to be
// called with finalizeMutex held:
void finalizeUnchecked() noexcept;
// END functions that need to be called with finalizeMutex held
QByteArrayView resultView() const noexcept { return result.toByteArrayView(); }
const QCryptographicHash::Algorithm method;
@ -183,7 +187,16 @@ 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<quint8, MaxHashLength> m_data;
static_assert(MaxHashLength <= std::numeric_limits<std::uint8_t>::max());
@ -201,24 +214,13 @@ public:
QByteArrayView toByteArrayView() const noexcept
{ return QByteArrayView{m_data.data(), size()}; }
};
// Only write to the result under this mutex
// protects result in finalize()
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
void QCryptographicHashPrivate::sha3Finish(SmallByteArray *tmpresult, int bitCount,
Sha3Variant sha3Variant)
void QCryptographicHashPrivate::sha3Finish(int bitCount, Sha3Variant sha3Variant)
{
/*
FIPS 202 §6.1 defines SHA-3 in terms of calculating the Keccak function
@ -242,7 +244,7 @@ void QCryptographicHashPrivate::sha3Finish(SmallByteArray *tmpresult, int bitCou
*/
static const unsigned char sha3FinalSuffix = 0x80;
tmpresult->resizeForOverwrite(bitCount / 8);
result.resizeForOverwrite(bitCount / 8);
SHA3Context copy = sha3Context;
@ -254,7 +256,7 @@ void QCryptographicHashPrivate::sha3Finish(SmallByteArray *tmpresult, int bitCou
break;
}
sha3Final(&copy, tmpresult->data());
sha3Final(&copy, result.data());
}
#endif
@ -545,22 +547,31 @@ QByteArray QCryptographicHash::result() const
*/
QByteArrayView QCryptographicHash::resultView() const noexcept
{
d->finalize();
// resultView() is a const function, so concurrent calls are allowed; protect:
{
const auto lock = qt_scoped_lock(d->finalizeMutex);
// check that no other thread already finalizeUnchecked()'ed before us:
if (d->result.isEmpty())
d->finalizeUnchecked();
}
// resultView() remains(!) valid even after we dropped the mutex
return d->resultView();
}
void QCryptographicHashPrivate::finalize() noexcept
{
if (!result.isEmpty())
return;
/*!
\internal
SmallByteArray tmpresult;
Must be called with finalizeMutex held (except from static hash() function,
where no sharing can take place).
*/
void QCryptographicHashPrivate::finalizeUnchecked() noexcept
{
switch (method) {
case QCryptographicHash::Sha1: {
Sha1State copy = sha1Context;
tmpresult.resizeForOverwrite(20);
result.resizeForOverwrite(20);
sha1FinalizeState(&copy);
sha1ToHash(&copy, tmpresult.data());
sha1ToHash(&copy, result.data());
break;
}
#ifdef QT_CRYPTOGRAPHICHASH_ONLY_SHA1
@ -571,52 +582,52 @@ void QCryptographicHashPrivate::finalize() noexcept
#else
case QCryptographicHash::Md4: {
md4_context copy = md4Context;
tmpresult.resizeForOverwrite(MD4_RESULTLEN);
md4_final(&copy, tmpresult.data());
result.resizeForOverwrite(MD4_RESULTLEN);
md4_final(&copy, result.data());
break;
}
case QCryptographicHash::Md5: {
MD5Context copy = md5Context;
tmpresult.resizeForOverwrite(16);
MD5Final(&copy, tmpresult.data());
result.resizeForOverwrite(16);
MD5Final(&copy, result.data());
break;
}
case QCryptographicHash::Sha224: {
SHA224Context copy = sha224Context;
tmpresult.resizeForOverwrite(SHA224HashSize);
SHA224Result(&copy, tmpresult.data());
result.resizeForOverwrite(SHA224HashSize);
SHA224Result(&copy, result.data());
break;
}
case QCryptographicHash::Sha256: {
SHA256Context copy = sha256Context;
tmpresult.resizeForOverwrite(SHA256HashSize);
SHA256Result(&copy, tmpresult.data());
result.resizeForOverwrite(SHA256HashSize);
SHA256Result(&copy, result.data());
break;
}
case QCryptographicHash::Sha384: {
SHA384Context copy = sha384Context;
tmpresult.resizeForOverwrite(SHA384HashSize);
SHA384Result(&copy, tmpresult.data());
result.resizeForOverwrite(SHA384HashSize);
SHA384Result(&copy, result.data());
break;
}
case QCryptographicHash::Sha512: {
SHA512Context copy = sha512Context;
tmpresult.resizeForOverwrite(SHA512HashSize);
SHA512Result(&copy, tmpresult.data());
result.resizeForOverwrite(SHA512HashSize);
SHA512Result(&copy, result.data());
break;
}
case QCryptographicHash::RealSha3_224:
case QCryptographicHash::RealSha3_256:
case QCryptographicHash::RealSha3_384:
case QCryptographicHash::RealSha3_512: {
sha3Finish(&tmpresult, 8 * hashLengthInternal(method), Sha3Variant::Sha3);
sha3Finish(8 * hashLengthInternal(method), Sha3Variant::Sha3);
break;
}
case QCryptographicHash::Keccak_224:
case QCryptographicHash::Keccak_256:
case QCryptographicHash::Keccak_384:
case QCryptographicHash::Keccak_512: {
sha3Finish(&tmpresult, 8 * hashLengthInternal(method), Sha3Variant::Keccak);
sha3Finish(8 * hashLengthInternal(method), Sha3Variant::Keccak);
break;
}
case QCryptographicHash::Blake2b_160:
@ -625,8 +636,8 @@ void QCryptographicHashPrivate::finalize() noexcept
case QCryptographicHash::Blake2b_512: {
const auto length = hashLengthInternal(method);
blake2b_state copy = blake2bContext;
tmpresult.resizeForOverwrite(length);
blake2b_final(&copy, tmpresult.data(), length);
result.resizeForOverwrite(length);
blake2b_final(&copy, result.data(), length);
break;
}
case QCryptographicHash::Blake2s_128:
@ -635,17 +646,12 @@ void QCryptographicHashPrivate::finalize() noexcept
case QCryptographicHash::Blake2s_256: {
const auto length = hashLengthInternal(method);
blake2s_state copy = blake2sContext;
tmpresult.resizeForOverwrite(length);
blake2s_final(&copy, tmpresult.data(), length);
result.resizeForOverwrite(length);
blake2s_final(&copy, result.data(), length);
break;
}
#endif
}
// we're called from a const function, so only write to this->result under
// a mutex
QMutexLocker locker(&finalizeMutex);
result = std::move(tmpresult);
}
/*!
@ -658,7 +664,7 @@ QByteArray QCryptographicHash::hash(QByteArrayView data, Algorithm method)
{
QCryptographicHashPrivate hash(method);
hash.addData(data);
hash.finalize();
hash.finalizeUnchecked(); // no mutex needed: no-one but us has access to 'hash'
return hash.resultView().toByteArray();
}