Tidy up attribute management inside psa_crypto #197
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit blindingly copies the size from the attributes. This is not correct for copy and import.
Add a parameter to the key import method of a secure element driver to make it report the key size in bits. This is necessary (otherwise the core has no idea what the bit-size is), and making import report it is easier than adding a separate method (for other key creation methods, this information is an input, not an output).
For a key in a secure element, save the bit size alongside the slot number. This is a quick-and-dirty implementation where the storage format depends on sizeof(size_t), which is fragile. This should be replaced by a more robust implementation before going into production.
Move the "core attributes" to a substructure of psa_key_attribute_t. The motivation is to be able to use the new structure psa_core_key_attributes_t internally.
65528 bits is more than any reasonable key until we start supporting post-quantum cryptography. This limit is chosen to allow bit-sizes to be stored in 16 bits, with 65535 left to indicate an invalid value. It's a whole number of bytes, which facilitates some calculations, in particular allowing a key of exactly PSA_CRYPTO_MAX_STORAGE_SIZE to be created but not one bit more. As a resource usage limit, this is arguably too large, but that's out of scope of the current commit. Test that key import, generation and derivation reject overly large sizes.
This is larger than the maximum key size introduced in the previous commit, by design. Make some room for flags (not used yet).
Change the type of key slots in memory to use psa_core_key_attributes_t rather than separate fields. The goal is to simplify some parts of the code. This commit only does the mechanical replacement, not the substitution. The bit-field `allocate` is now a flag `PSA_KEY_SLOT_FLAG_ALLOCATED` in the `flags` field. Write accessor functions for flags. Key slots now contain a bit size field which is currently unused. Subsequent commits will make use of it.
There is now a field for the key size in the key slot in memory. Use it. This makes psa_get_key_attributes() marginally faster at the expense of memory that is available anyway in the current memory layout (16 bits for the size, 16 bits for flags). That's not the goal, though: the goal is to simplify the code, in particular to make it more uniform between transparent keys (whose size can be recomputed) and keys in secure elements (whose size cannot be recomputed). For keys in a secure element, the bit size is now saved by serializing the type psa_key_bits_t (which is an alias for uint16_t) rather than size_t.
Key creation and psa_get_key_attributes
The flag to mark key slots as allocated was introduced to mark slots that are claimed and in use, but do not have key material yet, at a time when creating a key used several API functions: allocate a slot, then progressively set its metadata, and finally create the key material. Now that all of these steps are combined into a single API function call, the notion of allocated-but-not-filled slot is no longer relevant. So remove the corresponding flag. A slot is occupied iff there is a key in it. (For a key in a secure element, the key material is not present, but the slot contains the key metadata.) This key must have a type which is nonzero, so use this as an indicator that a slot is in use.
Add a non-regression test.
Consolidate attribute validation at the beginning of key creation into a single function. Improve comments.
gilles-peskine-arm
added
enhancement
gilles-peskine-arm
added
the
needs: preceding PR
This is cleaner and solves a complaint from MSVC about truncation from size_t to psa_key_bits_t.
|
CI is as ok as can be expected (the failures are only with TLS, which is broken in the API branch, and the ABI check fails as expected since this changes the ABI). |
This was referenced Aug 1, 2019
k-stachowiak
previously approved these changes
Aug 5, 2019
gilles-peskine-arm
removed
the
needs: preceding PR
yanesca
previously approved these changes
Aug 7, 2019
When psa_generate_random fails, psa_generate_key_internal frees the key buffer but a the pointer to the now-freed buffer in the slot. Then psa_generate_key calls psa_fail_key_creation which sees the pointer and calls free() again. This bug was introduced by ff5f0e7 "Implement atomic-creation psa_{generate,generator_import}_key" which changed how psa_generate_key() cleans up on errors. I went through the code and could not find a similar bug in cleanup on an error during key creation. Fix ARMmbed#207
Add tests that call psa_generate_random() (possibly via psa_generate_key()) with a size that's larger than MBEDTLS_CTR_DRBG_MAX_REQUEST. This causes psa_generate_random() to fail because it calls mbedtls_ctr_drbg_random() without taking the maximum request size of CTR_DRBG into account. Non-regression test for ARMmbed#206
mbedtls_ctr_drbg_random can only return up to MBEDTLS_CTR_DRBG_MAX_REQUEST (normally 1024) bytes at a time. So if more than that is requested, call mbedtls_ctr_drbg_random in a loop.
gilles-peskine-arm
dismissed stale reviews from yanesca and k-stachowiak
via
f181eca
k-stachowiak
approved these changes
Aug 8, 2019
gilles-peskine-arm
removed
the
needs: review
This was referenced Aug 8, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request changes the layout of key attributes and key slots to simplify some parts of the code. There are no user-visible or driver-visible changes. Some of these changes are in preparation for adding a
slot_numberkey attribute, others are just general maintenance.Successor of #191. The first new commit is "Move attribute fields to a substructure".
Also fix (on the API branch only) #206 and #207 which I discovered through one of the new tests. There's no non-regression test for #207 because we don't have a way to make the RNG fail (#208).