SSL host name verification disabled by default #197

617m4rc · 2013-01-09T11:22:17Z

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.

The text was updated successfully, but these errors were encountered:

harbulot · 2013-07-29T23:50:13Z

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 · 2014-01-29T06:04:47Z

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.

… to HostnameChecker.

Fix for #197 -- use a hostname verifier that does hostname verification

slandelle · 2014-03-30T15:49:38Z

Closed by #197

wsargent · 2014-04-02T15:53:15Z

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

wsargent · 2014-04-06T22:18:08Z

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 · 2014-04-07T00:48:21Z

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);
    }

wsargent · 2014-04-07T00:51:04Z

Still seeing this for the Grizzly test though:

2014-04-06 17:48:52,442 [main] INFO org.asynchttpclient.providers.grizzly.ConnectionManager - completed: session = [Session-1, SSL_NULL_WITH_NULL_NULL], id = , isValid = false, host = 127.0.0.1

For reference, Netty now looks like this:

2014-04-06 17:50:44,028 [nioEventLoopGroup-2-1] DEBUG org.asynchttpclient.providers.netty.request.NettyConnectListener - onFutureSuccess: session = [Session-2, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA], id = U0H2Y2vR7SoAe+QJl4TFoRWnmWdBFIbBZUeDXi9Gg6s=, isValid = true, host = 127.0.0.1

slandelle · 2014-04-07T07:52:24Z

Hi @wsargent,

Why do you resolve the SslHandler once again?

Should the presence of a hostnameVerifier be checked first so that the listener is not added if it doesn't exist?

Shouldn't requestSender.writeRequest(future, channel) only happen once the hostnameVerifier has done its job?

Did you notice a performance impact?

slandelle · 2014-04-07T07:56:44Z

BTW, the SOF question you pointed seems to be an Android issue: netty/netty#1823

wsargent · 2014-04-07T17:04:47Z

Why do you resolve the SslHandler once again?

This is cut and pasted code from the complete handler. You're correct that it should use the same handler inside the scope.

Should the presence of a hostnameVerifier be checked first so that the listener is not added if it doesn't exist?

Yes. This is very much first draft code.

Shouldn't requestSender.writeRequest(future, channel) only happen once the hostnameVerifier has done its job?

Yes, you're correct.

Did you notice a performance impact?

Haven't tested it yet -- I noticed that when I called session.getPeerCertificates() it would throw an exception unless the handshake was established. I'm not sure why it doesn't come up in the Play WS integration tests -- maybe there's a race condition resolved against the remote host which means the handshake completes by the time the event is received.

The SOF question is just an API reference, so I can see the idioms.

wsargent · 2014-04-08T06:49:56Z

I would very much like to have #525 merged into the 1.8.x branch as well -- Play 2.3 is coming out shortly, and having hostname verification turned on by default is something I can only do if I can ensure with that patch.

slandelle · 2014-04-08T08:04:49Z

I had guessed ;)
@jfarcand Should this be 1.8.6 or 1.9.0?
@rlubke Is the Grizzly part fine for you?

close #197

…ncHttpClient#510, close AsyncHttpClient#197

* Change version for jenkins * [SECURITY-650] Introduce acceptAnyCertificate config, defaulting to false * Use a hostname verifier that does hostname verification, backport AsyncHttpClient#510, close AsyncHttpClient#197 * Bump netty version * Restore necessary compatibility * [JENKINS-54601] Fix test failures. * [JENKINS-54601] Correct POM info. * Add script to make it easier to get a working JDK7 environment * [JENKINS-54601] Include proper hostname verifier logic. * [JENKINS-54601] Update README.

slandelle mentioned this issue Jul 30, 2013

SSL/TLS certificate verification disabled #352

Closed

wsargent added a commit to wsargent/async-http-client that referenced this issue Mar 25, 2014

Fix for AsyncHttpClient#197 -- use a hostname verifier that calls out…

db6716a

… to HostnameChecker.

wsargent mentioned this issue Mar 25, 2014

Fix for #197 -- use a hostname verifier that does hostname verification #510

Merged

harbulot mentioned this issue Mar 26, 2014

Consider using the host name in the SSLEngineFactory #513

Closed

jfarcand added a commit that referenced this issue Mar 30, 2014

Merge pull request #510 from wsargent/fix-197

3c9152e

Fix for #197 -- use a hostname verifier that does hostname verification

slandelle added this to the 2.0.0.Alpha1 milestone Mar 30, 2014

slandelle added the Enhancement label Mar 30, 2014

slandelle closed this as completed Mar 30, 2014

slandelle assigned jfarcand Mar 30, 2014

wsargent mentioned this issue Apr 7, 2014

Move the hostname verification to after the SSL handshake has completed. #525

Merged

slandelle assigned slandelle and unassigned jfarcand Jun 13, 2014

slandelle modified the milestones: 1.9.0, 2.0.0.Alpha1 Jul 10, 2014

slandelle pushed a commit that referenced this issue Jul 10, 2014

Use a hostname verifier that does hostname verification, backport #510,

a894583

close #197

slandelle modified the milestone: 1.9.0 Jul 10, 2014

wsargent mentioned this issue Aug 5, 2014

SSL/TLS: Add client-side certificate verification to akka-remote akka/akka#13874

Closed

wsargent mentioned this issue Aug 21, 2014

[2.3.3] WS: Log hostname when hostname can't be verified playframework/playframework#3310

Closed

wsargent mentioned this issue Jul 6, 2015

WS play.ws.ssl.loose.disableHostnameVerification=true doesn't work? playframework/playframework#4796

Closed

varyvol pushed a commit to varyvol/async-http-client that referenced this issue Nov 13, 2018

Use a hostname verifier that does hostname verification, backport Asy…

c8e7c7f

…ncHttpClient#510, close AsyncHttpClient#197

rdhabalia mentioned this issue Feb 7, 2019

How to disable hostname verification in AsyncHttpClient #1611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SSL host name verification disabled by default #197

SSL host name verification disabled by default #197

617m4rc commented Jan 9, 2013

harbulot commented Jul 29, 2013

Uh oh!

stsiano commented Jan 29, 2014

Uh oh!

slandelle commented Mar 30, 2014

Uh oh!

wsargent commented Apr 2, 2014

Uh oh!

wsargent commented Apr 6, 2014

Uh oh!

wsargent commented Apr 7, 2014

Uh oh!

wsargent commented Apr 7, 2014

Uh oh!

slandelle commented Apr 7, 2014

Uh oh!

slandelle commented Apr 7, 2014

Uh oh!

wsargent commented Apr 7, 2014

Uh oh!

wsargent commented Apr 8, 2014

Uh oh!

slandelle commented Apr 8, 2014

Uh oh!

SSL host name verification disabled by default #197

SSL host name verification disabled by default #197

Comments

617m4rc commented Jan 9, 2013

harbulot commented Jul 29, 2013

Uh oh!

stsiano commented Jan 29, 2014

Uh oh!

slandelle commented Mar 30, 2014

Uh oh!

wsargent commented Apr 2, 2014

Uh oh!

wsargent commented Apr 6, 2014

Uh oh!

wsargent commented Apr 7, 2014

Uh oh!

wsargent commented Apr 7, 2014

Uh oh!

slandelle commented Apr 7, 2014

Uh oh!

slandelle commented Apr 7, 2014

Uh oh!

wsargent commented Apr 7, 2014

Uh oh!

wsargent commented Apr 8, 2014

Uh oh!

slandelle commented Apr 8, 2014

Uh oh!