-
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
Fail multipart aead operations if a new key is found in the keyslot #5123
base: development
Are you sure you want to change the base?
Fail multipart aead operations if a new key is found in the keyslot #5123
Conversation
@@ -159,6 +159,8 @@ struct psa_aead_operation_s | |||
* any driver (i.e. none of the driver contexts are active). */ | |||
unsigned int MBEDTLS_PRIVATE(id); | |||
|
|||
uint64_t MBEDTLS_PRIVATE(associated_key_id); | |||
mbedtls_svc_key_id_t MBEDTLS_PRIVATE(key_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER
is active, this is a two-element structure (owner and id).
rnd_status = psa_generate_random( (uint8_t *) &slot->key_id, | ||
sizeof(slot->key_id) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does setting a random key id help, compared to setting to some fixed value which is guaranteed not to be a valid key id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should we not set a valid ID? This random value just shouldn't be the one that's marked as the wrong, initial, "slot unused" one.
Random key ID makes it so that the ID generator is stateless, and the generated ID's have a larger hamming distance from each other.
This might not be inside the thread model for PSA (fault injection) however.
Cons - possible ID clash for two identical IDs. Even then, it's an issue if we generate the same number twice on the same slot, not if we generate two same IDs in two different slots.
rnd_status = psa_generate_random( (uint8_t *) &slot->key_id, | ||
sizeof(slot->key_id) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the slot is reused immediately afterwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I follow what's the problem here - if you mean immediately as in after calling this function - we should have a mutex in place in the future. If you mean after the call to slot wipe - no problem here.
The implementation prioritizes readers. Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Make the multipart operation objects store key IDs of used keys to check if they changed before each multipart operation. Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>
f08b25b
to
50cc174
Compare
We are now converting older PRs to draft PRs where the following conditions are met: They have not been updated in the last 3 months, and they need more than non-trivial work to complete. |
Make the multipart operation objects store key IDs of used keys to check if they changed before each multipart operation.
A randomly generated
uint64_t
is added to each key slot and is populated in each call topsa_wipe_key_slot
.This ID is copied to a multipart operation objects as a part of
psa_aead_setup
.Do not merge yet: documentation needs an update and additional tests for
psa_aead_verify
have to be added. PR created mainly to find any CI issues and serve as a base for discussion.Also: This PR is based on this one, implementing a global readers-writer lock.