New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSL host name verification disabled by default #197

Closed
losar opened this Issue Jan 9, 2013 · 12 comments

Comments

Projects
None yet
6 participants
@losar
Copy link

losar commented Jan 9, 2013

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 comment has been minimized.

Copy link

harbulot commented Jul 29, 2013

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

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

@stsiano

This comment has been minimized.

Copy link

stsiano commented Jan 29, 2014

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.

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

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

Merge pull request #510 from wsargent/fix-197
Fix for #197 -- use a hostname verifier that does hostname verification

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

@slandelle

This comment has been minimized.

Copy link
Contributor

slandelle commented Mar 30, 2014

Closed by #197

@wsargent

This comment has been minimized.

Copy link
Contributor

wsargent commented Apr 2, 2014

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

@wsargent

This comment has been minimized.

Copy link
Contributor

wsargent commented Apr 6, 2014

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 comment has been minimized.

Copy link
Contributor

wsargent commented Apr 7, 2014

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

This comment has been minimized.

Copy link
Contributor

wsargent commented Apr 7, 2014

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

This comment has been minimized.

Copy link
Contributor

slandelle commented Apr 7, 2014

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

This comment has been minimized.

Copy link
Contributor

slandelle commented Apr 7, 2014

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

@wsargent

This comment has been minimized.

Copy link
Contributor

wsargent commented Apr 7, 2014

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

This comment has been minimized.

Copy link
Contributor

wsargent commented Apr 8, 2014

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

This comment has been minimized.

Copy link
Contributor

slandelle commented Apr 8, 2014

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

varyvol added a commit to jenkinsci/lib-async-http-client that referenced this issue Nov 20, 2018

[JENKINS-54601] Include security fixes from 1.9.40 in 1.7.24.X (#3)
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment