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

DONTMERGE [common] Update ciphersuite used for securing pipe/UDS connections #1918

Closed
wants to merge 1 commit into from

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Jun 20, 2024

Description of the changes

Previously, Gramine-SGX used MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256 ciphersuite when creating TLS-secured pipes and Unix Domain Sockets. This ciphersuite is considered deprecated, thus we switch to MBEDTLS_TLS_DHE_PSK_WITH_AES_128_GCM_SHA256. This is recommended for perfect forward secrecy.

Unfortunately mbedTLS v3.6.0 doesn't provide an ECDHE PSK based ciphersuite that has AES-GCM, but only the one with AES-CBC. For the lack of options, we use the DHE PSK with AES-GCM.

DONTMERGE:

  • This hampers performance of applications that use pipes/UDSes a lot.

This is an alternative to #1911.

More info see also in Mbed-TLS/mbedtls#8170

How to test this PR?

CI is enough.


This change is Reviewable

…ections

Previously, Gramine-SGX used MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256
ciphersuite when creating TLS-secured pipes and Unix Domain Sockets.
This ciphersuite is considered deprecated, thus we switch to
MBEDTLS_TLS_DHE_PSK_WITH_AES_128_GCM_SHA256. This is recommended for
perfect forward secrecy.

Unfortunately mbedTLS v3.6.0 doesn't provide an ECDHE PSK based
ciphersuite that has AES-GCM, but only the one with AES-CBC. For the
lack of options, we use the DHE PSK with AES-GCM.

DONTMERGE:
- This hampers performance of applications that use pipes/UDSes a lot.

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "DONTMERGE" found in commit messages' one-liners

a discussion (no related file):
Functionality-wise everything works.

But performance-wise this PR significantly deteriorates performance:

$ gramineproject/gramine/libos/test/regression$ SGX=1 gramine-test pytest -v

With DHE (branch dimakuv/change-mbedtls-key-exchange-to-dhe): 355.70 sec
With RSA (current master): 291.19 sec

I'm really surprised by these numbers, since DHE is only involved during TLS handshake, and the rest is done using the same AES-GCM crypto. Is it really so that DHE handshake is that much slower than RSA?

I think with such performance overheads, this change is a no-no.


@SabaEskandarian
Copy link

I had a chance to talk to @donporter about this yesterday, so I thought I'd chime in.

I think the cost may be coming from the fact that you've switched from a cipher suite that uses only symmetric key encryption to one that uses public key encryption. TLS_PSK_WITH_AES_128_GCM_SHA256 just has the parties use a pre-shared key with no RSA or Diffie-Hellman key exchange involved. TLS_DHE_PSK_WITH_AES_128_GCM_SHA256 does all the things that were being done with just PSK but additionally does an ephemeral Diffie-Hellman key exchange to get forward secrecy and prevent dictionary attacks. Going from no public key crypto to using public key crypto is going to be costly -- wolfSSL reports a 2x slowdown using RSA vs just PSK.

I think the best performance you could hope for with forward secrecy would be ECDHE with AES_128_GCM, but this combination doesn't seem to be supported by mbedTLS. Maybe you could ask for it to be added? Elliptic curve diffie hellman gets the same security level with smaller field elements, so it saves on communication/computation costs versus DHE, and AES-GCM benefits from hardware acceleration and provides built-in ciphertext integrity, saving the cost of separately computing MACs on messages compared to AES-CBC (mentioned in the alternative proposal). Even if mbedTLS adds ECDHE with AES-GCM, there will still be a performance hit.

If you're in the process of changing TLS stuff, it might be worth thinking about moving to TLS 1.3, which drops support for older cipher suites and provides opportunities for faster connection setup (although paying something for forward secrecy remains unavoidable).

@mkow
Copy link
Member

mkow commented Jun 26, 2024

I think the best performance you could hope for with forward secrecy would be [...]

But I don't think we gain anything from forward secrecy? The symmetric keys we use are ephemeral anyways, and exchanged via hardware means. As far as I understand we just got forced into it, because mbedtls is dropping all the combinations without forward secrecy, right? So, if we want to request anything from them, then it would be to preserve some non-forward-secrecy PSK combination.

If you're in the process of changing TLS stuff, it might be worth thinking about moving to TLS 1.3 [...]

I think we're already on 1.3, see #1832.

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @SabaEskandarian very much for your inputs!

I think the cost may be coming from the fact that you've switched from a cipher suite that uses only symmetric key encryption to one that uses public key encryption. TLS_PSK_WITH_AES_128_GCM_SHA256 just has the parties use a pre-shared key with no RSA or Diffie-Hellman key exchange involved. TLS_DHE_PSK_WITH_AES_128_GCM_SHA256 does all the things that were being done with just PSK but additionally does an ephemeral Diffie-Hellman key exchange to get forward secrecy and prevent dictionary attacks.

Yes, thank you, that was my mistake. You're right that the TLS_PSK_WITH_AES_128_GCM_SHA256 ciphersuite (currently used in Gramine) doesn't use public key encryption.

Going from no public key crypto to using public key crypto is going to be costly -- wolfSSL reports a 2x slowdown using RSA vs just PSK.

Thanks for this link. I didn't expect such a huge slowdown. This may explain the overheads I observe in this PR.

I think the best performance you could hope for with forward secrecy would be ECDHE with AES_128_GCM, but this combination doesn't seem to be supported by mbedTLS. Maybe you could ask for it to be added? Elliptic curve diffie hellman gets the same security level with smaller field elements, so it saves on communication/computation costs versus DHE, and AES-GCM benefits from hardware acceleration and provides built-in ciphertext integrity, saving the cost of separately computing MACs on messages compared to AES-CBC (mentioned in the alternative proposal). Even if mbedTLS adds ECDHE with AES-GCM, there will still be a performance hit.

Thanks, I added your response to mbedTLS's discussion: Mbed-TLS/mbedtls#8170

But I don't think we gain anything from forward secrecy? The symmetric keys we use are ephemeral anyways, and exchanged via hardware means. As far as I understand we just got forced into it, because mbedtls is dropping all the combinations without forward secrecy, right? So, if we want to request anything from them, then it would be to preserve some non-forward-secrecy PSK combination.

Yes, I agree that our particular use case doesn't need additional forward secrecy. I think we could continue using TLS_PSK_WITH_AES_128_GCM_SHA256. (Again, not a crypto expert.) I'll add your comment to mbedTLS issue.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "DONTMERGE" found in commit messages' one-liners

@mpg
Copy link

mpg commented Jun 28, 2024

MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256 [...] is considered deprecated

I'm surprised. Can you point to the document that deprecates it?

As far as I understand we just got forced into it, because mbedtls is dropping all the combinations without forward secrecy, right?

Speaking as an Mbed TLS maintainer, I don't think we are! We are dropping the (plain) RSA and RSA-PSK key exchanges for several reasons, including reliance on PKCS#1 v1.5 encryption which is notoriously hard to implement securely (see the long series of attacks starting with Bleichenbacher's, the latest of which being MARVIN), being officially deprecated, and lacking forward secrecy despite incurring the cost of asymmetric crypto.

This last bit is important: it is well understood within the TLS community that some sometimes you can't afford the computational (and memory) costs of asymmetric crypto, or simply don't need any of the things it provides, and the PSK key exchange is there for that. I'll note that TLS 1.3, which removed a lot of old stuff (including RSA PKCS#1 v1.5, and RSA encryption entirely - only RSA-PSS signatures are kept), did keep a PSK key exchange mode, alongside Ephemeral-PSK and Ephemeral.

Mbed TLS implements all 3 keys exchange modes in TLS 1.3, including (non-ephemeral) PSK, and I don't think we have any intention of removing the pure-PSK key exchange in TLS 1.2 (as opposed to RSA-PSK which will be dropped due to the RSA part, and DHE-PSK dropped due to the DH part in 1.2 - but we'll keep FFDH in 1.3).

@dimakuv
Copy link
Contributor Author

dimakuv commented Jun 28, 2024

As explained by @mpg in #1918 (comment), there is no deprecation of the currently-used MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256. Closing this PR.

@dimakuv dimakuv closed this Jun 28, 2024
@dimakuv dimakuv deleted the dimakuv/change-mbedtls-key-exchange-to-dhe branch June 28, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants