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

Support the negative option MBEDTLS_BLOCK_CIPHER_NO_DECRYPT #8124

Merged
merged 65 commits into from
Nov 23, 2023

Conversation

yanrayw
Copy link

@yanrayw yanrayw commented Aug 28, 2023

Description

Fix #7368 fix #7369 fix #7370 fix #7367
Fix #8460

This PR adds a new configuration option MBEDTLS_BLOCK_CIPHER_NO_DECRYPT to remove decryption operation for AES/ARIA/CAMELLIA in some cipher modes. This option is always incompatible with MBEDTLS_DES_C, BEDTLS_CIPHER_MODE_CBC, MBEDTLS_CIPHER_MODE_XTS and MBEDTLS_NIST_KW_C.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

  • changelog added
  • backport not required because this a new feature.
  • tests provided

Yanray Wang added 2 commits August 24, 2023 11:11
Variable RSb is only used for either computing reverse tables
in aes_gen_tables or AES-decryption function. This commit provides
more guards for when RSb is defined and used.

Signed-off-by: Yanray Wang <[email protected]>
@yanrayw yanrayw added needs-work needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon size-optimisation labels Aug 28, 2023
@yanrayw yanrayw self-assigned this Aug 28, 2023
@yanrayw yanrayw marked this pull request as draft August 28, 2023 09:45
@yanrayw yanrayw force-pushed the support_cipher_encrypt_only branch from 4603fba to ae2227a Compare August 28, 2023 09:47
@yanrayw yanrayw changed the title support and implicitly enable CIPHER_ENCRYPT_ONLY WIP: Support and implicitly enable CIPHER_ENCRYPT_ONLY Aug 28, 2023
@yanrayw yanrayw force-pushed the support_cipher_encrypt_only branch 5 times, most recently from 0ce21b4 to c218de2 Compare September 1, 2023 03:58
Yanray Wang added 11 commits September 1, 2023 16:35
Some cipher modes use cipher-encrypt to encrypt and decrypt.
(E.g: ECB, CBC). This commit adds support to automatically
enable CIPHER_ENCRYPT_ONLY by PSA when requested cipher modes don't
need cipher_decrypt.

Signed-off-by: Yanray Wang <[email protected]>
This is a pre-step to remove *setkey_dec_func in cipher_wrap ctx
when CIPHER_ENCRYPT_ONLY is enabled.

Signed-off-by: Yanray Wang <[email protected]>
There is no need to set decrypt key under CIPHER_ENCRYPT_ONLY,
so we can remove *setkey_dec_func from ctx to save extra code size.

Signed-off-by: Yanray Wang <[email protected]>
dh_client requests AES-ECB to do decryption. So it needs to be
removed under CIPHER_ENCRYPT_ONLY.

Signed-off-by: Yanray Wang <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests labels Nov 22, 2023
@gilles-peskine-arm
Copy link
Contributor

The recently added component check_test_dependencies is failing. MBEDTLS_BLOCK_CIPHER_NO_DECRYPT needs to be added to the whitelist in all.sh.

@yanrayw yanrayw dismissed stale reviews from valeriosetti and lpy4105 via 0df5512 November 23, 2023 03:17
Yanray Wang added 2 commits November 23, 2023 14:34
- add !MBEDTLS_BLOCK_CIPHER_NO_DECRYPT in whitelist

Signed-off-by: Yanray Wang <[email protected]>
@yanrayw yanrayw force-pushed the support_cipher_encrypt_only branch from 0df5512 to 42be1ba Compare November 23, 2023 06:36
valeriosetti
valeriosetti previously approved these changes Nov 23, 2023
Copy link
Contributor

@valeriosetti valeriosetti left a comment

Choose a reason for hiding this comment

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

I checked the last 2 commits and they are fine. I'm approving it, hoping that also the CI will agree :)

@mpg mpg requested a review from lpy4105 November 23, 2023 08:00
@yanrayw yanrayw added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Nov 23, 2023
@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Nov 23, 2023
# By default, sort (v8.25) on ubuntu-16 and sort (v8.30) on ubuntu-20
# sort text in different order. We use -d option to sort text in
# an order considering only blanks and alphanumeric characters.
sort -ud > $found
Copy link
Contributor

Choose a reason for hiding this comment

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

sort -ud would consider FOO and !FOO the same and omit the one with later occurrence.

I'm not sure if it is major, since we are seemly to have very low possibility to have both FOO and !FOO dependencies in psa tests.

In fact, the different results are related to reference:

Collation order and (multi-byte) character type are influenced by your locale.

So, I would suggest to use sort -u | sort -d > $found or LC_ALL=C sort -u > $found.

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! I think I like LC_ALL_C sort -u better if it works, otherwise LC_ALL=C sort -u | sort -d.

Copy link
Contributor

Choose a reason for hiding this comment

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

-d doesn't protect against locale differences. Please use LC_ALL=C sort … or add export LC_COLLATE=C to pre_initialize_variables. (Not LC_ALL=C because I think we should preserve at least LC_CTYPE.)

Copy link
Author

Choose a reason for hiding this comment

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

To avoid similar sorting issues in our test components, I add export LC_COLLATE=C to pre_initialize_variables. Please take a look at the new changes.

By default, 'sort' sorts characters with system default locale,
which causes unexpected sorting order. To sort characters in ASCII
from computer perspective, export LC_COLLATE=C to specify character
collation for regular expressions and sorting with C locale.

Signed-off-by: Yanray Wang <[email protected]>
@yanrayw yanrayw added the needs-ci Needs to pass CI tests label Nov 23, 2023
Copy link
Contributor

@lpy4105 lpy4105 left a comment

Choose a reason for hiding this comment

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

LGTM

@daverodgman daverodgman 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 Nov 23, 2023
@daverodgman daverodgman added this pull request to the merge queue Nov 23, 2023
Merged via the queue into Mbed-TLS:development with commit 8cd4bc4 Nov 23, 2023
1 check passed
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 component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon size-m Estimated task size: medium (~1w) size-optimisation
Projects
No open projects
7 participants