Don't send TLS_FALLBACK_SCSV if max version is >= 1.2 (#651)

TLS_FALLBACK_SCSV protects against downgrade attacks when clients
implement a version fallback independent of TLS version
negotiation, but if it's set on a non-fallback connection
attempt, it will prevent an otherwise-safe connection if the
server supports a version higher than the client does.  Because
the default OpenJDK TLS implementation doesn't support
TLS_FALLBACK_SCSV, some developers mistakenly enable it on every
connection due to thinking it's a normal cipher suite, which is
starting to cause issues when servers upgrade to TLS 1.3.

We can obviously omit it on connections with a max version of 1.3,
since that's Conscrypt's max version, so it can't be a version
fallback.

As far as connections with a max version of 1.2 are concerned, this
type of fallback is generally not needed any longer, since TLS
1.3-supporting servers should all perform version negotiation
properly.  (Chrome and Firefox have both disabled version fallback
entirely.)  Thus TLS_FALLBACK_SCSV's presence in connections with a
max version of 1.2 is significantly more likely to be a
misconfiguration than a true fallback indication.

We continue to include the cipher suite for connections with a max
version of 1.1 or lower.  First, flaws in pre-1.2 versions are more
likely to exist than flaws in 1.2, so the benefit of flagging
downgrades to those versions are higher.  As well, fallback is most
likely to be useful when dealing with buggy TLS 1.2 servers.

Fixes #574

common/src/main/java/org/conscrypt/NativeCrypto.java

@@ -968,8 +968,16 @@ static String[] getSupportedProtocols() {
         return SUPPORTED_PROTOCOLS.clone();
     }
 
-    static void setEnabledProtocols(long ssl, NativeSsl ssl_holder, String[] protocols) {
-        checkEnabledProtocols(protocols);
+    private static class Range {
+        public final String min;
+        public final String max;
+        public Range(String min, String max) {
+            this.min = min;
+            this.max = max;
+        }
+    }
+
+    private static Range getProtocolRange(String[] protocols) {
         // TLS protocol negotiation only allows a min and max version
         // to be set, despite the Java API allowing a sparse set of
         // protocols to be enabled.  Use the lowest contiguous range
@@ -992,7 +1000,14 @@ static void setEnabledProtocols(long ssl, NativeSsl ssl_holder, String[] protoco
         if ((min == null) || (max == null)) {
             throw new IllegalArgumentException("No protocols enabled.");
         }
-        SSL_set_protocol_versions(ssl, ssl_holder, getProtocolConstant(min), getProtocolConstant(max));
+        return new Range(min, max);
+    }
+
+    static void setEnabledProtocols(long ssl, NativeSsl ssl_holder, String[] protocols) {
+        checkEnabledProtocols(protocols);
+        Range range = getProtocolRange(protocols);
+        SSL_set_protocol_versions(
+            ssl, ssl_holder, getProtocolConstant(range.min), getProtocolConstant(range.max));
     }
 
     private static int getProtocolConstant(String protocol) {
@@ -1037,15 +1052,22 @@ static String[] checkEnabledProtocols(String[] protocols) {
      */
     static native long[] SSL_get_ciphers(long ssl, NativeSsl ssl_holder);
 
-    static void setEnabledCipherSuites(long ssl, NativeSsl ssl_holder, String[] cipherSuites) {
+    static void setEnabledCipherSuites(long ssl, NativeSsl ssl_holder, String[] cipherSuites,
+            String[] protocols) {
         checkEnabledCipherSuites(cipherSuites);
+        String maxProtocol = getProtocolRange(protocols).max;
         List<String> opensslSuites = new ArrayList<String>();
         for (int i = 0; i < cipherSuites.length; i++) {
             String cipherSuite = cipherSuites[i];
             if (cipherSuite.equals(TLS_EMPTY_RENEGOTIATION_INFO_SCSV)) {
                 continue;
             }
-            if (cipherSuite.equals(TLS_FALLBACK_SCSV)) {
+            // Only send TLS_FALLBACK_SCSV if max version >= 1.2 to prevent inadvertent connection
+            // problems when servers upgrade.  See https://github.com/google/conscrypt/issues/574
+            // for more discussion.
+            if (cipherSuite.equals(TLS_FALLBACK_SCSV)
+                    && (maxProtocol.equals(SUPPORTED_PROTOCOL_TLSV1)
+                        || maxProtocol.equals(SUPPORTED_PROTOCOL_TLSV1_1))) {
                 SSL_set_mode(ssl, ssl_holder, NativeConstants.SSL_MODE_SEND_FALLBACK_SCSV);
                 continue;
             }

common/src/main/java/org/conscrypt/NativeSsl.java

@@ -312,7 +312,8 @@ void initialize(String hostname, OpenSSLKey channelIdPrivateKey) throws IOExcept
                     + " is no longer supported and was filtered from the list");
         }
         NativeCrypto.setEnabledProtocols(ssl, this, parameters.enabledProtocols);
-        NativeCrypto.setEnabledCipherSuites(ssl, this, parameters.enabledCipherSuites);
+        NativeCrypto.setEnabledCipherSuites(
+            ssl, this, parameters.enabledCipherSuites, parameters.enabledProtocols);
 
         if (parameters.applicationProtocols.length > 0) {
             NativeCrypto.setApplicationProtocols(ssl, this, isClient(), parameters.applicationProtocols);

openjdk-integ-tests/src/test/java/org/conscrypt/javax/net/ssl/SSLSocketTest.java

@@ -74,6 +74,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import tests.net.DelegatingSSLSocketFactory;
 import tests.util.ForEachRunner;
 import tests.util.ForEachRunner.Callback;
 import tests.util.Pair;
@@ -946,6 +947,29 @@ public Void call() throws Exception {
         context.close();
     }
 
+    @Test
+    public void test_SSLSocket_tlsFallback_byVersion() throws Exception {
+        for (final String protocol : new String[] { "TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3" }) {
+            SSLSocketFactory factory = new DelegatingSSLSocketFactory((SSLSocketFactory) SSLSocketFactory.getDefault()) {
+                @Override protected SSLSocket configureSocket(SSLSocket socket) {
+                    socket.setEnabledProtocols(new String[] {protocol});
+                    String[] enabled = socket.getEnabledCipherSuites();
+                    String[] cipherSuites = new String[socket.getEnabledCipherSuites().length + 1];
+                    System.arraycopy(enabled, 0, cipherSuites, 0, enabled.length);
+                    cipherSuites[cipherSuites.length - 1] = StandardNames.CIPHER_SUITE_FALLBACK;
+                    socket.setEnabledCipherSuites(cipherSuites);
+                    return socket;
+                }
+            };
+            ClientHello clientHello = TlsTester.captureTlsHandshakeClientHello(executor, factory);
+            if (protocol.equals("TLSv1.2") || protocol.equals("TLSv1.3")) {
+                assertFalse(clientHello.cipherSuites.contains(CipherSuite.valueOf("TLS_FALLBACK_SCSV")));
+            } else {
+                assertTrue(clientHello.cipherSuites.contains(CipherSuite.valueOf("TLS_FALLBACK_SCSV")));
+            }
+        }
+    }
+
     private <T> Future<T> runAsync(Callable<T> callable) {
         return executor.submit(callable);
     }

openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java

@@ -696,8 +696,9 @@ public long beforeHandshake(long context) throws SSLException {
             } else {
                 cipherSuites.addAll(enabledCipherSuites);
             }
+            // Protocol list is included for determining whether to send TLS_FALLBACK_SCSV
             NativeCrypto.setEnabledCipherSuites(
-                    s, null, cipherSuites.toArray(new String[cipherSuites.size()]));
+                    s, null, cipherSuites.toArray(new String[cipherSuites.size()]), new String[] {"TLSv1.2"});
 
             if (channelIdPrivateKey != null) {
                 NativeCrypto.SSL_set1_tls_channel_id(s, null, channelIdPrivateKey.getNativeRef());