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

Code Size: AES Encrypt Only: Determine configurations #7367

Closed
tom-cosgrove-arm opened this issue Mar 30, 2023 · 8 comments · Fixed by #8124
Closed

Code Size: AES Encrypt Only: Determine configurations #7367

tom-cosgrove-arm opened this issue Mar 30, 2023 · 8 comments · Fixed by #8124
Assignees
Labels
size-optimisation size-s Estimated task size: small (~2d)

Comments

@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Mar 30, 2023

(maybe below the line)

Gather list of AES-related configuration macros.

Look for "can we exclude AES decrypt if only these ones set"

DoD: We know which macros to examine to know if we can disable AES decryption code

The work in #2891 can be used as a reference

Related:

@tom-cosgrove-arm tom-cosgrove-arm added size-s Estimated task size: small (~2d) size-optimisation labels Mar 30, 2023
@gilles-peskine-arm
Copy link
Contributor

With the mbedtls legacy API, that's a tricky one because single-block processing (what we call ECB mode) is always available. You can't tell from the existing configuration macros whether the application wants single-block processing in both directions, only for encryption, or not at all. So I don't see a way to get around adding a configuration option MBEDTLS_AES_ONLY_ENCRYPT or MBEDTLS_CIPHER_ONLY_ENCRYPT as in #2891 (which might be incompatible with MBEDTLS_CIPHER_MODE_CBC and MBEDTLS_CIPHER_MODE_XTS for simplicity).

With the PSA API, the PSA_WANT_xxx mechanism doesn't distinguish between encryption and decryption. But ECB mode has to be requested explicitly (and there is no API for single-block processing). So “do I need decryption?” can be approximated as “is a mode with decryption enabled?”.

The modes that require both directions are ECB, CBC and XTS. The modes that require only encryption are CFB, OFB and CTR.

@tom-cosgrove-arm
Copy link
Contributor Author

The modes that require both directions are ECB, CBC and XTS. The modes that require only encryption are CFB, OFB and CTR.

Also encryption-only should be GCM and CCM, as they're based on CTR mode

@yanrayw
Copy link

yanrayw commented Jul 6, 2023

My plan is to add a configuration MBEDTLS_AES_ENCRYPT_ONLY which is automatically enabled via PSA when a cipher mode which doesn't need AES-decryption function is expressed.

So I think we don't need MBEDTLS_AES_ENCRYPT_ONLY defined in mbedtls_config.h. This option should only be enabled through PSA.

@yanrayw
Copy link

yanrayw commented Jul 7, 2023

Reconsider the configuration option MBEDTLS_AES_ENCRYPT_ONLY. Its functionality should be considered as MBEDTLS_AES_ENCRYPT_ONLY = MBEDTLS_AES_SETKEY_DEC_ALT + MBEDTLS_AES_DECRYPT_ALT without implementing mbedtls_aes_setkey_dec and mbedtls_aes_setkey_dec.

@yuhaoth
Copy link
Contributor

yuhaoth commented Aug 14, 2023

I think that should be an internal option. If MBEDTLS_CIPHER_MODE_CBC and MBEDTLS_CIPHER_MODE_XTS are disabled and GCM or CCM is enabled, AES decrypt should be disabled.

@yanrayw
Copy link

yanrayw commented Aug 28, 2023

I created two separate PRs for this issue.

  1. Implement AES_ENCRYPT_ONLY specifically for AES: Code Size: AES: auto-enable AES_ENCRYPT_ONLY via PSA #7603
  2. Implement CIPHER_ENCRYPT_ONLY for cipher of AES / ARIA / CAMELLIA / DES: Support the negative option MBEDTLS_BLOCK_CIPHER_NO_DECRYPT #8124 (It's still a WIP draft).

Personally, I think we should go with CIPHER_ENCRYPT_ONLY for following reasons.

  1. It's reasonable to remove mbedtls_aes_setkey_dec completely, therefore we should modify *setkey_dec_func in cipher_wrap.h.
  2. CBC is guarded by MBEDTLS_CIPHER_MODE_CBC for AES / ARIA / CAMELLIA / DES. Once a user says we don't need CBC through PSA_WANT_ which means all CBC code will be removed in those ciphers. It seems it's not necessary to consider AES as a separate cipher in our scenario.
  3. If we remove xxx_setkey_dec for AES / ARIA / CAMELLIA / DES. We can remove *setkey_dec_func directly to save extra code size in cipher_wrap.c.

Any comments? @tom-cosgrove-arm @gilles-peskine-arm

@gilles-peskine-arm
Copy link
Contributor

I think we should go with CIPHER_ENCRYPT_ONLY for following reasons.

I agree, this seems easier than managing differences between AES and ARIA/Camellia.

@tom-cosgrove-arm
Copy link
Contributor Author

I think we should go with CIPHER_ENCRYPT_ONLY for following reasons.

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-optimisation size-s Estimated task size: small (~2d)
Projects
Status: Code Size Optimisation
Development

Successfully merging a pull request may close this issue.

4 participants