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 FFDH in TLS 1.2 #5278

Closed
mpg opened this issue Dec 7, 2021 · 13 comments
Closed

Remove FFDH in TLS 1.2 #5278

mpg opened this issue Dec 7, 2021 · 13 comments
Labels
api-break This issue/PR breaks the API and must wait for a new major version component-tls size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented Dec 7, 2021

There's a mismatch between what TLS 1.2 expects and what PSA Crypto provides regarding FFDH. See the documentation on PSA limitations for details. (Note: this is only a problem for (D)TLS 1.2, not (D)TLS 1.3.)

This task is:

  1. Come to a decision. Done: we're fully removing FFDH in TLS 1.2.
  2. Create a series of estimated tasks implementing that decision.
@mpg
Copy link
Contributor Author

mpg commented Jun 7, 2022

Note that the relevance of FFDH-with-TLS-1.2 is currently quite low and will presumably only decrease over time as TLS 1.3 becomes dominant. Here are some stats from a crawler on top web sites:

Total: 561143
TLS 1.3: 380793 (67.86%)
ECDH: 177593 (31.65% total, 98.47% pre-1.3)
FFDH: 1338 (0.24% total, 0.74% pre-1.3)
other: 1419 (0.25% total, 0.79% pre-1.3)

Even restricting ourselves to versions of TLS lower than 1.3, FFDH does not even represent 1% of the connections. Presumably this would be even less in the constrained space, as people have a stronger incentive to switch to ECDH which uses less RAM and computation time / energy.

For reference, here's the script that was used to generate those stats:

#!/usr/bin/python3

# Usage:
# wget -q -O- https://crawler.ninja/files/ciphers.txt | python3 cs.py

import re
import sys

(total, tls13, ecdh, ffdh, other) = (0, 0, 0, 0, 0)

for l in sys.stdin.readlines():
    if l.strip() == "Cipher Suites:":
        continue

    (cs, n) = l.strip().split()

    n = int(re.sub(",", "", n))
    total += n

    if cs.startswith("TLS_"):
        tls13 += n
    elif cs.startswith("ECDHE-"):
        ecdh += n
    elif cs.startswith("DHE-"):
        ffdh += n
    else:
        other += n

pre13 = total - tls13

print("Total:", total)
print("TLS 1.3: {} ({:.2f}%)".format(tls13, tls13 / total * 100))
print("ECDH: {} ({:.2f}% total, {:.2f}% pre-1.3)".format(
    ecdh, ecdh / total * 100, ecdh / pre13 * 100))
print("FFDH: {} ({:.2f}% total, {:.2f}% pre-1.3)".format(
    ffdh, ffdh / total * 100, ffdh / pre13 * 100))
print("other: {} ({:.2f}% total, {:.2f}% pre-1.3)".format(
    other, other / total * 100, other / pre13 * 100))

@mpg
Copy link
Contributor Author

mpg commented Jun 7, 2022

Based on this data, it is tempting to:

  • do nothing until 4.0, that is TLS-DHE-xxx ciphersuites keep using the legacy API even when USE_PSA_CRYPTO is enabled;
  • in 4.0, simply remove support for these ciphersuites in TLS 1.2 (FFDH can still be used in TLS 1.3 for people who want algorithm variety in case ECC collapses)

@mpg
Copy link
Contributor Author

mpg commented Jun 10, 2022

Reminder to self: advertise this plan on the mailing-list and ask for feedback.

@mpg
Copy link
Contributor Author

mpg commented Jun 27, 2022

Note: FFDH support will be added to TLS 1.3 (based on PSA) by #5979 - currently planned for the next quarter.

@mpg
Copy link
Contributor Author

mpg commented Jun 27, 2022

Sent an email to the list asking people to speak up if the above plan would cause trouble for them.

@mpg mpg added the api-break This issue/PR breaks the API and must wait for a new major version label Jun 27, 2022
@mpg
Copy link
Contributor Author

mpg commented Jun 27, 2022

Labeling "api-break" based on the current plan, so that we don't forget about it when preparing 4.0.

@thomas-fossati
Copy link
Contributor

Just noting that this is in line with the upcoming TLS BCP -- see in particular the bit starting with "However, [...]" at https://www.ietf.org/archive/id/draft-ietf-uta-rfc7525bis-08.html#section-4.1-2.7.1

@mpg
Copy link
Contributor Author

mpg commented Jun 2, 2023

Just noting that this is in line with the upcoming TLS BCP -- see in particular the bit starting with "However, [...]" at https://www.ietf.org/archive/id/draft-ietf-uta-rfc7525bis-08.html#section-4.1-2.7.1

Note: the draft mentioned is now an RFC (since November 2022) with official Best Current Practice status.

Now there's also a WG draft to formally deprecate FFDH in TLS 1.2 (as well a RSA-encryption-based key exchanges).

@daverodgman
Copy link
Contributor

Marking as SHOULD because this removes the final public-facing dependency on bignum.h: #6792 (comment)

@gilles-peskine-arm
Copy link
Contributor

Based on the (lack of) feedback on the 2022 mailing list thread and on the reasons cited in this issue, we have decided to remove FFDH support in TLS 1.2 in Mbed TLS 4.0.

Next steps: identify what code to remove:

  • Remove the options MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED and MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED, and all the code guarded by these options alone (in library, unit tests, test programs and ssl-opt.sh).
  • Remove all code guarded by MBEDTLS_DHM_C in TLS (including the dhm_ctx field in struct mbedtls_ssl_handshake_params). (TLS 1.3, which keeps supporting FFDH, uses PSA, not mbedtls_dhm_ functions.)
  • Remove DHM cipher suite support from compat.sh.
  • More? E.g. are there buffer sizes that should become shorter now that only ECDH is supported for TLS 1.2 key exchange?

@gilles-peskine-arm gilles-peskine-arm moved this from Planning needed to Design needed in Mbed TLS 4.0 planning Jun 5, 2024
@mpg
Copy link
Contributor Author

mpg commented Jun 12, 2024

More? E.g. are there buffer sizes that should become shorter now that only ECDH is supported for TLS 1.2 key exchange?

I don't think extra work should be needed for that. All buffer sizes that depend on whether DHM is enabled should already be guarded by MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED or MBEDTLS_DHM_C. If that isn't the case, we're already using suboptimal buffer sizes in configs without FFDH, which I'd consider a pre-existing bug that removal of FFDH-in-1.2 isn't making worse.

Though of course we should keep an eye out for this, because if we have such bugs, then this would be an excellent opportunity to catch them.

@mpg
Copy link
Contributor Author

mpg commented Aug 6, 2024

Note: there are some tests cases in ssl-opt.sh that use DHE-RSA for testing other things (like checking of the keyUsage extension in certificates) that will need to be replace - presumably just moving them use use ECDHE-RSA instead.

@gilles-peskine-arm gilles-peskine-arm changed the title Study: decide strategy for FFDH in TLS 1.2 Remove FFDH in TLS 1.2 Aug 8, 2024
@yanesca yanesca moved this to 4.0 - PSA Crypto always on in Mbed TLS Backlog Aug 27, 2024
@github-project-automation github-project-automation bot moved this to Mbed TLS 4.0 SHOULD in Backlog for Mbed TLS Aug 30, 2024
@gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm closed this as not planned Won't fix, can't repro, duplicate, stale Nov 4, 2024
@github-project-automation github-project-automation bot moved this from Planning needed to Done in Mbed TLS 4.0 planning Nov 4, 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 component-tls size-m Estimated task size: medium (~1w)
Projects
Status: Mbed TLS 4.0 SHOULD
Status: Done
Status: No status
Development

No branches or pull requests

5 participants