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.

Sign up for GitHub

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

Jump to bottom

Limit supported TLS versions and cipher suites #4192

Merged
merged 1 commit into from
Oct 25, 2021
Merged

Limit supported TLS versions and cipher suites #4192

merged 1 commit into from
Oct 25, 2021

Conversation

yostyle
Copy link
Contributor

@yostyle yostyle commented Oct 8, 2021 •
edited
Loading

By default, OkHttp will attempt a MODERN_TLS connection : Documentation

This default configuration accepts some weak certificates :

        // Note that the following cipher suites are all on HTTP/2's bad cipher suites list. We'll
        // continue to include them until better suites are commonly available.
        CipherSuite.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
        CipherSuite.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
        CipherSuite.TLS_RSA_WITH_AES_128_GCM_SHA256,
        CipherSuite.TLS_RSA_WITH_AES_256_GCM_SHA384,
        CipherSuite.TLS_RSA_WITH_AES_128_CBC_SHA,
        CipherSuite.TLS_RSA_WITH_AES_256_CBC_SHA,
        CipherSuite.TLS_RSA_WITH_3DES_EDE_CBC_SHA

It's highly recommended to follow RFC7540 and not support cipher suites contained in this list. To do that it's better to use RESTRICTED_TLS connection.

It is always possible to change this parameter in MatrixConfiguration (Unauthenticated connection only) and/or HomeServerConnectionConfig

@yostyle yostyle changed the title Limit supported TLS and cipher suites Limit supported TLS versions and cipher suites Oct 8, 2021
@yostyle yostyle force-pushed the yostyle/cipher_suites branch from e04a1e7 to 155861a Compare October 8, 2021 16:10
@yostyle yostyle marked this pull request as ready for review October 8, 2021 16:10
@yostyle yostyle requested a review from bmarty October 8, 2021 16:11
@yostyle yostyle force-pushed the yostyle/cipher_suites branch from 155861a to cbcb620 Compare October 8, 2021 16:16
@github-actions
Copy link

github-actions bot commented Oct 8, 2021

Unit Test Results

  42 files  ±0    42 suites  ±0   54s ⏱️ +7s
  83 tests ±0    83 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
220 runs  ±0  220 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit cbcb620. ± Comparison against base commit 3a387c5.

@yostyle yostyle mentioned this pull request Oct 11, 2021
Closed
@ouchadam
Copy link
Contributor

does the current homeserver spec enforce one of these supported ciphers?

I'm wondering if this could cause some peoples client to stop working and if so, should we have some UI?

@yostyle
Copy link
Contributor Author

yostyle commented Oct 15, 2021 •
edited
Loading

does the current homeserver spec enforce one of these supported ciphers?

homeserver spec does not specify anything about this. I guess TLS/SSL configuration is not a part of Synapse. The problem is on client side. We should not authorise weak ciphers.

I'm wondering if this could cause some peoples client to stop working ?

Yes it's possible if a client tries to connect on homeserver that provides a weak cipher suite, but this is probably already the case on web browsers and iOS. By default they don't support blacklisted cipher suites.
Security team is agree to increase TLS security on Element android. That shouldn't be a problem.

and if so, should we have some UI?

Why not, I let @bmarty to decide about this, but I'm not sure that's easy to implement this.

bmarty
bmarty reviewed Oct 25, 2021
View reviewed changes
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

One question before I can merge this PR. Thanks!

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/ssl/CertUtil.kt
@@ -237,14 +235,14 @@ internal object CertUtil {
* @return a list of accepted TLS specifications.
*/
fun newConnectionSpecs(hsConfig: HomeServerConnectionConfig): List<ConnectionSpec> {
val builder = ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS)
val builder = ConnectionSpec.Builder(ConnectionSpec.RESTRICTED_TLS)
Copy link
Member

Choose a reason for hiding this comment

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

We should use the value of MatrixConfiguration.connectionSpec here, no?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, this is a base config, that we will update with the lines below.

@bmarty
Copy link
Member

bmarty commented Oct 25, 2021

Advertised in Element Android room : https://matrix.to/#/!AZozoWghOYSIAfaZjJ:matrix.org/$3VcH_2lglYiYScLktl19On7-_fCs4VMRlZAErg8qkdk?via=matrix.org&via=privacytools.io&via=tchncs.de

and Synapse Admins room: https://matrix.to/#/!ehXvUhWNASUkSLvAGP:matrix.org/$gZ1ntcegG0Xkv0cwueZEi6Q57-yPsUWpO2xau1-I9g4?via=matrix.org&via=matrix.breakpointingbad.com&via=libera.chat

@bmarty bmarty merged commit f2c22c1 into develop Oct 25, 2021
@bmarty bmarty deleted the yostyle/cipher_suites branch October 25, 2021 11:07
@bmarty
Copy link
Member

bmarty commented Oct 25, 2021

In Element Android (and SDK) 1.3.6

@ouchadam ouchadam mentioned this pull request Dec 24, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmarty bmarty bmarty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yostyle @ouchadam @bmarty