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.

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

PSA: don't run tests for unsupported curves #4250

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

Curve448 and secp224k1 currently don't work through the PSA crypto API. Don't enable them in the configuration.

Urgent because CI broke due to the combination of #4214 (adding systematically generated test cases for all curves including these, but the test cases didn't run because the dependency symbols weren't declared) and #4158 (adding the dependency symbols for all curves including these).

Filed as Mbed-TLS#4249. In the
meantime, disable the feature.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Filed as Mbed-TLS#3541. In the
meantime, disable the feature.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review component-psa PSA keystore/dispatch layer (storage, drivers, …) labels Mar 23, 2021
@bensze01 bensze01 self-requested a review March 23, 2021 11:12
If an elliptic curve was enabled in the Mbed TLS classic API (#define
MBEDTLS_ECP_DP_xxx), but not enabled in the PSA configuration (#define
PSA_WANT_ECC_xxx), it would still work if you tried to use it through
PSA.

This is generally benign, but could be a security issue if you want to
disable a curve in PSA for some security reason (such as a known bug
in its implementation, which may not matter in the classic API if Mbed
TLS is running in a secure enclave and is only reachable from
untrusted callers through the PSA API). More urgently, this broke
test_suite_psa_crypto_not_supported.generated.

So if a curve is not enabled in the PSA configuration, ensure that
it's treated as unsupported through the PSA software implementation.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Filed as Mbed-TLS#3541. In the
meantime, disable the ssl-opt.sh test case that uses it.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@ronald-cron-arm ronald-cron-arm self-requested a review March 23, 2021 14:49
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Mar 23, 2021
@ronald-cron-arm ronald-cron-arm removed the needs-ci Needs to pass CI tests label Mar 23, 2021
Copy link
Contributor

@bensze01 bensze01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too.

@bensze01 bensze01 added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 23, 2021
@ronald-cron-arm ronald-cron-arm merged commit 49eee98 into Mbed-TLS:development Mar 23, 2021
daverodgman pushed a commit that referenced this pull request Apr 23, 2021
PSA: don't run tests for unsupported curves
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces component-psa PSA keystore/dispatch layer (storage, drivers, …)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants