Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.2k stars 2.54k forks source link

Remove mbedtls_md_process and underlying functions #4657

Open gilles-peskine-arm opened 3 years ago

gilles-peskine-arm commented 3 years ago

Context

The function mbedtls_md_process and the underlying mbedtls_internal_<hash>_process functions were first released in Mbed TLS 1.3 as part of a partial countermeasure for timing-based Lucky 13 attacks in TLS. This countermeasure has since then improved and the functions in question are no longer used.

These functions are documented as “internal use” and their semantics is not fully clear. As of Mbed TLS 2.26, alternative implementations are still supposed to implement them, but it's useless since Mbed TLS won't call them and applications aren't supposed to call them.

Proposal

Remove mbedtls_md_process and the underlying functions from the public API.

Work to do

mpg commented 3 years ago

Since these functions are declared as “internal use”, arguably, we can remove them from Mbed TLS 2.2x before it becomes an LTS.

There are associated MBEDTLS_xxx_PROCESS_ALT config options which people might be using to accelerate a hash without replacing the entire module. I don't think it's OK to remove that in 2.2x, as that would force some users to do significant changes in their code.

gilles-peskine-arm commented 3 years ago

Indeed we shouldn't remove the PROCESS_ALT options, even in 3.0. But we can remove the process functions from the API even if we keep the possibility of having alt implementations for them.

mpg commented 3 years ago

[edit: this message is about 2.2x] If we can do that in a way that people using PROCESS_ALT don't have to change their code, sure. But I'm not convinced we can. People probably rely on the declaration of the function, so I don't see how we can remove it without affecting them. Though we could start by wrapping it in #if defined(MBEDTLS_xxx_PROCESS_ALT) - that way, the declaration is not visible with the default config, only for people using PROCESS_ALT.

gilles-peskine-arm commented 3 years ago

For ECP non-public alt functions, the declarations are in include/mbedtls/ecp_internal.h (2.x) or library/ecp_internal_alt.h (3.x), only enabled if the corresponding ALT symbol is enabled. I think it would be reasonable to keep the declarations of hash non-public alt functions in the header for the hash module, only when the ALT symbol is enabled. And we can do that in the same way in 2.x and 3.x.

daverodgman commented 1 year ago

mbedtls_md_process was removed in #7120 and shipped in 3.4.0. Some underlying functions (mbedtls_internal_xxx_process, for md5, ripemd, sha1, sha256, sha512) remain, as does MBEDTLS_XXX_PROCESS_ALT. Removing the latter is an API break.

daverodgman commented 1 year ago

Treating as SHOULD because this would be part of the ALT interface removal.

gilles-peskine-arm commented 2 months ago

mbedtls_md_process was removed in 3.4.0. The module-specific process functions will be internal in 4.0 so we can just remove them at our leisure. Therefore this is no longer an API break, just a matter of removing dead code.