[3.6] psa_load_builtin_key_into_slot: prevent accessing the PSA storage if key ID is in volatile range #10473
+397
−0
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
…ent key Do not try to load a persistent key whose key ID is in the volatile range. Signed-off-by: Valerio Setti <[email protected]>
…atile range Signed-off-by: Valerio Setti <[email protected]>
valeriosetti
added
needs-review
github-project-automation
bot
moved this to In Development
in Roadmap pull requests (new board)
4 tasks
|
@bjwtaylor @gilles-peskine-arm I took the liberty to add you as reviewers of this PR since you are also checking the one in tf-psa-crypto. Let me know if this is Ok for you ;) |
valeriosetti
force-pushed
the
issue488-3.6
branch
from
767db01 to
5278870
Compare
Add some assertions on the various ranges of key identifiers to ensure that they're disjoint and they comply with documented guarantees. Signed-off-by: Gilles Peskine <[email protected]>
Test what happens when the application tries to access a key and the storage contains something invalid: either a corrupted file, or a key ID that's outside the standard range for persistent keys. Coverage of APIs in this commit: * `psa_get_key_attributes()` (generally as a proxy for any key access); * `psa_export_key()` (minor, but does provide some coverage of what happens if only the key material is corrupted); * `psa_destroy_key()`, which hopefully should work even for a corrupted file. Coverage of key IDs in this commit: * Key IDs in various ranges: user (i.e. the normal range for persistent keys), builtin, volatile, reserved file ID, none of the above. * Includes coverage for nonzero owner ID. No coverage of corrupted files in this commit. Assert the behavior that I think is the right thing. Subsequent commits will reconcile the library behavior with the code as needed. Signed-off-by: Gilles Peskine <[email protected]>
Signed-off-by: Valerio Setti <[email protected]>
When testing what happens with when accessing a key ID in the built-in or volatile range and a file exists in storage, we were skipping the test case when the key existed. When the volatile or built-in key exists, the expectations on the test case are wrong, but the test case is still useful: we should ensure that the existence of the file doesn't somehow prevent access to the built-in or volatile key. So, instead of skipping, change the test assertions on the fly to ensure that we are accessing the existing key. Signed-off-by: Gilles Peskine <[email protected]>
valeriosetti
force-pushed
the
issue488-3.6
branch
from
5278870 to
0e59579
Compare
valeriosetti
changed the title
gilles-peskine-arm
added
needs-work
and removed
needs-review
Previous tests were backported from tf-psa-crypto and they work fine there. However the library implementation is not the same between 3.6 and tf-psa-crypto: in 3.6 we only prevent loading of persistent keys if their ID is within the volatile range, but the built-in one is still allowed. Therefore this commit fix expected return values for the 3.6 branch when built-in keys are accessed. Signed-off-by: Valerio Setti <[email protected]>
valeriosetti
force-pushed
the
issue488-3.6
branch
from
897ca93 to
cbc6bc5
Compare
valeriosetti
added
needs-review
gilles-peskine-arm
requested changes
Nov 3, 2025
Comment on lines
573
to
581
| /* Only remove the key file if it was created during this test. | ||
| * This helps for the case where MBEDTLS_PSA_INJECT_ENTROPY is enabled and | ||
| * PSA_CRYPTO_ITS_RANDOM_SEED_UID is being tested. This would case the file | ||
| * to be cancelled and the following test case to fail at the PSA_INIT() | ||
| * call. | ||
| */ | ||
| if (remove_key_on_exit) { | ||
| psa_its_remove(uid); | ||
| } |
There was a problem hiding this comment.
This is functionally correct, but the reason to skip the removal is not “if it was created during this test”, it's “if the key file is a reserved file”. This would be better expressed as
if (!KEY_ID_IN_RESERVED_FILE_ID_RANGE(key_id_arg) {
/* The key ID corresponds to a reserved file (e.g. transaction
* log or entropy seed). Don't corrupt that file. */
psa_its_remove(uid);
}
Given that it's hard to understand and remember every subtlety about the semantics of file UIDs, please change the logic to this.
That wasn't a problem in Mbed-TLS/TF-PSA-Crypto#492 because we currently don't implement anything that uses a reserved file as of 1.0.0, but it would come to bite if we start doing so. So please fix this in the crypto PR too.
Do not remove keys that belong to the reserved range. Signed-off-by: Valerio Setti <[email protected]>
valeriosetti
force-pushed
the
issue488-3.6
branch
from
5353fa8 to
8102fe3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Description
Backport of Mbed-TLS/TF-PSA-Crypto#492
What has been backported?
PR checklist