Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some MAX_SIZE macros are too small when PSA ECC is accelerated #6622

Closed
gilles-peskine-arm opened this issue Nov 18, 2022 · 3 comments · Fixed by #7103
Closed

Some MAX_SIZE macros are too small when PSA ECC is accelerated #6622

gilles-peskine-arm opened this issue Nov 18, 2022 · 3 comments · Fixed by #7103
Assignees
@valeriosetti
Labels
bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

At some time before Mbed TLS 2.28 and 3.0, we added support for signature through PSA accelerators. Accelerators can support elliptic curves that the core doesn't. So the definition of PSA_VENDOR_ECC_MAX_CURVE_BITS, which determines PSA_EXPORT_KEY_PAIR_MAX_SIZE, PSA_EXPORT_PUBLIC_KEY_MAX_SIZE and PSA_SIGNATURE_MAX_SIZE, is wrong. If the largest accelerated curve is larger than the largest built-in curve (for example if ECC is accelerated only), the MAX_SIZE macros may end up being wrong (mostly, if there is no RSA support).

This will also concern PSA_RAW_KEY_AGREEMENT_OUTPUT_MAX_SIZE when we add support for key agreement acceleration.
@gilles-peskine-arm gilles-peskine-arm added bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d) labels Nov 18, 2022
@gilles-peskine-arm gilles-peskine-arm self-assigned this Nov 18, 2022
@gilles-peskine-arm gilles-peskine-arm mentioned this issue Dec 23, 2022
Closed
@gilles-peskine-arm gilles-peskine-arm mentioned this issue Feb 8, 2023
Closed
@valeriosetti valeriosetti self-assigned this Feb 10, 2023
@valeriosetti
Copy link
Contributor

valeriosetti commented Feb 10, 2023 •
edited
Loading

@gilles-peskine-arm @mpg
Sorry for the basic question: would it be worth to add a test in all.sh that exercises the (potentially) critical case?
I mean something like:

  • build with accelerators for EC
  • disables RSA support

Perhaps test_psa_crypto_config_accel_ecdh is close to what I mean, but it seems to me that it does not disables RSA.

@gilles-peskine-arm
Copy link
Contributor Author

Yes, we should test configurations like this.

@valeriosetti
Copy link
Contributor

As agreed with @mpg, I am putting the work on this issue on hold until we found a solution to the problem mentioned here

@valeriosetti valeriosetti mentioned this issue Feb 15, 2023
Merged
3 tasks
@gilles-peskine-arm gilles-peskine-arm mentioned this issue Feb 15, 2023
Closed
3 tasks
@mpg mpg closed this as completed in #7103 Apr 3, 2023
mpg added a commit to valeriosetti/mbedtls that referenced this issue Sep 20, 2023
@mpg
Remove components that are no longer needed
473d82d 
Those components were introduced in Mbed-TLS#7103, resolving Mbed-TLS#6622: Some PSA
ECC size macros are too small when the largest accelerated curve is
larger than the largest built-in curve.

At that point, it was not possible yet to omit all built-in curves,
so we made these components that had only one (small) curve built-in and
all the others accelerated.

Now that it's possible to disable all ECC built-ins, and we have tests
doing that, we don't need that kind of fiddling any more.

Note: these component disabled RSA in order to make sure max key size
macros were not taken from RSA. We have test components with all of ECC
accelerated and RSA disabled
(component_test_psa_crypto_config_accel_ecc_no_bignum and
component_test_psa_crypto_config_accel_ecc_ffdh_no_bignum), making the
"all curves except one" components really redundant.

Note: removing them was one of the items in Mbed-TLS#7757.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to valeriosetti/mbedtls that referenced this issue Sep 21, 2023
@mpg
Remove components that are no longer needed
945f0c5 
Those components were introduced in Mbed-TLS#7103, resolving Mbed-TLS#6622: Some PSA
ECC size macros are too small when the largest accelerated curve is
larger than the largest built-in curve.

At that point, it was not possible yet to omit all built-in curves,
so we made these components that had only one (small) curve built-in and
all the others accelerated.

Now that it's possible to disable all ECC built-ins, and we have tests
doing that, we don't need that kind of fiddling any more.

Note: these component disabled RSA in order to make sure max key size
macros were not taken from RSA. We have test components with all of ECC
accelerated and RSA disabled
(component_test_psa_crypto_config_accel_ecc_no_bignum and
component_test_psa_crypto_config_accel_ecc_ffdh_no_bignum), making the
"all curves except one" components really redundant.

Note: removing them was one of the items in Mbed-TLS#7757.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to valeriosetti/mbedtls that referenced this issue Sep 24, 2023
@mpg
Remove components that are no longer needed
fc45574 
Those components were introduced in Mbed-TLS#7103, resolving Mbed-TLS#6622: Some PSA
ECC size macros are too small when the largest accelerated curve is
larger than the largest built-in curve.

At that point, it was not possible yet to omit all built-in curves,
so we made these components that had only one (small) curve built-in and
all the others accelerated.

Now that it's possible to disable all ECC built-ins, and we have tests
doing that, we don't need that kind of fiddling any more.

Note: these component disabled RSA in order to make sure max key size
macros were not taken from RSA. We have test components with all of ECC
accelerated and RSA disabled
(component_test_psa_crypto_config_accel_ecc_no_bignum and
component_test_psa_crypto_config_accel_ecc_ffdh_no_bignum), making the
"all curves except one" components really redundant.

Note: removing them was one of the items in Mbed-TLS#7757.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
mpg added a commit to valeriosetti/mbedtls that referenced this issue Sep 25, 2023
@mpg
Remove components that are no longer needed
a6aecd0 
Those components were introduced in Mbed-TLS#7103, resolving Mbed-TLS#6622: Some PSA
ECC size macros are too small when the largest accelerated curve is
larger than the largest built-in curve.

At that point, it was not possible yet to omit all built-in curves,
so we made these components that had only one (small) curve built-in and
all the others accelerated.

Now that it's possible to disable all ECC built-ins, and we have tests
doing that, we don't need that kind of fiddling any more.

Note: these component disabled RSA in order to make sure max key size
macros were not taken from RSA. We have test components with all of ECC
accelerated and RSA disabled
(component_test_psa_crypto_config_accel_ecc_no_bignum and
component_test_psa_crypto_config_accel_ecc_ffdh_no_bignum), making the
"all curves except one" components really redundant.

Note: removing them was one of the items in Mbed-TLS#7757.

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@valeriosetti valeriosetti

Labels
bug component-crypto Crypto primitives and low-level interfaces size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Some MAX_SIZE macros are too small when PSA ECC is accelerated valeriosetti/mbedtls
2 participants
@gilles-peskine-arm @valeriosetti