1556591 - Concurrency errors in PK11_SetWrapKey and its usage

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Martin Thomson [:mt:]]

PK11_SetWrapKey (and its companion PK11_GetWrapKey) are completely unsynchronized. When the calling code in ssl3_CacheWrappedSecret (the only usage I could find) runs concurrently, it is possible that two different keys are written to the slot. This ultimately results in logic errors.

The logic error results in two different keys being used for the same wrapped TLS master secret. One wraps the secret, the other is used to unwrap it. Because the key wrapping mechanism used (CKM_DES3_ECB apparently) doesn't include an integrity check, the net effect is that the master secret is garbled by the process. In the case I examined, a TLS 1.3 client uses the garbled value as its resumption master secret, causing it to produce a bad PSK binder.

This is only a problem at session startup and only if the first TLS connections that are completed occur at almost exactly the same time, so that two (or more) connections both hit the path where they believe that they should be setting the wrapping key before the call to PK11_SetWrapKey can be invoked and the result propagated to all threads.

The fix I am contemplating here uses the slot monitor in PK11_{Set|Get}WrapKey. But that isn't sufficient. To avoid having to hold a lock for the entire process in ssl3_CacheWrappedSecret, PK11_SetWrapKey will have to silently fail if a wrapping key is already set. And then ssl3_CacheWrappedSecret will have to reacquire the wrapping key after setting it in case another thread was first to set the value. This is gross, but I can't change the public API to PK11_SetWrapKey.

It might be better to avoid key wrapping entirely, but that might make it impossible to store master secrets from tokens that refuse to expose the raw value of keys without wrapping. Another approach might be to store these wrapping keys in libssl rather than on the slot.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Bug 1556591 - Eliminate races in uses of PK11_SetWrapKey, r?KevinJacobs

These functions are terrible, but in the spirit of ABI compatibility, I
can't really change their signature. And it isn't important enough to fix them,
so this introduces a quick hack.

This is wrapped up in the mess that is Bug 1556588, but I'm proceeding here
under the assumption that unconditional use of the lock is safe and that a
non-thread-safe module will explode in many other ways.

Comment 2

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/f8f056b035a7f760c491616d279fb48a0b30fbc3