Move NIST_KW to PSA API #180
No reviewers
Labels
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/TF-PSA-Crypto!180
Loading…
Reference in a new issue
No description provided.
Delete branch "kw-api-rework"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Description
Rework of the KW API to update to the TF-PSA-CRYPTO interface, removal of support for KW from cipher.c and conversion of the legacy self test to a standard unit test. needs https://github.com/Mbed-TLS/mbedtls/pull/9983 resolves Mbed-TLS/mbedtls#9382
PR checklist
Note: test failures are expected here; the CI on the companion PR https://github.com/Mbed-TLS/mbedtls/pull/9983 is green so that's good.
Note for future PRs: please do smaller, atomic commits. I realise that when changing an API, a lot of things need to be changed at the same time, but here I think at least the part where you remove support for NIST-KW in Cipher could have been a separate commit. (Perhaps there were other opportunities for splitting things up, I haven't looked deep yet.)
Edit: good candidates for separate commits include renaming variables, moving the selftest function to a different file, changing the existing test function to use exact-sized buffers.
I'm not requesting a history rewrite here (note that the other reviewer might), but in the future please try to make atomic commits.
Just a high-level first pass, releasing the comments I have so far.
Please explain that this key must be for AES with
ECB_NO_PADDINGand must allowUSAGE_ENCRYPT(I'm assuming). Similarly for the unwrap function, except I'll guess it'll beUSAGE_DECRYPT(I didn't check, though).Looks like this file is now empty - if so, let's delete it.
@ -1,28 +1,27 @@NIST KW self testmbedtls_nist_kw_self_test:#Test vectors taken from NISTSeems wrong to have to add this dependency declaration on every single test case here. Unless I'm mistaken, the NIST-KW function will need this regardless of what parameters are passed. So, I think
NIST_KW_Cshould auto-enable what it needs ininclude/psa/crypto_adjust_config_dependencies.h.Also, since since can only be used with AES,
NIST_KW_Cshould also auto-enableWANT_ALG_AESthere. (And then we can remove the check incheck_config.h, and edit the documentation ininclude/psa/crypto_config.h.)@ -3,0 +4,4 @@int alg,int key_type,mbedtls_svc_key_id_t *key_id,const data_t *key)Looks to me like it also depends on
WANT_ALG_AES. Unless the whole module does now?ack
@ -1,28 +1,27 @@NIST KW self testmbedtls_nist_kw_self_test:#Test vectors taken from NISTack
@ -3,0 +4,4 @@int alg,int key_type,mbedtls_svc_key_id_t *key_id,const data_t *key)ack
ack
Interesting when you click on the link it says the file has been deleted and it doesn't appear if you clone, so I think it's already gone?
@ -1,28 +1,27 @@NIST KW self testmbedtls_nist_kw_self_test:#Test vectors taken from NISTSo I've added this in, however it appears to cause issues. My understanding is that the test is overriding these configs by calling
So I think we will need these configs in the test unless we change this behavior, let me know what you think.
@ -1,28 +1,27 @@NIST KW self testmbedtls_nist_kw_self_test:#Test vectors taken from NISTAh, good point about test configs, though I think it's actually the other way round: if a component in
all.shdisablesECB_NO_PADDINGwhile leavingNIST_KW_Cenable, theninclude/psa/crypto_adjust_config_dependencies.hwill override it and enableECB_NO_PADDINGanyway. So, if a test component really needsECB_NO_PADDINGdisabled, it shoud disableNIST_KW_Ctoo.Let me take a look at the components that disable
ECB_NO_PADDING, there are 4 matches forunset.*ECB_NO_PADDING, all intests/scripts/components-configuration-crypto.sh(still on the Mbed TLS side...)component_test_full_no_cipheralready disablesNIST_KW_Cso no question here.component_test_full_no_ccm_star_no_tagwants to have no built-in cipher, so we'll need to addunset NIST_KW_Cwith a comment that it would re-enableECB_NO_PADDING.component_build_aes_variationsalready disablesNIST_KW_Cso no question here.config_block_cipher_no_decryptalready disablesNIST_KW_Cso again nothing to change there.@ -1,28 +1,27 @@NIST KW self testmbedtls_nist_kw_self_test:#Test vectors taken from NISTFYI if you need to check configuration changes in
all.shcomponents, you can compare the PASS/SKIP status oftest_suite_configtest cases inoutcomes.csv(outcomes.csv.xzattachment to the PR-head job) before/after the change.@ -1,28 +1,27 @@NIST KW self testmbedtls_nist_kw_self_test:#Test vectors taken from NISTThis should now be resolved, I've removed the dependencies in the function file and moved them into the test files.
@ -3,0 +4,4 @@int alg,int key_type,mbedtls_svc_key_id_t *key_id,const data_t *key)PSA_WANT_KEY_TYPE_AES has been added to include/psa/crypto_adjust_config_dependencies.h
Now updated
I did an initial pass to familiarize with the PR and left a few minor comments.
Some failures in the CI seems to be related to
test/selftest.cstill relying on the "old" self test function that is being removed in this PR. Checking the CI https://github.com/Mbed-TLS/mbedtls/pull/9983 would be the only way to see if overall (mbedtls + tf-psa-crypto) this PR is OK, but thetf-psa-cryptoreference in 9983 is not updated. Can you please update it?@ -0,0 +1,5 @@API changesA similar comment apply also for the unwrap function
@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){Once you'll switch
retto be of typepsa_status_t, all theseret != 0should becomeret != PSA_SUCCESS@ -222,1 +196,3 @@if (ret != 0) {ret = psa_cipher_update(&wrap_operation,inbuff, 16, outbuff, sizeof(outbuff), &olen);if (ret != PSA_SUCCESS) {retshould be of typepsa_status_tnow that you are using PSA functions.@ -3,0 +4,4 @@int alg,int key_type,mbedtls_svc_key_id_t *key_id,const data_t *key)I think you added these
mbedtls_printfto mimic the behvior the of the self test function that was used before. However I don't think there are useful intest_suite_xxxfiles, so we can remove them as well as theverboseparameter.There should be
TEST_MEMORY_COMPAREfor this kind of test@ -3,0 +4,4 @@int alg,int key_type,mbedtls_svc_key_id_t *key_id,const data_t *key)So the TEST_MEMORY_COMPARE as far as I can tell can only do the second part. Is it best to split it into two operations for the length compare and the mem compare?
@ -3,0 +4,4 @@int alg,int key_type,mbedtls_svc_key_id_t *key_id,const data_t *key)TEST_MEMORY_COMPAREcompares both the length and the content. It was introduced after we noticed some tests were only comparing the content but not the length.Note that your implementation here is wrong: you're asserting that the lengths OR the (partial) content match, instead of AND.
TEST_MEMORY_COMPAREalso has the advantage that it prints the lengths on failure. And provides us a single place to change if we want to print the content on failure.Just reviewing the new commits since my previous partial review - still not a complete review.
Note to self: check what tests we have about usage.
I think the change from DE to ENcrypt was not intended?
@ -48,4 +48,8 @@#define PSA_WANT_ALG_CMAC 1Nit: the outer parentheses are not needed.
ITYM
PSA_WANT_ALG_AEShere (and also in the comment incrypto_config.h).@ -763,1 +760,4 @@* Enable the 128-bit key wrapping modes from NIST SP 800-38F:* KW (also known as RFC 3394) and KWP (RFC 5649).* Currently these modes are only supported with AES.*We should update "Requires" to "Auto-enables".
In the past, we used "Requires" because it was up to users to enable dependencies for things they wanted. Now we are doing it for them in the various
crypto_adjust_configfiles, so they're no longer required to do it. We can still keep the info here if they're curious, but should not give the impression that there's something they need to do about it.I'm not sure I agree with this removal.
NIST_KW_CusesALG_ECB_NO_PADDINGwhich is still listed as incompatible with this feature.@ -6,3 +25,3 @@* depends_on:MBEDTLS_NIST_KW_C* depends_on:MBEDTLS_NIST_KW_C:PSA_WANT_KEY_TYPE_AES* END_DEPENDENCIES*/Thanks, but please also remove all other calls to
mbedtls_printf()from this function, and theverboseparameter as well.@ -15,1 +33,3 @@/* END_CASE */unsigned char *out = NULL;size_t olen = 0;mbedtls_svc_key_id_t key_id = MBEDTLS_SVC_KEY_ID_INIT;Thanks but there's another occurrence in this function, can you fix it as well?
Indeed, the diff view now says "the file was deleted". There must have been a glitch last time, either in the diff view or between the chair and the keyboard. Anyway, marking as resolved.
@ -1,28 +1,27 @@NIST KW self testmbedtls_nist_kw_self_test:#Test vectors taken from NISTThe dependencies are updated here and the
all.shcomponent in https://github.com/Mbed-TLS/mbedtls/pull/9983 so marking this conversation as resolved.@ -48,4 +48,8 @@#define PSA_WANT_ALG_CMAC 1Fixed in a later commit.
Still not a complete review (that's a pretty large PR, so I'm proceeding step by step).
@ -0,0 +1,5 @@API changesI'd remove "type" at the, I think it's potentially confusing, and not helpful: "now take a mbedtls_svc_key_id_t key".
PSA all caps (it's an acronym) (and also a trademark or something).
PSA all caps
Typos: extra "and" (1st one), extra period (end of line). Can we just go back to the text as it was before this PR?
@ -3,0 +4,4 @@int alg,int key_type,mbedtls_svc_key_id_t *key_id,const data_t *key)Thanks for adding AES there - unless I'm mistaken you've also added
ECB_NO_PADDINGin the same place, so we no longer need to declared it as a dependency here.Same point about dependencies. (Multiple occurrences, I won't comment on each of them after this one.)
@ -264,0 +148,4 @@TEST_CALLOC(result, result_size);PSA_ASSERT(setup_psa_key(PSA_KEY_USAGE_ENCRYPT, PSA_ALG_ECB_NO_PADDING,This test obviously makes no sense as it was with the new API, so removing it is the correct move of course.
But this test served the purpose of checking what happens when we select an unsuitable parameters for the key. We should have a new test function that serves the same purpose. The documentation now says we want a key with type AES, alg ECB and usage ENCRYPT for wrap / DECRYPT for unwrap. What happens when we pass a key with the wrong type, or wrong alg, or wrong usage? We should have tests for that.
@ -302,0 +227,4 @@/* BEGIN_CASE */void mbedtls_nist_kw_unwrap_key_test(int mode,data_t *key,Please only set
ENCRYPThere, for the wrap test.I understand that for tests that call both wrap and unwrap it's convenient to set both, so that we don't have to have twice as many keys. But here we're only doing wrap, so we should really test that it works with only ENCRYPT set, as the documentation says.
Similarly to my comment on the previous function, please only set DECRYPT here.
@ -264,0 +148,4 @@TEST_CALLOC(result, result_size);PSA_ASSERT(setup_psa_key(PSA_KEY_USAGE_ENCRYPT, PSA_ALG_ECB_NO_PADDING,Many thanks for your comment, it's a very good spot. I think I was wrongly assuming that the the system would check the values of usage, algorithm and key_type when you call the relevant setup function here
github.com/bjwtaylor/TF-PSA-Crypto@f9c79995ce/drivers/builtin/src/nist_kw.c (L70)and then return an error. I've added some tests and dug into this in more detail and it appears it does some checks heregithub.com/bjwtaylor/TF-PSA-Crypto@f9c79995ce/core/psa_crypto.c (L1091), however they don't seem sufficient and some of the tests I've added can trigger segmentation faults if they use parameters that are not allowed. So I think we have two potential solutions:the first option seems the better one as it would solve the problem for everyone, though it's more complex and will effect some tests which appear to exploit this 'feature'. I'm also not sure of the history is there a reason it is currently coded like this? Do you know if there is a good reason it should be like this and therefore we should go for option 2?
@ -264,0 +148,4 @@TEST_CALLOC(result, result_size);PSA_ASSERT(setup_psa_key(PSA_KEY_USAGE_ENCRYPT, PSA_ALG_ECB_NO_PADDING,Some checks are expected to be done by setup indeed, for example when calling
psa_cipher_encrypt_setup(&wrap_operation, key, PSA_ALG_ECB_NO_PADDING)with a key that doesn't support/allow this algorithm or doesn't allow encrypt, we should be getting an error.But OTOH the fact that the key type must be AES (as opposed to other block ciphers like Aria or Camellia which support the same set of operations) is specific to NIST-KW, so we can't expect PSA functions to magically guess and instead we need to add checks in the NIST-KW code.
@ -264,0 +148,4 @@TEST_CALLOC(result, result_size);PSA_ASSERT(setup_psa_key(PSA_KEY_USAGE_ENCRYPT, PSA_ALG_ECB_NO_PADDING,Regardless of whether we expect the check to be done by
psa_cipher_{en,de}crypt_setup(&wrap_operation, key, PSA_ALG_ECB_NO_PADDING)or directly by the NIST-KW code, we should have a test case covering it.@bjwtaylor Please avoid rewriting commits that have been already reviewed, unless it becomes necessary to resolve conflicts. It's OK to rewrite commits that have not been reviewed, but rewriting commits that have been reviewed make it harder for reviewers to see what's changed since their last review.
As a team, we find that review bandwidth is often a limiting factor, so we try to optimise our process to make things easier for reviewers.
Just reviewing new things since my last review. Thanks for addressing my feedback, I think we have good test coverage now!
@ -123,0 +63,4 @@/** Currently NIST KW only supports PSA_KEY_TYPE_AES, so verify this is* set in the key attributes.*/Once you've called
get_key_attributes, you need to callpsa_reset_key_attributesbefore you return. Quoting from the spec:You probably want to add ad call to
reset_attributes()after the existingcleanuplabel.That's not correct. Before jumping to cleanup we need to set
retto a meaningful non-zero value, otherwise the function will return success.This should be
ret = ...; goto cleanup.@ -306,0 +287,4 @@outbuff + *output_length,sizeof(outbuff) - *output_length,&part_length);if (ret != PSA_SUCCESS) {Same remarks as for the other function.
@ -480,2 +445,4 @@depends_on:!MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTHmbedtls_nist_kw_wrap:MBEDTLS_KW_MODE_KWP:"5840df6e29b02af1ab493b705bf16ea1ae8338f4dcc176a8":"c37b7e6492584340bed12207808941155068f738":"138bdeaa9b8fa7fc61f97742e72248ee5ae6ae5360d1ae6a5f54f373fa543b6a"KWP AES-192 wrap rfc 5649 [#2]Well actually shouldn't we have the same tests with
MODE_KWPas well?Thank you, the following 6 cases are exactly the coverage I wanted :)
You'll probably need to declare a few dependencies for some of them though (
PSA_WANT_ALG_ECB_NO_PADDINGresp.PSA_WANT_KEY_TYPE_ARIA).Honestly all this is a bit redundant, we're already testing this in the other positive cases. Here we could remove the
msgandexpected_resultparameters and computations of expected output length, and just go withunsigned char input[16] = {0};+unsigned char output[32];(or whatever value is large enough for a 16-byte input, but I think 32 should do it).This would reduce noise a bit in the test data. Not a blocker though.
Minor: we should use
TEST_EQUAL.See comment above about simplifying this function.
Style: the coma should be at the end of the previous line.
Same thoughts as for the wrap test function; also optional. (However if you simplify both functions, we might even consider merging them as they'd end up being pretty similar.)
@ -0,0 +1,5 @@API changesI think we should also mention (probably in a separate bullet point still in "API changes") the fact that the
ciphermodule is no more able to dispatch to thenist_kwone.Adding the comment here because it's the 1st edited line of this file, but actually the comment refers to line 50. You can remove also this block from there
On line 51 of this file you can remove this block
@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){I'm not sure about this, so this is likely going to be a question for @mpg: do we really need this check?
Rationale behind my question is that if the key was not setup correctly in PSA then all operations using it will fail automatically on the following PSA calls.
Instead if we want to stick with this pattern, here we're just checking the key type, but I think we should also (at least) verify the key's associated algorithm is correct. Isn't it?
@ -48,4 +48,8 @@#define PSA_WANT_ALG_CMAC 1In case we really need to keep this change (see my other comment about
check_config.h) this should be as follows@ -104,4 +102,1 @@#error "MBEDTLS_NIST_KW_C defined, but not all prerequisites"#endif#if defined(MBEDTLS_BLOCK_CIPHER_NO_DECRYPT)Since we are targetting 4.0 release, which is a major number change, I don't fully agree with this change. Major release means that we can ask the user to change its build configuration, so I prefer to do things in the proper way, i.e:
crypto_adjust_config_dependencies.h.I guess this is another question for @mpg since it's a more general design decision.
So the aria dependency was added here https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/180/commits/7c30261225af3bb2951f44312e31011e3f435599. The PSA_WANT_ALG_ECB_NO_PADDING was on all the test cases however we removed it and added it to crypto_config.h, do we still need to add it here also?
Decided in the end to not merge them, but to move out the common functionality into a separate function to simplify. Let me know what you think?
@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){@mpg asked for this. The idea is system will check the alg and the usage as it knows from the key declaration what they should be. However the key_type is specified by the nist_kw spec, so it has no way of knowing whether it is correct, This is why this check is needed. @mpg @valeriosetti let me know if you are happy with the current implementation, or if it needs more work?
Ah, bad typo of mine here. I meant to write that the test cases that use CBC as the "bad" algorithm should probably depend on
PSA_WANT_ALG_CBC_NO_PADDING, sorry. You're absolutely right about ECB or course.@ -104,4 +102,1 @@#error "MBEDTLS_NIST_KW_C defined, but not all prerequisites"#endif#if defined(MBEDTLS_BLOCK_CIPHER_NO_DECRYPT)In the "past" (that is, 3.x) we had a hybrid world where
MBEDTLS_symbols were like "if you want to enable this symbol you'll need to figure out all its dependencies and enable them yourself" andPSA_WANTsymbols where like "just ask for what you want and we'll handle dependencies for you" (one big reason for that is it support accelerators/drivers a lot better).In 1.0 most of the public crypto feature symbols will be
PSA_WANT. (Ideally that would be all of them, but we're keeping some legacy symbols for crypto mechanisms that are not in PSA yet, like NIST-KW here, or LMS.) So I think it makes sense for those few remaining legacy symbols to obey the same logic as thePSA_WANTsymbols, rather than retaining a hybrid model just for them.@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){Yes exactly. If the key type was something like ECDSA, then clearly
psa_encrypt_setup()would throw an error as there's no way an ECDSA key can do symmetric encryption. But if the key type is Aria, Camellia or (3)DES, then that's something that absolutely works withpsa_encrypt_setup(..., ECB)(and all other operations we're doing). It just happens not to be allowed by the NIST spec. So we need to add a manual check.The documentation says that we require key type AES, with alg ECB and usage en/decrypt (for wrap resp. unwrap) - the last two parts are checked for us by
psa_en/decrypt_setup()but the first part only partially (it would reject non-block ciphers but accept other block ciphers than AES), hence this check.@valeriosetti does that make sense?
@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){Actually the fact that you asked may be a hint that we should have a comment in the code along the lines of "Algorithm and usage will be checked implicitly when trying to use the key, but we need an explicit check for the key type to ensure we're accepting only AES and not other block ciphers."
@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){Because indeed such checks are a bit unusual when using PSA, most of the type we're trying to be generic and just rely on the
psa_functions to check type alg and usage for us.@ -0,0 +1,5 @@API changesWell, the cipher module will become internal in 1.0, so I don't think we need to announced changes to it. But it also doesn't hurt - we'll have a final pass over the ChangeLog to consolidate entries before the release anyway.
@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){Agreed, more comments on the rational are probably useful here. I'll add something.
@ -123,0 +63,4 @@/** Currently NIST KW only supports PSA_KEY_TYPE_AES, so verify this is* set in the key attributes.*/Currently nothing breaks if you don't call
psa_reset_key_attributes(). But it used to be the case for some key types, and it may be the case again in the future. (What can break is a memory leak, if there is a pointer in the attribute structure to extra data in a heap-allocated buffer. Which is currently not the case, but has been the case for RSA keys in the past, and we may have a case for it in the future.)Because nothing actually breaks if you don't reset the attributes, we may already have places where it's not done, so it wouldn't be a big deal to not do it.
Thanks for fixing this. Unfortunately I found another occurrence of
MBEDTLS_NIST_KW_Cat line 100 in this file, but I think it should be removed as well.@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){Thanks guys for the explanation and the fix!
I'm basically OK with the current status. I only left some minor comment. (+ https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/180#discussion_r2033301960)
That's really minor, I know, but in case you have some other change to add, can you please align the indentation with other entries below?
Note: this is not worth a commit if there's nothing else to do ;)
@ -1578,9 +1574,9 @@ static int mbedtls_cipher_aead_decrypt(mbedtls_cipher_context_t *ctx,}here and also below for the
mbedtls_cipher_auth_decrypt_extfunction (sorry, but I cannot make the comment directly there)@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){TF-PSA-Crypto is the repo. The same holds also for line 321 in
mbedtls_nist_kw_unwrap@ -104,4 +102,1 @@#error "MBEDTLS_NIST_KW_C defined, but not all prerequisites"#endif#if defined(MBEDTLS_BLOCK_CIPHER_NO_DECRYPT)Oh OK, that's the part that I was missing. Thanks for the explanation!
I have reviewed everything except
nist_kw.c. I'll review it once the updated tests pass.In some places, the same problem appears more than once and I only made one comment (e.g. if I make a comment about wrap, please check if it also applies to unwrap).
@ -0,0 +1,5 @@API changescipher.hwill not be a public API, so please remove this line.mbedtls_svc_key_id_tis a nonstandard alias for the standard typepsa_key_id_tin most configurations, so we should avoid mentioningmbedtls_svc_key_id_tin documentation. In any case I don't think we need to give an explicit type name here:Should we also mention that the output size and output length parameters have been swapped, to make them follow the PSA order? Or is that too much detail for the changelog entry?
Please use the Doxygen syntax to link macro names in the documentation.
and so on below (
#PSA_ALG_ECB_NO_PADDING,#MBEDTLS_KW_MODE_KW,#PSA_ERROR_DATA_INVALID, ...).PSA_ERROR_DATA_INVALIDis intended for invalid data read from storage. Here the appropriate error would bePSA_ERROR_INVALID_ARGUMENT.We're now moving from module-specific errors to PSA-standard errors, although it's unclear whether we'll finish this migration before the 1.0 release. Please change the documentation to something more generic like
For unwrapping, we should also document which error is returned when the ciphertext is invalid. That's
PSA_ERROR_INVALID_SIGNATURE(same as with “proper” AEAD algorithms, the PSA equivalent ofMBEDTLS_ERR_CIPHER_AUTH_FAILED).I see that the code returns
PSA_ERROR_CORRUPTION_DETECTED. This error code is a “can't happen” error that must not be returned on invalid user input.@ -1616,4 +1588,2 @@#if defined(MBEDTLS_CIPHER_MODE_AEAD)/* AEAD case: check length before passing on to shared function */if (output_len < ilen + tag_len) {Please update
cipher.has well.@ -763,1 +760,4 @@* Enable the 128-bit key wrapping modes from NIST SP 800-38F:* KW (also known as RFC 3394) and KWP (RFC 5649).* Currently these modes are only supported with AES.*KW and KWP could be used with other block ciphers (e.g. Camellia-KW is specified in RFC 3657). So I think we should continue to require at least one supported block cipher, rather than automatically enabling AES.
Expanding KW to allow other block ciphers is out of scope here, but we shouldn't make it difficult if we want to do it in TF-PSA-Crypto 1.x.
As a consequence, in addition to not auto-enabling AES, I think we should keep a guard on
PSA_WANT_KEY_TYPE_AESaround any part of the code that is specific to AES, in particular the test data.This change looks wrong. How could the same input have been previously valid, but now is no longer valid?
And at least one other case above. Currently
::is treated a single separator in data files, but we should avoid depending on this arcane detail.For an output buffer that's too short, the PSA error is
PSA_ERROR_BUFFER_TOO_SMALL.@ -1,176 +1,80 @@/* BEGIN_HEADER */Please use
PSA_ASSERTfor all things that can fail, so that the test framework reports exactly what failed, rather than reporting thatsetup_psa_keyhas failed. The common idiom for that is something like:Please use
PSA_INIT()in test code. It's equivalent toPSA_ASSERT(psa_crypto_init()), but usingPSA_INITgives us the ability to customize it in some test builds (which we currently do forPSA_DONEbut not forPSA_INITitself, I think, but it could happen).There are no test vectors around here. I guess the comment belongs in the
.datafile.This isn't what is called “self test” in the context of cryptographic modules: a self-test is a test that is contained in the library code. (It's a kind of useless thing that is a historical survivance that security evaluations often insist on. Having good self-tests is outside the scope of 1.0, though we're trying to keep the
selftestprogram useful.)This function does a wrap-unwrap test with known data, so a good name would be
nist_kw_wrap_unwrap. I'm not sure we need it given that we already have known answer tests for wrap and for unwrap, but it's fine to have it.Please avoid using an
mbedtls_orpsa_prefix for test entry points. Using those prefixes can cause confusion between test entry points and library functions — they have their own namespace in C (because the C function hasmbedtls_test_prefixed to it), but things like tag lookup gets confused.(Historically some test files give the same name to the library function and to the main test function that tests it, but it has caused confusion, so we shouldn't do it in new code.)
Please don't do it yourself. We have a history of getting this wrong — such as here where it should be
out_len == olen && …instead ofout_len == olen || ….Rather than hard-coding a size, we should allocate an output buffer of exactly the expected size. This way, we can validate that the tested function doesn't insist on having a larger output buffer.
and
mbedtls_free(out)in the cleanup code.(Sometimes we also test that the function works correctly when the output buffer is larger than needed, but we only do it when a white-box approach suggests that it might be a concern, which is not the case here.)
When we test with Asan (which we often do), it validates that the code doesn't try to access beyond the end of the buffer.
Isn't
kw_out_lenredundant as a parameter? Isn't it alwayskw_res->len?Please don't combine an uninitialized variable and an initialized variable on the same line. It's hard to read, and in particular it's hard to notice that
olenis not initialized.Please avoid lines wider than 80 characters. It's not an absolute rule, for example it's ok to go a little beyond when there's a long identifier or a long string, or when indenting existing code just a little more. (The code style checker has a hard stop at 100.) Here
PSA_ALG_ECB_NO_PADDINGshould just be on the next line.Now that there are no contexts, what test coverage does this function bring?
(I'm inclined to remove it, so I haven't reviewed its updated code.)
I prefer to initialize things as early as possible, thus
rather than relying on assignments and memset further below. This way there's no risk of forgetting to initialize on some code paths (e.g. in case of an early error).
@ -267,1 +162,4 @@mbedtls_free(result);psa_destroy_key(key_id);PSA_DONE();}In new code, we prefer
TEST_EQUAL(…, res)which shows the values on both sides if the test fails. We normally don't rewrite old code when making minor changes like you're doing here (with respect to this part of the test code), but feel free to change toTEST_EQUALif it helps you diagnose test failures.@ -302,0 +229,4 @@void mbedtls_nist_kw_unwrap_key_test(int mode,data_t *key,int usage,int alg,Allocating memory can fail! Also
TEST_CALLOCis consistent about a length of 0 (alwaysNULL, whereas the C standard leaves it implementation-defined).Minor: style (I'm surprised that uncrustify doesn't complain):
Redundant with the
TEST_MEMORY_COMPAREjust below.As above, please combine the declaration with initialization whenever practical, and avoid declaring multiple variables on the same line (especially if they're initialized).
goto exitjust beforeexit:is pointless.Good point I will investigate.
@ -1,176 +1,80 @@/* BEGIN_HEADER */OK, I think it's still useful as it test the known data from the NIST KW spec I will rename it as suggested though.
@ -1,176 +1,80 @@/* BEGIN_HEADER */I've adjusted this line and others that are similar, let me know what you think.
This comment is on the input buffer and refers to the output. However am a right to assume this comment refers to both the correct code is PSA_ERROR_BUFFER_TOO_SMALL for a input or output buffer?
@ -1,176 +1,80 @@/* BEGIN_HEADER */Now removed.
@ -0,0 +1,5 @@API changesThink this is probably too much. I've updated the changelog as per the other comments, as discussed in stand up there have been multiple iterations of this. Let me know what you think of the current one.
@ -763,1 +760,4 @@* Enable the 128-bit key wrapping modes from NIST SP 800-38F:* KW (also known as RFC 3394) and KWP (RFC 5649).* Currently these modes are only supported with AES.*This makes a very good point, let's discuss with @mpg when he gets back as he requested we move the parameters here so is probably will be interested in this discussion.
@ -763,1 +760,4 @@* Enable the 128-bit key wrapping modes from NIST SP 800-38F:* KW (also known as RFC 3394) and KWP (RFC 5649).* Currently these modes are only supported with AES.*Ah, I wasn't aware of that RFC or that NIST-KW(P) could be used with other block ciphers. Since it's (as the name suggests) a NIST thing, I only checked NIST standard, which naturally only mentions NIST-approved block ciphers, that is AES only these days.
Indeed that changes things, so in light of this we should only auto-enable ECB but not AES, and as a consequence declare a dependency on
PSA_WANT_KEY_TYPE_AESfor all current test data. (More precisely, the test function if AES is hard-coded in the function, or the data when the the function takes the cipher as an input parameter and the data passes AES. That is, the place that makes us use AES should have the dependency on AES.)In the library code we should make sure that if
PSA_WANT_KEY_TYPE_AESis not enabled, then the function always return an error (until we start actually supporting ciphers other than AES).@ -763,1 +760,4 @@* Enable the 128-bit key wrapping modes from NIST SP 800-38F:* KW (also known as RFC 3394) and KWP (RFC 5649).* Currently these modes are only supported with AES.*Sounds good, I'll update to reflect these changes. I think for the last bit we can still use the implementation previously suggested and tested here https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/180/commits/2161e78f7d9ea69d983e1cb79fcc79f6291df293. Though let me know if you think we need something further.
From the CI:
Seem legit, we've removed this function from the library, we probably want to stop calling it in the programs :)
@mpg, yes had the same conversation with @gilles-peskine-arm. The PR pair is quite interdependent due to the self test. So the plan is I've updated the mbedtls part to remove the submodule update, this removes the self test which I'm hoping will mean it will pass. That means we can merge this part and then this PR, though lets wait for the CI to prove this. We may need to do further splitting.
Ping @gilles-peskine-arm @valeriosetti for re-review.
I've now done a full review. 701b188923a1a7282c5f3e356f90d79c92a3e7a7 looks good to me apart from a couple of documentation issues and a few problems in error conditions/reporting.
Some of my comments on
mbedtls_nist_kw_wrap()also apply to unwrapping.Here and elsewhere:
(
\ctypesets the next word as code verbatim, that's not what we need here.)[in]: that annotation is just for pointers.@ -121,2 +52,4 @@}static int verify_input(mbedtls_svc_key_id_t *key){More accurately, all we ask of the PSA API is to perform ECB, so any block cipher key type would be accepted (but other key types would be rejected). The code here only works with 128-bit block ciphers, so at a minimum it needs to reject block ciphers with a different block size (which, in the PSA API, is just DES). Additionally, we don't allow block ciphers other than AES (which, currently, would be Camellia and ARIA), which is a self-imposed limitation (just because we don't have tests).
NOT_PERMITTEDis meant to indicate some kind of key-specific policy restriction. That's not what is going on here. EitherINVALID_ARGUMENTorNOT_SUPPORTEDwould work, withNOT_SUPPORTEDbeing more accurate for 128-bit block ciphers other than AES andINVALID_ARGUMENTbeing more accurate for other key types.I'm not sure if it's worth it, but you could check accurate does-it-make-sense (i.e. not invalid argument) with
The type checks in wrap and unwrap are the same. Consider making them an auxiliary function.
@ -221,2 +196,2 @@inbuff, 16, outbuff, &olen);if (ret != 0) {ret = psa_cipher_update(&wrap_operation,inbuff, 16, outbuff, sizeof(outbuff), &olen);If this call ever processes some data, I don't think it would be correct. After encrypting a block, further things happen in
outbuff. If a call topsa_cipher_updatewith a 16-byte input does not process the given block fully, the output is already wrong at this point.So I think we should remove the call to
finish, and maybe assert thatolen == 16after the call to update above.This doesn't look right: there is a buffer overflow on error when
1 <= output_size <= semiblocks * KW_SEMIBLOCK_LENGTH - 1. I'm not sure if this case is reached: it depends on exactly how thegoto cleanupare positioned with respect toouptut_sizechecks andsemiblocksupdates. But it's at least fragile.We only need to clean up if there may be confidential data in
output, so we could check thatoutput_sizeis sufficiently large here. I'm not 100% sure about the number of semiblocks here though. Alternatively we could keep it simple and domemset(output, 0, output_size), which is inefficient if there is an error andoutput_sizeis very large, but that isn't an important scenario.This is partly preexisting, but only partly: there are more error cases now, and
output_sizeis fully validated now, so there may be new cases that could trigger an overflow.@ -306,1 +291,4 @@goto cleanup;}*output_length = (semiblocks - 1) * KW_SEMIBLOCK_LENGTH;Non-blocker:
mbedtls_buffer_offsetis only needed to handle empty buffers correctly ({.p=NULL, .len=0}is a valid empty buffer, butp + lenis not valid C whenp == NULLeven whenlen == 0). KW and KWP do not accept empty input, and this function has already made size checks that guarantee thatoutput_size > 0, sooutput + update_output_lengthwould be correct here.@ -309,3 +296,4 @@if (ret != PSA_SUCCESS) {memset(output, 0, (semiblocks - 1) * KW_SEMIBLOCK_LENGTH);}mbedtls_platform_zeroize(inbuff, sizeof(inbuff));As previously noted, when the output buffer is too small, the error code should be
PSA_ERROR_BUFFER_TOO_SMALL. The errorINVALID_ARGUMENTconveys that some data is bad (whether it's an unknown value in an enum-like integer, a known value that isn't in an appropriate category, an integer out of range, a badly formatted string, etc.). If an error can be solved by allocating more memory for a buffer, without changing any data, that error should be conveyed byBUFFER_TOO_SMALL.There is a buffer overflow here if the value of
*output_lengthon entry happens to be large. The obvious fix would be to set*output_length = 0earlier, before the firstgoto cleanup.@ -48,4 +48,8 @@#define PSA_WANT_ALG_CMAC 1As discussed previously, KW doesn't intrinsically require AES, it's just an implementation limitation. So
MBEDTLS_NIST_KW_Cshould not auto-enable AES. (It should auto-enablePSA_WANT_ALG_ECB_NO_PADDING, because that's an internal detail of howMBEDTLS_NIST_KW_Cis implemented — auto-enabling ECB would not be necessary if PSA supported KW natively, andMBEDTLS_NIST_KW_Cwould instead auto-enablePSA_WANT_ALG_NIST_KW.)Preexisting: the fact that only AES is approved by NIST is a bit besides the point. AES is the only 128-bit block cipher that NIST approves of in general, this isn't specific to KW/KWP. What matters here is that our library only supports AES.
Sorry, I had missed your question here.
PSA_ERROR_BUFFER_TOO_SMALLspecifically means that an output buffer is not large enough for the output data. The errorINVALID_ARGUMENTconveys that some data is bad (whether it's an unknown value in an enum-like integer, a known value that isn't in an appropriate category, an integer out of range, a badly formatted string, etc.). If an error can be solved by allocating more memory for a buffer, without changing any data, that error should be conveyed byBUFFER_TOO_SMALL.@ -48,4 +48,8 @@#define PSA_WANT_ALG_CMAC 1Apologies, I'd added it in the test and forgot to remove it in module dependencies. I've also optimised the test dependency declarations. Let me know what you think?
I've reworded this, let me know what you think.
Should now be resolved.
OK, I've updated the error codes. Hopefully I've understood what we want correctly and they are now correct, though let me know what you think and if you want me to make some amendments I can.
We're getting close but a few things still aren't quite right.
Non-blocker:
output_sizeis still not a pointer so it should not be annotated[in](but Doxygen andclang -Wdocumentationdon't care).@ -38,76 +39,6 @@ static const unsigned char NIST_KW_ICV1[] = { 0xA6, 0xA6, 0xA6, 0xA6, 0xA6, 0xA6/*! The 32-bit default integrity check value (ICV) for KWP mode. */static const unsigned char NIST_KW_ICV2[] = { 0xA6, 0x59, 0x59, 0xA6 };As previously discussed, this isn't some kind of policy violation, so the error code should not be
NOT_PERMITTED. EitherINVALID_ARGUMENTorNOT_SUPPORTEDwould be ok.@ -120,86 +51,131 @@ static void calc_a_xor_t(unsigned char A[KW_SEMIBLOCK_LENGTH], uint64_t t)}Please make
attributesa local variable (and don't forget to clean up on error paths), since it isn't used outside this function.@ -221,2 +196,2 @@inbuff, 16, outbuff, &olen);if (ret != 0) {ret = psa_cipher_update(&wrap_operation,inbuff, 16, outbuff, sizeof(outbuff), &olen);This is not addressed.
This is not addressed. I'm not sure there's actually a bug right now, but at the very least this code depends on the invariant that
output_size >= semiblocks * KW_SEMIBLOCK_LENGTHand this invariant is not documented.@ -757,13 +757,13 @@/**ECB is an implementation detail. It isn't relevant here.
The wording is also a little weird because KW and KWP are all the modes that can be used with AES, anyway. (SP 800-38F defines three modes, and the third mode is only for 64-bit block ciphers, which means DES in NIST specifications.)
Also, preexisting: these modes are often known from the RFC rather than from NIST, so it would be helpful to cite the RFC here as well.
@ -169,3 +159,2 @@NIST KWP wrap AES-128 CAVS 21.4 PLAINTEXT LENGTH = 4096 count 1depends_on:MBEDTLS_AES_Cmbedtls_nist_kw_wrap:MBEDTLS_CIPHER_ID_AES:MBEDTLS_KW_MODE_KWP:"6b8ba9cc9b31068ba175abfcc60c1338":"8af887c58dfbc38ee0423eefcc0e032dcc79dd116638ca65ad75dca2a2459f13934dbe61a62cb26d8bbddbabf9bf52bbe137ef1d3e30eacf0fe456ec808d6798dc29fe54fa1f784aa3c11cf39405009581d3f1d596843813a6685e503fac8535e0c06ecca8561b6a1f22c578eefb691912be2e1667946101ae8c3501e6c66eb17e14f2608c9ce6fbab4a1597ed49ccb3930b1060f98c97d8dc4ce81e35279c4d30d1bf86c9b919a3ce4f0109e77929e58c4c3aeb5de1ec5e0afa38ae896df9121c72c255141f2f5c9a51be5072547cf8a3b067404e62f9615a02479cf8c202e7feb2e258314e0ebe62878a5c4ecd4e9df7dab2e1fa9a7b532c2169acedb7998d5cd8a7118848ce7ee9fb2f68e28c2b279ddc064db70ad73c6dbe10c5e1c56a709c1407f93a727cce1075103a4009ae2f7731b7d71756eee119b828ef4ed61eff164935532a94fa8fe62dc2e22cf20f168ae65f4b6785286c253f365f29453a479dc2824b8bdabd962da3b76ae9c8a720155e158fe389c8cc7fa6ad522c951b5c236bf964b5b1bfb098a39835759b95404b72b17f7dbcda936177ae059269f41ecdac81a49f5bbfd2e801392a043ef06873550a67fcbc039f0b5d30ce490baa979dbbaf9e53d45d7e2dff26b2f7e6628ded694217a39f454b288e7906b79faf4a407a7d207646f93096a157f0d1dca05a7f92e318fc1ff62ce2de7f129b187053":"aea19443d7f8ad7d4501c1ecadc6b5e3f1c23c29eca608905f9cabdd46e34a55e1f7ac8308e75c903675982bda99173a2ba57d2ccf2e01a02589f89dfd4b3c7fd229ec91c9d0c46ea5dee3c048cd4611bfeadc9bf26daa1e02cb72e222cf3dab120dd1e8c2dd9bd58bbefa5d14526abd1e8d2170a6ba8283c243ec2fd5ef07030b1ef5f69f9620e4b17a3639341005887b9ffc793533594703e5dcae67bd0ce7a3c98ca65815a4d067f27e6e66d6636cebb789732566a52ac3970e14c37310dc2fcee0e739a16291029fd2b4d534e30445474b26711a8b3e1ee3cc88b09e8b1745b6cc0f067624ecb232db750b01fe5457fdea77b251b10fe95d3eeedb083bdf109c41dba26cc9654f787bf95735ff07070b175cea8b62302e6087b91a0415474605691099f1a9e2b626c4b3bb7aeb8ead9922bc3617cb427c669b88be5f98aea7edb8b0063bec80af4c081f89778d7c7242ddae88e8d3aff1f80e575e1aab4a5d115bc27636fd14d19bc59433f697635ecd870d17e7f5b004dee4001cddc34ab6e377eeb3fb08e9476970765105d93e4558fe3d4fc6fe053aab9c6cf032f1116e70c2d65f7c8cdeb6ad63ac4291f93d467ebbb29ead265c05ac684d20a6bef09b71830f717e08bcb4f9d3773bec928f66eeb64dc451e958e357ebbfef5a342df28707ac4b8e3e8c854e8d691cb92e87c0d57558e44cd754424865c229c9e1abb28e003b6819400b"mbedtls_nist_kw_wrap:MBEDTLS_KW_MODE_KWP:"6b8ba9cc9b31068ba175abfcc60c1338":"8af887c58dfbc38ee0423eefcc0e032dcc79dd116638ca65ad75dca2a2459f13934dbe61a62cb26d8bbddbabf9bf52bbe137ef1d3e30eacf0fe456ec808d6798dc29fe54fa1f784aa3c11cf39405009581d3f1d596843813a6685e503fac8535e0c06ecca8561b6a1f22c578eefb691912be2e1667946101ae8c3501e6c66eb17e14f2608c9ce6fbab4a1597ed49ccb3930b1060f98c97d8dc4ce81e35279c4d30d1bf86c9b919a3ce4f0109e77929e58c4c3aeb5de1ec5e0afa38ae896df9121c72c255141f2f5c9a51be5072547cf8a3b067404e62f9615a02479cf8c202e7feb2e258314e0ebe62878a5c4ecd4e9df7dab2e1fa9a7b532c2169acedb7998d5cd8a7118848ce7ee9fb2f68e28c2b279ddc064db70ad73c6dbe10c5e1c56a709c1407f93a727cce1075103a4009ae2f7731b7d71756eee119b828ef4ed61eff164935532a94fa8fe62dc2e22cf20f168ae65f4b6785286c253f365f29453a479dc2824b8bdabd962da3b76ae9c8a720155e158fe389c8cc7fa6ad522c951b5c236bf964b5b1bfb098a39835759b95404b72b17f7dbcda936177ae059269f41ecdac81a49f5bbfd2e801392a043ef06873550a67fcbc039f0b5d30ce490baa979dbbaf9e53d45d7e2dff26b2f7e6628ded694217a39f454b288e7906b79faf4a407a7d207646f93096a157f0d1dca05a7f92e318fc1ff62ce2de7f129b187053":"aea19443d7f8ad7d4501c1ecadc6b5e3f1c23c29eca608905f9cabdd46e34a55e1f7ac8308e75c903675982bda99173a2ba57d2ccf2e01a02589f89dfd4b3c7fd229ec91c9d0c46ea5dee3c048cd4611bfeadc9bf26daa1e02cb72e222cf3dab120dd1e8c2dd9bd58bbefa5d14526abd1e8d2170a6ba8283c243ec2fd5ef07030b1ef5f69f9620e4b17a3639341005887b9ffc793533594703e5dcae67bd0ce7a3c98ca65815a4d067f27e6e66d6636cebb789732566a52ac3970e14c37310dc2fcee0e739a16291029fd2b4d534e30445474b26711a8b3e1ee3cc88b09e8b1745b6cc0f067624ecb232db750b01fe5457fdea77b251b10fe95d3eeedb083bdf109c41dba26cc9654f787bf95735ff07070b175cea8b62302e6087b91a0415474605691099f1a9e2b626c4b3bb7aeb8ead9922bc3617cb427c669b88be5f98aea7edb8b0063bec80af4c081f89778d7c7242ddae88e8d3aff1f80e575e1aab4a5d115bc27636fd14d19bc59433f697635ecd870d17e7f5b004dee4001cddc34ab6e377eeb3fb08e9476970765105d93e4558fe3d4fc6fe053aab9c6cf032f1116e70c2d65f7c8cdeb6ad63ac4291f93d467ebbb29ead265c05ac684d20a6bef09b71830f717e08bcb4f9d3773bec928f66eeb64dc451e958e357ebbfef5a342df28707ac4b8e3e8c854e8d691cb92e87c0d57558e44cd754424865c229c9e1abb28e003b6819400b"Test data that is specific to a particular key type should pass the key type as an argument and should have a dependency on the key type.
But never mind, let's stop flip-flopping on this and just go with what we have and fix it later as part of the technical debt backlog. It's pretty low priority.
@ -169,3 +159,2 @@NIST KWP wrap AES-128 CAVS 21.4 PLAINTEXT LENGTH = 4096 count 1depends_on:MBEDTLS_AES_Cmbedtls_nist_kw_wrap:MBEDTLS_CIPHER_ID_AES:MBEDTLS_KW_MODE_KWP:"6b8ba9cc9b31068ba175abfcc60c1338":"8af887c58dfbc38ee0423eefcc0e032dcc79dd116638ca65ad75dca2a2459f13934dbe61a62cb26d8bbddbabf9bf52bbe137ef1d3e30eacf0fe456ec808d6798dc29fe54fa1f784aa3c11cf39405009581d3f1d596843813a6685e503fac8535e0c06ecca8561b6a1f22c578eefb691912be2e1667946101ae8c3501e6c66eb17e14f2608c9ce6fbab4a1597ed49ccb3930b1060f98c97d8dc4ce81e35279c4d30d1bf86c9b919a3ce4f0109e77929e58c4c3aeb5de1ec5e0afa38ae896df9121c72c255141f2f5c9a51be5072547cf8a3b067404e62f9615a02479cf8c202e7feb2e258314e0ebe62878a5c4ecd4e9df7dab2e1fa9a7b532c2169acedb7998d5cd8a7118848ce7ee9fb2f68e28c2b279ddc064db70ad73c6dbe10c5e1c56a709c1407f93a727cce1075103a4009ae2f7731b7d71756eee119b828ef4ed61eff164935532a94fa8fe62dc2e22cf20f168ae65f4b6785286c253f365f29453a479dc2824b8bdabd962da3b76ae9c8a720155e158fe389c8cc7fa6ad522c951b5c236bf964b5b1bfb098a39835759b95404b72b17f7dbcda936177ae059269f41ecdac81a49f5bbfd2e801392a043ef06873550a67fcbc039f0b5d30ce490baa979dbbaf9e53d45d7e2dff26b2f7e6628ded694217a39f454b288e7906b79faf4a407a7d207646f93096a157f0d1dca05a7f92e318fc1ff62ce2de7f129b187053":"aea19443d7f8ad7d4501c1ecadc6b5e3f1c23c29eca608905f9cabdd46e34a55e1f7ac8308e75c903675982bda99173a2ba57d2ccf2e01a02589f89dfd4b3c7fd229ec91c9d0c46ea5dee3c048cd4611bfeadc9bf26daa1e02cb72e222cf3dab120dd1e8c2dd9bd58bbefa5d14526abd1e8d2170a6ba8283c243ec2fd5ef07030b1ef5f69f9620e4b17a3639341005887b9ffc793533594703e5dcae67bd0ce7a3c98ca65815a4d067f27e6e66d6636cebb789732566a52ac3970e14c37310dc2fcee0e739a16291029fd2b4d534e30445474b26711a8b3e1ee3cc88b09e8b1745b6cc0f067624ecb232db750b01fe5457fdea77b251b10fe95d3eeedb083bdf109c41dba26cc9654f787bf95735ff07070b175cea8b62302e6087b91a0415474605691099f1a9e2b626c4b3bb7aeb8ead9922bc3617cb427c669b88be5f98aea7edb8b0063bec80af4c081f89778d7c7242ddae88e8d3aff1f80e575e1aab4a5d115bc27636fd14d19bc59433f697635ecd870d17e7f5b004dee4001cddc34ab6e377eeb3fb08e9476970765105d93e4558fe3d4fc6fe053aab9c6cf032f1116e70c2d65f7c8cdeb6ad63ac4291f93d467ebbb29ead265c05ac684d20a6bef09b71830f717e08bcb4f9d3773bec928f66eeb64dc451e958e357ebbfef5a342df28707ac4b8e3e8c854e8d691cb92e87c0d57558e44cd754424865c229c9e1abb28e003b6819400b"mbedtls_nist_kw_wrap:MBEDTLS_KW_MODE_KWP:"6b8ba9cc9b31068ba175abfcc60c1338":"8af887c58dfbc38ee0423eefcc0e032dcc79dd116638ca65ad75dca2a2459f13934dbe61a62cb26d8bbddbabf9bf52bbe137ef1d3e30eacf0fe456ec808d6798dc29fe54fa1f784aa3c11cf39405009581d3f1d596843813a6685e503fac8535e0c06ecca8561b6a1f22c578eefb691912be2e1667946101ae8c3501e6c66eb17e14f2608c9ce6fbab4a1597ed49ccb3930b1060f98c97d8dc4ce81e35279c4d30d1bf86c9b919a3ce4f0109e77929e58c4c3aeb5de1ec5e0afa38ae896df9121c72c255141f2f5c9a51be5072547cf8a3b067404e62f9615a02479cf8c202e7feb2e258314e0ebe62878a5c4ecd4e9df7dab2e1fa9a7b532c2169acedb7998d5cd8a7118848ce7ee9fb2f68e28c2b279ddc064db70ad73c6dbe10c5e1c56a709c1407f93a727cce1075103a4009ae2f7731b7d71756eee119b828ef4ed61eff164935532a94fa8fe62dc2e22cf20f168ae65f4b6785286c253f365f29453a479dc2824b8bdabd962da3b76ae9c8a720155e158fe389c8cc7fa6ad522c951b5c236bf964b5b1bfb098a39835759b95404b72b17f7dbcda936177ae059269f41ecdac81a49f5bbfd2e801392a043ef06873550a67fcbc039f0b5d30ce490baa979dbbaf9e53d45d7e2dff26b2f7e6628ded694217a39f454b288e7906b79faf4a407a7d207646f93096a157f0d1dca05a7f92e318fc1ff62ce2de7f129b187053":"aea19443d7f8ad7d4501c1ecadc6b5e3f1c23c29eca608905f9cabdd46e34a55e1f7ac8308e75c903675982bda99173a2ba57d2ccf2e01a02589f89dfd4b3c7fd229ec91c9d0c46ea5dee3c048cd4611bfeadc9bf26daa1e02cb72e222cf3dab120dd1e8c2dd9bd58bbefa5d14526abd1e8d2170a6ba8283c243ec2fd5ef07030b1ef5f69f9620e4b17a3639341005887b9ffc793533594703e5dcae67bd0ce7a3c98ca65815a4d067f27e6e66d6636cebb789732566a52ac3970e14c37310dc2fcee0e739a16291029fd2b4d534e30445474b26711a8b3e1ee3cc88b09e8b1745b6cc0f067624ecb232db750b01fe5457fdea77b251b10fe95d3eeedb083bdf109c41dba26cc9654f787bf95735ff07070b175cea8b62302e6087b91a0415474605691099f1a9e2b626c4b3bb7aeb8ead9922bc3617cb427c669b88be5f98aea7edb8b0063bec80af4c081f89778d7c7242ddae88e8d3aff1f80e575e1aab4a5d115bc27636fd14d19bc59433f697635ecd870d17e7f5b004dee4001cddc34ab6e377eeb3fb08e9476970765105d93e4558fe3d4fc6fe053aab9c6cf032f1116e70c2d65f7c8cdeb6ad63ac4291f93d467ebbb29ead265c05ac684d20a6bef09b71830f717e08bcb4f9d3773bec928f66eeb64dc451e958e357ebbfef5a342df28707ac4b8e3e8c854e8d691cb92e87c0d57558e44cd754424865c229c9e1abb28e003b6819400b"So, I was taking a look at this code and noticed we can add the dependency on the functions as every function is currently dependent on AES. So I've added it here
github.com/bjwtaylor/TF-PSA-Crypto@d6b9e396c8/tests/suites/test_suite_nist_kw.function (L24)and removed it from the data. Currently the key has to be passed anyway and the key type is encoded in the parameters, so I'm not sure we need it passed into the function also? Though I can add this if you think it would be useful. Let me know what you think?@ -221,2 +196,2 @@inbuff, 16, outbuff, &olen);if (ret != 0) {ret = psa_cipher_update(&wrap_operation,inbuff, 16, outbuff, sizeof(outbuff), &olen);Apologies this one got lost in the the noise. So, I've removed the finish and added a check for the length. I wasn't sure on the error code, I went with PSA_ERROR_BAD_STATE as it seemed most appropriate. Let me know if you want me to change it though.
@ -221,2 +196,2 @@inbuff, 16, outbuff, &olen);if (ret != 0) {ret = psa_cipher_update(&wrap_operation,inbuff, 16, outbuff, sizeof(outbuff), &olen);Apologies, also seems to have got lost in the noise. I've updated to include the suggested output_size addition. Let me know if this is sufficient or if you think we need to do something more efficient?
I've removed the annotation.
LGTM except for a risk of a memory leak in the future.
@ -139,0 +95,4 @@ret = verify_input(&key);if (ret != PSA_SUCCESS) {goto cleanup;}psa_get_key_attributesmay allocate memory. It currently doesn't, but it did in some cases in the past and it might do so in the future. So to avoid a potential memory leak, we should callpsa_reset_key_attributeson the error paths. (Especially in the bad-type case — given that we write generally the library to be robust, ifpsa_get_key_attributesfails, we would normally arrange for it to clean up the output structure.)BAD_STATEdoesn't really make sense here since there's no context that's left in a bad state. But since this is a “shouldn't happen” error, it doesn't really matter. TechnicallyCORRUPTION_DETECTEDwould be the most fitting error.@ -169,3 +159,2 @@NIST KWP wrap AES-128 CAVS 21.4 PLAINTEXT LENGTH = 4096 count 1depends_on:MBEDTLS_AES_Cmbedtls_nist_kw_wrap:MBEDTLS_CIPHER_ID_AES:MBEDTLS_KW_MODE_KWP:"6b8ba9cc9b31068ba175abfcc60c1338":"8af887c58dfbc38ee0423eefcc0e032dcc79dd116638ca65ad75dca2a2459f13934dbe61a62cb26d8bbddbabf9bf52bbe137ef1d3e30eacf0fe456ec808d6798dc29fe54fa1f784aa3c11cf39405009581d3f1d596843813a6685e503fac8535e0c06ecca8561b6a1f22c578eefb691912be2e1667946101ae8c3501e6c66eb17e14f2608c9ce6fbab4a1597ed49ccb3930b1060f98c97d8dc4ce81e35279c4d30d1bf86c9b919a3ce4f0109e77929e58c4c3aeb5de1ec5e0afa38ae896df9121c72c255141f2f5c9a51be5072547cf8a3b067404e62f9615a02479cf8c202e7feb2e258314e0ebe62878a5c4ecd4e9df7dab2e1fa9a7b532c2169acedb7998d5cd8a7118848ce7ee9fb2f68e28c2b279ddc064db70ad73c6dbe10c5e1c56a709c1407f93a727cce1075103a4009ae2f7731b7d71756eee119b828ef4ed61eff164935532a94fa8fe62dc2e22cf20f168ae65f4b6785286c253f365f29453a479dc2824b8bdabd962da3b76ae9c8a720155e158fe389c8cc7fa6ad522c951b5c236bf964b5b1bfb098a39835759b95404b72b17f7dbcda936177ae059269f41ecdac81a49f5bbfd2e801392a043ef06873550a67fcbc039f0b5d30ce490baa979dbbaf9e53d45d7e2dff26b2f7e6628ded694217a39f454b288e7906b79faf4a407a7d207646f93096a157f0d1dca05a7f92e318fc1ff62ce2de7f129b187053":"aea19443d7f8ad7d4501c1ecadc6b5e3f1c23c29eca608905f9cabdd46e34a55e1f7ac8308e75c903675982bda99173a2ba57d2ccf2e01a02589f89dfd4b3c7fd229ec91c9d0c46ea5dee3c048cd4611bfeadc9bf26daa1e02cb72e222cf3dab120dd1e8c2dd9bd58bbefa5d14526abd1e8d2170a6ba8283c243ec2fd5ef07030b1ef5f69f9620e4b17a3639341005887b9ffc793533594703e5dcae67bd0ce7a3c98ca65815a4d067f27e6e66d6636cebb789732566a52ac3970e14c37310dc2fcee0e739a16291029fd2b4d534e30445474b26711a8b3e1ee3cc88b09e8b1745b6cc0f067624ecb232db750b01fe5457fdea77b251b10fe95d3eeedb083bdf109c41dba26cc9654f787bf95735ff07070b175cea8b62302e6087b91a0415474605691099f1a9e2b626c4b3bb7aeb8ead9922bc3617cb427c669b88be5f98aea7edb8b0063bec80af4c081f89778d7c7242ddae88e8d3aff1f80e575e1aab4a5d115bc27636fd14d19bc59433f697635ecd870d17e7f5b004dee4001cddc34ab6e377eeb3fb08e9476970765105d93e4558fe3d4fc6fe053aab9c6cf032f1116e70c2d65f7c8cdeb6ad63ac4291f93d467ebbb29ead265c05ac684d20a6bef09b71830f717e08bcb4f9d3773bec928f66eeb64dc451e958e357ebbfef5a342df28707ac4b8e3e8c854e8d691cb92e87c0d57558e44cd754424865c229c9e1abb28e003b6819400b"mbedtls_nist_kw_wrap:MBEDTLS_KW_MODE_KWP:"6b8ba9cc9b31068ba175abfcc60c1338":"8af887c58dfbc38ee0423eefcc0e032dcc79dd116638ca65ad75dca2a2459f13934dbe61a62cb26d8bbddbabf9bf52bbe137ef1d3e30eacf0fe456ec808d6798dc29fe54fa1f784aa3c11cf39405009581d3f1d596843813a6685e503fac8535e0c06ecca8561b6a1f22c578eefb691912be2e1667946101ae8c3501e6c66eb17e14f2608c9ce6fbab4a1597ed49ccb3930b1060f98c97d8dc4ce81e35279c4d30d1bf86c9b919a3ce4f0109e77929e58c4c3aeb5de1ec5e0afa38ae896df9121c72c255141f2f5c9a51be5072547cf8a3b067404e62f9615a02479cf8c202e7feb2e258314e0ebe62878a5c4ecd4e9df7dab2e1fa9a7b532c2169acedb7998d5cd8a7118848ce7ee9fb2f68e28c2b279ddc064db70ad73c6dbe10c5e1c56a709c1407f93a727cce1075103a4009ae2f7731b7d71756eee119b828ef4ed61eff164935532a94fa8fe62dc2e22cf20f168ae65f4b6785286c253f365f29453a479dc2824b8bdabd962da3b76ae9c8a720155e158fe389c8cc7fa6ad522c951b5c236bf964b5b1bfb098a39835759b95404b72b17f7dbcda936177ae059269f41ecdac81a49f5bbfd2e801392a043ef06873550a67fcbc039f0b5d30ce490baa979dbbaf9e53d45d7e2dff26b2f7e6628ded694217a39f454b288e7906b79faf4a407a7d207646f93096a157f0d1dca05a7f92e318fc1ff62ce2de7f129b187053":"aea19443d7f8ad7d4501c1ecadc6b5e3f1c23c29eca608905f9cabdd46e34a55e1f7ac8308e75c903675982bda99173a2ba57d2ccf2e01a02589f89dfd4b3c7fd229ec91c9d0c46ea5dee3c048cd4611bfeadc9bf26daa1e02cb72e222cf3dab120dd1e8c2dd9bd58bbefa5d14526abd1e8d2170a6ba8283c243ec2fd5ef07030b1ef5f69f9620e4b17a3639341005887b9ffc793533594703e5dcae67bd0ce7a3c98ca65815a4d067f27e6e66d6636cebb789732566a52ac3970e14c37310dc2fcee0e739a16291029fd2b4d534e30445474b26711a8b3e1ee3cc88b09e8b1745b6cc0f067624ecb232db750b01fe5457fdea77b251b10fe95d3eeedb083bdf109c41dba26cc9654f787bf95735ff07070b175cea8b62302e6087b91a0415474605691099f1a9e2b626c4b3bb7aeb8ead9922bc3617cb427c669b88be5f98aea7edb8b0063bec80af4c081f89778d7c7242ddae88e8d3aff1f80e575e1aab4a5d115bc27636fd14d19bc59433f697635ecd870d17e7f5b004dee4001cddc34ab6e377eeb3fb08e9476970765105d93e4558fe3d4fc6fe053aab9c6cf032f1116e70c2d65f7c8cdeb6ad63ac4291f93d467ebbb29ead265c05ac684d20a6bef09b71830f717e08bcb4f9d3773bec928f66eeb64dc451e958e357ebbfef5a342df28707ac4b8e3e8c854e8d691cb92e87c0d57558e44cd754424865c229c9e1abb28e003b6819400b"The general rule is that the place that mentions a feature explicitly should have the corresponding dependency. So any test case that has
PSA_KEY_TYPE_AESin a parameter should havedepends_on:PSA_WANT_KEY_TYPE_AES, and any function whose code usesPSA_KEY_TYPE_AESshould havedepends_on:PSA_WANT_KEY_TYPE_AES.But this is not critical. It doesn't make the tests wrong, or reduce the test coverage. It just makes the code a little harder to understand. At this point I just want to get this merged.
This has two approvals, but needs a rebase or a merge to handle a conflict in
nist_kw.h.Looks good to me after rebase.
Prior to the rebase, there was a conflict in
nist_kw.hwhich was both modified and moved. The move confusesgit range-diff, you can help it with by specifying an orderfile:This show only cosmetic differences in the patches.
b19bac702ce977c9288fae63290e48620a572ad3 was approved by Valerio, so I only reviewed the rebase since that commit, and found it good.
(Thank you Gilles for the orderfile trick, it would have taken me a moment to remember how it was called an how to use it).