Skip to content

fix: Allow encrypt key usage for a CMAC operation #155

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Harshal5
Copy link

@Harshal5 Harshal5 commented Apr 8, 2025

Description

The key that would be used for MAC operations using CMAC should also have the PSA_KEY_USAGE_ENCRYPT flag set. CMAC operation internally performs AES/DES encryption thus the key that is used should have the PSA_KEY_USAGE_ENCRYPT flag set.

Currently this is not an issue because the psa_mac_ APIs do not invoke psa_cipher_ API calls thus PSA key usage is not checked during the cipher operation that takes place internally. IMO ideally the psa_mac_ APIs should internally call the psa_cipher_ layer which would check the usage flags of the key, and thus we would need to set the PSA_KEY_USAGE_ENCRYPT flag.

Related

Mbed-TLS/TF-PSA-Crypto#211

This PR is needed for: Mbed-TLS/TF-PSA-Crypto#249

PR checklist

Please add the numbers (or links) of the associated pull requests for consuming branches. You can omit branches where this pull request is not needed.

  • TF-PSA-Crypto PR provided:
  • 3.6 PR not required.

@Harshal5 Harshal5 force-pushed the fix/allow_encrypt_key_usage_for_a_cmac_operation branch from 64c67c5 to e7b30ee Compare April 8, 2025 02:52
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.

No, a key used for CMAC does not need to have the ENCRYPT or DECRYPT usage.

The implementation of CMAC may use an internal implementation of CBC or ECB under the hood, but this is an implementation detail. Such internal usage would not care about policies on the key.

@gilles-peskine-arm
Copy link
Contributor

IMO ideally the psa_mac_ APIs should internally call the psa_cipher_ layer which would check the usage flags of the key, and thus we would need to set the PSA_KEY_USAGE_ENCRYPT flag.

No, that's not how it would work. A CMAC driver that is built on top of the block cipher in EBC mode would not call psa_cipher_encrypt on a key identifier. The driver doesn't even officially have access to the key identifier (it's currently accessible in the attributes in the Mbed TLS implementation, but that's not guaranteed). It has access to the key material for a transparent key, or to a driver-dependent representation for an opaque key. No policies apply to this key representation.

@Harshal5
Copy link
Author

Harshal5 commented Apr 8, 2025

I envision that the CMAC calculation using psa_mac_ APIs should invoke psa_cipher_ API calls, so that the CMAC operations could use the driver dispatch layer of the cipher module.

Thus, the key that is supplied to the MAC module when would be passed into the cipher module would need the ENCRYPT usage flag set otherwise the psa_cipher_setup() would raise an error that the key does not have valid usage permissions. What do you think would be the correct way to tackle this issue?

@gilles-peskine-arm
Copy link
Contributor

psa_xxx() API functions (psa_crypto.c) retrieve the key representation from the key store (which may involve loading from persistent storage). They pass this key representation to the dispatch layer (psa_driver_wrapper_xxx() in psa_crypto_driver_wrapper*). The dispatch layer calls a driver, by default the built-in driver (mbedtls_psa_xxx() in psa_crypto_{mac,cipher,...}.c).

The driver can call back to the dispatch layer. We already do this in some cases, for example PBKDF2 calls the MAC dispatch layer. We're also doing this systematically for hashes, but since hashes don't involve a key, the dispatch layer is the API layer.

An architecture where the driver calls back to the API layer would not work for several reasons:

  • By the time the key representation gets to the driver, it may not be tied to a key identifier any longer. I think it currently always is, but it would add an architectural constraint, especially with opaque keys.
  • As seen here, the key's policy might not be right for the lower level operation.
  • On multithreaded systems, the API layer may have a lock on the key.
  • On client-server systems, the API layer might not even exist on the server side.

@Harshal5
Copy link
Author

Harshal5 commented Apr 8, 2025

Thanks for the detailed explanation, as I understand the major change that you are suggesting is that the psa_mac_ APIs should internally call the driver dispatch layer of the cipher module instead of the psa_cipher_ APIs.

The proposed workflow would be psa_mac (PSA API) -> psa_mac_driver_wrapper_ (driver dispatch) -> mbedtls_psa_mac_ (builtin driver) -> psa_cipher_driver_wrapper_ (driver dispatch) -> mbedtls_psa_cipher_ (builtin driver).

Here the issue would be the jump from mbedtls_psa_mac_ (builtin driver) -> psa_cipher_driver_wrapper_ (driver dispatch), which does not seem possible as the driver build would need to include psa_crypto_driver_wrappers.h, a header that is generated during build.

The driver can call back to the dispatch layer. We already do this in some cases, for example PBKDF2 calls the MAC dispatch layer.

This seems to be possible only when the driver is present in psa_crypto.c / in the core PSA crypto, that is, I think moving the driver psa_crypto_mac.c into psa_crypto.c?

@gilles-peskine-arm
Copy link
Contributor

which does not seem possible as the driver build would need to include psa_crypto_driver_wrappers.h, a header that is generated during build.

The declarations of the dispatch functions don't directly depend on the available drivers. Some take a pointer to a context structure whose content depends on the available drivers, but that can be an incomplete type when the compiler reaches the function declaration. We probably need to reorganize header files there. It's one of the things we need to do to generalize intermediate dispatch. I haven't fully thought this through yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants