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 single-DES and 2-DES from the public API #4396

Closed
mpg opened this issue Apr 22, 2021 · 8 comments
Closed

Remove single-DES and 2-DES from the public API #4396

mpg opened this issue Apr 22, 2021 · 8 comments
Labels
api-break This issue/PR breaks the API and must wait for a new major version enhancement needs-design-approval size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Apr 22, 2021

Single-DES is thoroughly broken by brute-force, to the point where it can be cracked as a service for a few dozen dollars - it was also disallowed by NIST a long time ago.

Two-key triple-DES (aka two-key TDEA or 2-DES or DES-EDE2) is currently disallowed by NIST for encryption, and only allowed for legacy decryption. Also, it never had an adoption quite as large as 3DES (triple-key TDEA), or DES before it.

We should remove those from our public API, and only keep 3DES.

(Note: 3DES has been designed in such as way that it's possible to derive 2DES and single-DES from it by setting the keys appropriately, so if anyone still really needs 2DES for legacy decryption, they can still get it from an API that officially only supports 3DES.)

Task definition:

  1. Remove support for MBEDTLS_CIPHER_DES_ECB, MBEDTLS_CIPHER_DES_CBC, MBEDTLS_CIPHER_DES_EDE_ECB, and MBEDTLS_CIPHER_DES_EDE_CBC in the cipher layer (cipher.h, cipher.candcipher_wrap.c`).
  2. Remove the following APIs from des.h: the mbedtls_des_context type, and all mbedtls_des_xxx() and mbedtls_des3_set2key_xxx() functions (except mbedtls_des_self_test()) - we can start by making them static and then remove those that the compiler flags as unused.
  3. Once the previous is done, check which MBEDTLS_DES_xxx_ALT flags are still present in the code, and remove the others from config.h. If it turns out we want to keep exposing mbedtls_des_setkey() to alt implementers, we should declare it in a new header file library/des_alt.h rather than in include/mbedtls/des.h where it is now.
  4. Remove all the tests for single-DES and 2-DES.
@mpg mpg added mbedtls-3 enhancement size-s Estimated task size: small (~2d) labels Apr 22, 2021
@gilles-peskine-arm
Copy link
Contributor

PSA also supports 1-key and 2-key DES, so that would have to go away as well. Given the number of parts involved in the removal, I think this is (size:M).

Also, some of our users (at least OP-TEE) want 1DES and 2DES because it's required for GlobalPlatform TEE API compliance (even though no one in their right mind would have used them even back when that standard was defined).

There would be maintenance gains to removing DES altogether, but not for just removing 1DES and 2DES. So I don't think we should expend our limited 3.0 bandwidth on that.

@mpg
Copy link
Contributor Author

mpg commented Apr 22, 2021

Well, PSA could keep supporting 1-key and 2-key DES by building them on top of 3-key DES, but that would also increase the amount of work needed.

I was already on the fence about putting this in "3.0" or "3.0 optional", so I'm moving it to optional now, and updating the size label as suggested.

@mpg mpg added size-m Estimated task size: medium (~1w) and removed size-s Estimated task size: small (~2d) labels Apr 22, 2021
@garmae
Copy link

garmae commented Apr 24, 2021

Nice proposal. One of the slowest moving sectors, the banking industry, (correction) still relies on 2-TDES. Even so, they are slowly moving to AES (yes, this late).

@mpg
Copy link
Contributor Author

mpg commented May 31, 2021

@TRodziewicz since this is a rather large task and a bit controversial (I'm not sure we really have a consensus on doing it), I think it's best to leave it alone for now - we'll probably revisit that in 4.0 actually.

@mpg
Copy link
Contributor Author

mpg commented May 31, 2021

Labeling "needs design approval" to reflect the fact that I'm not confident we have a consensus about what, if anything, should be done for 3.0.

@TRodziewicz TRodziewicz removed their assignment May 31, 2021
@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
@mpg
Copy link
Contributor Author

mpg commented Dec 28, 2023

Note: des.h will be made internal (non-public), see #8663

@gilles-peskine-arm
Copy link
Contributor

Given that we missed the window in 2021, I propose to completely remove DES in 2025.

@gilles-peskine-arm
Copy link
Contributor

Moot since we are removing DES altogether.

@gilles-peskine-arm gilles-peskine-arm closed this as not planned Won't fix, can't repro, duplicate, stale Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break This issue/PR breaks the API and must wait for a new major version enhancement needs-design-approval size-m Estimated task size: medium (~1w)
Projects
Status: Mbed TLS 4.0 SHOULD
Status: Done
Development

No branches or pull requests

6 participants