Repository: netty/netty
Issue: Using underscore character('_') as part of SNI leads JVM crash
Labels: defect

Hello guys, we use Netty as our network implementation of microservices framework. We plan to support https and thus enable authentication on both client side and server side. But we encounter a problem when we want to use underscore character('_') in TLS SNI.

### Steps to reproduce
#### 1. Netty Server implementation:
```
// define ssl provider
private static final SslProvider SSL_PROVIDER = SslProvider.OPENSSL;

// factory method for creating ssl context
public SslContext serverCtx(InputStream certificate, InputStream privateKey) {
        try {
            return SslContextBuilder.forServer(certificate, privateKey)
                                    .sslProvider(SSL_PROVIDER)
                                    .trustManager(TwoWayTrustManagerFactory.INSTANCE)
                                    .clientAuth(ClientAuth.REQUIRE)
                                    .build();
        } catch (Exception e) {
            throw new RuntimeException("failed to create ssl context", e);
        }
}

// SNI Matcher implementation
@Slf4j
public class MySNIMatcher extends SNIMatcher {

    private final byte[] hostIdentifier;

    public MySNIMatcher(Identifier hostIdentifier) {
        super(0);
        this.hostIdentifier = hostIdentifier.idDistKey().getBytes(StandardCharsets.US_ASCII);
    }

    @Override
    public boolean matches(SNIServerName sniServerName) {
        if (log.isDebugEnabled()) {
            log.debug("match sni {}", new String(sniServerName.getEncoded(), StandardCharsets.US_ASCII));
        }
        return Arrays.equals(sniServerName.getEncoded(), hostIdentifier);
    }
}

// Netty bootstrap
bootstrap.childHandler(new ChannelInitializer<Channel>() {
            @Override
            protected void initChannel(Channel ch) throws Exception {
                ChannelPipeline pipeline = ch.pipeline();

                if (conf.logEnable())
                    pipeline.addLast("logging", new LoggingHandler());

                if (conf.sslEnable()) {
                    // conf.sslCtx() will actually use serverCtx() to create context
                    SSLEngine engine = conf.sslCtx().newEngine(ch.alloc());
                    SSLParameters s = engine.getSSLParameters();
                    s.setSNIMatchers(new MySNIMatcher(...));
                    s.setEndpointIdentificationAlgorithm("HTTPS");
                    engine.setSSLParameters(s);
                    pipeline.addLast("ssl", new SslHandler(engine, true));
                }

                codec(pipeline);

                int idleTimeout = (int) TimeUnit.MILLISECONDS.toSeconds(conf.idleTimeout());
                if (0 < idleTimeout)
                    pipeline.addLast("idle", new IdleStateHandler(0, 0, idleTimeout));

                pipeline.addLast("handler", handler);
            }
        })
```

#### 2. Openssl Client test:
```
openssl s_client -cert service.crt -key service.key -CAfile root_ca.pem -connect 127.0.0.1:8080 -servername rb8hx3pww30y3tvw0mwy.v1_1
CONNECTED(00000003)
write:errno=0
---
no peer certificate available
---
No client certificate CA names sent
---
SSL handshake has read 0 bytes and written 210 bytes
Verification: OK
---
New, (NONE), Cipher is (NONE)
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
    Protocol  : TLSv1.2
    Cipher    : 0000
    Session-ID:
    Session-ID-ctx:
    Master-Key:
    PSK identity: None
    PSK identity hint: None
    SRP username: None
    Start Time: 1532336403
    Timeout   : 7200 (sec)
    Verify return code: 0 (ok)
    Extended master secret: no
---

```
As we can see from the command, we used `rb8hx3pww30y3tvw0mwy.v1_1` as sni

stdout of JVM process when crash:
```
java: ../ssl/handshake_server.c:541: negotiate_version: Assertion `!ssl->s3->have_version' failed.
```

Please note that if we use sni which excluded underscore character(e.g. `rb8hx3pww30y3tvw0mwy.v1`), everything works fine, I can see sni logs in MySNIMatcher.

### Netty version
Netty: 4.1.14.Final
Boringssl: netty-tcnative-boringssl-static:2.0.5.Final
### Openssl Version
1.0.2o
### JVM version (e.g. `java -version`)
java version "1.8.0_112"
Java(TM) SE Runtime Environment (build 1.8.0_112-b16)
Java HotSpot(TM) 64-Bit Server VM (build 25.112-b16, mixed mode)
### OS version (e.g. `uname -a`)
macOS High Sierra 10.13.5
Darwin Kernel Version 17.6.0

Issue comments:
interesting... Let me investigate 
@normanmaurer I have tested this case on netty 4.1.27.Final and netty-tcnative-boringssl-static 2.0.12.Final, the process would not crash any more. It seems that the latest version of boringssl have fixed this issue. Then the validation actually takes place in the io.netty.handler.ssl. ReferenceCountedOpenSslEngine#checkSniHostnameMatch() and it blocks me since underscore character is not allowed in definition of host name in rfc 1123 (which is based on rfc 952).

> A "name" (Net, Host, Gateway, or Domain name) is a text string up to 24 characters drawn from the alphabet (A-Z), digits (0-9), minus sign (-), and period (.).

But I‘m just curious about the JDK that provided the abstract class SNIMatcher which used 
SNIServerName to represent the "server name", and it seems to be a liberal way to implement SNI. I have also tested my case on JDK provided SSL implementation and the underscore character is passed the test.
@r9liucc can you please verify https://github.com/netty/netty/pull/8150 ?
@normanmaurer I have verified this on branch `sni_underscore` and the results seems ok.
Openssl Client test:
```
openssl s_client -cert service.crt -key service.key  -CAfile root_ca.pem -connect 127.0.0.1:8080 -servername rb7yh582ov5d0umxr2td.v1_1
CONNECTED(00000003)
depth=1 C = CN, ST = SC, L = CD, O = HCB, OU = HCB, CN = my_org
verify return:1
depth=0 C = CN, ST = SC, L = CD, O = HCB, OU = HCB, CN = my_cn
verify return:1
---
Certificate chain
 0 s:/C=CN/ST=SC/L=CD/O=HCB/OU=HCB/CN=my_cn
   i:/C=CN/ST=SC/L=CD/O=HCB/OU=HCB/CN=my_org
---
Server certificate
-----BEGIN CERTIFICATE-----
...ignore
-----END CERTIFICATE-----
subject=/C=CN/ST=SC/L=CD/O=HCB/OU=HCB/CN=my_cn
issuer=/C=CN/ST=SC/L=CD/O=HCB/OU=HCB/CN=my_org
---
No client certificate CA names sent
Client Certificate Types: RSA sign, ECDSA sign
Requested Signature Algorithms: ECDSA+SHA256:0x04+0x08:RSA+SHA256:ECDSA+SHA384:0x05+0x08:RSA+SHA384:0x06+0x08:RSA+SHA512:RSA+SHA1
Shared Requested Signature Algorithms: ECDSA+SHA256:RSA+SHA256:ECDSA+SHA384:RSA+SHA384:RSA+SHA512:RSA+SHA1
Peer signing digest: SHA256
Server Temp Key: ECDH, P-256, 256 bits
---
SSL handshake has read 1364 bytes and written 2507 bytes
---
New, TLSv1/SSLv3, Cipher is ECDHE-RSA-AES128-GCM-SHA256
Server public key is 2048 bit
Secure Renegotiation IS supported
No ALPN negotiated
SSL-Session:
    Protocol  : TLSv1.2
    Cipher    : ECDHE-RSA-AES128-GCM-SHA256
    Session-ID: 9C33100A039306570D48E007DF129AE9E36EAC63E5E0044D99F52FA5DC4F261F
    Session-ID-ctx:
    Master-Key: 05D02E543544B0F9CC5BB0BC6D38B21FA440C77810B1857553FC24F534E408E4CD727E57CA814B1623B6D85FB7B2B6EA
    Key-Arg   : None
    PSK identity: None
    PSK identity hint: None
    SRP username: None
    Start Time: 1532596523
    Timeout   : 300 (sec)
    Verify return code: 0 (ok)
---
GET /hello HTTP/1.1

HTTP/1.1 200 OK
Content-Type: application/json;charset=UTF-8
Content-Length: 7

"hello"
```

I have also verified outbound data and the client sni has successfully been sent out for negotiation.
Client code snippets:
```
private final List<SNIServerName> sni = new ArrayList<>();
// init sni
this.sni.add(new SNIHostName(remoteIdentifier.idDistKey().getBytes(StandardCharsets.UTF_8)));

// set sni
SSLParameters sslParameters = sslEngine.getSSLParameters();
sslParameters.setServerNames(sni);
sslEngine.setSSLParameters(sslParameters);
...
```

Pull request review comments:
Is there an issue on tcnarive for this?
Not yet... will open one

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/Java8SslUtils.java b/handler/src/main/java/io/netty/handler/ssl/Java8SslUtils.java
index 583d4cf4986..b40c121748d 100644
--- a/handler/src/main/java/io/netty/handler/ssl/Java8SslUtils.java
+++ b/handler/src/main/java/io/netty/handler/ssl/Java8SslUtils.java
@@ -69,7 +69,7 @@ static void setSNIMatchers(SSLParameters sslParameters, Collection<?> matchers)
     }
 
     @SuppressWarnings("unchecked")
-    static boolean checkSniHostnameMatch(Collection<?> matchers, String hostname) {
+    static boolean checkSniHostnameMatch(Collection<?> matchers, byte[] hostname) {
         if (matchers != null && !matchers.isEmpty()) {
             SNIHostName name = new SNIHostName(hostname);
             Iterator<SNIMatcher> matcherIt = (Iterator<SNIMatcher>) matchers.iterator();
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
index b98e9858d7b..8b147357667 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -20,6 +20,7 @@
 import io.netty.internal.tcnative.Buffer;
 import io.netty.internal.tcnative.SSL;
 import io.netty.util.AbstractReferenceCounted;
+import io.netty.util.CharsetUtil;
 import io.netty.util.ReferenceCounted;
 import io.netty.util.ResourceLeakDetector;
 import io.netty.util.ResourceLeakDetectorFactory;
@@ -1817,7 +1818,7 @@ private boolean isDestroyed() {
         return destroyed != 0;
     }
 
-    final boolean checkSniHostnameMatch(String hostname) {
+    final boolean checkSniHostnameMatch(byte[] hostname) {
         return Java8SslUtils.checkSniHostnameMatch(matchers, hostname);
     }
 
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java
index 9f09393f01e..b9c8cf60b8c 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java
@@ -19,6 +19,7 @@
 import io.netty.internal.tcnative.SSL;
 import io.netty.internal.tcnative.SSLContext;
 import io.netty.internal.tcnative.SniHostNameMatcher;
+import io.netty.util.CharsetUtil;
 import io.netty.util.internal.PlatformDependent;
 import io.netty.util.internal.logging.InternalLogger;
 import io.netty.util.internal.logging.InternalLoggerFactory;
@@ -244,7 +245,8 @@ private static final class OpenSslSniHostnameMatcher implements SniHostNameMatch
         public boolean match(long ssl, String hostname) {
             ReferenceCountedOpenSslEngine engine = engineMap.get(ssl);
             if (engine != null) {
-                return engine.checkSniHostnameMatch(hostname);
+                // TODO: In the next release of tcnative we should pass the byte[] directly in and not use a String.
+                return engine.checkSniHostnameMatch(hostname.getBytes(CharsetUtil.UTF_8));
             }
             logger.warn("No ReferenceCountedOpenSslEngine found for SSL pointer: {}", ssl);
             return false;


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/Java8SslTestUtils.java b/handler/src/test/java/io/netty/handler/ssl/Java8SslTestUtils.java
index 32219ff155a..50fb935d9dc 100644
--- a/handler/src/test/java/io/netty/handler/ssl/Java8SslTestUtils.java
+++ b/handler/src/test/java/io/netty/handler/ssl/Java8SslTestUtils.java
@@ -23,17 +23,18 @@
 import javax.net.ssl.SSLEngine;
 import javax.net.ssl.SSLParameters;
 import java.security.Provider;
+import java.util.Arrays;
 import java.util.Collections;
 
 final class Java8SslTestUtils {
 
     private Java8SslTestUtils() { }
 
-    static void setSNIMatcher(SSLParameters parameters) {
+    static void setSNIMatcher(SSLParameters parameters, final byte[] match) {
         SNIMatcher matcher = new SNIMatcher(0) {
             @Override
             public boolean matches(SNIServerName sniServerName) {
-                return false;
+                return Arrays.equals(match, sniServerName.getEncoded());
             }
         };
         parameters.setSNIMatchers(Collections.singleton(matcher));
diff --git a/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java
index 9be57e54680..99fb8fa82e2 100644
--- a/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/OpenSslEngineTest.java
@@ -22,6 +22,8 @@
 import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
 import io.netty.handler.ssl.util.SelfSignedCertificate;
 import io.netty.internal.tcnative.SSL;
+import io.netty.util.CharsetUtil;
+import io.netty.util.internal.EmptyArrays;
 import io.netty.util.internal.PlatformDependent;
 import org.junit.Assume;
 import org.junit.BeforeClass;
@@ -986,7 +988,7 @@ public void testSNIMatchersDoesNotThrow() throws Exception {
         SSLEngine engine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
         try {
             SSLParameters parameters = new SSLParameters();
-            Java8SslTestUtils.setSNIMatcher(parameters);
+            Java8SslTestUtils.setSNIMatcher(parameters, EmptyArrays.EMPTY_BYTES);
             engine.setSSLParameters(parameters);
         } finally {
             cleanupServerSslEngine(engine);
@@ -994,6 +996,28 @@ public void testSNIMatchersDoesNotThrow() throws Exception {
         }
     }
 
+    @Test
+    public void testSNIMatchersWithSNINameWithUnderscore() throws Exception {
+        assumeTrue(PlatformDependent.javaVersion() >= 8);
+        byte[] name = "rb8hx3pww30y3tvw0mwy.v1_1".getBytes(CharsetUtil.UTF_8);
+        SelfSignedCertificate ssc = new SelfSignedCertificate();
+        serverSslCtx = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
+                .sslProvider(sslServerProvider())
+                .build();
+
+        SSLEngine engine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
+        try {
+            SSLParameters parameters = new SSLParameters();
+            Java8SslTestUtils.setSNIMatcher(parameters, name);
+            engine.setSSLParameters(parameters);
+            assertTrue(unwrapEngine(engine).checkSniHostnameMatch(name));
+            assertFalse(unwrapEngine(engine).checkSniHostnameMatch("other".getBytes(CharsetUtil.UTF_8)));
+        } finally {
+            cleanupServerSslEngine(engine);
+            ssc.delete();
+        }
+    }
+
     @Test(expected = IllegalArgumentException.class)
     public void testAlgorithmConstraintsThrows() throws Exception {
         SelfSignedCertificate ssc = new SelfSignedCertificate();