Replace RAND_bytes() with fillFromSecureRng(); FixedBuffer::operator[]

With this commit, fillFromSecureRng becomes a common way to fill up
a buffer with (truly, if possible) random data. QByteArray-friendly
overload can be made later if necessary.

Also: add operator[] to FixedBufferBase and FixedBuffer.

Quotient/e2ee/cryptoutils.cpp

@@ -56,17 +56,9 @@ SslExpected<QByteArray> Quotient::aesCtr256Encrypt(const QByteArray& plaintext,
                                                    const QByteArray& key,
                                                    const QByteArray& iv)
 {
-    int length = 0;
-    int ciphertextLength = 0;
-
-    auto encrypted = QByteArray(plaintext.size() + AES_BLOCK_SIZE, u'\0');
-    if (RAND_bytes(reinterpret_cast<unsigned char*>(encrypted.data()), encrypted.size()) != 1) {
-        qWarning() << ERR_error_string(ERR_get_error(), nullptr);
-        return ERR_get_error();
-    }
-    auto data = encrypted.data();
+    auto encrypted = getRandom(unsignedSize(plaintext) + AES_BLOCK_SIZE);
     constexpr auto mask = static_cast<std::uint8_t>(~(1U << (63 / 8)));
-    data[15 - 63 % 8] &= mask;
+    encrypted[15 - 63 % 8] &= mask;
 
     const ContextHolder ctx(EVP_CIPHER_CTX_new(), &EVP_CIPHER_CTX_free);
     if (!ctx) {
@@ -79,20 +71,20 @@ SslExpected<QByteArray> Quotient::aesCtr256Encrypt(const QByteArray& plaintext,
         return ERR_get_error();
     }
 
+    int length = 0;
     if (EVP_EncryptUpdate(ctx.get(), reinterpret_cast<unsigned char*>(encrypted.data()), &length, reinterpret_cast<const unsigned char *>(&plaintext.data()[0]), (int) plaintext.size()) != 1) {
         qWarning() << ERR_error_string(ERR_get_error(), nullptr);
         return ERR_get_error();
     }
 
-    ciphertextLength = length;
+    int ciphertextLength = length;
     if (EVP_EncryptFinal_ex(ctx.get(), reinterpret_cast<unsigned char*>(encrypted.data()) + length, &length) != 1) {
         qWarning() << ERR_error_string(ERR_get_error(), nullptr);
         return ERR_get_error();
     }
 
     ciphertextLength += length;
-    encrypted.resize(ciphertextLength);
-    return encrypted;
+    return encrypted.viewAsByteArray().left(ciphertextLength);
 }
 
 #define CALL_OPENSSL(Call_)                                           \

Quotient/e2ee/e2ee_common.cpp

@@ -23,6 +23,52 @@ QByteArray Quotient::byteArrayForOlm(size_t bufferSize)
     return {};
 }
 
+void Quotient::fillFromSecureRng(std::span<byte_t> bytes)
+{
+    // Discussion of QRandomGenerator::system() vs. OpenSSL's RAND_bytes
+    //
+    // It is a rather close call between the two; to be honest, TL;DR from the
+    // below is "it doesn't really matter". Going through the source code of
+    // both libraries, along with reading others' investigations like
+    // https://github.com/golang/go/issues/33542, left me (@kitsune) with
+    // the following:
+    // 1. Both ultimately use more or less the same stuff internally - which is
+    //    is not surprising and is reassuring in TL;DR. Contrary to occasional
+    //    claims on interwebs though, QRandomGenerator does not rely on OpenSSL,
+    //    calling system primitives directly instead.
+    // 2. Qt's code is massively simpler - again, effectively it's a rather
+    //    thin wrapper around what I know for a reasonably good practice across
+    //    the platforms supported by Quotient.
+    // 3. OpenSSL is more flexible in that you can specify which RNG engine
+    //    you actually want: you can, e.g., explicitly use the hardware-based
+    //    (aka "true") RNG on modern Intel CPUs. QRandomGenerator "simply"
+    //    offers the nice Qt interface to whatever best the operating system
+    //    provides (and at least Linux kernel happens to also use true RNGs
+    //    where it can, so ¯\(ツ)/¯)
+    // 3. QRandomGenerator produces stuff in uint32_t (or uint64_t in case of
+    //    QRandomGenerator64) chunks; OpenSSL generates bytes, which is sorta
+    //    more convenient. Quotient never needs quantities of randomness that
+    //    are not multiples of 16 _bytes_; the Q_UNLIKELY branch below is merely
+    //    to cover an edge case that should never happen in the first place.
+    //    So, ¯\(ツ)/¯ again.
+    //
+    // Based on this, QRandomGenerator::system() wins by a tiny non-functional
+    // margin; my somewhat educated opinion for now is that both APIs are
+    // equally good from the functionality and security point of view.
+
+    // QRandomGenerator::fillRange works in terms of 32-bit words,
+    // and FixedBuffer happens to deal with sizes that are multiples
+    // of those (16, 32, etc.)
+    const qsizetype wordsCount = bytes.size() / 4;
+    QRandomGenerator::system()->fillRange(
+        reinterpret_cast<uint32_t*>(bytes.data()), wordsCount);
+    if (const int remainder = bytes.size() % 4; Q_UNLIKELY(remainder != 0)) {
+        // Not normal; but if it happens, apply best effort
+        QRandomGenerator::system()->generate(bytes.end() - remainder,
+                                             bytes.end());
+    }
+}
+
 auto initializeSecureHeap()
 {
 #if !defined(LIBRESSL_VERSION_NUMBER)
@@ -79,22 +125,9 @@ FixedBufferBase::FixedBufferBase(size_t bufferSize, InitOptions options)
     if (options == Uninitialized)
         return;
 
-    data_ = allocate(size_, options == FillWithZeros);
-    if (options == FillWithRandom) {
-        // QRandomGenerator::fillRange works in terms of 32-bit words,
-        // and FixedBuffer happens to deal with sizes that are multiples
-        // of those (16, 32, etc.)
-        const qsizetype wordsCount = size_ / 4;
-        QRandomGenerator::system()->fillRange(
-            reinterpret_cast<uint32_t*>(data_), wordsCount);
-        if (const auto remainder = size_ % 4; Q_UNLIKELY(remainder != 0)) {
-            // Not normal; but if it happens, apply best effort
-            // NOLINTBEGIN(cppcoreguidelines-pro-bounds-pointer-arithmetic)
-            const auto end = data_ + size_;
-            QRandomGenerator::system()->generate(end - remainder, end);
-            // NOLINTEND(cppcoreguidelines-pro-bounds-pointer-arithmetic)
-        }
-    }
+    data_ = allocate(bufferSize, options == FillWithZeros);
+    if (options == FillWithRandom)
+        fillFromSecureRng({ data_, bufferSize });
 }
 
 void FixedBufferBase::fillFrom(QByteArray&& source)

Quotient/e2ee/e2ee_common.h

@@ -77,13 +77,21 @@ QUOTIENT_API QByteArray byteArrayForOlm(size_t bufferSize);
 
 //! \brief Get a size of Qt container coerced to size_t
 //!
-//! It's a safe cast since size_t can easily accommodate the range between
-//! 0 and INTMAX - 1 that Qt containers support; yet compilers complain...
+//! It's a safe cast since size_t can always accommodate the range between
+//! 0 and SIZE_MAX / 2 - 1 that Qt containers support; yet compilers complain...
 inline size_t unsignedSize(const auto& qtBuffer)
 {
     return static_cast<size_t>(qtBuffer.size());
 }
 
+// Can't use std::byte normally recommended for the purpose because both Olm
+// and OpenSSL get uint8_t* pointers, and std::byte* is not implicitly
+// convertible to uint8_t (and adding explicit casts in each case kinda defeats
+// the purpose of all the span machinery below meant to replace reinterpret_ or
+// any other casts).
+
+using byte_t = uint8_t;
+
 class QUOTIENT_API FixedBufferBase {
 public:
     enum InitOptions { Uninitialized, FillWithZeros, FillWithRandom };
@@ -122,6 +130,11 @@ class QUOTIENT_API FixedBufferBase {
 
     uint8_t* dataForWriting() { return data_; }
     const uint8_t* data() const { return data_; }
+    const uint8_t& operator[](size_t idx) const
+    {
+        Q_ASSERT(idx < size());
+        return data()[idx];
+    }
 
 private:
     uint8_t* data_ = nullptr;
@@ -146,6 +159,12 @@ class QUOTIENT_API FixedBuffer : public FixedBufferBase {
 
     using FixedBufferBase::data;
     uint8_t* data() requires DataIsWriteable { return dataForWriting(); }
+    uint8_t& operator[](size_t idx)
+        requires DataIsWriteable
+    {
+        Q_ASSERT(idx < size());
+        return dataForWriting()[idx];
+    }
 };
 
 inline auto getRandom(size_t bytes)
@@ -159,6 +178,16 @@ inline auto getRandom()
     return FixedBuffer<SizeN>{ FixedBufferBase::FillWithRandom };
 }
 
+//! \brief Fill the buffer with the securely generated random bytes
+//!
+//! You should use this throughout Quotient where pseudo-random generators
+//! are not enough (i.e. in crypto cases). Don't use it when proper randomness
+//! is not critical; it tries to rely on system entropy that is in (somewhat)
+//! limited supply.
+//! There's no fancy stuff internally, it's just a way to unify secure RNG usage
+//! in Quotient. See the function definition for details if you want/need.
+QUOTIENT_API void fillFromSecureRng(std::span<byte_t> bytes);
+
 class PicklingKey : public FixedBuffer<128, /*DataIsWriteable=*/false> {
 private:
     // `using` would have exposed the constructor as it's public in the parent