Exception: Handshake has already been started on Android 5+ #4718

ganskef · 2016-01-15T22:28:08Z

With https://github.com/ganskef/LittleProxy-mitm I use SSL on all Java platforms. Android devices up to 4.4 are supported well since #3904 is fixed. With Android 5.0 it's been better since #4116 is fixed, but there is still one concern with Android 5+. All connection attempts are interrupted with this:

java.lang.IllegalStateException: Handshake has already been started
    at com.android.org.conscrypt.OpenSSLEngineImpl.beginHandshake(OpenSSLEngineImpl.java:139) ~[na:0.0]
    at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1350) ~[na:0.0]
    at io.netty.handler.ssl.SslHandler.channelActive(SslHandler.java:1389) ~[na:0.0]
    at io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelActiveNow(ChannelHandlerInvokerUtil.java:48) ~[na:0.0]
    at io.netty.channel.DefaultChannelHandlerInvoker.invokeChannelActive(DefaultChannelHandlerInvoker.java:79) ~[na:0.0]
    at io.netty.channel.AbstractChannelHandlerContext.fireChannelActive(AbstractChannelHandlerContext.java:125) ~[na:0.0]
    at io.netty.channel.DefaultChannelPipeline.fireChannelActive(DefaultChannelPipeline.java:884) ~[na:0.0]
    at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.fulfillConnectPromise(AbstractNioChannel.java:260) ~[na:0.0]
    at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.finishConnect(AbstractNioChannel.java:290) ~[na:0.0]

This hint is figured out by @MediumOne: After commenting out this line it's mostly working, but not perfect:
https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L1266

diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
index 7dde9d0..5f4939d 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -1263,7 +1263,7 @@
             // Begin the initial handshake.
             // channelActive() event has been fired already, which means this.channelActive() will
             // not be invoked. We have to initialize here instead.
-            handshake(null);
+//            handshake(null);
         } else {
             // channelActive() event has not been fired yet.  this.channelOpen() will be invoked
             // and initialization will occur there.

On Mac OS X it looks good (Windows is not been tested), but on Linux and Android 5+ it's not totally stable, so it's not a fix. Some connections are cancelled, and sometimes blocking occurs, mostly at the first connection.

I can reproduce this behavior on emulators, with Android 5.0, 5.1 and 6.0. This code was been introduced with this commit: 50fafdc on 06/07/2012 by Trustin. Do you have any suggestions please?

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

Scottmitch · 2016-01-19T22:27:25Z

@ganskef - What version of netty are you using?

com.android.org.conscrypt.OpenSSLEngineImpl.beginHandshake does not permit multiple calls to beginHandshake(). I wonder if the assumed logic between handlerAdded and channelActive is being violated ... can you try with PR #4716? If you are using Netty 4.0 just cherry-pick the change back on top of the 4.0 branch HEAD and try with this.

ganskef · 2016-01-19T22:48:14Z

@Scottmitch - I use the Netty 4.1 branch and I've checked out bf24ffd but this happens with 4.1.0.Beta8 and before too.
I'll give #4716 a try.

ganskef · 2016-01-19T23:34:18Z

@Scottmitch - now I've built commit e578e92
Author: Norman Maurer norman_maurer@apple.com
Date: Fri Jan 15 14:23:41 2016 +0100

And I depend on netty-handler-4.1.0.CR1-SNAPSHOT.jar but there's no difference:

17655  2016-01-19 18:11:07,633 DEBUG [MoCuishle-0-ProxyToServerWorker-2] o.l.p.i.ProxyToServerConnection - (HANDSHAKING) [id: 0x7fa20aca, /10.0.3.15:49450 => aus5.mozilla.org/63.245.213.49:443]: ConnectionFlowStep failed
java.lang.IllegalStateException: Handshake has already been started
    at com.android.org.conscrypt.OpenSSLEngineImpl.beginHandshake(OpenSSLEngineImpl.java:139) ~[na:0.0]
    at io.netty.handler.ssl.SslHandler.handshake(SslHandler.java:1350) ~[na:0.0]
    at io.netty.handler.ssl.SslHandler.channelActive(SslHandler.java:1389) ~[na:0.0]
    at io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelActiveNow(ChannelHandlerInvokerUtil.java:48) ~[na:0.0]
    at io.netty.channel.DefaultChannelHandlerInvoker.invokeChannelActive(DefaultChannelHandlerInvoker.java:79) ~[na:0.0]
    at io.netty.channel.AbstractChannelHandlerContext.invokeChannelActive(AbstractChannelHandlerContext.java:379) ~[na:0.0]
    at io.netty.channel.AbstractChannelHandlerContext.fireChannelActive(AbstractChannelHandlerContext.java:134) ~[na:0.0]

Scottmitch · 2016-01-20T00:36:53Z

@ganskef - Thanks for trying. Will dig deeper and get back to you.

Scottmitch · 2016-01-20T00:49:31Z

To reiterate/clarify what I mentioned before ... jdk's SSLEngineImpl and Netty's OpenSslEngine both tolerate multiple calls to beginHandshake() and just proceed if already in the process of doing a handshake but android's OpenSSLEngineImpl will throw an IllegalStateException exception in this case. The SSLEngine.beginHandshake is a bit vague as to the expected behavior in this scenario, but it seems reasonable for use to make sure we only call beginHandshake 1 time. @normanmaurer - WDYT?

Scottmitch · 2016-01-20T01:08:22Z

@nmittler - What are your thoughts? I would be curious to see if Android devs would be willing to make behavior consistent with OpenJdk SSLEngineImpl (as indicated above). IllegalStateException is a generic exception but it is defined as IllegalStateException - if the client/server mode has not yet been set. on OpenJdk's interface. The Android JDK API for SSLEngine.beginHandshake has a similar but arguably "more generic" definition IllegalStateException - if the engine does not have all the needed settings (e.g. client/server mode not set)..

Scottmitch · 2016-01-20T04:20:20Z

Opened an an issue to discuss with Android folks https://code.google.com/p/android/issues/detail?id=199204

nmittler · 2016-01-20T15:31:44Z

@Scottmitch I would not expect that we would be calling beginHandshake() more than once ... is there any reason that we do that? Would it be hard to fix? Without getting too far into the details, I wouldn't rely on lenient behavior of some implementations if we can avoid it.

Scottmitch · 2016-01-20T16:41:16Z

@ganskef - Can you provide the following to help with the android issue?

Android build
Which Android build are you using? (e.g. KVT49L)

Device used
Which device did you use to reproduce this issue?

Scottmitch · 2016-01-20T16:47:48Z

is there any reason that we do that?

@nmittler - We currently may call it from handlerAdded and channelActive. I'm guessing we don't guard against this because it is not defined to be a problem in the SSLEngine interface, and the current implementations we support (JDK SSL engine, Netty OpenSslEngine) handle this gracefully.

Without getting too far into the details, I wouldn't rely on lenient behavior of some implementations if we can avoid it.

We can avoid this if necessary (which will have to do if we want to support currently released versions of Android 5+ and 6+) but it would be nice to have clarity / consistency in JDK behavior.

nmittler · 2016-01-20T17:05:50Z

... it would be nice to have clarity / consistency in JDK behavior.

Agreed :)

So it sounds like we need to fix Netty regardless, correct? That would mean that this bug is not dependent on resolving https://code.google.com/p/android/issues/detail?id=199204.

Scottmitch · 2016-01-20T17:56:53Z

Yes if we want to support existing Android releases (5+ and 6+) then Netty code needs to change. I have a PR pending.

ganskef · 2016-01-20T18:16:18Z

@Scottmitch - I've seen it on a Nexus 5 device too but it's not my own. So I have to use the emulator, by Genymotion my choice:

5.0 LRX21M
5.1 LMY47D
6.0 MRA58K

Handshaking works well with:

4.4.4 KTU84Q CyanogenMod on a Samsung i9300
4.1.1 JRO03S Genymotion emulator...

Scottmitch · 2016-01-20T18:53:57Z

@ganskef - Thanks for the info. Yes Android 4.x appears to use a different implementation of SSLEngineImpl which behaves the same way as the OpenJDK's SSLEngineImpl, and so I wouldn't expect to see this issue.

normanmaurer · 2016-01-20T18:57:58Z

@Scottmitch @nmittler I can take a stab on this once back from vacation (next week).

Scottmitch · 2016-01-20T19:05:13Z

@normanmaurer - I have a PR pending, which you will be on the hook for reviewing 😉

johnou · 2016-01-22T09:35:56Z

@Scottmitch internal PR or just not published?

normanmaurer · 2016-01-22T13:00:01Z

@johnou yes should be upstream soon.

johnou · 2016-01-25T15:04:24Z

@Scottmitch think the fix for this will make 4.0.34.Final?

normanmaurer · 2016-01-25T15:06:53Z

Yes

Am 25.01.2016 um 16:04 schrieb Johno Crawford notifications@github.com:

@Scottmitch think the fix for this will make 4.0.34.Final?

—
Reply to this email directly or view it on GitHub.

Motivation: Not all SSLEngine implementations permit beginHandshake being called while a handshake is in progress during the initial handshake. We should ensure we only go through the initial handshake code once to prevent unexpected exceptions from being thrown. Modifications: - Only call beginHandshake if there is not currently a handshake in progress Result: SslHandler's handshake method is compatible with OpenSSLEngineImpl in Android 5.0+ and 6.0+. Fixes netty#4718

Motivation: Not all SSLEngine implementations permit beginHandshake being called while a handshake is in progress during the initial handshake. We should ensure we only go through the initial handshake code once to prevent unexpected exceptions from being thrown. Modifications: - Only call beginHandshake if there is not currently a handshake in progress Result: SslHandler's handshake method is compatible with OpenSSLEngineImpl in Android 5.0+ and 6.0+. Fixes #4718

normanmaurer · 2016-01-28T12:28:56Z

Fixed by #4764. Please re-open if you still see issues.

ganskef · 2016-01-28T20:24:13Z

I've integrated it here but I haven't got an Android 5+ device. It works well :-) in emulator. Will try it with desktop platforms too.

Sorry, it's binary only, see http://ganskef.github.io/MoCuishle/license/.

Motivation: Not all SSLEngine implementations permit beginHandshake being called while a handshake is in progress during the initial handshake. We should ensure we only go through the initial handshake code once to prevent unexpected exceptions from being thrown. Modifications: - Only call beginHandshake if there is not currently a handshake in progress Result: SslHandler's handshake method is compatible with OpenSSLEngineImpl in Android 5.0+ and 6.0+. Fixes netty#4718

Scottmitch self-assigned this Jan 20, 2016

Scottmitch added this to the 4.0.34.Final milestone Jan 20, 2016

Scottmitch added the android label Jan 20, 2016

Scottmitch mentioned this issue Jan 27, 2016

SslHandler should call beginHanshake once for the initial handshake #4764

Closed

normanmaurer closed this as completed in e1d34ef Jan 28, 2016

ganskef referenced this issue in ganskef/LittleProxy-mitm Feb 29, 2016

Fixed Android 5+ using Netty 4.1.0.CR2-SNAPSHOT documented.

8fb7c92

MediumOne mentioned this issue Mar 2, 2016

Consider upgrading netty version to 4.0.34.Final adamfisk/LittleProxy#274

Closed

janseris mentioned this issue May 28, 2022

SQL Server pre-login handshake failed Android 5,6,8.1 dotnet/android#6990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exception: Handshake has already been started on Android 5+ #4718

Exception: Handshake has already been started on Android 5+ #4718

ganskef commented Jan 15, 2016

Scottmitch commented Jan 19, 2016

ganskef commented Jan 19, 2016

ganskef commented Jan 19, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

nmittler commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

nmittler commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

ganskef commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

normanmaurer commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

johnou commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

johnou commented Jan 25, 2016

normanmaurer commented Jan 25, 2016

normanmaurer commented Jan 28, 2016

ganskef commented Jan 28, 2016

Exception: Handshake has already been started on Android 5+ #4718

Exception: Handshake has already been started on Android 5+ #4718

Comments

ganskef commented Jan 15, 2016

Scottmitch commented Jan 19, 2016

ganskef commented Jan 19, 2016

ganskef commented Jan 19, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

nmittler commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

nmittler commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

ganskef commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

normanmaurer commented Jan 20, 2016

Scottmitch commented Jan 20, 2016

johnou commented Jan 22, 2016

normanmaurer commented Jan 22, 2016

johnou commented Jan 25, 2016

normanmaurer commented Jan 25, 2016

normanmaurer commented Jan 28, 2016

ganskef commented Jan 28, 2016