net: tcp: use PSA functions for ISN generation instead of legacy MbedTLS ones #71827

valeriosetti · 2024-04-23T15:01:58Z

This PR:

replaces MD5 with SHA256 for the computation of ISN (see this comment for details)
replaces mbedtls_xxx() with psa_hash_compute()
replaces sys_rand_get() with sys_csrand_get()

This PR is an alternative to #71747 in the sense that there is no mbedtls_sha256() alternative option. We go directly to PSA functions instead. This is not a problem because we assume that a PSA provider is always present in the build should it either be built-in into MbedTLS (when MBEDTLS_PSA_CRYPTO_C is enabled) or provided externally through TFM (when BUILD_WITH_TFM is enabled).

jukkar

+1 to networking changes.

ceolin

It needs clarification about the mix usage of cryptographically safe random generator.
Please check comments.

ceolin · 2024-04-26T16:45:25Z

modules/mbedtls/zephyr_init.c

+		return PSA_ERROR_GENERIC_ERROR;
+	}
+#else
+	sys_rand_get(output, output_size);


Doesn't it need to be cryptographicallly secure ? AFAIU it needs, this fallback shouldn't be an option IMHO.

Yes, sorry, I pushed an updated version that fixes this.

ceolin · 2024-04-26T16:47:40Z

subsys/net/ip/tcp.c

 		sys_rand_get(unique_key, sizeof(unique_key));
+#endif


this condition really scares me. Does it need to be cryptographically secure or not ?
If yes, we should only use sys_crand_get and make it a requirement, if not, why we don't always use the cheaper sys_rand_get ?

IMO this is slightly different that the previous case. As far as I understand from RFC6528 at section 3 we just need the ISN to be "not easy to guess". For this we pick some random number and some extra data (input port, output port, etc) and hash all of this. In case the provided random number is not "random enough" then we can apply some hashing function to improve that.
This would mean that sys_rand_get() would be enough, but if CSPRNG is available, why not benefit from it anyway? Wdyt?

Not easy to guess sys_csrand_get doesn't allow test-modes (If I'm not mistaken). If you have predictive RNG numbers then they are definitely guessable. Please use only sys_csrand_get so we can remove any possible weak holes (e.g. due to RNG behaving "random looking" but still be predictive)

ceolin

It needs clarification about the mix usage of cryptographically safe random generator.
Please check comments.

frkv · 2024-05-07T13:04:27Z

subsys/net/ip/Kconfig.tcp

@@ -230,8 +230,8 @@ config NET_TCP_ISN_RFC6528
 	default y
 	depends on NET_TCP
 	select MBEDTLS
-	select MBEDTLS_MD
-	select MBEDTLS_MAC_MD5_ENABLED
+	select MBEDTLS_PSA_CRYPTO_C


Suggested change

select MBEDTLS_PSA_CRYPTO_C

select MBEDTLS_PSA_CRYPTO_C if !BUILD_WITH_TFM

Selecting MBEDTLS_PSA_CRYPTO_C is incompatible with BUILD_WITH_TFM. Not sure if this dependency will help, but you could try it out

Use only cryptographically secure random number generators for ISN. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Add a choice to select between legacy modules (i.e. ENTROPY + CTR_DRBG/HMAC_DRBG) and CSPRNG as random generators for PSA_CRYPTO_C. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

Create a new Kconfig named CONFIG_PSA_WANT_ALG_SHA_256 which allows to enable PSA_WANT_ALG_SHA_256. This allows to use PSA functions to compute SHA256 hashes. When PSA is provided by TFM this allows also to remove legacy mbedtls_sha256() support and therefore reduce footprint for the NS side. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti · 2024-05-08T07:45:35Z

Comments received so far should be addressed and CI is green. This PR is ready for review.

tomi-font

Noting that

some MbedTLS configuration changes come from mgmt: mcumgr: replace Tinycrypt by PSA #71947,
the non-default enabling of MBEDTLS_USE_PSA_CRYPTO if MBEDTLS_PSA_CRYPTO_C is temporary (will be possible to revert with Add Kconfigs for PSA_WANT symbols #72243),
and that the newly introduced Kconfig options relating to PSA in Kconfig.tls-generic will be moved to Kconfig.psa when rebasing this PR.

jukkar · 2024-05-08T10:25:00Z

subsys/net/ip/tcp.c

+	size_t hash_len;
+
+	psa_hash_compute(PSA_ALG_SHA_256, (const unsigned char *)&buf, sizeof(buf),
+					 hash, sizeof(hash), &hash_len);


jukkar · 2024-05-08T10:25:08Z

subsys/net/ip/tcp.c

+	size_t hash_len;
+
+	psa_hash_compute(PSA_ALG_SHA_256, (const unsigned char *)&buf, sizeof(buf),
+					 hash, sizeof(hash), &hash_len);


Both indentations should be OK now

frkv

LGTM (assuming alignment is fixed)

When BUILD_WITH_TFM is enabled we can dispatch hash computation to TFM. This allows to remove the built-in support of SHA256 from the non-secure side (if it's not used for any other purpose, of course). Signed-off-by: Valerio Setti <vsetti@baylibre.com>

CONFIG_MINIMAL_LIBC was required for: - CONFIG_MINIMAL_LIBC_NON_REENTRANT_FUNCTIONS - CONFIG_MINIMAL_LIBC_RAND while CONFIG_ENTROPY_GENERATOR and CONFIG_MBEDTLS_ZEPHYR_ENTROPY are required for CRYPTO_C. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

PICOLIBC misses the dirent.h header which is required to emulate ITS (internal trusted storage) in PSA APIs. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

MBEDTLS_PSA_CRYPTO_C and MBEDTLS_USE_PSA_CRYPTO are 2 different things and the former should not automatically enable the latter. The reson is that the user might want the MbedTLS PSA crypto toolbox to be built, but at the same time he/she does not want TLS/DTLS (and other intermediate modules such as PK, MD and Cipher) to use PSA APIs. For this reason this commit introduces a new Kconfig option named CONFIG_MBEDTLS_USE_PSA_CRYPTO to enable the corresponding build symbol. By default USE_PSA_CRYPTO is disabled. It is only explicilty enabled in tests/samples that were previously setting CRYPTO_C (since in those cases USE_PSA was set). Signed-off-by: Valerio Setti <vsetti@baylibre.com>

zephyrbot added area: Networking area: Crypto / RNG labels Apr 23, 2024

zephyrbot requested review from ceolin, d3zd3z, jukkar, pdgendt, rlubos, ssharks and tbursztyka April 23, 2024 15:02

zephyrbot assigned d3zd3z Apr 23, 2024

This was referenced Apr 23, 2024

net: tcp: use PSA functions (if possible) for ISN generation instead of legacy MbedTLS ones #71747

Closed

tls_credentials: use PSA functions instead of MbedTLS for SHA256 computation #71830

Merged

valeriosetti force-pushed the fix-tcp-psa-only branch 5 times, most recently from c38aecd to 271c9aa Compare April 24, 2024 16:25

jukkar previously approved these changes Apr 25, 2024

View reviewed changes

rlubos previously approved these changes Apr 25, 2024

View reviewed changes

ceolin requested changes Apr 26, 2024

View reviewed changes

valeriosetti dismissed stale reviews from rlubos and jukkar via 84dee98 April 29, 2024 14:18

valeriosetti force-pushed the fix-tcp-psa-only branch from 271c9aa to 84dee98 Compare April 29, 2024 14:18

zephyrbot requested a review from ithinuel April 29, 2024 14:19

valeriosetti force-pushed the fix-tcp-psa-only branch 3 times, most recently from e0ea3f7 to 787434f Compare April 30, 2024 03:40

valeriosetti marked this pull request as draft April 30, 2024 04:32

valeriosetti force-pushed the fix-tcp-psa-only branch from 787434f to 67b1cba Compare April 30, 2024 05:38

valeriosetti force-pushed the fix-tcp-psa-only branch 3 times, most recently from 4ed09d8 to 8849ed3 Compare May 7, 2024 09:52

zephyrbot added area: TF-M ARM Trusted Firmware-M (TF-M) area: Samples Samples labels May 7, 2024

frkv reviewed May 7, 2024

View reviewed changes

valeriosetti added 3 commits May 8, 2024 05:53

net: tcp: use CS random generators to generate ISN

cce3976

Use only cryptographically secure random number generators for ISN. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

mbedtls: add option to use CSPRNG as random source for PSA_CRYPTO_C

e6b68b1

Add a choice to select between legacy modules (i.e. ENTROPY + CTR_DRBG/HMAC_DRBG) and CSPRNG as random generators for PSA_CRYPTO_C. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti force-pushed the fix-tcp-psa-only branch from 8849ed3 to db03ff2 Compare May 8, 2024 04:00

valeriosetti requested a review from frkv May 8, 2024 06:49

tomi-font previously approved these changes May 8, 2024

View reviewed changes

jukkar reviewed May 8, 2024

View reviewed changes

frkv approved these changes May 8, 2024

View reviewed changes

valeriosetti added 4 commits May 8, 2024 16:29

mbedtls: do not include PSA ITS when using PICOLIBC

6addad9

PICOLIBC misses the dirent.h header which is required to emulate ITS (internal trusted storage) in PSA APIs. Signed-off-by: Valerio Setti <vsetti@baylibre.com>

valeriosetti dismissed tomi-font’s stale review via 1608b89 May 8, 2024 14:29

valeriosetti force-pushed the fix-tcp-psa-only branch from db03ff2 to 1608b89 Compare May 8, 2024 14:29

valeriosetti requested review from tomi-font, frkv and jukkar May 8, 2024 14:30

jukkar approved these changes May 8, 2024

View reviewed changes

ceolin approved these changes May 9, 2024

View reviewed changes

carlescufi merged commit f539b66 into zephyrproject-rtos:main May 9, 2024
26 checks passed

nordic-piks mentioned this pull request May 14, 2024

tests: crypto: mbedtls: crypto.mbedtls does not have permission on entropy driver #72716

Closed

valeriosetti mentioned this pull request May 22, 2024

Contributor nomination: valeriosetti #73127

Closed

valeriosetti mentioned this pull request May 30, 2024

doc: update migration guide and release notes for PRs #71827 and #72243 #73508

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: tcp: use PSA functions for ISN generation instead of legacy MbedTLS ones #71827

net: tcp: use PSA functions for ISN generation instead of legacy MbedTLS ones #71827

valeriosetti commented Apr 23, 2024 •

edited

Loading

jukkar left a comment

ceolin left a comment

ceolin Apr 26, 2024

valeriosetti May 6, 2024

ceolin Apr 26, 2024

valeriosetti May 6, 2024

frkv May 7, 2024 •

edited

Loading

ceolin left a comment

frkv May 7, 2024 •

edited

Loading

valeriosetti commented May 8, 2024

tomi-font left a comment

jukkar May 8, 2024

jukkar May 8, 2024

valeriosetti May 8, 2024

frkv left a comment •

edited

Loading

	select MBEDTLS_PSA_CRYPTO_C
	select MBEDTLS_PSA_CRYPTO_C if !BUILD_WITH_TFM

net: tcp: use PSA functions for ISN generation instead of legacy MbedTLS ones #71827

net: tcp: use PSA functions for ISN generation instead of legacy MbedTLS ones #71827

Conversation

valeriosetti commented Apr 23, 2024 • edited Loading

jukkar left a comment

Choose a reason for hiding this comment

ceolin left a comment

Choose a reason for hiding this comment

ceolin Apr 26, 2024

Choose a reason for hiding this comment

valeriosetti May 6, 2024

Choose a reason for hiding this comment

ceolin Apr 26, 2024

Choose a reason for hiding this comment

valeriosetti May 6, 2024

Choose a reason for hiding this comment

frkv May 7, 2024 • edited Loading

Choose a reason for hiding this comment

ceolin left a comment

Choose a reason for hiding this comment

frkv May 7, 2024 • edited Loading

Choose a reason for hiding this comment

valeriosetti commented May 8, 2024

tomi-font left a comment

Choose a reason for hiding this comment

jukkar May 8, 2024

Choose a reason for hiding this comment

jukkar May 8, 2024

Choose a reason for hiding this comment

valeriosetti May 8, 2024

Choose a reason for hiding this comment

frkv left a comment • edited Loading

Choose a reason for hiding this comment

valeriosetti commented Apr 23, 2024 •

edited

Loading

frkv May 7, 2024 •

edited

Loading

frkv May 7, 2024 •

edited

Loading

frkv left a comment •

edited

Loading