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.22k stars 2.56k forks source link

Remove FFDH in TLS 1.2 #5278

Open mpg opened 2 years ago

mpg commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Based on this data, it is tempting to:

mpg commented 2 years ago

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

mpg commented 2 years ago

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

mpg commented 2 years ago

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

mpg commented 2 years ago

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

thomas-fossati commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

Marking as SHOULD because this removes the final public-facing dependency on bignum.h: https://github.com/Mbed-TLS/mbedtls/issues/6792#issuecomment-1375278767

gilles-peskine-arm commented 3 months ago

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:

mpg commented 3 months ago

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 commented 1 month ago

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.