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

adjust_legacy_crypto: enable CIPHER_C when PSA CMAC is builtin #9178

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

valeriosetti
Copy link
Contributor

@valeriosetti valeriosetti commented May 24, 2024

psa_crypto_mac.c uses mbedtls_cipher_xxx() functions to perform CMAC operations. Therefore we need to enable CIPHER_C when PSA CMAC is builtin.
Resolves #9209

PR checklist

Sorry, something went wrong.

psa_crypto_mac.c uses mbedtls_cipher_xxx() functions to perform
CMAC operations. Therefore we need to enable CIPHER_C when
PSA CMAC is builtin.

Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels May 24, 2024
@valeriosetti valeriosetti self-assigned this May 24, 2024
@valeriosetti valeriosetti removed the needs-ci Needs to pass CI tests label May 24, 2024
@tom-daubney-arm
Copy link
Contributor

Just curious: Does the presence of this bug indicate a gap in our test coverage? Why would we not have seen CI failures from this before?

@valeriosetti
Copy link
Contributor Author

Just curious: Does the presence of this bug indicate a gap in our test coverage? Why would we not have seen CI failures from this before?

Yes, I think so. I can investigate all.sh and see if I can add some new component that re-creates the issue I was facing in Zephyr.

@valeriosetti
Copy link
Contributor Author

Ok, let's say that you want to build the crypto library (no tests or programs) using only the PSA_WANT symbols to declare what you need. For this scope you define an mbedtls_config.h equivalent file (name it mbedtls.h) which only contains:

#define MBEDTLS_PSA_CRYPTO_C
#define MBEDTLS_PSA_CRYPTO_CONFIG
#define MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG

and then there's the crypto_config.h equivalent file (name it psa.h) which contains what you need:

#define PSA_WANT_KEY_TYPE_AES    1
#define PSA_WANT_ALG_CMAC    1

Build just the crypto library with:

make -j CFLAGS='-DMBEDTLS_CONFIG_FILE=\"mbedtls.h\" -DMBEDTLS_PSA_CRYPTO_CONFIG_FILE=\"psa.h\"' lib

The last command will give you failures from check_config.h, one of which is:

../include/mbedtls/check_config.h:99:2: error: #error "MBEDTLS_CMAC_C defined, but not all prerequisites"
   99 | #error "MBEDTLS_CMAC_C defined, but not all prerequisites"

Whereas, if you apply the fix, the build succeeds.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-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 apart from the missing changelog entry

include/mbedtls/config_adjust_legacy_crypto.h Show resolved Hide resolved
@@ -48,7 +48,8 @@
defined(MBEDTLS_PSA_BUILTIN_ALG_ECB_NO_PADDING) || \
defined(MBEDTLS_PSA_BUILTIN_ALG_CBC_NO_PADDING) || \
defined(MBEDTLS_PSA_BUILTIN_ALG_CBC_PKCS7) || \
defined(MBEDTLS_PSA_BUILTIN_ALG_CCM_STAR_NO_TAG))
defined(MBEDTLS_PSA_BUILTIN_ALG_CCM_STAR_NO_TAG) || \
defined(MBEDTLS_PSA_BUILTIN_ALG_CMAC))
#define MBEDTLS_CIPHER_C
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: Does the presence of this bug indicate a gap in our test coverage? Why would we not have seen CI failures from this before?

Yes, I think so. I can investigate all.sh and see if I can add some new component that re-creates the issue I was facing in Zephyr.

The bug is that, with MBEDTLS_PSA_CRYPTO_CONFIG enabled, if you request CMAC but no unauthenticated cipher, the build fails. The failure is easy to work around and not dangerous.

I personally don't think this requires a non-regression test. It's low-risk and impacts few configurations. It's likely to end up being tested in 4.0 once we adapt depends.py, and it's unlikely to re-break in 3.6 LTS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explaination @gilles-peskine-arm. In this case I would agree that we don't need a non-regression test. Also thanks @valeriosetti for digging into it.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

The changelog entry isn't quite right.

ChangeLog.d/fix-psa-cmac.txt Outdated Show resolved Hide resolved
@gilles-peskine-arm gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels May 31, 2024
Signed-off-by: Valerio Setti <[email protected]>
@valeriosetti valeriosetti added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels May 31, 2024
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-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

@gilles-peskine-arm gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Jun 3, 2024
Copy link
Contributor

@tom-daubney-arm tom-daubney-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 - thanks!

@tom-daubney-arm tom-daubney-arm 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 Jun 3, 2024
@tom-cosgrove-arm tom-cosgrove-arm added this pull request to the merge queue Jun 3, 2024
Merged via the queue into Mbed-TLS:development with commit 98ffc8e Jun 3, 2024
6 checks passed
@valeriosetti valeriosetti deleted the fix-psa-cmac branch December 6, 2024 06:00
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 needs-backports Backports are missing or are pending review and approval. priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Projects
Status: 3.6.1 patch release
Development

Successfully merging this pull request may close these issues.

CIPHER_C not enabled when PSA CMAC is required as builtin, but no other unauthenticated cipher is
4 participants