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

Request to provide an API to set the TLS Cipher Suit List #54901

Open
sergio-a-martinez-mdt opened this issue Feb 12, 2024 · 23 comments
Open

Request to provide an API to set the TLS Cipher Suit List #54901

sergio-a-martinez-mdt opened this issue Feb 12, 2024 · 23 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-io type-enhancement A request for a change that isn't a bug

Comments

@sergio-a-martinez-mdt
Copy link

Hello dart team,

the team I am working with and myself are looking for a way to set a very specific TLS Cipher Suit to the Dart HTTPClient and SecurityContext.

By looking at this file https://github.com/dart-lang/sdk/blob/main/runtime/bin/security_context.cc, it seems that the TLS version and CipherSuite is set and cannot be configured:

  SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION);
  SSL_CTX_set_cipher_list(ctx, "HIGH:MEDIUM");

Having this functionality is critical to our app to leave out CipherSuit that are deemed unsafe, and/or set only strong ones.

Can this functionality be exposed in Dart via the SecurityContext?

@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io type-enhancement A request for a change that isn't a bug labels Feb 14, 2024
@brianquinlan
Copy link
Contributor

@brianquinlan
Copy link
Contributor

This would be symmetrical with the Python SSLContext.set_ciphers method.

@brianquinlan brianquinlan self-assigned this Feb 15, 2024
@brianquinlan brianquinlan added P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team and removed P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team labels Feb 15, 2024
@brianquinlan
Copy link
Contributor

Hey @sergio-a-martinez-mdt,

Can you explain your use case a bit more? Do you control the server that you are communicating with? If so, can you enforce the cipher suite there?

The documentation for BoringSSL says:

Cipher lists apply only to TLS 1.0 through TLS 1.2
In TLS 1.3 there are only three named ciphersuites: AEAD-CHACHA20-POLY1305, AEAD-AES128-GCM-SHA256, and AEAD-AES256-GCM-SHA384. Thus, TLS 1.3 ciphersuites are completely disjoint from those in TLS 1.0 through 1.2.

This is different from TLS 1.2, which supported ciphersuites from earlier versions by name. You'll notice also that these ciphersuites do not include the key exchange, which is to say that a TLS 1.3 ciphersuite specifies less behavior than a TLS 1.2 ciphersuite specifies.

Hence, a traditional string ciphersuite list will affect only TLS versions 1.0 through 1.2. Our TLS 1.3 implementation will enable all three ciphersuites. We will not expose APIs to disable or reorder them until there is a specific need to do so. If/when we do that, it will probably not look like string list of named ciphersuites.

Most sites support TLS 1.3 so I'm not sure that it makes sense to support a configuration option that only works with a legacy TLS version. Would allowing you to control the minimum TLS version be sufficient?

@brianquinlan brianquinlan added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 16, 2024
@sergio-a-martinez-mdt
Copy link
Author

Hi @brianquinlan , of course. Thanks for following up on this.

Having the ability to set the minimum TLS version to 1.3 would be a great start.

As per setting the specific ciphers, I can think on the following reasons for having the ability to set the ciphers:

  • We need to have that control on the app side. Even though this may be configured in the server side (when the server is controlled by us), enforcing it on the app side, adds an extra defense mechanism. For example, if for any reason the server gets misconfigured and starts accepting weak ciphers, the app will reject such requests.
  • Setting strong Ciphers have another defense mechanism, say, If the app has a MITM attack and the attacker tries to use a weak cipher. (Granted, this is not the only mechanism to protect against MITM attacks, but it adds a defense layer ).
  • If well is true that most sites support TLS 1.3, some still do not. And TLS 1.2 supports some ciphers that we would like to avoid. For example TLS_RSA_WITH_NULL_SHA256, or TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256

For TLS 1.3 aren't these supported and cannot be set?

TLS_AES_128_GCM_SHA256                     TLS_AES_128_GCM_SHA256
TLS_AES_256_GCM_SHA384                     TLS_AES_256_GCM_SHA384
TLS_CHACHA20_POLY1305_SHA256               TLS_CHACHA20_POLY1305_SHA256
TLS_AES_128_CCM_SHA256                     TLS_AES_128_CCM_SHA256
TLS_AES_128_CCM_8_SHA256                   TLS_AES_128_CCM_8_SHA256

Reference: https://www.openssl.org/docs/manmaster/man1/openssl-ciphers.html section TLS v1.3 cipher suites

If so, for TLS 1.3, if any of those become busted, it would be advised to be able to allow only the strong ones.

Hope this helps

@github-actions github-actions bot removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 16, 2024
@brianquinlan
Copy link
Contributor

Hey @sergio-a-martinez-mdt ,

I still don't understand your use case. For your particular use case, do you control the server?

We use BoringSSL, which only supports three TLS 1.3 ciphers (AEAD-CHACHA20-POLY1305, AEAD-AES128-GCM-SHA256, and AEAD-AES256-GCM-SHA384). It also don't have an API to configure TLS 1.3 cipers, which OpenSSL does.

BoringSSL also doesn't promise to keep ciphers that they consider bad (they removed RC4 and DHE entirely) so, if we supported setting the cipher list (which would only work for TLS 1.2), upgrading BoringSSL could break applications.

Setting the minimum SSL version is something that BoringSSL supports and they consider acceptable practice. For their docs:

If you must specify versions, they should be clearly documented to mean "TLS X or higher." It is reasonable to specify a minimum version, but do not have knobs that are documented to mean "version X exactly".

@brianquinlan brianquinlan added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 16, 2024
@sergio-a-martinez-mdt
Copy link
Author

Hi @brianquinlan,

Thanks for providing the TLS 1.3 supported Ciphers. Would you mind sharing or pointing me to the supported ciphers for TLS 1.2?

Going to your question, technically, my team does not control it. It's handle by a different team. Although, what I am trying to achieve should be independent of the server configuration. What we're trying to achieve is to set the control on the application side, as a defense mechanism as described above.

You bring up a good point about breaking the application with unsupported ciphers. This brings up a couple of questions:

  • How is BoringSSL updated? Is it embedded in Dart?
  • Is BoringSSL the implementation for both, Android and iOS?

@github-actions github-actions bot removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 20, 2024
@brianquinlan
Copy link
Contributor

I think that the list of ciphers is found in the source here: https://boringssl.googlesource.com/boringssl/+/refs/heads/master/ssl/ssl_cipher.cc

  • How is BoringSSL updated? Is it embedded in Dart?

We periodically sync BoringSSL through a configuration-time dependency. It is embedded in Dart.

  • Is BoringSSL the implementation for both, Android and iOS?

Yes, it is implemented for all platforms that have access to dart:io.

@sergio-a-martinez-mdt
Copy link
Author

hi @brianquinlan ,

Thanks for sharing the ssl_cipher.cc file link.

This is great. Going back to providing a cipher suit not supported, this gives us the ability to re-test and re-evaluate the cipher suit list currently in used for every Flutter/Dart upgrade.

We periodically sync BoringSSL through a configuration-time dependency. It is embedded in Dart.

If setting the cipher suit for TLS 1.2 is currently supported in BoringSSL, would it be possible to expose setting the cipher suit only for TLS 1.2?

@prasad-pappu
Copy link

prasad-pappu commented Feb 21, 2024

@brianquinlan How to enforce the strong ciphers and TLS version from client side. The requirement is that client should not connect to a server which is not supporting TLS 1.3. We need the ability to set SSL_CTX_set_min_proto_version to TLS1_3_VERSION and SSL_CTX_set_cipher_list as HIGH:MEDIUM. Currently security context is not having the setter method to set those attributes. Looks like these two attributes are hardcoded in security_context.

You may like to consider default value as TLS 1.2, but giving an option to configure the security context based on the application needs. By detault it can have TLS1_2_VERSION and MEDIUM, but if any application wants TLS1_3_VERSION and SSL_CTX_set_cipher_list as HIGH it should be able to set.

Articulating the usecase: Client should use only TLS1_3_VERSION and SSL_CTX_set_cipher_list as HIGH. If the server does not support, it should fail and should not connect to that server.

@brianquinlan
Copy link
Contributor

If we make it possible to set the minimum TLS version then SSL_CTX_set_cipher_list would not be necessary, right? Because that function does not affect the TLS 1.3 cipher list.

@brianquinlan brianquinlan added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 22, 2024
@prasad-pappu
Copy link

prasad-pappu commented Feb 23, 2024

@brianquinlan
Thank you so much. To start with if dart allows minimum TLS Version, and allows SSL_CTX_set_cipher_list then it serves our purpose, so that it gives the ability of the client to set TLS version to 1.3 and cipher list to HIGH. This would be helpful. Because want to use SSL_CTX_set_cipher_list HIGH.

@github-actions github-actions bot removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 23, 2024
@brianquinlan
Copy link
Contributor

@sergio-a-martinez-mdt Would controlling the minimum TLS version be sufficient for you?

@brianquinlan brianquinlan added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 26, 2024
@sergio-a-martinez-mdt
Copy link
Author

hi @brianquinlan , having the ability the TLS version will be a great start. It would be unfortunately not enough since the app needs to set a specific set of string ciphers for TLS 1.2

@brianquinlan
Copy link
Contributor

brianquinlan commented Feb 26, 2024

Could you just not support TLS 1.2?

@sergio-a-martinez-mdt
Copy link
Author

That decision would be outside of the scope of this thread.
It appears there is no appetite to expose the Ciphers for TLS 1.2. Would adding this be a big challenge? what are the cons you see?

@brianquinlan
Copy link
Contributor

That decision would be outside of the scope of this thread.

I think that we are trying to work towards a solution that would be acceptable to you, so it would be helpful to know whether that solution would work.

It appears there is no appetite to expose the Ciphers for TLS 1.2. Would adding this be a big challenge?

In terms of development effort, I don't think that it would be significantly more difficult than supporting the minimum TLS version.

what are the cons you see?

  1. As I said before, the BoringSSL team does not promise to support any particular TLS 1.2 ciphers. So we could no longer upgrade BoringSSL without checking to see if they removed a cipher. If they did remove a cipher then we would be in the position of either not upgrading BoringSSL anymore (security risk), breaking our users, or re-adding the cipher to our own fork of BoringSSL (way too much effort)
  2. We open up the possibility that developers unintentionally reduce their security.

@prasad-pappu
Copy link

@brianquinlan At least allowing setting the TLS version would hep us.

By detault it can have TLS1_2_VERSION, but if any application wants to set minimum TLS Version for example to TLS1_3_VERSION, if that flexibility is given, at least it would serve the purpose.

@github-actions github-actions bot removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Feb 27, 2024
@sergio-a-martinez-mdt
Copy link
Author

hi @brianquinlan , I'm circling back on this request. I wanted to check/confirm if Dart team decided to support setting the TLS version?

@lrhn lrhn added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. and removed area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Apr 21, 2024
@brianquinlan
Copy link
Contributor

hi @brianquinlan , I'm circling back on this request. I wanted to check/confirm if Dart team decided to support setting the TLS version?

Hi @sergio-a-martinez-mdt

We haven't made an decisions yet. Adding the ability to set the minimum TLS version would be a breaking change so I'd prefer to only do that if I'm sure that it will satisfy your use case.

So can you confirm that it would be sufficient to control the minimum TLS version?

@brianquinlan brianquinlan added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Apr 24, 2024
@sergio-a-martinez-mdt
Copy link
Author

hi @brianquinlan , Yes, we have discussed this internally and this will satisfy our requirements! Thank you!

@github-actions github-actions bot removed the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Apr 25, 2024
@brianquinlan
Copy link
Contributor

OK, I'm going to work on a proof-of-concept and then there is some process around breaking changes (which this will be).

@sergio-a-martinez-mdt
Copy link
Author

@brianquinlan that sounds great. Please let me know if I can help test any POC before is released. Thanks

@brianquinlan
Copy link
Contributor

The public API that I'm imagining is this: https://dart-review.googlesource.com/c/sdk/+/365664/3/sdk/lib/io/security_context.dart

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-io type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants