Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upPSA: Add support macros for IV/nonce sizes #3787
Conversation
|
I reviewed the PR and left comments with a suggestion for improvement and a request for change. |
| * and AEAD algorithm that it recognizes, but does not support. | ||
| */ | ||
| #define PSA_AEAD_NONCE_LENGTH(key_type, alg) \ | ||
| (((key_type) == PSA_KEY_TYPE_AES || \ |
yanesca
Oct 15, 2020
Contributor
Maybe we could make it more simple and future proof by using PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER() here.
Maybe we could make it more simple and future proof by using PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER() here.
bensze01
Oct 16, 2020
Author
Contributor
Done.
Done.
gilles-peskine-arm
Oct 26, 2020
Contributor
On the contrary, I don't think PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER should appear here. Different algorithms have different nonces. Currently the two algorithms that it matches are CCM and GCM, but we might add other algorithms with a different nonce size.
And isn't the maximum nonce size for CCM 13?
On the contrary, I don't think PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER should appear here. Different algorithms have different nonces. Currently the two algorithms that it matches are CCM and GCM, but we might add other algorithms with a different nonce size.
And isn't the maximum nonce size for CCM 13?
yanesca
Oct 26, 2020
Contributor
On the contrary, I don't think PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER should appear here.
I don't have strong opinion on this, I am happy either way.
And isn't the maximum nonce size for CCM 13?
Yes, it is, but in the other discussion @athoelke said that these macros are about the default PSA sizes and not the maximum:
#3787 (comment)
and we assumed that to be true.
On the contrary, I don't think PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER should appear here.
I don't have strong opinion on this, I am happy either way.
And isn't the maximum nonce size for CCM 13?
Yes, it is, but in the other discussion @athoelke said that these macros are about the default PSA sizes and not the maximum:
#3787 (comment)
and we assumed that to be true.
bensze01
Oct 29, 2020
Author
Contributor
@gilles-peskine-arm I've changed the check back to the original version, checking for CCM and GCM explicitly. I've left the size for CCM at 12, as per @yanesca 's comment.
@gilles-peskine-arm I've changed the check back to the original version, checking for CCM and GCM explicitly. I've left the size for CCM at 12, as per @yanesca 's comment.
| * | ||
| * See also #PSA_AEAD_NONCE_LENGTH(). | ||
| */ | ||
| #define PSA_AEAD_NONCE_MAX_SIZE 12 |
yanesca
Oct 15, 2020
Contributor
The maximum size for CCM is 13:
Lines 182 to 184
in
8f24a8b
For GCM the upper limit is 2^61 bytes:
Line 288
in
8f24a8b
But of course this is far off from what anybody would need in practice and using this value would severely limit the utility of this macro. To do justice to GCM and still preserve the utility of this macro, I suggest we limit the IV for GCM to 14 bytes. Of course we need to explain this in the comment and hope that it does not break anybody's use case. @gilles-peskine-arm what do you think?
The maximum size for CCM is 13:
Lines 182 to 184 in 8f24a8b
For GCM the upper limit is 2^61 bytes:
Line 288 in 8f24a8b
But of course this is far off from what anybody would need in practice and using this value would severely limit the utility of this macro. To do justice to GCM and still preserve the utility of this macro, I suggest we limit the IV for GCM to 14 bytes. Of course we need to explain this in the comment and hope that it does not break anybody's use case. @gilles-peskine-arm what do you think?
bensze01
Oct 15, 2020
•
Author
Contributor
@yanesca @gilles-peskine-arm While I thought the wording in the spec as to what PSA_AEAD_NONCE_MAX_SIZE should mean was a bit vague, I took it to mean MAX(PSA_AEAD_NONCE_LENGTH(key_type, alg), for all key_type, for all alg), ie. the maximum of the default nonce lengths for the supported key and algorithm pairs.
I arrived to this conclusion due to the spec repeatedly suggesting one uses either PSA_AEAD_NONCE_LENGTH or PSA_AEAD_NONCE_MAX_SIZE to allocate a buffer large enough to hold the nonce generated by the PSA Crypto implementation, which I took to be the macro's main use case
@yanesca @gilles-peskine-arm While I thought the wording in the spec as to what PSA_AEAD_NONCE_MAX_SIZE should mean was a bit vague, I took it to mean MAX(PSA_AEAD_NONCE_LENGTH(key_type, alg), for all key_type, for all alg), ie. the maximum of the default nonce lengths for the supported key and algorithm pairs.
I arrived to this conclusion due to the spec repeatedly suggesting one uses either PSA_AEAD_NONCE_LENGTH or PSA_AEAD_NONCE_MAX_SIZE to allocate a buffer large enough to hold the nonce generated by the PSA Crypto implementation, which I took to be the macro's main use case
gilles-peskine-arm
Oct 15, 2020
Contributor
Indeed, the primary intended use for these macros is to determine an output buffer size that's guaranteed to be large enough. This does warrant a clarification in the specification, I'll file it for a 1.0.x update.
Indeed, the primary intended use for these macros is to determine an output buffer size that's guaranteed to be large enough. This does warrant a clarification in the specification, I'll file it for a 1.0.x update.
yanesca
Oct 15, 2020
Contributor
@gilles-peskine-arm am I interpreting this correctly, do you mean that 12 is the correct value for this macro here?
@gilles-peskine-arm am I interpreting this correctly, do you mean that 12 is the correct value for this macro here?
gilles-peskine-arm
Oct 15, 2020
Contributor
I'm sure that 16 is correct. I'd have to check exactly how we generate nonces for supported algorithms to see if 12 is correct.
I'm sure that 16 is correct. I'd have to check exactly how we generate nonces for supported algorithms to see if 12 is correct.
yanesca
Oct 16, 2020
Contributor
@athoelke Thank you for the clarification! I think I still have a question: what would be the benefit/justification of psa_aead_generate_nonce() generating longer nonces than the maximum size of any AEAD nonce supported by the implementation?
@athoelke Thank you for the clarification! I think I still have a question: what would be the benefit/justification of psa_aead_generate_nonce() generating longer nonces than the maximum size of any AEAD nonce supported by the implementation?
athoelke
Oct 16, 2020
Contributor
what would be the benefit/justification of psa_aead_generate_nonce() generating longer nonces than the maximum size of any AEAD nonce supported by the implementation?
That wouldn't make sense. psa_aead_generate_nonce() is defined to output PSA_AEAD_NONE_LENGTH(key_type, alg).
@bensze01's came to the correct conclusion that PSA_AEAD_NONCE_MAX_SIZE must be a value equal to (or greater than)
max { PSA_AEAD_NONCE_LENGTH(key_type, alg), for all supported AEAD (key_type, alg) }
PSA_AEAD_NONCE_LENGTH(key_type, alg) is not the maximum size of nonce supported as input to psa_aead_set_nonce(), psa_aead_encrypt() or psa_aead_decrypt(), just the default size that is generated by psa_aead_generate_nonce().
what would be the benefit/justification of
psa_aead_generate_nonce()generating longer nonces than the maximum size of any AEAD nonce supported by the implementation?
That wouldn't make sense. psa_aead_generate_nonce() is defined to output PSA_AEAD_NONE_LENGTH(key_type, alg).
@bensze01's came to the correct conclusion that PSA_AEAD_NONCE_MAX_SIZE must be a value equal to (or greater than)
max { PSA_AEAD_NONCE_LENGTH(key_type, alg), for all supported AEAD (key_type, alg) }
PSA_AEAD_NONCE_LENGTH(key_type, alg) is not the maximum size of nonce supported as input to psa_aead_set_nonce(), psa_aead_encrypt() or psa_aead_decrypt(), just the default size that is generated by psa_aead_generate_nonce().
bensze01
Oct 16, 2020
Author
Contributor
👀 ! - it says this precisely once, in psa_aead_generate_nonce().
Yeah, sorry. I remembered seeing this sentence multiple times when working on this patch, but those were other xxx_MAX_SIZE macros.
👀 ! - it says this precisely once, in psa_aead_generate_nonce().
Yeah, sorry. I remembered seeing this sentence multiple times when working on this patch, but those were other xxx_MAX_SIZE macros.
|
LGTM. |
| * and AEAD algorithm that it recognizes, but does not support. | ||
| */ | ||
| #define PSA_AEAD_NONCE_LENGTH(key_type, alg) \ | ||
| (((key_type) == PSA_KEY_TYPE_AES || \ |
gilles-peskine-arm
Oct 26, 2020
Contributor
On the contrary, I don't think PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER should appear here. Different algorithms have different nonces. Currently the two algorithms that it matches are CCM and GCM, but we might add other algorithms with a different nonce size.
And isn't the maximum nonce size for CCM 13?
On the contrary, I don't think PSA_ALG_IS_AEAD_ON_BLOCK_CIPHER should appear here. Different algorithms have different nonces. Currently the two algorithms that it matches are CCM and GCM, but we might add other algorithms with a different nonce size.
And isn't the maximum nonce size for CCM 13?
This commit defines the following new macros added to the PSA Crypto API between versions 1.0 beta 3 and 1.0.0 final: PSA_AEAD_NONCE_LENGTH PSA_AEAD_NONCE_MAX_SIZE PSA_CIPHER_IV_LENGTH PSA_CIPHER_IV_MAX_SIZE Signed-off-by: Bence Szépkúti <[email protected]>
|
Looks good to me. |
gilles-peskine-arm
merged commit 52f32c9
into
ARMmbed:development
Description
This commit defines the following new macros added to the PSA Crypto API between versions 1.0 beta 3 and 1.0.0 final:
PSA_AEAD_NONCE_LENGTH
PSA_AEAD_NONCE_MAX_SIZE
PSA_CIPHER_IV_LENGTH
PSA_CIPHER_IV_MAX_SIZE
The in-code documentation was adapted from the PSA Cryptography API spec (see above the links to the individual macros)
Closes #3270
Status
READY
Requires Backporting
NO
Migrations
NO