Fix overzealous assertion in SslHandler.decode

Motivation:

This AE was seen in the wild at a non-negligible rate among AeroFS
clients (JDK 8, TLS 1.2, mutual auth with RSA certs).

Upon examination of SslHandler's code a few things became apparent:

- the AE is unnecessary given the contract of decode()
- the AE was introduced between 3.8 and 3.9
- the AE is no longer present in in 4.x and master
- branches that do not have the AE skip all the bytes being fed to
  unwrap()

It is not entirely clear what sequence of SSL records can trip the
assert but it seems to happen before the handshake is completed. The
little detailed data we've been able to gather shows the assert being
triggered when

- SSLEngine.unwrap returns NEED_WRAP
- the remaining buffer is a TLS heartbeat record

Likewise, it is not entirely clear if skipping the remaining bytes is
the right thing to do or if they should be fed back to unwrap.

Modifications:

Mirror behavior in newer versions by removing the assert and skipping
bytes fed to unwrap()

Add logging in an effort to get a better understanding of this corner
case.

Result:

Avoid crashes
This commit is contained in:
Hugues Bruant 2015-04-03 17:45:36 -04:00 committed by Trustin Lee
parent c44c5a0453
commit 1fb6c7e115

View File

@ -913,9 +913,9 @@ public class SslHandler extends FrameDecoder
//
// See https://github.com/netty/netty/issues/1534
final ByteBuffer inNetBuf = in.toByteBuffer(in.readerIndex(), totalLength);
unwrapped = unwrap(ctx, channel, in, inNetBuf, totalLength, true);
assert !inNetBuf.hasRemaining() || engine.isInboundDone();
in.skipBytes(totalLength);
final ByteBuffer inNetBuf = in.toByteBuffer(startOffset, totalLength);
unwrapped = unwrap(ctx, channel, inNetBuf, totalLength, true);
}
if (nonSslRecord) {
@ -1229,7 +1229,7 @@ public class SslHandler extends FrameDecoder
*/
private void unwrapNonAppData(
ChannelHandlerContext ctx, Channel channel, boolean mightNeedHandshake) throws SSLException {
unwrap(ctx, channel, ChannelBuffers.EMPTY_BUFFER, EMPTY_BUFFER, -1, mightNeedHandshake);
unwrap(ctx, channel, EMPTY_BUFFER, -1, mightNeedHandshake);
}
/**
@ -1237,10 +1237,9 @@ public class SslHandler extends FrameDecoder
*/
private ChannelBuffer unwrap(
ChannelHandlerContext ctx, Channel channel,
ChannelBuffer nettyInNetBuf, ByteBuffer nioInNetBuf,
ByteBuffer nioInNetBuf,
int initialNettyOutAppBufCapacity, boolean mightNeedHandshake) throws SSLException {
final int nettyInNetBufStartOffset = nettyInNetBuf.readerIndex();
final int nioInNetBufStartOffset = nioInNetBuf.position();
final ByteBuffer nioOutAppBuf = bufferPool.acquireBuffer();
@ -1298,10 +1297,6 @@ public class SslHandler extends FrameDecoder
} finally {
outAppBuf.flip();
// Sync the offset of the inbound buffer.
nettyInNetBuf.readerIndex(
nettyInNetBufStartOffset + nioInNetBuf.position() - nioInNetBufStartOffset);
// Copy the unwrapped data into a smaller buffer.
if (outAppBuf.hasRemaining()) {
if (nettyOutAppBuf == null) {
@ -1349,6 +1344,20 @@ public class SslHandler extends FrameDecoder
if (result.getStatus() == Status.BUFFER_UNDERFLOW ||
result.bytesConsumed() == 0 && result.bytesProduced() == 0) {
if (nioInNetBuf.hasRemaining() && !engine.isInboundDone()) {
// We expect SSLEngine to consume all the bytes we feed it, but
// empirical evidence indicates that we sometimes end up with leftovers
// Log when this happens to get a better understanding of this corner
// case.
// See https://github.com/netty/netty/pull/3584
logger.warn("Unexpected leftover data after SSLEngine.unwrap():"
+ " status=" + result.getStatus()
+ " handshakeStatus=" + result.getHandshakeStatus()
+ " consumed=" + result.bytesConsumed()
+ " produced=" + result.bytesProduced()
+ " remaining=" + nioInNetBuf.remaining()
+ " data=" + ChannelBuffers.hexDump(ChannelBuffers.wrappedBuffer(nioInNetBuf)));
}
break;
}
}