Skip to content

Commit

Permalink
Fix IndexOutOfBoundsException from SslHandler on JDK 8
Browse files Browse the repository at this point in the history 
Motivation:

When SslHandler.unwrap() copies SSL records into a heap buffer, it does
not update the start offset, causing IndexOutOfBoundsException.

Modifications:

- Copy to a heap buffer before calling unwrap() for simplicity
- Do not copy an empty buffer to a heap buffer.
  - unwrap(... EMPTY_BUFFER ...) never involves copying now.
- Use better parameter names for unwrap()
- Clean-up log messages

Result:

- Bugs fixed
- Cleaner code
  • Loading branch information
@trustin
trustin committed Jan 13, 2015
1 parent 1bb818b commit 392fb76
Showing 1 changed file with 32 additions and 43 deletions.
75 changes: 32 additions & 43 deletions handler/src/main/java/io/netty/handler/ssl/SslHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ public void run() {
flush(ctx);
} catch (Exception e) {
if (!future.tryFailure(e)) {
logger.warn("flush() raised a masked exception.", e);
logger.warn("{} flush() raised a masked exception.", ctx.channel(), e);
}
}
}
Expand Down Expand Up @@ -524,7 +524,7 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
}
}
} catch (SSLException e) {
setHandshakeFailure(e);
setHandshakeFailure(ctx, e);
throw e;
} finally {
finishWrap(ctx, out, promise, inUnwrap);
Expand Down Expand Up @@ -599,7 +599,7 @@ private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) throws
}
}
} catch (SSLException e) {
setHandshakeFailure(e);
setHandshakeFailure(ctx, e);
throw e;
} finally {
if (out != null) {
Expand Down Expand Up @@ -669,7 +669,7 @@ private SSLEngineResult wrap(ByteBufAllocator alloc, SSLEngine engine, ByteBuf i
public void channelInactive(ChannelHandlerContext ctx) throws Exception {
// Make sure to release SSLEngine,
// and notify the handshake future if the connection has been closed during handshake.
setHandshakeFailure(CHANNEL_CLOSED);
setHandshakeFailure(ctx, CHANNEL_CLOSED);
super.channelInactive(ctx);
}

Expand All @@ -680,8 +680,8 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E
// 'broken pipe' error after sending close_notify.
if (logger.isDebugEnabled()) {
logger.debug(
"Swallowing a harmless 'connection reset by peer / broken pipe' error that occurred " +
"while writing close_notify in response to the peer's close_notify", cause);
"{} Swallowing a harmless 'connection reset by peer / broken pipe' error that occurred " +
"while writing close_notify in response to the peer's close_notify", ctx.channel(), cause);
}

// Close the connection explicitly just in case the transport
Expand Down Expand Up @@ -916,7 +916,19 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
// See https://github.com/netty/netty/issues/1534

in.skipBytes(totalLength);
unwrap(ctx, in, startOffset, totalLength);

// If SSLEngine expects a heap buffer for unwrapping, do the conversion.
if (in.isDirect() && wantsInboundHeapBuffer) {
ByteBuf copy = ctx.alloc().heapBuffer(totalLength);
try {
copy.writeBytes(in, startOffset, totalLength);
unwrap(ctx, copy, 0, totalLength);
} finally {
copy.release();
}
} else {
unwrap(ctx, in, startOffset, totalLength);
}
}

if (nonSslRecord) {
Expand All @@ -925,7 +937,7 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
"not an SSL/TLS record: " + ByteBufUtil.hexDump(in));
in.skipBytes(in.readableBytes());
ctx.fireExceptionCaught(e);
setHandshakeFailure(e);
setHandshakeFailure(ctx, e);
}
}

Expand Down Expand Up @@ -954,37 +966,23 @@ private void unwrapNonAppData(ChannelHandlerContext ctx) throws SSLException {
/**
* Unwraps inbound SSL records.
*/
private void unwrap(ChannelHandlerContext ctx, ByteBuf packet,
int readerIndex, int initialOutAppBufCapacity) throws SSLException {

int len = initialOutAppBufCapacity;
// If SSLEngine expects a heap buffer for unwrapping, do the conversion.
final ByteBuf oldPacket;
final ByteBuf newPacket;
if (wantsInboundHeapBuffer && packet.isDirect()) {
newPacket = ctx.alloc().heapBuffer(packet.readableBytes());
newPacket.writeBytes(packet, readerIndex, len);
oldPacket = packet;
packet = newPacket;
} else {
oldPacket = null;
newPacket = null;
}
private void unwrap(
ChannelHandlerContext ctx, ByteBuf packet, int offset, int length) throws SSLException {

boolean wrapLater = false;
boolean notifyClosure = false;
ByteBuf decodeOut = allocate(ctx, initialOutAppBufCapacity);
ByteBuf decodeOut = allocate(ctx, length);
try {
for (;;) {
final SSLEngineResult result = unwrap(engine, packet, readerIndex, len, decodeOut);
final SSLEngineResult result = unwrap(engine, packet, offset, length, decodeOut);
final Status status = result.getStatus();
final HandshakeStatus handshakeStatus = result.getHandshakeStatus();
final int produced = result.bytesProduced();
final int consumed = result.bytesConsumed();

// Update indexes for the next iteration
readerIndex += consumed;
len -= consumed;
offset += consumed;
length -= consumed;

if (status == Status.CLOSED) {
// notify about the CLOSED state of the SSLEngine. See #137
Expand Down Expand Up @@ -1019,7 +1017,7 @@ private void unwrap(ChannelHandlerContext ctx, ByteBuf packet,

break;
default:
throw new IllegalStateException("Unknown handshake status: " + handshakeStatus);
throw new IllegalStateException("unknown handshake status: " + handshakeStatus);
}

if (status == Status.BUFFER_UNDERFLOW || consumed == 0 && produced == 0) {
Expand All @@ -1035,16 +1033,9 @@ private void unwrap(ChannelHandlerContext ctx, ByteBuf packet,
sslCloseFuture.trySuccess(ctx.channel());
}
} catch (SSLException e) {
setHandshakeFailure(e);
setHandshakeFailure(ctx, e);
throw e;
} finally {
// If we converted packet into a heap buffer at the beginning of this method,
// we should synchronize the position of the original buffer.
if (newPacket != null) {
oldPacket.readerIndex(readerIndex + packet.readerIndex());
newPacket.release();
}

if (decodeOut.isReadable()) {
ctx.fireChannelRead(decodeOut);
} else {
Expand Down Expand Up @@ -1227,7 +1218,7 @@ private void setHandshakeSuccess() {
handshakePromise.trySuccess(ctx.channel());

if (logger.isDebugEnabled()) {
logger.debug(ctx.channel() + " HANDSHAKEN: " + engine.getSession().getCipherSuite());
logger.debug("{} HANDSHAKEN: {}", ctx.channel(), engine.getSession().getCipherSuite());
}
ctx.fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS);

Expand All @@ -1240,7 +1231,7 @@ private void setHandshakeSuccess() {
/**
* Notify all the handshake futures about the failure during the handshake.
*/
private void setHandshakeFailure(Throwable cause) {
private void setHandshakeFailure(ChannelHandlerContext ctx, Throwable cause) {
// Release all resources such as internal buffers that SSLEngine
// is managing.
engine.closeOutbound();
Expand All @@ -1254,7 +1245,7 @@ private void setHandshakeFailure(Throwable cause) {
// See https://github.com/netty/netty/issues/1340
String msg = e.getMessage();
if (msg == null || !msg.contains("possible truncation attack")) {
logger.debug("SSLEngine.closeInbound() raised an exception.", e);
logger.debug("{} SSLEngine.closeInbound() raised an exception.", ctx.channel(), e);
}
}
notifyHandshakeFailure(cause);
Expand Down Expand Up @@ -1438,9 +1429,7 @@ private void safeClose(
timeoutFuture = ctx.executor().schedule(new Runnable() {
@Override
public void run() {
logger.warn(
ctx.channel() + " last write attempt timed out." +
" Force-closing the connection.");
logger.warn("{} Last write attempt timed out; force-closing the connection.", ctx.channel());
ctx.close(promise);
}
}, closeNotifyTimeoutMillis, TimeUnit.MILLISECONDS);
Expand Down

0 comments on commit 392fb76

Please sign in to comment.