Our SNI hostname validation was really basic and could lead to except…

…ions later on (#11742) Motivation: Our validation of the SNI hostname was incomplete and so we could end up with an exception once the user calls getSSLParameters() later on Modifications: - Just use SNIHostName to validate the name when on Java8 and later - Add unit test Result: Don't throw exceptions once we try to retrieve the SSLParamaters when an invalid SNI hostname was used while creating the engine
netty · Oct 8, 2021 · b602133 · b602133
1 parent acb5ba5
commit b602133
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 7 deletions.
diff --git a/handler/src/main/java/io/netty/handler/ssl/Java8SslUtils.java b/handler/src/main/java/io/netty/handler/ssl/Java8SslUtils.java
@@ -54,6 +54,15 @@ static void setSniHostNames(SSLParameters sslParameters, List<String> names) {
         sslParameters.setServerNames(getSniHostNames(names));
     }
 
+    static boolean isValidHostNameForSNI(String hostname) {
+        try {
+            new SNIHostName(hostname);
+            return true;
+        } catch (IllegalArgumentException illegal) {
+            return false;
+        }
+    }
+
     static List getSniHostNames(List<String> names) {
         if (names == null || names.isEmpty()) {
             return Collections.emptyList();

diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -342,8 +342,17 @@ public List<byte[]> getStatusResponses() {
                 // Use SNI if peerHost was specified and a valid hostname
                 // See https://github.com/netty/netty/issues/4746
                 if (clientMode && SslUtils.isValidHostNameForSNI(peerHost)) {
-                    SSL.setTlsExtHostName(ssl, peerHost);
-                    sniHostNames = Collections.singletonList(peerHost);
+                    // If on java8 and later we should do some extra validation to ensure we can construct the
+                    // SNIHostName later again.
+                    if (PlatformDependent.javaVersion() >= 8) {
+                        if (Java8SslUtils.isValidHostNameForSNI(peerHost)) {
+                            SSL.setTlsExtHostName(ssl, peerHost);
+                            sniHostNames = Collections.singletonList(peerHost);
+                        }
+                    } else {
+                        SSL.setTlsExtHostName(ssl, peerHost);
+                        sniHostNames = Collections.singletonList(peerHost);
+                    }
                 }
 
                 if (enableOcsp) {

diff --git a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java
@@ -480,7 +480,7 @@ static ByteBuf toBase64(ByteBufAllocator allocator, ByteBuf src) {
     static boolean isValidHostNameForSNI(String hostname) {
         return hostname != null &&
                hostname.indexOf('.') > 0 &&
-               !hostname.endsWith(".") &&
+               !hostname.endsWith(".") && !hostname.startsWith("/") &&
                !NetUtil.isValidIpV4Address(hostname) &&
                !NetUtil.isValidIpV6Address(hostname);
     }
@@ -493,10 +493,6 @@ static boolean isTLSv13Cipher(String cipher) {
         return TLSV13_CIPHERS.contains(cipher);
     }
 
-    static boolean isEmpty(Object[] arr) {
-        return arr == null || arr.length == 0;
-    }
-
     private SslUtils() {
     }
 }
diff --git a/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java b/handler/src/test/java/io/netty/handler/ssl/ConscryptJdkSslEngineInteropTest.java
@@ -79,4 +79,12 @@ protected boolean mySetupMutualAuthServerIsValidServerException(Throwable cause)
     protected void invalidateSessionsAndAssert(SSLSessionContext context) {
         // Not supported by conscrypt
     }
+
+    @MethodSource("newTestParams")
+    @ParameterizedTest
+    @Disabled("Disabled due a conscrypt bug")
+    @Override
+    public void testInvalidSNIIsIgnoredAndNotThrow(SSLEngineTestParam param) throws Exception {
+        super.testInvalidSNIIsIgnoredAndNotThrow(param);
+    }
 }
diff --git a/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java b/handler/src/test/java/io/netty/handler/ssl/ConscryptOpenSslEngineInteropTest.java
@@ -191,6 +191,14 @@ public void testSessionCacheSize(SSLEngineTestParam param) throws Exception {
         super.testSessionCacheSize(param);
     }
 
+    @MethodSource("newTestParams")
+    @ParameterizedTest
+    @Disabled("Disabled due a conscrypt bug")
+    @Override
+    public void testInvalidSNIIsIgnoredAndNotThrow(SSLEngineTestParam param) throws Exception {
+        super.testInvalidSNIIsIgnoredAndNotThrow(param);
+    }
+
     @Override
     protected SSLEngine wrapEngine(SSLEngine engine) {
         return Java8SslTestUtils.wrapSSLEngineForTesting(engine);

diff --git a/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/ConscryptSslEngineTest.java
@@ -88,4 +88,12 @@ public void testSessionCacheTimeout(SSLEngineTestParam param) throws Exception {
     public void testRSASSAPSS(SSLEngineTestParam param) {
         // skip
     }
+
+    @MethodSource("newTestParams")
+    @ParameterizedTest
+    @Disabled("Disabled due a conscrypt bug")
+    @Override
+    public void testInvalidSNIIsIgnoredAndNotThrow(SSLEngineTestParam param) throws Exception {
+        super.testInvalidSNIIsIgnoredAndNotThrow(param);
+    }
 }
diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
@@ -4192,6 +4192,37 @@ public void testRSASSAPSS(SSLEngineTestParam param) throws Exception {
         rethrowIfNotNull(serverException);
     }
 
+    @MethodSource("newTestParams")
+    @ParameterizedTest
+    public void testInvalidSNIIsIgnoredAndNotThrow(SSLEngineTestParam param) throws Exception {
+        clientSslCtx = wrapContext(param, SslContextBuilder.forClient()
+                .trustManager(InsecureTrustManagerFactory.INSTANCE)
+                .sslProvider(sslClientProvider())
+                .sslContextProvider(clientSslContextProvider())
+                .protocols(param.protocols())
+                .ciphers(param.ciphers())
+                .build());
+        SelfSignedCertificate ssc = new SelfSignedCertificate();
+        serverSslCtx = wrapContext(param, SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
+                .sslProvider(sslServerProvider())
+                .sslContextProvider(serverSslContextProvider())
+                .protocols(param.protocols())
+                .ciphers(param.ciphers())
+                .build());
+        SSLEngine clientEngine = null;
+        SSLEngine serverEngine = null;
+        try {
+            clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT, "/invalid.path", 80));
+            serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
+            handshake(param.type(), param.delegate(), clientEngine, serverEngine);
+            assertNotNull(clientEngine.getSSLParameters());
+            assertNotNull(serverEngine.getSSLParameters());
+        } finally {
+            cleanupClientSslEngine(clientEngine);
+            cleanupServerSslEngine(serverEngine);
+        }
+    }
+
     protected SSLEngine wrapEngine(SSLEngine engine) {
         return engine;
     }

diff --git a/handler/src/test/java/io/netty/handler/ssl/SslUtilsTest.java b/handler/src/test/java/io/netty/handler/ssl/SslUtilsTest.java
@@ -100,4 +100,9 @@ public void shouldGetPacketLengthOfGmsslProtocolFromByteBuffer() {
         assertEquals(bodyLength + SslUtils.SSL_RECORD_HEADER_LENGTH, packetLength);
         buf.release();
     }
+
+    @Test
+    public void testValidHostNameForSni() {
+        assertFalse(SslUtils.isValidHostNameForSNI("/test.de"));
+    }
 }