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

Wishlist for removals in 4.0 #6792

Closed
mpg opened this issue Dec 15, 2022 · 27 comments
Closed

Wishlist for removals in 4.0 #6792

mpg opened this issue Dec 15, 2022 · 27 comments
Labels
api-break This issue/PR breaks the API and must wait for a new major version

Comments

@mpg
Copy link
Contributor

mpg commented Dec 15, 2022

This issue provide a place to collect ideas of things we'd like to remove in 4.0. When the time comes to start working on 4.0, we can split it and create a task for each item that we agree to remove.

One comment per proposal so that people can upvote / downvote (but obviously feel free to discuss in comments as well).

Edit: I'm breaking these out into separate issues and linking in the comments below.

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

mpg commented Dec 15, 2022

Existing issues for reference:

@mpg
Copy link
Contributor Author

mpg commented Dec 15, 2022

Proposal: Remove support for CBC ciphersuites in TLS (1.2). - see #8169

AEAD has been the preferred option since at least 2015 (RFC 7525).

CBC requires complex counter-measures to Lucky 13, whose tests significantly slow down the SSL test suite and which would require a special case when we get around to isolating session secrets with PSA.

Removing it would get rid of that, plus allow getting rid of the Encrypt-then-MAC extension.

OTOH, last time we checked, some IoT / M2M standards were still mandating CBC ciphersuites. See also #3854

(Also, the mandatory-to-implement ciphersuite for TLS 1.2 is TLS_RSA_WITH_AES_128_CBC_SHA, but RFC 7525 (which has official Best Current Practices status) encourages implementors to ignore that.)

@mpg
Copy link
Contributor Author

mpg commented Dec 15, 2022

Proposal: Remove support for the RSA key exchange in TLS (1.2).

See #8170

This is RSA and RSA-PSK, as opposed to ECDHE-RSA (or currently DHE-RSA). Using forward-secure key exchanges has been officially recommended since at least 2015 (RFC 7525 again). Edit: now there's even a WG draft to formally deprecate it, as in "MUST NOT negotiate".

Also, this key exchange requires care to avoid timing attacks (see all the timing-based variants of Bleichenbacher), and would also cause trouble (perhaps even more than CBC) when implementing isolation of session secrets with PSA.

(OTOH, the mandatory-to-implement ciphersuite for TLS 1.2 is TLS_RSA_WITH_AES_128_CBC_SHA, which uses this key exchange, but RFC 7525 (which has official Best Current Practices status) encourages implementors to ignore that.)

@mpg
Copy link
Contributor Author

mpg commented Dec 15, 2022

Proposal: Remove curves smaller than 255 bits

See #8136

That is, remove secp192r1, secp224r1, secp192k1 and secp224k1.

On the security front, these curves provide 96 resp. 112 bits of security, which is smaller than the generally-accepted baseline of 128 bits. Also, they're less widely used (they're no longer usable in TLS for example), so perhaps less studied and tested.

On the implementation front, secp224k1 is an annoying source of corner cases as it's the only curve with bitlen(n) != bitlen(p), and even has bytelen(n) > bytelen(p). Getting rid of it would allow some simplifications in the source code. Edit: seems we already did some of the simplifications on the PSA side, and as a result secp224k1 is not working with PSA already.

@DemiMarie
Copy link
Contributor

Proposal: Remove support for CBC ciphersuites in TLS (1.2).

AEAD has been the preferred option since at least 2015 (RFC 7525).

CBC requires complex counter-measures to Lucky 13, whose tests significantly slow down the SSL test suite and which would require a special case when we get around to isolating session secrets with PSA.

Removing it would get rid of that, plus allow getting rid of the Encrypt-then-MAC extension.

OTOH, last time we checked, some IoT / M2M standards were still mandating CBC ciphersuites. See also #3854

What about refusing to use CBC without encrypt-then-MAC?

@mpg
Copy link
Contributor Author

mpg commented Dec 19, 2022

What about refusing to use CBC without encrypt-then-MAC?

Good point, that's a compromise worth considering. GnuTLS has an option for that (%FORCE_ETM in the priority string), so there's some precedent - we could do the same except non-optional.

However, it's not ideal due to the way things are negotiated in TLS: a client needs to decide whether to offer CBC ciphersuites before it knows whether the server support the EtM extension, so if the server responds by picking a CBC ciphersuite but not accepting the EtM extension, the client has no choice but to break the handshake, which is not ideal.

Also, for the cases where some standards / the ecosystem still mandate use of CBC ciphersuites, we should check if EtM is an option at all.

@DemiMarie
Copy link
Contributor

Are there old ciphers that can be ripped out? Camellia?

@mpg
Copy link
Contributor Author

mpg commented Dec 28, 2022

Camellia does not sound particularly old to me, it's about the same age as AES, and AFAICT it's still recommended by the Japanese government. So, I don't really see a reason to remove it.

Note that individual ciphers are really not much of a maintenance burden: they're usually isolated in their own module, and rather easy to test. So, there's not that much to be gained by removing them. I'm inclined to remove only those that are known to be weak and/or officially deprecated.

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jan 6, 2023

Proposal: remove DES

See #4396, #7024, #9164

DES is nice in some respects (it's not sensitive to side channels the way AES is), but its small block size is a killer. The world has mostly moved away from it. As far as I know, it's still relevant to banking, but that's not a domain where Mbed TLS is used much if at all. Maybe it's still used in old encrypted PEMs?

We already removed DES from TLS in 3.0.

Technical argument, which works both ways: removing DES allows us to simplify our code since all the block ciphers will support will have 128-bit blocks. This also means we may make supporting a hypothetical 256-bit block
cipher harder — but no such block cipher seems to be emerging (stream ciphers are the rage nowadays).

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jan 6, 2023

Proposal: remove negative numbers from bignum

See #8155 (this can then be done in the 4.x lifetime)

Negative numbers don't really belong in a cryptographic library. Right now we use them in a few places in the ECC code, but they're actively being rewritten and will be gone around 3.5-ish.

We will eventually remove bignum.h altogether, but we might not be ready for that yet in 4.0, depending on when it's released.

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jan 6, 2023

Proposal: remove “fluff” from md, cipher and pk

See #8133

The high-level legacy crypto modules are gradually being replaced by PSA, but we'll probably keep them in 4.0 and remove them in 5.0. (The rule of thumb for API migration is: one major release to introduce the new API and deprecate the old one, and another major release to remove the old one.)

We don't have to keep all the features of md, cipher and pk, especially those that don't match well with PSA and aren't very useful.

  • Remove ad hoc names of algorithms (mbedtls_md_info_from_string, etc.). They aren't used anywhere else in the library and don't follow any particular standards.
  • Remove ccm.h and gcm.h as public interfaces, keeping only the cipher.h abstraction.
  • Remove access to KW/KWP from cipher.h. It belongs differently from the others, and that has caused bugs in the past.
  • Remove padding other than PKCS7 from cipher.h.
  • more?

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Jan 6, 2023

Remove explicit RNG arguments when used for blinding

See #8191

It is useful in some applications to specify an RNG when it influences the result — though we won't have this in PSA. However, once psa_crypto_init becomes officially mandatory, we'll have an RNG available at all times when needed, so we can remove RNG arguments. I propose that we do so when the RNG is only used for blinding.

@DemiMarie
Copy link
Contributor

once psa_crypto_init becomes officially mandatory

This is a giant footgun, especially for non-embedded applications. Thread-safe lazy init is much less error-prone.

@gilles-peskine-arm
Copy link
Contributor

@DemiMarie On a high-end platform like Linux or Windows, automatic initialization might make sense. As you note, it would have to be thread-safe. In many embedded setups, initialization needs to be under the application's control because it has implications. On platforms where crypto happens inside the secure world, psa_crypto_init initiates a connection to the crypto service. Otherwise psa_crypto_init initializes the RNG, which requires the entropy peripheral to be available. psa_crypto_init also requires access to storage in some circumstances.

@DemiMarie
Copy link
Contributor

@gilles-peskine-arm Given how bad applications are at checking for otherwise-impossible errors, I think it would be better to trap if psa_crypto_init() has not been called. This would ensure that code that forgets to call psa_crypto_init() cannot escape into the wild.

@mpg
Copy link
Contributor Author

mpg commented Jan 9, 2023

We will eventually remove bignum.h altogether, but we might not be ready for that yet in 4.0, depending on when it's released.

I'm not sure keeping bignum.h public but changing it to no longer support negative numbers is really much easier than making in private. As far as I'm aware there are only two one blockers for making it private, which are the places where its types are use in the API of higher-level modules. The first is in X.509 and is being addressed right now [edit: now merged]. The second is in TLS 1.2 for FFDH key exchanges, which we're planning on removing in 4.0 anyway.

So, I'm not sure I see why we wouldn't be able to make bignum.h private in 4.0?

@mpg
Copy link
Contributor Author

mpg commented May 2, 2023

For reference: things that are currently deprecated

See #8138

  • mbedtls_asn1_free_named_data() -> use mbedtls_asn1_free_named_data_list() or mbedtls_asn1_free_named_data_list_shallow() instead
  • MBEDTLS_PSA_CRYPTO_SE_C -> use the unified driver interface instead
  • MBEDTLS_SSL_DTLS_CONNECTION_ID_COMPAT -> let's only support the standard version
  • mbedtls_cipher_setup_psa() -> let's see how that evolves with driver-only ciphers work
  • compat-2.x.h -> just remove
  • mbedtls_pkcs5_pbkdf2_hmac() -> use mbedtls_pkcs5_pbkdf2_hmac_ext() instead (of the PSA Crypto API)
  • mbedtls_ssl_conf_curves() -> use mbedtls_ssl_conf_groups() instead
  • mbedtls_ssl_conf_max/min_version() -> use mbedtls_ssl_conf_max/min_tls_version() instead
  • mbedtls_x509write_crt_set_serial() -> use mbedtls_x509write_crt_set_serial_raw() instead

@gilles-peskine-arm
Copy link
Contributor

Remove mbedtls_pk_ec and mbedtls_pk_rsa. See #7572

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented May 12, 2023

Null cipher

See #8192

We've previously talked about removing support for the null cipher in TLS, i.e. removing support for cipher suites where the data is authenticated but not encrypted. Mbed TLS supports this (though not in the default build). Note that some users do use the null cipher, so we should perhaps keep it.

@mpg
Copy link
Contributor Author

mpg commented May 12, 2023

Regarding NULL ciphersuites in TLS 1.2, I think it's also interesting to note that they were removed from TLS 1.3... only to be re-introduced by RFC 9150, published in April 2022 (but not the 1st). I think that this RFC, plus the issue report you linked to, can be taken as indications that some people still care about, as the RFC calls it, Authentication and Integrity-Only Cipher Suites, so perhaps we shouldn't remove them.

OTOH, in TLS 1.2, they have a non-zero maintenance cost, as they are the only ones in the "stream cipher" category. And of course their availability creates a risk of misconfiguration. However, I think that risk can be mitigated by making them opt-in not just at compile time (which they are now) but also at compile-time (exclude them from the list of ciphersuites unless explicitly requested - that's not the case right now).

@mpg
Copy link
Contributor Author

mpg commented May 19, 2023

Proposal: remove (partial) support for parsing SpecifiedECDomain (withdrawn).

When encoding an ECC key, the curve must be encoded somehow. Most of the time it's by naming a known curve. However, it is also possible to encode all of the curve parameters (modulus, constants a and b in the equation, coordinates of the conventional base point) - this is SpecifiedECDomain, defined for example by SEC 1 Appendix C.2.

We have optional limited support for that, controlled by the compile time option MBEDTLS_PK_PARSE_EC_EXTENDED: we parse the SpecifiedECDomain structure and try to match the parameters to a known curve. If a match is found, the structure is accepted, otherwise it's rejected.

However, as the documentation for this option points out, both RFC 5480 (ECC Subject Public Key Information, 2009) and RFC 5915 (EC Private Key Structure) mandate use of NamedCurve, not SpecifiedECDomain. Moreover, in recent years, there have been a clear move away from custom curves (for example, they were allowed in RFC 4492 (2006) and removed by RFC 8422 (2018)). So, it seems unlikely that a lot of tools still encode keys as SpecifiedECDomain by default this days, or doesn't have an option to encode as NamedCurve.

If people still have old key material encoded that way, it can be converted to a NamedCurve encoding using external tools (for example OpenSSL).

Benefits of removing support for this:

  • drops a config option;
  • removes some parsing code from the code base;
  • the current implementation isn't compatible with having ECC fully driver-only (it requires at least ECP_LIGHT).

@DemiMarie
Copy link
Contributor

DemiMarie commented May 19, 2023

@mpg: There are some standards (notably electronic passports IIRC) that require SpecifiedECDomain and forbid NamedCurve. However, it is possible to avoid parsing SpecifiedECDomain (and NamedCurve, for that matter!) by treating both as an opaque byte array that gets looked up in a table. This should be faster, less code, remove the ECP_LIGHT dependency, and have vastly less attack surface. It is even possible to treat the entire AlgorithmID as opaque! The webpki library takes this approach, as does my own barebones-x509.

@mpg
Copy link
Contributor Author

mpg commented May 22, 2023

Ah, thanks for the info! Let's keep it, then.

However, it is possible to avoid parsing SpecifiedECDomain (and NamedCurve, for that matter!) by treating both as an opaque byte array that gets looked up in a table.

Yes, that's the backup plan I had in mind to make it work without ECP_LIGHT However I had a number of question: is the encoding always DER? what about the version field? optional fields cofactor and hash (probably not a problem since they're at the end)? optional field seed inside Curve (more of a problem, right in the middle)?

Good to know that this strategy has already been implemented and thanks for the references, that should answer all of my questions!

@mpg
Copy link
Contributor Author

mpg commented May 22, 2023

Good to know that this strategy has already been implemented and thanks for the references, that should answer all of my questions!

Perhaps I didn't look correctly, but I didn't seem to find support for SpecifiedECDomain. In order not to hijack this thread, I've created a separate issue: #7628. @DemiMarie If you have any insight to share there, that would be appreciated as always!

@mpg
Copy link
Contributor Author

mpg commented Jul 21, 2023

Proposal: remove support for multi-valued RDNs in X.509

See #8193

See #5431 (comment) for a reference saying "That capability should not be used in a PKI context."

Supporting this significantly complicates a lot of code (and our data structure for handling this is not ideal, with the rather dirty next_merged field - see the discussion in #5431 for example), so if it's not needed according to the standard, let's remove that complexity from our code base!

@DemiMarie
Copy link
Contributor

@mpg I’m concerned that there will be certificates in the wild that do not follow the standard. Instead, I recommend that distinguished names be validated for syntactic correctness but otherwise be treated as opaque blobs. Anyone who really needs the information in a Subject or an Issuer can parse it themselves using mbedtls_asn1_traverse_sequence_of. TLS only needs bitwise equality checks.

@daverodgman
Copy link
Contributor

All these comments now have an independent issue - closing this wishlist.

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
Projects
None yet
Development

No branches or pull requests

4 participants