Make reseeding thread-safe

[inorick]

Suggested enhancement

I was looking at the thread-safety limitations of mbedtls_ctr_drbg_reseed. Currently, the docs of mbedtls_ctr_drbg_seed state some limitations [1]:

When Mbed TLS is built with threading support, after this function returns successfully, it is safe to call mbedtls_ctr_drbg_random() from multiple threads. Other operations, including reseeding, are not thread-safe.

When looking at code that actually triggers a reseed, I can see that the caller takes care of locking [2]. Is there any reason why the non-thread-safe functions like mbedtls_ctr_drbg_reseed do not lock the mutex of the context themselves to make them thread-safe? Is it safe to lock the private mutex of the context myself before calling mbedtls_ctr_drbg_reseed?

[1] https://github.com/Mbed-TLS/mbedtls/blob/550a18d4d6b29e62b6824201d8a49b8224e61c97/tf-psa-crypto/drivers/builtin/include/mbedtls/ctr_drbg.h#L280-L284 [2] https://github.com/Mbed-TLS/mbedtls/blob/7e5b7f91ca8efd5252a36765502ce9115ba73e61/library/ctr_drbg.c#L547-L550

Justification

Without this, manual RNG reseeding is not possible in a multi-threaded environment.

[gilles-peskine-arm]

I agree that it looks weird, and it was probably an omission when threading was added. I can see why the initial seeding wouldn't be protected: if other threads already have access to the DRBG object while the initial seeding is in progress, that adds more states to worry about. But I don't see a difficulty in making reseeding thread-safe.

Even if we decide that reseeding should be thread-safe, I'm not sure what to do about it. We can't change the blocking behavior of an existing function except in a major release: that would be an incompatible behavior change. We can add a new function mbedtls_ctr_drbg_reseed_locked that take the lock. But:

• 2.28 and 3.6 are long-time support branches, and in principle, we don't add new functionality to LTS branches, only bug fixes. This extra function would have very low risk and very low code size though.
• The next release will be a major release, where we can change what we want. But we're going to make ctr_drbg.h private: there will be a single, global RNG instance used under the hood. And there's currently no interface for explicit reseeding (we're considering one, but so far we don't plan to implement it soon).

[inorick]

I get the point about this being a breaking change. I would not mind having mbedtls_ctr_drbg_reseed_locked in version 3.6 to avoid it. If it is unacceptable for inclusion in version 3.6 then I guess adding explicit reseeding to version 4 is the way to go.

As a workaround, is locking the DRBG mutex like this correct before calling mbedtls_ctr_drbg_reseed?

#if defined(MBEDTLS_THREADING_C)
    mbedtls_mutex_init(&_drbg->private_mutex);
#endif

[irwir]

I can see why the initial seeding wouldn't be protected: if other threads already have access to the DRBG object while the initial seeding is in progress

Why any seeding (initial or reseeding) should not lock the internal object the same way as mbedtls_ctr_drbg_random does?

[gilles-peskine-arm]

As a workaround, is locking the DRBG mutex like this correct before calling mbedtls_ctr_drbg_reseed?

Yes, that's how we'd implement mbedtls_ctr_drbg_reseed_locked.

Why any seeding (initial or reseeding) should not lock the internal object the same way as mbedtls_ctr_drbg_random does?

Reseeding, yes. Initial seeding, why not, but also, the initial seeding generally takes place before sharing the object with other threads, so it's usually not needed (but wouldn't be harmful). Anyway, in the Mbed TLS 3 API, we can only add new functions, not change the existing behavior. And in the next major release, this won't be a public API any longer. So there's no point in discussing how the API should have looked like in an alternative world.

[irwir]

Initial seeding, why not, but also, the initial seeding generally takes place before sharing the object with other threads, so it's usually not needed (but wouldn't be harmful).

This will ensure that locks are functional.

By the way, iff typo: https://github.com/Mbed-TLS/mbedtls/blob/8067879c1fa08b7357ae47567ee875b6eec46e5d/tf-psa-crypto/drivers/builtin/src/ctr_drbg.c#L100

[gilles-peskine-arm]

iff is not a typo.

[irwir]

Thanks, but in that case only if part is incorrect, because mbedtls_mutex_init has no error checking and f_entropy was assigned unconditionally.

[gilles-peskine-arm]

Thanks, but in that case only if part is incorrect, because mbedtls_mutex_init has no error checking and f_entropy was assigned unconditionally.

Since mbedtls_mutex_init does not return any error condition, the mutex is considered initialized if mbedtls_mutex_init has been called and mbedtls_mutex_free has not been called subsequently.

If there are platforms where mbedtls_mutex_init can put the mbedtls_threading_mutex_t object in a state where calling mbedtls_mutex_free is incorrect, that's a problem with our mutex interface, unrelated to how it's used in CTR_DRBG. Or more precisely that's a problem with the implementation of the mutex interface on a given platform. We only ship a mutex implementation on top of pthreads and it doesn't have this problem.

[irwir]

https://github.com/Mbed-TLS/mbedtls/blob/8067879c1fa08b7357ae47567ee875b6eec46e5d/tf-psa-crypto/drivers/builtin/src/threading.c#L65 This is going too deep into offtopic, but should the value be returned rather than ignored?