Fix psa_key_derivation_input_integer() not detecting bad state #186
+27
−5
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
waleed-elmelegy-arm
added
needs-review
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
waleed-elmelegy-arm
force-pushed
the
fix-key-deriv-bad-state-error
branch
from
64f48d6 to
6b7da82
Compare
gilles-peskine-arm
added
priority-medium
gilles-peskine-arm
requested changes
Feb 27, 2025
| @@ -8954,6 +8954,25 @@ void derive_input(int alg_arg, | |||
| } | |||
| TEST_EQUAL(actual_output_status, expected_output_status); | |||
|
|
|||
| /* Test calling input functions after operation has been aborted | |||
There was a problem hiding this comment.
Thanks for adding a test! But please don't add extra code to this function, which is already more complex than I'd like.
Proposal 1: keep this function, but skip the call to psa_key_derivation_setup() when alg == 0. Add test cases that expect that each input attempt returns BAD_STATE.
Proposal 2: make a new function that tries input_xxx without setup.
gilles-peskine-arm
added
needs-work
and removed
needs-review
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
waleed-elmelegy-arm
added
needs-review
gilles-peskine-arm
previously approved these changes
Feb 28, 2025
| @@ -8900,7 +8900,9 @@ void derive_input(int alg_arg, | |||
| psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_DERIVE); | |||
| psa_set_key_algorithm(&attributes, alg); | |||
|
|
|||
| PSA_ASSERT(psa_key_derivation_setup(&operation, alg)); | |||
| if (alg != 0) { | |||
There was a problem hiding this comment.
Minor: the code has 0 and the test data has PSA_ALG_NONE. The value of PSA_ALG_NONE won't ever change, but it's a bit confusing for readers who aren't very familiar with the API.
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Open
7 tasks
Open
7 tasks
waleed-elmelegy-arm
removed
needs-backports
core/psa_crypto.c
Outdated
| @@ -7612,6 +7612,12 @@ static psa_status_t psa_key_derivation_input_internal( | |||
| psa_status_t status; | |||
| psa_algorithm_t kdf_alg = psa_key_derivation_get_kdf_alg(operation); | |||
|
|
|||
| if (kdf_alg == 0) { | |||
There was a problem hiding this comment.
Suggested change
| if (kdf_alg == 0) { | |
| if (kdf_alg == PSA_ALG_NONE) { |
3 occurences.
I think the intent of @gilles-peskine-arm's comment was to keep it consistent between codebase and tests?
It woudl be good for users revewing the code to understand that this exit's purpose is.
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
minosgalanakis
added
approved
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.
Description
fixes #185
Fix psa_key_derivation_input_integer() not detecting bad state after an operation has been aborted.
changed some existing tests my justification is after they have returned PSA_ERROR_BAD_STATE they
were expected to return PSA_ERROR_INVALID_ARGUMENT but this shouldn't happen as the user is
expected to call abort() after a function has returned an error and we call abort() internally in case of an error
and without calling setup() first we should return PSA_ERROR_BAD_STATE and not any other error.
PR checklist
Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.