SE keys: fix psa_destroy_key; add Github issue numbers for missing code #221
Conversation
This file contains hidden or 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
When a key slot is wiped, a copy of the key material may remain in operations. This is undesirable, but does not violate the safety of the code. Tracked in https://github.com/ARMmbed/mbed-crypto/issues/86
Adopt a simple method for tracking whether there was a failure: each fallible operation sets overall_status, unless overall_status is already non-successful. Thus in case of multiple failures, the function always reports whatever failed first. This may not always be the right thing, but it's simple. This revealed a bug whereby if the only failure was the call to psa_destroy_se_key(), i.e. if the driver reported a failure or if the driver lacked support for destroying keys, psa_destroy_key() would ignore that failure. For a key in a secure element, if creating a transaction file fails, don't touch storage, but close the key in memory. This may not be right, but it's no wronger than it was before. Tracked in ARMmbed#215
Drivers that allow destroying a key must have a destroy method. This test bug was previously not caught because of an implementation bug that lost the error triggered by the missing destroy method.
gilles-peskine-arm
added
needs: review
yanesca
approved these changes
Aug 14, 2019
gilles-peskine-arm
added
bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
I went through FIXME/TODO comments in the code and added the corresponding issue numbers:
I also modified a comment relating to https://github.com/ARMmbed/mbed-crypto/issues/86 (not related to secure elements) but did not add an issue number because this is not exactly a bug and may even be resolved differently.
While doing this, I found two bugs in
psa_destroy_keywhich I fixed and added a test for (in one case, I actually fixed a test bug that was hidden by the implementation bug).I'm making this PR to
psa-api-1.0-betato be on top of the latest SE work. Once this PR is merged, we should do another merge frompsa-api-1.0-betafollowing #212.