SSL host name verification disabled by default

[617m4rc]

AllowAllHostnameVerifier is used if user does not explicitly set another HostnameVerifier.

I consider this to be a security issue and think this should be an opt-in setting instead. If there are any good reasons to do so, then default behavior should at least be clearly indicated in user guide.

[harbulot]

This is indeed a security issue (just like issue #352).

A quick grep through the code suggest that these are the places to fix:

• Never return true blindly:
  https://github.com/AsyncHttpClient/async-http-client/blob/master/api/src/main/java/org/asynchttpclient/AsyncHttpClientConfigBean.java#L69
• Never use the AllowAllHostnameVerifier (certainly never by default):
  https://github.com/AsyncHttpClient/async-http-client/blob/master/api/src/main/java/org/asynchttpclient/AsyncHttpClientConfig.java#L619

[stsiano]

As the hostname verification thing is not the easiest thing to do. In a project I set the verifier to
HttpsURLConnection.getDefaultHostnameVerifier()
which will make the library behave like URL connections from the JDK according to host name verification. I would guess that this is a better default than the current one.

[slandelle]

Closed by #197

[wsargent]

Testing hostname verification can require some specialized knowledge -- wrote it up in http://tersesystems.com/2014/03/31/testing-hostname-verification/

[wsargent]

It looks like some tests in the test suite are failing because they ask for the SSL session's peer certificates before the session has actually completed the handshake. Both NettyBasicHttpsTest andGrizzlyBasicHttpsTest` will fail the tests unless I set the hostname verifier explicitly to null. Turning on SSL debugging -Djavax.net.debug="ssl session"

java.net.ConnectException: HostnameVerifier exception
    at org.asynchttpclient.providers.netty.request.NettyConnectListener.onFutureSuccess(NettyConnectListener.java:67) [classes/:na]
    at org.asynchttpclient.providers.netty.request.NettyConnectListener.operationComplete(NettyConnectListener.java:103) [classes/:na]
    at org.asynchttpclient.providers.netty.request.NettyConnectListener.operationComplete(NettyConnectListener.java:39) [classes/:na]
    at io.netty.util.concurrent.DefaultPromise.notifyListener0(DefaultPromise.java:631) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.util.concurrent.DefaultPromise.notifyListeners0(DefaultPromise.java:596) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.util.concurrent.DefaultPromise.notifyListeners(DefaultPromise.java:553) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.util.concurrent.DefaultPromise.trySuccess(DefaultPromise.java:399) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.channel.DefaultChannelPromise.trySuccess(DefaultChannelPromise.java:82) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.fulfillConnectPromise(AbstractNioChannel.java:222) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.finishConnect(AbstractNioChannel.java:256) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:502) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:452) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:346) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:101) [netty-all-4.0.15.Final.jar:4.0.15.Final]
    at java.lang.Thread.run(Thread.java:744) [na:1.7.0_45]

first, and then way down the line there is:

nioEventLoopGroup-2-1, called closeOutbound()
nioEventLoopGroup-2-1, closeOutboundInternal()
nioEventLoopGroup-2-1, SEND TLSv1 ALERT:  warning, description = close_notify
nioEventLoopGroup-2-1, WRITE: TLSv1 Alert, length = 2
nioEventLoopGroup-2-1, called closeOutbound()
nioEventLoopGroup-2-1, closeOutboundInternal()
nioEventLoopGroup-2-1, called closeInbound()
nioEventLoopGroup-2-1, fatal error: 80: Inbound closed before receiving peer's close_notify: possible truncation attack?
javax.net.ssl.SSLException: Inbound closed before receiving peer's close_notify: possible truncation attack?
nioEventLoopGroup-2-1, SEND TLSv1 ALERT:  fatal, description = internal_error
nioEventLoopGroup-2-1, Exception sending alert: java.io.IOException: writer side was already closed.
2014-04-06 14:16:15,954 [nioEventLoopGroup-2-1] DEBUG io.netty.handler.ssl.SslHandler - Failed to complete handshake
java.nio.channels.ClosedChannelException: null
Allow unsafe renegotiation: false
Allow legacy hello messages: true
Is initial handshake: true
Is secure renegotiation: false

I comment out the hostname verifier, and the handshake happens successfully, much much later after the NettyConnectListener.onFutureSuccess.

qtp651360603-19, READ: TLSv1 Change Cipher Spec, length = 1
qtp651360603-19, READ: TLSv1 Handshake, length = 48
*** Finished
verify_data:  { 216, 186, 200, 173, 216, 9, 19, 124, 168, 216, 139, 177 }
***
qtp651360603-19, WRITE: TLSv1 Change Cipher Spec, length = 1
*** Finished
verify_data:  { 226, 184, 192, 190, 117, 143, 146, 123, 203, 233, 35, 209 }
***
qtp651360603-19, WRITE: TLSv1 Handshake, length = 48
%% Cached server session: [Session-1, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA]
nioEventLoopGroup-2-1, READ: TLSv1 Change Cipher Spec, length = 1
nioEventLoopGroup-2-1, READ: TLSv1 Handshake, length = 48
*** Finished
verify_data:  { 226, 184, 192, 190, 117, 143, 146, 123, 203, 233, 35, 209 }
***
%% Cached client session: [Session-2, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA]
nioEventLoopGroup-2-1, WRITE: TLSv1 Application Data, length = 187
qtp651360603-18, WRITE: TLSv1 Application Data, length = 348
qtp651360603-18, WRITE: TLSv1 Application Data, length = 1
qtp651360603-18, WRITE: TLSv1 Application Data, length = 6
2014-04-06 15:13:23,661 [nioEventLoopGroup-2-1] DEBUG org.asynchttpclient.providers.netty.handler.HttpProtocol - 

Request DefaultFullHttpRequest(decodeResult: success)

Either there's something wrong in the tests, or the verification is happening in the wrong place and is only working by accident.

[wsargent]

This seems to work more reliably, if I defer NettyConnectListener to listen for a handshakeFuture:

    public void onFutureSuccess(final Channel channel) throws ConnectException {
        Channels.setDefaultAttribute(channel, future);
        final SslHandler sslHandler = Channels.getSslHandler(channel);
        final String host = future.getURI().getHost();
        // https://stackoverflow.com/questions/18663061/channelfuturelistener-operationcomplete-of-sslhandler-handshake-not-being-call
        sslHandler.handshakeFuture().addListener(new GenericFutureListener<Future<? super Channel>>() {
            @Override
            public void operationComplete(Future<? super Channel> handshakeFuture) throws Exception {
                if (handshakeFuture.isSuccess()) {
                    Channel channel = (Channel) handshakeFuture.getNow();
                    SslHandler sslHandler = Channels.getSslHandler(channel);
                    SSLEngine engine = sslHandler.engine();
                    SSLSession session = engine.getSession();

                    LOGGER.debug("onFutureSuccess: session = {}, id = {}, isValid = {}, host = {}", session.toString(), Base64.encode(session.getId()), session.isValid(), host);
                    HostnameVerifier hostnameVerifier = config.getHostnameVerifier();
                    if (hostnameVerifier != null && !hostnameVerifier.verify(host, session)) {
                        ConnectException exception = new ConnectException("HostnameVerifier exception");
                        future.abort(exception);
                        throw exception;
                    }
                }
            }
        });
        requestSender.writeRequest(future, channel);
    }