implement ECH Retry Config handling

eighthave · eighthave · commit 18baf53dcdc9 · 2021-11-11T15:51:30.000+01:00
This introduces a new Exception so that clients can respond only to
the ECH retry request without having to parse SSLHandshakeExceptions
in general.  This exception should probably be implemented in
boringssl or native_crypto.cc.
diff --git a/common/src/jni/main/cpp/conscrypt/native_crypto.cc b/common/src/jni/main/cpp/conscrypt/native_crypto.cc
@@ -10474,6 +10474,50 @@ static jboolean NativeCrypto_SSL_set1_ech_config_list(JNIEnv* env, jclass, jlong
     return !!ret;
 }
 
+static jstring NativeCrypto_SSL_get0_ech_name_override(JNIEnv* env, jclass, jlong ssl_address,
+                                                       CONSCRYPT_UNUSED jobject ssl_holder) {
+    CHECK_ERROR_QUEUE_ON_RETURN;
+    SSL* ssl = to_SSL(env, ssl_address, true);
+    JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_name_override()", ssl);
+    if (ssl == nullptr) {
+        JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_name_override() => nullptr", ssl);
+        return nullptr;
+    }
+    const char* ech_name_override;
+    size_t ech_name_override_len;
+    SSL_get0_ech_name_override(ssl, &ech_name_override, &ech_name_override_len);
+    if (ech_name_override_len > 0) {
+        jstring name = env->NewStringUTF(ech_name_override);
+        return name;
+    }
+    return nullptr;
+}
+
+static jbyteArray NativeCrypto_SSL_get0_ech_retry_configs(JNIEnv* env, jclass, jlong ssl_address,
+                                                          CONSCRYPT_UNUSED jobject ssl_holder) {
+    CHECK_ERROR_QUEUE_ON_RETURN;
+    SSL* ssl = to_SSL(env, ssl_address, true);
+    JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_retry_configs()", ssl);
+    if (ssl == nullptr) {
+        return nullptr;
+    }
+
+    const uint8_t *retry_configs;
+    size_t retry_configs_len;
+    SSL_get0_ech_retry_configs(ssl, &retry_configs, &retry_configs_len);
+
+    jbyteArray result = env->NewByteArray(static_cast<jsize>(retry_configs_len));
+    if (result == nullptr) {
+        JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_retry_configs() => creating byte array failed",
+                  ssl);
+        return nullptr;
+    }
+    env->SetByteArrayRegion(result, 0, static_cast<jsize>(retry_configs_len),
+                            (const jbyte*) retry_configs);
+    JNI_TRACE("ssl=%p NativeCrypto_SSL_get0_ech_retry_configs(%p) => %p", ssl, ssl, result);
+    return result;
+}
+
 /**
  * public static native long SSL_ech_accepted(long ssl);
  */
@@ -10851,6 +10895,8 @@ static JNINativeMethod sNativeCryptoMethods[] = {
         CONSCRYPT_NATIVE_METHOD(usesBoringSsl_FIPS_mode, "()Z"),
         CONSCRYPT_NATIVE_METHOD(SSL_set_enable_ech_grease, "(J" REF_SSL "Z)V"),
         CONSCRYPT_NATIVE_METHOD(SSL_set1_ech_config_list, "(J" REF_SSL "[B)Z"),
+        CONSCRYPT_NATIVE_METHOD(SSL_get0_ech_name_override, "(J" REF_SSL ")Ljava/lang/String;"),
+        CONSCRYPT_NATIVE_METHOD(SSL_get0_ech_retry_configs, "(J" REF_SSL ")[B"),
         CONSCRYPT_NATIVE_METHOD(SSL_ech_accepted, "(J" REF_SSL ")Z"),
         CONSCRYPT_NATIVE_METHOD(SSL_CTX_ech_enable_server, "(J" REF_SSL_CTX "[B[B)Z"),
 
diff --git a/common/src/main/java/org/conscrypt/AbstractConscryptEngine.java b/common/src/main/java/org/conscrypt/AbstractConscryptEngine.java
@@ -100,6 +100,10 @@ abstract class AbstractConscryptEngine extends SSLEngine {
 
     public abstract byte[] getEchConfigList();
 
+    public abstract String getEchNameOverride();
+
+    public abstract byte[] getEchRetryConfig();
+
     public abstract boolean echAccepted();
 
     /* @Override */
diff --git a/common/src/main/java/org/conscrypt/AbstractConscryptSocket.java b/common/src/main/java/org/conscrypt/AbstractConscryptSocket.java
@@ -761,5 +761,9 @@ abstract byte[] exportKeyingMaterial(String label, byte[] context, int length)
 
     public abstract byte[] getEchConfigList();
 
+    public abstract String getEchNameOverride();
+
+    public abstract byte[] getEchRetryConfig();
+
     public abstract boolean echAccepted();
 }
diff --git a/common/src/main/java/org/conscrypt/Conscrypt.java b/common/src/main/java/org/conscrypt/Conscrypt.java
@@ -510,6 +510,14 @@ public static byte[] getEchConfigList(SSLSocket socket) {
         return toConscrypt(socket).getEchConfigList();
     }
 
+    public static String getEchNameOverride(SSLSocket socket) {
+        return toConscrypt(socket).getEchNameOverride();
+    }
+
+    public static byte[] getEchRetryConfig(SSLSocket socket) {
+        return toConscrypt(socket).getEchConfigList();
+    }
+
     public static boolean echAccepted(SSLSocket socket) {
         return toConscrypt(socket).echAccepted();
     }
diff --git a/common/src/main/java/org/conscrypt/ConscryptEngine.java b/common/src/main/java/org/conscrypt/ConscryptEngine.java
@@ -415,6 +415,16 @@ public byte[] getEchConfigList() {
         return sslParameters.getEchConfigList();
     }
 
+    @Override
+    public String getEchNameOverride() {
+        return ssl.getEchNameOverride();
+    }
+
+    @Override
+    public byte[] getEchRetryConfig() {
+        return ssl.getEchRetryConfig();
+    }
+
     @Override
     public boolean echAccepted() {
         return ssl.echAccepted();
diff --git a/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java b/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java
@@ -420,6 +420,16 @@ public byte[] getEchConfigList() {
         return engine.getEchConfigList();
     }
 
+    @Override
+    public String getEchNameOverride() {
+        return engine.getEchNameOverride();
+    }
+
+    @Override
+    public byte[] getEchRetryConfig() {
+        return engine.getEchRetryConfig();
+    }
+
     @Override
     public boolean echAccepted() {
         return engine.echAccepted();
diff --git a/common/src/main/java/org/conscrypt/ConscryptFileDescriptorSocket.java b/common/src/main/java/org/conscrypt/ConscryptFileDescriptorSocket.java
@@ -931,6 +931,16 @@ public byte[] getEchConfigList() {
         return sslParameters.getEchConfigList();
     }
 
+    @Override
+    public String getEchNameOverride() {
+        return ssl.getEchNameOverride();
+    }
+
+    @Override
+    public byte[] getEchRetryConfig() {
+        return ssl.getEchRetryConfig();
+    }
+
     @Override
     public boolean echAccepted() {
         return ssl.echAccepted();
diff --git a/common/src/main/java/org/conscrypt/EchRejectedException.java b/common/src/main/java/org/conscrypt/EchRejectedException.java
@@ -0,0 +1,18 @@
+package org.conscrypt;
+
+import javax.net.ssl.SSLHandshakeException;
+
+/**
+ * The server rejected the ECH Config List, and might have supplied an ECH
+ * Retry Config.
+ * 
+ * @see NativeCrypto#SSL_get0_ech_retry_configs(long, NativeSsl)
+ */
+public class EchRejectedException extends SSLHandshakeException {
+    private static final long serialVersionUID = 98723498273473923L;
+
+    EchRejectedException(String message) {
+        super(message);
+    }
+}
+
diff --git a/common/src/main/java/org/conscrypt/Java8EngineWrapper.java b/common/src/main/java/org/conscrypt/Java8EngineWrapper.java
@@ -136,6 +136,16 @@ public byte[] getEchConfigList() {
         return delegate.getEchConfigList();
     }
 
+    @Override
+    public String getEchNameOverride() {
+        return delegate.getEchNameOverride();
+    }
+
+    @Override
+    public byte[] getEchRetryConfig() {
+        return delegate.getEchRetryConfig();
+    }
+
     @Override
     public boolean echAccepted() {
         return delegate.echAccepted();
diff --git a/common/src/main/java/org/conscrypt/NativeCrypto.java b/common/src/main/java/org/conscrypt/NativeCrypto.java
@@ -1454,6 +1454,10 @@ static native void ENGINE_SSL_shutdown(long ssl, NativeSsl ssl_holder, SSLHandsh
 
     static native boolean SSL_set1_ech_config_list(long ssl, NativeSsl ssl_holder, byte[] echConfig);
 
+    static native String SSL_get0_ech_name_override(long ssl, NativeSsl ssl_holder);
+
+    static native byte[] SSL_get0_ech_retry_configs(long ssl, NativeSsl ssl_holder);
+
     static native boolean SSL_ech_accepted(long ssl, NativeSsl ssl_holder);
 
     static native boolean SSL_CTX_ech_enable_server(long sslCtx, AbstractSessionContext holder,
diff --git a/common/src/main/java/org/conscrypt/NativeSsl.java b/common/src/main/java/org/conscrypt/NativeSsl.java
@@ -279,6 +279,14 @@ byte[] getTlsChannelId() throws SSLException {
         return NativeCrypto.SSL_get_tls_channel_id(ssl, this);
     }
 
+    String getEchNameOverride() {
+        return NativeCrypto.SSL_get0_ech_name_override(ssl, this);
+    }
+
+    byte[] getEchRetryConfig() {
+        return NativeCrypto.SSL_get0_ech_retry_configs(ssl, this);
+    }
+
     boolean echAccepted() {
         return NativeCrypto.SSL_ech_accepted(ssl, this);
     }
diff --git a/common/src/main/java/org/conscrypt/SSLUtils.java b/common/src/main/java/org/conscrypt/SSLUtils.java
@@ -359,6 +359,11 @@ static SSLHandshakeException toSSLHandshakeException(Throwable e) {
             return (SSLHandshakeException) e;
         }
 
+        if (e.getMessage().contains(":ECH_REJECTED ")) {
+            // TODO this should probably be implemented in boringssl
+            return (SSLHandshakeException) new EchRejectedException(e.getMessage()).initCause(e);
+        }
+
         return (SSLHandshakeException) new SSLHandshakeException(e.getMessage()).initCause(e);
     }
 
diff --git a/openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java b/openjdk/src/test/java/org/conscrypt/NativeCryptoTest.java
@@ -29,6 +29,7 @@
 import static org.conscrypt.NativeConstants.TLS1_VERSION;
 import static org.conscrypt.TestUtils.openTestFile;
 import static org.conscrypt.TestUtils.readTestFile;
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -528,6 +529,9 @@ public long beforeHandshake(long c) throws SSLException {
             public void afterHandshake(long session, long ssl, long context, Socket socket,
                                        FileDescriptor fd, SSLHandshakeCallbacks callback) throws Exception {
                 assertFalse(NativeCrypto.SSL_ech_accepted(ssl, null));
+                assertNull(NativeCrypto.SSL_get0_ech_name_override(ssl, null));
+                byte[] retryConfigs = NativeCrypto.SSL_get0_ech_retry_configs(ssl, null);
+                assertEquals(5, retryConfigs.length);  // should be the invalid ECH Config List
                 super.afterHandshake(session, ssl, context, socket, fd, callback);
             }
         };
@@ -626,6 +630,103 @@ public void afterHandshake(long session, long ssl, long context, Socket socket,
         assertTrue(serverCallback.serverCertificateRequestedInvoked);
     }
 
+    @Test
+    public void test_SSL_do_handshake_ech_retry_configs() throws Exception {
+        final ServerSocket listener = newServerSocket();
+
+        final byte[] key = readTestFile("boringssl-ech-private-key.bin");
+        final byte[] serverConfig = readTestFile("boringssl-server-ech-config.bin");
+        final byte[] originalClientConfigList = readTestFile("boringssl-ech-config-list.bin");
+        final byte[] clientConfigList = originalClientConfigList.clone();
+        clientConfigList[20] = (byte) (clientConfigList[20] % 255 + 1);  // corrupt it
+
+        Hooks cHooks = new ClientHooks() {
+            @Override
+            public long beforeHandshake(long c) throws SSLException {
+                long ssl = super.beforeHandshake(c);
+                assertEquals(1, NativeCrypto.SSL_set_protocol_versions(ssl, null, TLS1_VERSION, TLS1_3_VERSION));
+                assertTrue(NativeCrypto.SSL_set1_ech_config_list(ssl, null, clientConfigList));
+                return ssl;
+            }
+
+            @Override
+            public void afterHandshake(long session, long ssl, long context, Socket socket,
+                                       FileDescriptor fd, SSLHandshakeCallbacks callback) {
+                fail();
+            }
+        };
+        Hooks sHooks = new ServerHooks(getServerPrivateKey(), getEncodedServerCertificates()) {
+            @Override
+            public long beforeHandshake(long c) throws SSLException {
+                long ssl = super.beforeHandshake(c);
+                assertEquals(1, NativeCrypto.SSL_set_protocol_versions(ssl, null, TLS1_VERSION, TLS1_3_VERSION));
+                assertTrue(NativeCrypto.SSL_CTX_ech_enable_server(c, null, key, serverConfig));
+                return ssl;
+            }
+
+            @Override
+            public void afterHandshake(long session, long ssl, long context, Socket socket,
+                                       FileDescriptor fd, SSLHandshakeCallbacks callback) throws Exception {
+                assertTrue(NativeCrypto.SSL_ech_accepted(ssl, null));
+                super.afterHandshake(session, ssl, context, socket, fd, callback);
+            }
+        };
+        Future<TestSSLHandshakeCallbacks> client = handshake(listener, 0, true, cHooks, null, null, true);
+        Future<TestSSLHandshakeCallbacks> server = handshake(listener, 0, false, sHooks, null, null, true);
+        TestSSLHandshakeCallbacks clientCallback = null;
+        TestSSLHandshakeCallbacks serverCallback = null;
+        try {
+            clientCallback = client.get(TIMEOUT_SECONDS, TimeUnit.SECONDS);
+            serverCallback = server.get(TIMEOUT_SECONDS, TimeUnit.SECONDS);
+        } catch (ExecutionException e) {
+            // caused by SSLProtocolException
+        }
+        assertNull(clientCallback);
+        assertNull(serverCallback);
+        assertArrayEquals(originalClientConfigList, cHooks.echRetryConfigs);
+        assertEquals("example.com", cHooks.echNameOverride);
+        assertNotNull(cHooks.echRetryConfigs);
+        assertNull(sHooks.echNameOverride);
+        assertNull(sHooks.echRetryConfigs);
+
+        final byte[] echRetryConfigsFromPrevious = cHooks.echRetryConfigs;
+        cHooks = new ClientHooks() {
+            @Override
+            public long beforeHandshake(long c) throws SSLException {
+                long ssl = super.beforeHandshake(c);
+                assertEquals(1, NativeCrypto.SSL_set_protocol_versions(ssl, null, TLS1_VERSION, TLS1_3_VERSION));
+                assertTrue(NativeCrypto.SSL_set1_ech_config_list(ssl, null, echRetryConfigsFromPrevious));
+                return ssl;
+            }
+
+            @Override
+            public void afterHandshake(long session, long ssl, long context, Socket socket,
+                                       FileDescriptor fd, SSLHandshakeCallbacks callback) throws Exception {
+                assertTrue(NativeCrypto.SSL_ech_accepted(ssl, null));
+                super.afterHandshake(session, ssl, context, socket, fd, callback);
+            }
+        };
+
+        client = handshake(listener, 0, true, cHooks, null, null);
+        server = handshake(listener, 0, false, sHooks, null, null);
+        clientCallback = client.get(TIMEOUT_SECONDS, TimeUnit.SECONDS);
+        serverCallback = server.get(TIMEOUT_SECONDS, TimeUnit.SECONDS);
+        assertTrue(clientCallback.verifyCertificateChainCalled);
+        assertEqualCertificateChains(
+                getServerCertificateRefs(), clientCallback.certificateChainRefs);
+        assertFalse(serverCallback.verifyCertificateChainCalled);
+        assertFalse(clientCallback.clientCertificateRequestedCalled);
+        assertFalse(serverCallback.clientCertificateRequestedCalled);
+        assertFalse(clientCallback.clientPSKKeyRequestedInvoked);
+        assertFalse(serverCallback.clientPSKKeyRequestedInvoked);
+        assertFalse(clientCallback.serverPSKKeyRequestedInvoked);
+        assertFalse(serverCallback.serverPSKKeyRequestedInvoked);
+        assertTrue(clientCallback.handshakeCompletedCalled);
+        assertTrue(serverCallback.handshakeCompletedCalled);
+        assertFalse(clientCallback.serverCertificateRequestedInvoked);
+        assertTrue(serverCallback.serverCertificateRequestedInvoked);
+    }
+
     @Test
     public void test_SSL_set_enable_ech_grease() throws Exception {
         long c = NativeCrypto.SSL_CTX_new();
@@ -688,6 +789,11 @@ public void test_SSL_CTX_ech_enable_server() throws Exception {
         NativeCrypto.SSL_CTX_free(c, null);
     }
 
+    @Test(expected = NullPointerException.class)
+    public void test_SSL_get0_ech_retry_configs_withNullShouldThrow() throws Exception {
+        NativeCrypto.SSL_get0_ech_retry_configs(NULL, null);
+    }
+
     @Test(expected = NullPointerException.class)
     public void test_SSL_CTX_ech_enable_server_NULL_SSL_CTX() throws Exception {
         NativeCrypto.SSL_CTX_ech_enable_server(NULL, null, null, null);
@@ -933,6 +1039,8 @@ public static class Hooks {
         boolean pskEnabled;
         byte[] pskKey;
         List<String> enabledCipherSuites;
+        byte[] echRetryConfigs;
+        String echNameOverride;
 
         /**
          * @throws SSLException if an error occurs creating the context.
@@ -1254,6 +1362,12 @@ public void clientCertificateRequested(long s) {
     public static Future<TestSSLHandshakeCallbacks> handshake(final ServerSocket listener,
             final int timeout, final boolean client, final Hooks hooks, final byte[] alpnProtocols,
             final ApplicationProtocolSelectorAdapter alpnSelector) {
+        return handshake(listener, timeout, client, hooks, alpnProtocols, alpnSelector, false);
+    }
+
+    public static Future<TestSSLHandshakeCallbacks> handshake(final ServerSocket listener,
+            final int timeout, final boolean client, final Hooks hooks, final byte[] alpnProtocols,
+            final ApplicationProtocolSelectorAdapter alpnSelector, final boolean useEchRetryConfig) {
         ExecutorService executor = Executors.newSingleThreadExecutor();
         Future<TestSSLHandshakeCallbacks> future =
                 executor.submit(new Callable<TestSSLHandshakeCallbacks>() {
@@ -1294,7 +1408,19 @@ public TestSSLHandshakeCallbacks call() throws Exception {
                             if (!client && alpnSelector != null) {
                                 NativeCrypto.setHasApplicationProtocolSelector(s, null, true);
                             }
-                            NativeCrypto.SSL_do_handshake(s, null, fd, callback, timeout);
+                            if (useEchRetryConfig) {
+                                try {
+                                    NativeCrypto.SSL_do_handshake(s, null, fd, callback, timeout);
+                                } catch (SSLProtocolException e) {
+                                    hooks.echRetryConfigs =
+                                            NativeCrypto.SSL_get0_ech_retry_configs(s, null);
+                                    hooks.echNameOverride =
+                                            NativeCrypto.SSL_get0_ech_name_override(s, null);
+                                    throw e;
+                                }
+                            } else {
+                                NativeCrypto.SSL_do_handshake(s, null, fd, callback, timeout);
+                            }
                             session = NativeCrypto.SSL_get1_session(s, null);
                             if (DEBUG) {
                                 System.out.println("ssl=0x" + Long.toString(s, 16)

Original file line number	Diff line number	Diff line change
`@@ -761,5 +761,9 @@ abstract byte[] exportKeyingMaterial(String label, byte[] context, int length)`
`761`	`761`
`762`	`762`	`public abstract byte[] getEchConfigList();`
`763`	`763`
	`764`	`+ public abstract String getEchNameOverride();`
	`765`	`+`
	`766`	`+ public abstract byte[] getEchRetryConfig();`
	`767`	`+`
`764`	`768`	`public abstract boolean echAccepted();`
`765`	`769`	`}`
Original file line number	Diff line number	Diff line change
`@@ -510,6 +510,14 @@ public static byte[] getEchConfigList(SSLSocket socket) {`
`510`	`510`	`return toConscrypt(socket).getEchConfigList();`
`511`	`511`	`}`
`512`	`512`
	`513`	`+ public static String getEchNameOverride(SSLSocket socket) {`
	`514`	`+ return toConscrypt(socket).getEchNameOverride();`
	`515`	`+ }`
	`516`	`+`
	`517`	`+ public static byte[] getEchRetryConfig(SSLSocket socket) {`
	`518`	`+ return toConscrypt(socket).getEchConfigList();`
	`519`	`+ }`
	`520`	`+`
`513`	`521`	`public static boolean echAccepted(SSLSocket socket) {`
`514`	`522`	`return toConscrypt(socket).echAccepted();`
`515`	`523`	`}`
Original file line number	Diff line number	Diff line change
`@@ -279,6 +279,14 @@ byte[] getTlsChannelId() throws SSLException {`
`279`	`279`	`return NativeCrypto.SSL_get_tls_channel_id(ssl, this);`
`280`	`280`	`}`
`281`	`281`
	`282`	`+ String getEchNameOverride() {`
	`283`	`+ return NativeCrypto.SSL_get0_ech_name_override(ssl, this);`
	`284`	`+ }`
	`285`	`+`
	`286`	`+ byte[] getEchRetryConfig() {`
	`287`	`+ return NativeCrypto.SSL_get0_ech_retry_configs(ssl, this);`
	`288`	`+ }`
	`289`	`+`
`282`	`290`	`boolean echAccepted() {`
`283`	`291`	`return NativeCrypto.SSL_ech_accepted(ssl, this);`
`284`	`292`	`}`
Original file line number	Diff line number	Diff line change
`@@ -359,6 +359,11 @@ static SSLHandshakeException toSSLHandshakeException(Throwable e) {`
`359`	`359`	`return (SSLHandshakeException) e;`
`360`	`360`	`}`
`361`	`361`
	`362`	`+ if (e.getMessage().contains(":ECH_REJECTED ")) {`
	`363`	`+ // TODO this should probably be implemented in boringssl`
	`364`	`+ return (SSLHandshakeException) new EchRejectedException(e.getMessage()).initCause(e);`
	`365`	`+ }`
	`366`	`+`
`362`	`367`	`return (SSLHandshakeException) new SSLHandshakeException(e.getMessage()).initCause(e);`
`363`	`368`	`}`
`364`	`369`