search

TakahikoKawasaki / nv-websocket-client

High-quality WebSocket client implementation in Java.
Apache License 2.0
2.03k stars 292 forks source link

Bad SSL behaviour - insecure and does not support SNI #106

Open ondrap opened 7 years ago

ondrap commented 7 years ago

I just spent an hour digging through this so I might be wrong, but it seems to me the wss: protocol is seriously broken.

TL;DR It connectes to a server, doesn't support SNI and doesn't check the certificate hostname. (I used tcpdump/wireshark).

The reason: I'd say this: http://stackoverflow.com/a/28031673/149901

https://github.com/TakahikoKawasaki/nv-websocket-client/blob/master/src/main/java/com/neovisionaries/ws/client/WebSocketFactory.java#L622

leads to this:

https://github.com/TakahikoKawasaki/nv-websocket-client/blob/master/src/main/java/com/neovisionaries/ws/client/SocketConnector.java#L110

I.e. as per the stackoverflow issue, it seems that connecting to the IP adress results in skipping all these checks. (BTW: haproxy requires correct use of SNI)

ondrap commented 7 years ago

I just found #66 and also found that InetSocketAddress supports getHostName(). There seems to be something weird going on; I'm doing this on fairly new android, so the version shouldn't hopefully be the issue.

blunden commented 7 years ago

Yes, that is true. I noticed the same a few days ago myself and implemented a fix for it that verifies the hostname and maintains standard http proxy support. I didn't just want to report it openly without also providing a fix.

blunden commented 7 years ago

I pushed my solution from my private bitbucket repo and submitted it as pull request #107 .

ondrap commented 7 years ago

That looks horrifying. BTW: I tried just the follwing code:

SSLSocket s = (SSLSocket) f.createSocket("my.testing.site", 443);
s.startHandshake();

Done this way I get the SNI working, however hostname checking is still not performed by Android.

blunden commented 7 years ago

@ondrap Have you tested version 2.2 yet?

ondrap commented 7 years ago

Just did that: com.neovisionaries.ws.client.HostnameUnverifiedException: The certificate of the peer (CN=xxx.yyy.org) does not match the expected hostname (zzz.yyy.org)

The sockets are somewhat picky about which function you use to create the socket, with some it does SNI (e.g. see above), with some it doesn't (probably the one this library is using).

blunden commented 7 years ago

Ok, good. I'm sure it would be appreciated if you manage to get SNI working properly without workarounds and submit a pull request. I don't have any server that requires SNI and I'm not that motivated to set one up just for this. I'm guessing the developer doesn't have one either.

ondrap commented 7 years ago

I'll try, I will need this, so hopefully I get to understand maven et al...

blunden commented 7 years ago

You don't need to use Maven, that is just for deployment of official releases.

Fork this repo, git clone your fork, make changes and commit and push them when done. You can then open a pull request from the GitHub website.

ondrap commented 7 years ago

I am just looking into the docs - according to https://developer.android.com/reference/android/net/SSLCertificateSocketFactory.html createSocket(String host, int port) does check the certificate (and I think correctly uses SNI). However I think because of Proxy support we cannot quite use it.

Looking at the docs here, it seems to me you could have used getDefaultHostnameVerifier()? https://developer.android.com/training/articles/security-ssl.html#WarningsSslSocket Unfortunately the code snippet at that page says:

// This is due to lack of SNI support in the current SSLSocket.

However, in the https://developer.android.com/reference/javax/net/ssl/SSLParameters.html it seems there is a function setServerNames(List<SNIServerName> serverNames) so I think the whole thing to do is just before connection run getDefaultSSLParameters(), use setServerNames and set the parameters on the SSL socket.

blunden commented 7 years ago

The default HostnameVerifier on Android does indeed check the hostname if invoked. The same is seemingly not true for Java's equivalent however and since this is a Java library not restricted to Android, I had to pull in a working HostnameVerifier with a compatible license. There is an internal one within the sun namespace but that is not supposed to be used by apps, could be removed at any time and doesn't necessarily exist on Android.

None of this changes the fact that the library did not check the hostname in versions prior to 2.1. I confirmed that in practice both on Android 7.1.2 and Java 8. You can't just go by Android's documentation.

ondrap commented 7 years ago

I tried to implement it and it seems quite a problem...

ondrap commented 7 years ago

I have created a PR #109 with an example implementation with both methods.

bionicbrad commented 7 years ago

This is a problem for me too...any idea how I can easily use ondrap's repo as a gradle dependency?

ondrap commented 7 years ago

Just a note -

I ended up not using SNI, because I need to support the older Androids :(

ondrap commented 7 years ago

To use it - I am not a Java expert, but this worked for me: just clone it, run mvn package and use the result JAR.

bionicbrad commented 7 years ago

Yeah...I just dropped the files into my project, and that seemed okay.

However, I need to pass a cookie with an authentication token, like so:

socket.addHeader("Cookie", "token=abc")

The cookie does not appear to be sent (I get a 401 response now, instead of ssl certificate issues). I never got past the SNI problem, so I don't know if this is your modified version, or in original source source. Maybe I'm adding the cookie header wrong?

ondrap commented 7 years ago

Did you try to use the android-api24 branch? That one is very close to the original code. However, I actually used both versions with additional header ("Authorization Bearer") on android without any problems.

bionicbrad commented 7 years ago

That one crashes pretty much immediately when I call socket.connectAsynchronously() or socket.connect().

Does it require a minimum anrdoid API of 24? That's a bit of an issue for me...

ondrap commented 7 years ago

Yes, it does. But the problem is that for API<24 neither works. It seems to me there just isn't an API on Android side to support it, the master just compiles, but doesn't seem to use SNI for API<24 (I didn't use it, but a friend with API ~ 22 said it didn't work).

bionicbrad commented 7 years ago

Is there any other way I can easily override the hostname validation? I don't want to go with the NaiveSSLContext method if I can avoid it.

EDIT: I think I might go back to your master branch, @ondrap...it seemed to work with the exception of cookies not being passed. I can find an alternative to that for authentication purposes, as long as everything else works. I'll give it a try, and will note here if it works.

bionicbrad commented 7 years ago

Alright, ignore my problem with cookies.

@ondrap, after I went back to your master branch, I found it's working perfectly (so far). My problem wasn't that cookies weren't being sent (they are). I was neglecting to send the protocol for my socket (which will also result in an 401 response from the service I'm working with).

Is there any significant reason I wouldn't want to use this for Android apps (i.e., what's the rationale for the separate android-api24 branch)?

ondrap commented 7 years ago

The android-api24 branch uses android API 24 API. The master version doesn't, so it compiles with lower API correctly.

But: it seems to me that even the master version does not work with API<24; the http call is made, but (at least on some phones??) the SNI does not seem to be sent. I haven't tested it, a friend tried to use it on his phone (API ~ 22) and SNI wasn't sent. You can try it on your devices and report the result...

bionicbrad commented 7 years ago

I see. I did get it working on a device with API v23 (from your master branch). When I get a chance, I'll try on a lower version.

or-else commented 7 years ago

The timeout issue in https://github.com/TakahikoKawasaki/nv-websocket-client/pull/109 seems to be easily solvable by calling setSoTimeout right after creating the socket.

TakahikoKawasaki commented 7 years ago

I'm sorry I could have time to follow this discussion, but the problem seems to be serious? Has any pull request been posted?

testica commented 7 years ago

I'm having the same issue.

com.neovisionaries.ws.client.HostnameUnverifiedException: The certificate of the peer (CN=xx.yy.zz.co) does not match the expected hostname (ws.xx.yy.zz.co)
       at com.neovisionaries.ws.client.SocketConnector.verifyHostname(SocketConnector.java:171)
       at com.neovisionaries.ws.client.SocketConnector.doConnect(SocketConnector.java:126)
       at com.neovisionaries.ws.client.SocketConnector.connect(SocketConnector.java:83)
       at com.neovisionaries.ws.client.WebSocket.connect(WebSocket.java:2152)
       at com.neovisionaries.ws.client.ConnectThread.runMain(ConnectThread.java:32)
       at com.neovisionaries.ws.client.WebSocketThread.run(WebSocketThread.java:45)

It happens in devices with android version 6 and older.. (7 and 8 works)

ondrap commented 7 years ago

The problem is that Java < 8 and Android API < v23 (if I remember it correctly) doesn't support SNI and there doesn't seem a way to force it. I haven't tried with the higher APIs/Java, if my pull request is needed or not.

or-else commented 7 years ago

Just tested SNI with API 22, Java 7 using org.java_websocket.client.WebSocketClient (https://github.com/TooTallNate/Java-WebSocket/) and LetsEncrypt certificate and it worked fine. Here is the relevant code: https://github.com/tinode/android-example/blob/master/tinodesdk/src/main/java/co/tinode/tinodesdk/Connection.java

ondrap commented 7 years ago

@or-else Does it check certificates at all? The problem with the older APIs is that it doesn't check the certificates; obviously SNI seems to work then (unless you use the default settings of HA-proxy that switches virtual hosts based on SNI...).

vlasky commented 6 years ago

@TakahikoKawasaki nv-websocket-client needs SNI support so that WebSockets over SSL works with Cloudflare.

Two days ago, we enabled Cloudflare on our domain and our Android app broke.

Cloudflare has supported websocket connections since 2014. and we confirm that it works with our web browser client and our iOS app, but not our Android app which uses nv-websocket-client.

We debugged our Android app and determined that the reason WebSocket connections are failing is because nv-websocket-client is not sending SNI information to Cloudflare's servers. As a result, Cloudflare's servers do not know where to direct WebSocket traffic to.

It is not possible to work around this by setting a default IP address because Cloudflare's servers are not dedicated to individual customers - they are shared with all their customers.

As a result, we have had to disable Cloudflare for the time being.

Will you be able to implement a solution?

I found something here that might be of assistance:

https://stackoverflow.com/questions/24397419/android-https-sni-support-using-sslcertificatesocketfactory/40365234

TakahikoKawasaki commented 6 years ago

@vlasky Released nv-websocket-client 2.4. The following methods have been added to WebSocketFactory class and ProxySettings class.

  1. getServerNames()
  2. setServerNames(String[])
  3. setServerName(String)

If the underlying system has SSLParameters.setServerNames(List<SNIServerName>) method, the method is called via reflection. As mentioned in this thread, the method is a relatively new and it is not available before Java 1.8 and Android 7.0 (API Level 24). It is the reason the method is called via reflection.

I confirmed SSLParameters.setServerNames(List) was called for wss:. However, what I tested was only that. So, please check whether nv-websocket-client 2.4 can solve your issue or not.

Sorry, I've not read details of this thread yet. I just found SSLParameters.setServerNames when I googled "SNI java", and I reflexively implemented WebSocketFactory.setServerNames.

vlasky commented 6 years ago

@TakahikoKawasaki my testing shows that nv-websocket-client 2.4 solves the SNI issue for phones running Android 7.0 (Nougat) and onwards, but not for earlier versions which are currently the majority of Android users.

My idea for fixing nv-websocket-client in earlier versions of Android would be to perhaps create a Java HttpsURLConnection that is used only for negotiating an SNI connection with the server, then after the SNI phase is completed, nv-websocket-client would somehow take over HttpsURLConnection's internal socket object and use it.

Of course, I haven't had time to try and test the feasibility or practicality of this idea due to time pressures. Instead, I managed to work around the problem by signing up to Cloudflare's Pro plan.

All of Cloudflare's premium plans provide legacy support for clients without SNI. Basically, Cloudflare creates an SSL certificate for each edge server IP address with Subject Alternative Names (SANs) covering all the domains belonging to all premium customers assigned to that edge server.

This means that even if a client connects to Cloudflare's edge server without providing SNI, the SSL certificate will be verified as valid. Cloudflare's web server can then read the HOST request header sent by the client and know which server to forward the SSL connection to.

Here are some links on Cloudflare's website discussing SNI:

Although I have found an effective workaround for my immediate problem that now allows older Android phones to use our webapp, there is still a need for nv-websocket-client to have SNI support to cover the majority of Android users running versions of Android prior to Nougat.

Until that happens, those Android users and creators of WebSocket-enabled webapps will continue to experience the following limitations:

panekzogen commented 6 years ago

Hello there. I didn't read full issue. But i think that i have a simple solution for this.

Someone wrote above about using SSLSocketFactoryImpl.createSocket(String, int) instead of SSLSocketFactoryImpl.createSocket(). There are two difference, first method sets host and rawHostname fields and performs connect.

Now take a look on the part of code where doing SNI processing.

if (enableSNIExtension) {
        String hostname = getRawHostnameSE();
        if (hostname != null && hostname.indexOf('.') > 0 &&
                !IPAddressUtil.isIPv4LiteralAddress(hostname) &&
                !IPAddressUtil.isIPv6LiteralAddress(hostname)) {
            clientHelloMessage.addServerNameIndicationExtension(hostname);
        }
}

As you can see if rawHostname wasn't set, SNI proc will be skipped.

Solution: I propose to call SSLSocketImpl.setHost(String) right after SSLSocketFactoryImpl.createSocket(). P.S. I have poor knowledge about ssl and i could be wrong.

hpinhal commented 6 years ago

Any update regarding this topic? Is it planned to support SNI on pre-Nougat versions?

twogood commented 5 years ago

I think that I got SNI working on Android 4.4.1 and up with this code:

            String serverName = Uri.parse(url).getHost();

            webSocketFactory.setServerName(serverName);
            webSocket = webSocketFactory.createSocket(url);

  Socket socket = webSocket.getSocket();
        Log.d(TAG, "Enabling SNI for " + serverName);
        try {
            Method method = socket.getClass().getMethod("setHostname", String.class);
            method.invoke(socket, serverName);
        } catch (Exception e) {
            Log.w(TAG, "SNI configuration failed", e);
        }

Inspired by: https://github.com/smarek/httpclient-android/issues/7

minichma commented 5 years ago

We're seeing this issue when using Azure App Services in the basic plan. The basic plan doesn't support IP-based TLS bindings, so an upgrade to the Standard plan is required. We use the basic plan for our dev and test environments, so we need to upgrade all of them, which is quite costly.

twogood commented 5 years ago

@minichma Did you try my code snippet above?

minichma commented 5 years ago

@twogood No, we didn't, because we consider using reflection kind of a last resort solution. In our case we prefer using IP-based TLS even though it's more costly. However, it's good to know that another workaround exists, so thanks for sharing! Maybe we'll add it as a fallback, because in situations where it wouldn't work, it would at least not do any harm. But then again, it would make testing even more complex and error prone.

amentan commented 5 years ago

Android5.0 and Android6.0

issue: The certificate of the peer does not match the expected hostname (xxx.com)

solution: on com.neovisionaries:nv-websocket-client:2.2

webSocket = new WebSocketFactory().createSocket(uri, 3000);

java.lang.reflect.Method setHostnameMethod = ((SSLSocket) webSocket.getSocket()) .getClass().getMethod("setHostname", String.class); setHostnameMethod.invoke((webSocket.getSocket()),"xxx.com");

webSocket.setFrameQueueSize(5)//设置帧队列最大值为5 FRAME_QUEUE_SIZE .setMissingCloseFrameAllowed(false)//设置不允许服务端关闭连接却未发送关闭帧 .addListener(new WsListener())//添加回调监听 wsListener .connectAsynchronously();//异步