-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the RSA key mutex #4124
Comments
Note: the last sentence is correct if the application is not using the PK layer, but if it is, then switching to ECC keys should be safe, as the PK wrapper includes a hack that makes things thread-safe (at the cost of throwing away the cache, making Anyway, I'm in favour of removing this mutex but I have a few questions:
|
Thanks, I wasn't aware of the pk wrapper hack. (It could use some documentation!) I'm happy with keeping the mutexes in the DRBG and entropy modules because they have a reasonably clear usage (single-threaded set up then thread-safe random generation) and they address a very clear need: an application with a single RNG instance that's accessed from all threads. Our bug tracking record shows that we got complaints about the RNG mutexes when they broke. We got complaints about RSA/ECP concurrent usage because what was there was inconsistent. I think Making PSA crypto thread-safe is a few weeks' work. Not doable in the 3.0 time frame. And there's no point in starting optimistically, because it's useless until it's finished and likely to bitrot quickly. If we want to keep thread safety for key usage in the classic API, I think we should remove the mutex from RSA and put one in PK contexts. |
Good point, then I think it makes sense indeed to remove the RSA mutex but keep the RNG ones.
OK, and agreed that there's no point in starting if we can't complete in time.
Perhaps we should wait to see what the reactions are on the mailing-list: this could be a compromise if it turns out people miss the mutex. Otherwise, let's not do it (and we could always add it later if we find out later that there's interest.) |
Thinking a bit more about this: this would break programs such as our own
On another front, what's the migration path? What alternative are people supposed to use when upgrading? PSA is not thread-safe either, so people need to code up their own protection, and I think their only option to then integrate that into our SSL/TLS layer is to use the |
Ok, that's a good point: our only migration path is “deal with it”, and the fact that it's hard to deal with it if you're using the key for a TLS connection shows that this is not a reasonable path. If we had the time, I'd say to move the mutex to pk. But I fear that we don't. I propose to leave this in the “3.0 optional” category for now. |
In Mbed TLS 4.0 or more precisely TF-PSA-Crypto 1.0, |
There is now threading support in PSA Crypto and once #8147 is done these mutexes will become truly redundant and no migration path will be needed. |
RSA key objects include a mutex.
mbedtls_rsa_private
locks the mutex because it caches some auxiliary values used for blinding in the key object. (mbedtls_rsa_public
also locks the mutex but it seems pointless.) This allows applications to create a key (this must be done in a single-threaded way), then use that key concurrently.Unfortunately, the presence of the mutex complicates the lifecycle of RSA contexts. For example, part of the library assumes that
mbedtls_rsa_free
can be called twice on the same object, but as I write, this is not the case whenMBEDTLS_THREADING_C
is enabled and the platform does not allow callingmbedtls_mutex_free
on an already-freed mutex. (There's a fix in #4104, but it's a kludge and might not work in all sensible cases.)From a high-level architectural perspective, the RSA module is a low-level part of the code dedicated to peforming calculations. Managing concurrency is outside its scope. A better place to protect against concurrent access would be in the PSA dispatch code, where it is mandated by the PSA specification (as is concurrent key management) but not yet implemented in Mbed TLS.
The API is not well-documented, leading to unmet expectations.
ECC contexts do not have a mutex, even though they would need one: some values are cached in the
mbedtls_ecp_group
object to accelerate multiplication. So a multithreaded application that works with RSA keys can't easily be changed to ECC keys: without extra precautions, it would be prone to race conditions.Since mutexes don't really belong, complicate maintenance, and their presence is potentially misleading, I propose to remove mutexes from RSA keys in Mbed TLS 3.0.
Mailing list discussion: https://lists.trustedfirmware.org/pipermail/mbed-tls/2021-February/000294.html
The text was updated successfully, but these errors were encountered: