Double initialization of SSLSession in GelfTCPSSLSender

[alexander-katanov]

Hi,

I think we have a problem in method connect() of GelfTCPSSLSender class

logstash-gelf/src/main/java/biz/paluch/logging/gelf/intern/sender/GelfTCPSSLSender.java

Lines 55 to 65 in 7553c8e

	protected boolean connect() throws IOException {
	this.sslEngine = sslContext.createSSLEngine();
	this.sslEngine.setUseClientMode(true);
	this.sslSession = sslEngine.getSession();
	if (super.connect()) {
	// Begin handshake
	sslEngine.beginHandshake();
	doHandshake(channel(), sslEngine, ByteBuffer.allocate(sslSession.getPacketBufferSize()),
	ByteBuffer.allocate(sslSession.getPacketBufferSize()));

As you can see, you initialize sslEngine and sslSession fields right away, after connect method is done, we will have channel with sslEngine.
In multithreaded environment if two threads will call sendMessage method at the same time, one will initialise channel and sslSession, and second will recreate sslEngine and sslSession.

This leads us to situation like javax.net.ssl.SSLException: Received fatal alert: ... on client side,
and javax.crypto.BadPaddingException: ciphertext sanity check failed on server side

Make sure that:

• You have read the contribution guidelines.
• You specify the logstash-gelf version and environment so it's obvious which version is affected
• You provide a reproducible test case (either descriptive of as JUnit test) if it's a bug or the expected behavior differs from the actual behavior.

[mp911de]

Thanks for reporting an issue.

In multithreaded environment if two threads will call sendMessage method at the same time, one will

The call to connect() in sendMessage(…) is guarded with a lock, so there must be a loop hole somewhere. It would also make sense to move initialization into the if(super.connect(…)) block, but that's an optimization.

logstash-gelf/src/main/java/biz/paluch/logging/gelf/intern/sender/GelfTCPSender.java

Lines 100 to 105 in a8d749e

	// (re)-connect if necessary
	if (!isConnected()) {
	synchronized (ioLock) {
	connect();
	}
	}

I'm not exactly sure, I'd suspect rather concurrent access to SSLEngine and SSLSession changing their state without proper synchronization.

[alexander-katanov]

loop hole in overridden connect call in GelfTCPSSLSender, it recreates sslEngine and sslSession.
you should check whether connection is established or not inside overridden connect method, and accept my PR.
Or move initialization of sslEngine and sslSession under if(super.connect(…)), as you proposed.

Any way both solutions work, you should choose which one to use =)

[mp911de]

Thanks for the hint. It makes sense now. The double checked lock isn't properly implemented for SSL because two threads can sequentially access connect() and the second call to isConnected() happens in GelfTCPSender. A second caller recreates the SSL resources and that's the bug.

Do you want to adjust your PR to move the initialization of sslEngine and sslSession under if(super.connect(…))?

[alexander-katanov]

I don't understand your point, but moved initialization under if(super.connect(...)).

I just want to make it clear.
Two threads want to send a message.

1. They call sendMessage() method on object which is GelfTCPSSLSender instance.
   GelfTCPSSLSender class doesn't have method sendMessage(), so call goes to method sendMessage() in GelfTCPSender class of object which is GelfTCPSSLSender instance.
2. in method sendMessage() has check whether it is connected or not. (in our case it's not)
3. then happens synchronization for method connect()
4. in connect() method of GelfTCPSSLSender object we are trying to test connection one more time.
   It happens under synchronization, and if connection was established, no double initialization of sslEngine field wouldn't happen.

[mp911de]

Thanks a lot.

I don't understand your point

You basically explained the flow. What I wanted to express is that in point 3., we have synchronization but what also happens is, that two threads get to the synchronized block. The first thread performs the connect, while the second thread waits. The first thread completes the connect but the second thread isn't aware that the connect is done.

The second thread then continues with the connect() method because the synchronized lock is released and then we get to the double initialization.

[alexander-katanov]

In my first implementation second thread will be aware about connection was established because of isConnected check in the beginning of connect method. because it was under synchronization too.

[alexander-katanov]

But I agree with moving initialization of sslEngine field after connect establishing, because it makes more sense.

[mp911de]

All calls to connect() are synchronized (i.e. executed serially and not concurrently) regardless of where methods are declared. Both threads come to the if (super.connect()) { instruction in GelfTCPSSLSender, the second thread just yields false here and does not attempt to connect a second time.

[mp911de]

That's fixed via #182 now. Thanks a lot.

[alexander-katanov]

Okay, thank you.