Don't use mbedtls_ct_hmac() for NULL cipher in mbedtls_ssl_encrypt_buf()

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.21k stars 2.55k forks source link

Don't use mbedtls_ct_hmac() for NULL cipher in mbedtls_ssl_encrypt_buf() #7588

Open cldhms opened 1 year ago

cldhms commented 1 year ago

Suggested enhancement

From my understanding the hmac constant time version only makes sense if the data is encrypted as it's supposed to prevent the length of the data from being leaked. But if a NULL cipher is used it makes no sense to run the constant time version as the length of the data is already known.

Justification

By calculating the hmac as fast as possible (in a non-constant time) the performance is drastically improved for resource constrained devices as the constant time version is very time consuming.

gilles-peskine-arm commented 1 year ago

Indeed that makes sense. mbedtls_ct_hmac is important, specifically, with CBC due to the way TLS does padding with CBC. We could use an ordinary implementation when HMAC is used with a non-padded encryption or non-padded encryption. In TLS, since we stopped supporting RC4, the only cases that use HMAC are CBC and the null cipher.

The null cipher is a low-priority use case for us, so we're unlikely to work on this, but I think we would accept patches.

gilles-peskine-arm commented 1 year ago

By the way, we're considering removing null cipher support in the next major release of Mbed TLS. Is this something you're planning to keep using in the long term (beyond 2025 or so)?

cldhms commented 1 year ago

We could provide a patch, but we are using the mbedtls-2.28 branch. Would you accept patches for that branch as well?

It's not really up to us how long we need support for null cipher. We are developing products in the industrial automation segment where one of the protocols used is EtherNet/IP. The EtherNet/IP specification currently requires devices to support TLS_ECDHE_ECDSA_WITH_NULL_SHA and there are no plans to remove this requirement. Confidentiality is typically not required in this environment and is also left out for performance reasons. An RFC was released last year which introduces null cipher in TLS1.3 RFC9150.

gilles-peskine-arm commented 1 year ago

2.28 is a long-time support branch so it has to stay very stable. We normally only do bug fixes.

That being said, since null ciphers are specifically useful in very low-end environments, you could make the case that having needlessly slow code is a bug. So we might accept a patch in 2.28, but it would come with conditions:

We need to discuss this further inside the maintainer team.
The patch would have to clearly not break anything, and not require any nontrivial internal refactoring. I haven't looked at this part of the code recently and I don't know if that's achievable.
We'd also want a patch with similar functionality for 3.x: from our perspective it doesn't make sense to improve an LTS branch beyond development.

gilles-peskine-arm commented 1 year ago

@mpg and I had a look at the code and it looks like this has a simple fix. The team is still undecided whether it's an acceptable risk in an LTS branch though.

cldhms commented 1 year ago

I tested the proposed fix and it drastically improved the performance for us. The unnecessary overhead in mbedtls_ct_hmac() is still somewhat significant for us but I understand if you don't want to make more changes than necessary.