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

Remove mbedtls_md_process and underlying functions #4657

Open
gilles-peskine-arm opened this issue Jun 14, 2021 · 7 comments
Open

Remove mbedtls_md_process and underlying functions #4657

gilles-peskine-arm opened this issue Jun 14, 2021 · 7 comments
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jun 14, 2021

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

  • Remove mbedtls_md_process entirely.
  • Remove the underlying mbedtls_internal_<hash>_process functions from the public API and declare them as static (since they're a building block of the public mbedtls_<hash>_update functions).
@gilles-peskine-arm gilles-peskine-arm added enhancement component-crypto Crypto primitives and low-level interfaces mbedtls-3 size-s Estimated task size: small (~2d) labels Jun 14, 2021
@mpg
Copy link
Contributor

mpg commented Jun 15, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

mpg commented Jun 15, 2021

[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
Copy link
Contributor Author

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.

@bensze01 bensze01 modified the milestone: Mbed TLS 4.0 Jul 28, 2021
@bensze01 bensze01 removed this from the Mbed TLS 4.0 milestone Aug 11, 2021
@daverodgman daverodgman added api-break This issue/PR breaks the API and must wait for a new major version and removed mbedtls-3 labels May 13, 2022
@daverodgman
Copy link
Contributor

daverodgman commented Aug 30, 2023

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 daverodgman reopened this Aug 30, 2023
@daverodgman daverodgman added api-break This issue/PR breaks the API and must wait for a new major version and removed api-break This issue/PR breaks the API and must wait for a new major version labels Aug 30, 2023
@daverodgman
Copy link
Contributor

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

@gilles-peskine-arm
Copy link
Contributor Author

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.

@gilles-peskine-arm gilles-peskine-arm removed needs-design-approval api-break This issue/PR breaks the API and must wait for a new major version labels Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-s Estimated task size: small (~2d)
Projects
Status: No status
Development

No branches or pull requests

4 participants