Correctly filter out TLSv1.3 ciphers if TLSv1.3 is not enabled #10919

normanmaurer · 2021-01-11T09:52:34Z

Motivation:

We didnt correctly filter out TLSv1.3 ciphers when TLSv1.3 is not enabled.

Modifications:

Filter out ciphers that are not supported due the selected TLS version
Add unit test

Result:

Co-authored-by: Bryce Anderson banderson@twitter.com

Motivation: We didnt correctly filter out TLSv1.3 ciphers when TLSv1.3 is not enabled. Modifications: - Filter out ciphers that are not supported due the selected TLS version - Add unit test Result: Fixes #10911 Co-authored-by: Bryce Anderson <banderson@twitter.com>

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

normanmaurer · 2021-01-15T09:41:58Z

@bryce-anderson PTAL again

bryce-anderson · 2021-01-19T18:06:53Z

For some reason when I pull this and run the tests, I get failures to throw the expected expected SSLHandshakeException in testIncompatibleCiphers.

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

bryce-anderson · 2021-01-19T22:11:49Z

For some reason when I pull this and run the tests, I get failures to throw the expected expected SSLHandshakeException in testIncompatibleCiphers.

It looks like I'm getting TLSv1.1 with the TLS_RSA_WITH_AES_128_CBC_SHA suite negotiated. It seems that the client, since it had no TLSv1.(not 3) ciphers set enabled all of the tlsv1.(not 3) suites.

normanmaurer · 2021-01-21T13:26:41Z

@bryce-anderson you have a unit test that shows this ?

bryce-anderson · 2021-01-21T15:27:31Z

@bryce-anderson you have a unit test that shows this ?

@normanmaurer, yes, see here: bryce-anderson@117607c

normanmaurer · 2021-01-22T07:20:09Z

@bryce-anderson what java version / netty-tcnative version ? Also is this with BoringSSL or not ?

bryce-anderson · 2021-01-22T16:14:19Z

@bryce-anderson what java version / netty-tcnative version ? Also is this with BoringSSL or not ?

I have used both what is in the maven project description by default and switch to boringssl-static

io.netty:netty-tcnative:jar:osx-x86_64:2.0.35.Final
// or
io.netty:netty-tcnative-boringssl-static:jar:osx-x86_64:2.0.35.Final:compile

and they give the same result. Same with jdk8 and jdk11:

openjdk version "11.0.7.0.5" 2020-04-14
// or
openjdk version "1.8.0_242"

Fwiw, this is the behavior I'd expect: we didn't implicitly disable the TLSv1.(x<3) ciphers if the set is empty which I believe then defaults to all ciphers being available.
If you apply this patch I get the tests to pass but I'm not sure it's correct and maybe we need to disable SSLv[2|3] as well:

--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -1549,7 +1549,12 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
             if (!isDestroyed()) {
                 try {
                     // Set non TLSv1.3 ciphers.
-                    SSL.setCipherSuites(ssl, cipherSuiteSpec, false);
+                    if (cipherSuiteSpec.isEmpty()) {
+                        SSL.setOptions(ssl, SSL.getOptions(ssl) | SSL.SSL_OP_NO_TLSv1_2 |
+                            SSL.SSL_OP_NO_TLSv1_1 | SSL.SSL_OP_NO_TLSv1);
+                    } else {
+                        SSL.setCipherSuites(ssl, cipherSuiteSpec, false);
+                    }

                     if (OpenSsl.isTlsv13Supported()) {
                         if (cipherSuiteSpecTLSv13.isEmpty()) {

normanmaurer · 2021-01-22T16:22:47Z

super strange the it passes here without your change... hmmm.

bryce-anderson · 2021-01-22T17:37:23Z

super strange the it passes here without your change... hmmm.

Agreed.

bryce-anderson · 2021-01-26T22:54:11Z

@normanmaurer, let me know if there is anything you need from me on this.

normanmaurer · 2021-01-27T07:22:21Z

@bryce-anderson will do... I just didn't have time yet to check

normanmaurer · 2021-01-27T11:08:11Z

@bryce-anderson PTAL and verify :)

bryce-anderson

Thank you @normanmaurer!

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

normanmaurer · 2021-01-27T16:57:39Z

@bryce-anderson so all work now on your side ?

bryce-anderson · 2021-01-27T17:02:16Z

@normanmaurer, I haven't deployed and tested it but as far as I can tell it should fix our problem. If something else comes up I'll file another ticket.

normanmaurer · 2021-01-28T06:32:44Z

@slandelle @chrisvest thoughts ?

Motivation: We didnt correctly filter out TLSv1.3 ciphers when TLSv1.3 is not enabled. Modifications: - Filter out ciphers that are not supported due the selected TLS version - Add unit test Result: Fixes #10911 Co-authored-by: Bryce Anderson <banderson@twitter.com>

idelpivnitskiy

Change LGTM, one nit related to intermediate collections size, but I'm not sure if it really matters because it's not on the hot path:

idelpivnitskiy · 2021-01-28T18:58:19Z

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

                    if (OpenSsl.isTlsv13Supported()) {
                        // Set TLSv1.3 ciphers.
                        SSL.setCipherSuites(ssl, cipherSuiteSpecTLSv13, true);
                    }

+                    // We also need to update the enabled protocols to ensure we disable the protocol if there are
+                    // no compatible ciphers left.
+                    Set<String> protocols = new HashSet<String>(explicitlyEnabledProtocols.length);


The default load factor for HashSet is 0.75. Using explicitlyEnabledProtocols.length will cause resize of the internal hash table when the next line adds all explicitlyEnabledProtocols.
The following formula should be used: (int) (c.size()/.75f) + 1. Or use new HasSet(Arrays.asList(explicitlyEnabledProtocols)).

The similar situation may happen for line 1522:

Set<String> enabledSet = new LinkedHashSet<String>(enabled.length + extraCiphers.length);

If all ciphers will be added, it will cause resize of the internal hash table.

…#10919) Motivation: We didnt correctly filter out TLSv1.3 ciphers when TLSv1.3 is not enabled. Modifications: - Filter out ciphers that are not supported due the selected TLS version - Add unit test Result: Fixes netty#10911 Co-authored-by: Bryce Anderson <banderson@twitter.com>

normanmaurer mentioned this pull request Jan 11, 2021

Broken SslEngine [set|get]EnabledCipherSuites behavior #10911

Closed

normanmaurer requested a review from bryce-anderson January 11, 2021 09:53

normanmaurer added this to the 4.1.57.Final milestone Jan 11, 2021

normanmaurer force-pushed the ciphers branch from a77538a to e8abd64 Compare January 11, 2021 14:26

bryce-anderson reviewed Jan 11, 2021

View reviewed changes

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java Show resolved Hide resolved

normanmaurer modified the milestones: 4.1.57.Final, 4.1.58.Final, 4.1.59.Final Jan 13, 2021

Disable TLSv1.3 if no ciphers are left

9b6749a

bryce-anderson reviewed Jan 19, 2021

View reviewed changes

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java Show resolved Hide resolved

ensure we explicit disable protocols when ciphers are not compatible

7bb82d2

normanmaurer added 3 commits January 27, 2021 12:27

Improve test

3365f89

fix

3a1bdb3

fix

a448b4a

bryce-anderson approved these changes Jan 27, 2021

View reviewed changes

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java Outdated Show resolved Hide resolved

Update variable name

d4086ab

normanmaurer requested review from idelpivnitskiy, chrisvest and slandelle January 27, 2021 17:45

chrisvest approved these changes Jan 28, 2021

View reviewed changes

normanmaurer merged commit df10dfb into 4.1 Jan 28, 2021

normanmaurer deleted the ciphers branch January 28, 2021 10:00

idelpivnitskiy reviewed Jan 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly filter out TLSv1.3 ciphers if TLSv1.3 is not enabled #10919

Correctly filter out TLSv1.3 ciphers if TLSv1.3 is not enabled #10919

normanmaurer commented Jan 11, 2021

normanmaurer commented Jan 15, 2021

bryce-anderson commented Jan 19, 2021

bryce-anderson commented Jan 19, 2021

normanmaurer commented Jan 21, 2021

bryce-anderson commented Jan 21, 2021 •

edited

Loading

normanmaurer commented Jan 22, 2021

bryce-anderson commented Jan 22, 2021

normanmaurer commented Jan 22, 2021

bryce-anderson commented Jan 22, 2021

bryce-anderson commented Jan 26, 2021

normanmaurer commented Jan 27, 2021

normanmaurer commented Jan 27, 2021

bryce-anderson left a comment

normanmaurer commented Jan 27, 2021

bryce-anderson commented Jan 27, 2021

normanmaurer commented Jan 28, 2021

idelpivnitskiy left a comment

idelpivnitskiy Jan 28, 2021

Correctly filter out TLSv1.3 ciphers if TLSv1.3 is not enabled #10919

Correctly filter out TLSv1.3 ciphers if TLSv1.3 is not enabled #10919

Conversation

normanmaurer commented Jan 11, 2021

normanmaurer commented Jan 15, 2021

bryce-anderson commented Jan 19, 2021

bryce-anderson commented Jan 19, 2021

normanmaurer commented Jan 21, 2021

bryce-anderson commented Jan 21, 2021 • edited Loading

normanmaurer commented Jan 22, 2021

bryce-anderson commented Jan 22, 2021

normanmaurer commented Jan 22, 2021

bryce-anderson commented Jan 22, 2021

bryce-anderson commented Jan 26, 2021

normanmaurer commented Jan 27, 2021

normanmaurer commented Jan 27, 2021

bryce-anderson left a comment

Choose a reason for hiding this comment

normanmaurer commented Jan 27, 2021

bryce-anderson commented Jan 27, 2021

normanmaurer commented Jan 28, 2021

idelpivnitskiy left a comment

Choose a reason for hiding this comment

idelpivnitskiy Jan 28, 2021

Choose a reason for hiding this comment

bryce-anderson commented Jan 21, 2021 •

edited

Loading