Introduce config option of 128-bit key only in AES calculation

[yanrayw]

Description

Fix: #7376

This PR brings in changes from #2890 and introduce 128-bit key only in AES calculation, with new config option MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH.

Size measured at 3c3b94a:

GCC(8.4.0)	File Name	Old Size	New Size	Change (Bytes)	Change (%)
	libmbedcrypto.a	481434	480318	-1116	-0.23%

armclang(6.19)	File Name	Old Size	New Size	Change (Bytes)	Change (%)
	libmbedcrypto.a	79400	78916	-484	-0.61%

Size measured at d9bf370: (x86: ubuntu-18.04)

GCC(8.4.0)	File Name	Old Size	New Size	Change (Bytes)	Change (%)
	aes.o	21669	22249	-580	-2.61%
	aesni.o	1477	1042	-435	- 29.45%
	ctr_drbg.o	5430	5294	-136	- 2.5%
	gcm.o	10217	10115	-102	-1%
	nist_kw.o	5039	5056	17	0.34%
	version_features.o	4582	4630	48	1.05%
	libmbedcrypto.a	480850	479678	-1172	-0.24%

armclang(6.19)	File Name	Old Size	New Size	Change (Bytes)	Change (%)
	aes.o	2820	2448	-372	-13.19%
	cipher_wrap.o	760	758	-2	-0.26%
	ctr_drbg.o	1326	1312	-14	-1.06%
	psa_crypto.o	20136	20040	-96	-0.48%
	libmbedcrypto.a	79380	78896	-484	-0.61%

Size measured at e2bc158: (aarch64: ubuntu-22.04)

GCC(11.2.0)	File Name	Old Size	New Size	Change (Bytes)	Change (%)
	aesce.o	1026	866	-160	-15.59%

PR checklist

• changelog provided as there is a new config option
• backport not needed because this is a new feature
• tests provided

[yanrayw]

0d96764 is modified by a python script with command:
./tests/scripts/generate_dependency.py -r \\s*.*(AES.?192|AES.?256|KEY=32|KEY=24).* -d \!MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH

[yanrayw]

I see there are conflicts in check_config.h. As the review is started, let me know if I have to fix it or not. Personally I prefer to keeping it since code size is measured at 3c3b94a

[tom-daubney-arm]

I see there are conflicts in check_config.h. As the review is started, let me know if I have to fix it or not. Personally I prefer to keeping it since code size is measured at 3c3b94a

My review hasn't really started yet so would prefer the conflicts to be fixed first but depends on how far @aditya-deshpande-arm has got with his review. What do you think Aditya?

[yanrayw]

My review hasn't really started yet so would prefer the conflicts to be fixed first but depends on how far @aditya-deshpande-arm has got with his review. What do you think Aditya?

I have checked it's a small conflict. I think rebase won't affect your review. So I'll rebase and fix the conflict.

[tom-daubney-arm]

My review hasn't really started yet so would prefer the conflicts to be fixed first but depends on how far @aditya-deshpande-arm has got with his review. What do you think Aditya?

I have checked it's a small conflict. I think rebase won't affect your review. So I'll rebase and fix the conflict.

Thanks Yanray

[tom-cosgrove-arm]

Do we care about MBEDTLS_CTR_DRBG_USE_128_BIT_KEY if we don't have MBEDTLS_CTR_DRBG_H?

[gilles-peskine-arm]

Historically we haven't been very consistent about “sub-options”: if MBEDTLS_FOO_SETTING is an option that controls (mostly) foo.c, and MBEDTLS_FOO_C is disabled, is MBEDTLS_FOO_SETTING ignored (because the code it affects is fully disabled) or is setting it an error (because you can't have a partial foo without foo.c)?

Currently MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is ignored if MBEDTLS_CTR_DRBG_C is disabled, so it should continue this way.

Also, if MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is defined, maybe MBEDTLS_CTR_DRBG_USE_128_BIT_KEY should be automatically enabled? After all, if you've decided to only have AES-128, the fact AES-128 will be used rather than AES-256 wouldn't come as a surprise.

[yanrayw]

I think we don't care about MBEDTLS_CTR_DRBG_USE_128_BIT_KEY if we don't have MBEDTLS_CTR_DRBG_C. Since MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is needed to set a 128-bit key in mbedtls_ctr_drbg_seed. If we don't have MBEDTLS_CTR_DRBG_C, then MBEDTLS_CTR_DRBG_USE_128_BIT_KEY doesn't take any effect on MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH.

I didn't consider the combination when MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH enabled but MBEDTLS_CTR_DRBG_C disabled. After considering it, it looks more reasonable to automatically enable MBEDTLS_CTR_DRBG_USE_128_BIT_KEY if MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH enabled.

For current design, if MBEDTLS_CTR_DRBG_C and MBEDTLS_CTR_DRBG_USE_128_BIT_KEY disabled, MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH enabled. This config combination should be acceptable but check_config.h reports an error. Should I change it this way or any better solution?

[tom-cosgrove-arm]

Yes, I think the best way is @gilles-peskine-arm's suggestion: if MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is defined, and MBEDTLS_CTR_DRBG_C is too, then MBEDTLS_CTR_DRBG_USE_128_BIT_KEY should be automatically enabled.

If we have MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH but MBEDTLS_CTR_DRBG_C isn't set, we don't care about MBEDTLS_CTR_DRBG_USE_128_BIT_KEY

[tom-cosgrove-arm]

I don't think we can say "by 1,116 bytes" unconditionally:

Using size on an armclang 6.19 build with --target=arm-arm-none-eabi -mcpu=cortex-m33+nodsp -Oz and default configuration plus/minus +MBEDTLS_NO_PLATFORM_ENTROPY -MBEDTLS_FS_IO -MBEDTLS_PSA_ITS_FILE_C -MBEDTLS_TIMING_C -MBEDTLS_HAVE_TIME -MBEDTLS_HAVE_TIME_DATE -MBEDTLS_PSA_CRYPTO_STORAGE_C -MBEDTLS_NET_C I get the following (no AESNI or AESCE)

orig:ca4ca9a2f	pr:7451	Delta	Filename
7540, 0	7320, 0	-220+0	aes.o
2930, 0	2958, 0	+28+0	cmac.o
2078, 0	1936, 0	-142+0	ctr_drbg.o
6145, 0	6193, 0	+48+0	gcm.o
2737, 0	2799, 0	+62+0	nist_kw.o
3654, 0	3691, 0	+37+0	version_features.o
		-187+0	TOTAL

With MBEDTLS_SELF_TEST, MBEDTLS_VERSION_C and MBEDTLS_VERSION_FEATURES disabled I get

orig:ca4ca9a2f	pr:7451	Delta	Filename
4350, 0	4042, 0	-308+0	aes.o
1286, 0	1272, 0	-14+0	ctr_drbg.o
		-322+0	TOTAL

[gilles-peskine-arm]

We definitely can't give a number of bytes for code size, not only because it depends on the compiler, but because it depends on the architecture! Not everyone is compiling for Arm.

We can give a rough ballpark: it reduces the size of the AES code by about 10% (or whatever figure you end up with).

[yanrayw]

I don't think we can say "by 1,116 bytes" unconditionally

That's correct. I give 1116 bytes that's because it's measured by code_size_compare.py listed in description. But I don't understand why we measure code size with extra options enabled or disabled. What I understand is we measure original code size with MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH disabled then we measure improved code size with MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH enabeld. As the only introduction is MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH in this PR.

I totally agree we can't give an accurate number of bytes for code size saving as it depends on a bunch of options. This 1116 bytes followed 2890 and MBEDTLS_CAMELLIA_SMALL_MEMORY in development. As both of them give a number of bytes but I can't reproduce this number so I think accuracy of the value might be not crucial.

As @gilles-peskine-arm suggested, write something like reduces the size of the AES code by about 10% is a good option. But my concern is percentage might be in a big difference on different architecture and compiler. See code size measurement in issue description. It's -0.23% on x86 with gcc 8.4.0 but -0.61% on aarch32 with armclang 6.19.

[tom-cosgrove-arm]

(minor) it's more natural in English to say "for AES operations that use 192- or 256-bit keys"

[tom-cosgrove-arm]

@yanrayw I think the commits that are mainly Arto's work should keep his authorship

[DemiMarie]

I would prefer to start with dropping 192-bit key support; nobody I know of actually uses that.

[tom-daubney-arm]

I think this PR is ready now. The changes to the self test functions look right and the tests run as I would expect.

LGTM.

[gilles-peskine-arm]

(I've commented on this pull request, but I am not planning to do a full review.)

[tom-cosgrove-arm]

In mbedtls_config.h we currently have

/** \def MBEDTLS_CTR_DRBG_USE_128_BIT_KEY
 *
 * Uncomment this macro to use a 128-bit key in the CTR_DRBG module.
 * By default, CTR_DRBG uses a 256-bit key.
 */
//#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY

maybe it should say "By default, CTR_DRBG uses a 256-bit key unless MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set"?

and

/**
 * \def MBEDTLS_CTR_DRBG_C
 *
 * Enable the CTR_DRBG AES-based random generator.
 * The CTR_DRBG generator uses AES-256 by default.
 * To use AES-128 instead, enable \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY above.
 *
 * \note To achieve a 256-bit security strength with CTR_DRBG,
 *       you must use AES-256 *and* use sufficient entropy.
 *       See ctr_drbg.h for more details.
 *
 * Module:  library/ctr_drbg.c
 * Caller:
 *
 * Requires: MBEDTLS_AES_C
 *
 * This module provides the CTR_DRBG AES random number generator.
 */
#define MBEDTLS_CTR_DRBG_C

should we say something like

"To use AES-128 instead, enable \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY or \c MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH above"?

[yanrayw]

#7451 (comment)
@tom-cosgrove-arm I totally agree the changes for MBEDTLS_CTR_DRBG_USE_128_BIT_KEY.

However, regarding MBEDTLS_CTR_DRBG_C, if we enable MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH to use AES-128. This option forces to only support AES-128. Is this something we expect? Or should we write like below?

/**
 * \def MBEDTLS_CTR_DRBG_C
 *
 * Enable the CTR_DRBG AES-based random generator.
 * The CTR_DRBG generator uses AES-256 by default.
 * To use AES-128 instead, enable \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY above.
 * To only support AES-128 instead, enable \c MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH above.
 *
 * \note To achieve a 256-bit security strength with CTR_DRBG,
 *       you must use AES-256 *and* use sufficient entropy.
 *       See ctr_drbg.h for more details.
 *
 * Module:  library/ctr_drbg.c
 * Caller:
 *
 * Requires: MBEDTLS_AES_C
 *
 * This module provides the CTR_DRBG AES random number generator.
 */
#define MBEDTLS_CTR_DRBG_C

[gilles-peskine-arm]

To only support AES-128 instead, enable \c MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH above.

This is not correct in the context of the CTR_DRBG documentation: MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH turns off AES-128 and AES-192 for the whole library, not just for CTR_DRBG. MBEDTLS_CTR_DRBG_USE_128_BIT_KEY makes the DRBG use AES-128 key even when the library supports AES-256; the intended use case is for speed on platforms where AES-128 is preferred and AES-256 is provided because it's a requirement but the platform avoids using it because it's slow.

[tom-cosgrove-arm]

Yeah, maybe my wording wasn't great. But we should reference MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH here. How about

* To use AES-128 instead, enable \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY above.
* AES-128 will also be used if \c MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set.

[yanrayw]

@tom-cosgrove-arm @gilles-peskine-arm How about this?

Adding

* By default, CTR_DRBG uses a 256-bit key
* unless MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set.

/** \def MBEDTLS_CTR_DRBG_USE_128_BIT_KEY
 *
 * Uncomment this macro to use a 128-bit key in the CTR_DRBG module.
 * By default, CTR_DRBG uses a 256-bit key
 * unless MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set.
 */
//#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY

Adding

 * \note AES-128 will be used if \c MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set.

/**
 * \def MBEDTLS_CTR_DRBG_C
 *
 * Enable the CTR_DRBG AES-based random generator.
 * The CTR_DRBG generator uses AES-256 by default.
 * To use AES-128 instead, enable \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY above.
 *
 * \note AES-128 will be used if \c MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set.
 *
 * \note To achieve a 256-bit security strength with CTR_DRBG,
 *       you must use AES-256 *and* use sufficient entropy.
 *       See ctr_drbg.h for more details.
 *
 * Module:  library/ctr_drbg.c
 * Caller:
 *
 * Requires: MBEDTLS_AES_C
 *
 * This module provides the CTR_DRBG AES random number generator.
 */
#define MBEDTLS_CTR_DRBG_C

[tom-cosgrove-arm]

Rather than

 * By default, CTR_DRBG uses a 256-bit key
 * unless MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set.

I think

 * Without this, CTR_DRBG uses a 256-bit key
 * unless MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH is set.

I think the other one is fine.

@gilles-peskine-arm ?

[tom-cosgrove-arm]

@yanrayw I think you only need to make the changes to the descriptions of MBEDTLS_CTR_DRBG_USE_128_BIT_KEY and MBEDTLS_CTR_DRBG_C in the config files.

Gilles has given a thumbs-up to my suggestion of a minor change to your wording for MBEDTLS_CTR_DRBG_USE_128_BIT_KEY, and what you suggest for MBEDTLS_CTR_DRBG_C seems fine.

With those done, should be ready for approval.

[yanrayw]

@tom-cosgrove-arm @tom-daubney-arm CI is green, please re-review.

[tom-cosgrove-arm]

LGTM - thanks

[tom-daubney-arm]

I've reviewed the small changes that came up during Tom's review - LGTM.