Skip to content

Commit

Permalink
SslHandler wrap reentry bug fix (#11133)
Browse files Browse the repository at this point in the history 
Motivation:
SslHandler's wrap method notifies the handshakeFuture and sends a
SslHandshakeCompletionEvent user event down the pipeline before writing
the plaintext that has just been wrapped. It is possible the application
may write as a result of these events and re-enter into wrap to write
more data. This will result in out of sequence data and result in alerts
such as SSLV3_ALERT_BAD_RECORD_MAC.

Modifications:
- SslHandler wrap should write any pending data before notifying
  promises, generating user events, or anything else that may create a
  re-entry scenario.

Result:
SslHandler will wrap/write data in the same order.
  • Loading branch information
@Scottmitch
Scottmitch committed Apr 1, 2021
1 parent e4dd6ee commit 6b48e69
Show file tree
Hide file tree
Showing 3 changed files with 179 additions and 87 deletions.
131 changes: 47 additions & 84 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 @@ -821,17 +821,14 @@ private void wrapAndFlush(ChannelHandlerContext ctx) throws SSLException {
// This method will not call setHandshakeFailure(...) !
private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLException {
ByteBuf out = null;
ChannelPromise promise = null;
ByteBufAllocator alloc = ctx.alloc();
boolean needUnwrap = false;
ByteBuf buf = null;
try {
final int wrapDataSize = this.wrapDataSize;
// Only continue to loop if the handler was not removed in the meantime.
// See https://github.com/netty/netty/issues/5860
outer: while (!ctx.isRemoved()) {
promise = ctx.newPromise();
buf = wrapDataSize > 0 ?
ChannelPromise promise = ctx.newPromise();
ByteBuf buf = wrapDataSize > 0 ?
pendingUnencryptedWrites.remove(alloc, wrapDataSize, promise) :
pendingUnencryptedWrites.removeFirst(promise);
if (buf == null) {
Expand All @@ -844,9 +841,31 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti

SSLEngineResult result = wrap(alloc, engine, buf, out);

if (result.getStatus() == Status.CLOSED) {
if (buf.isReadable()) {
pendingUnencryptedWrites.addFirst(buf, promise);
// When we add the buffer/promise pair back we need to be sure we don't complete the promise
// later. We only complete the promise if the buffer is completely consumed.
promise = null;
} else {
buf.release();
buf = null;
}

// We need to write any data before we invoke any methods which may trigger re-entry, otherwise
// writes may occur out of order and TLS sequencing may be off (e.g. SSLV3_ALERT_BAD_RECORD_MAC).
if (out.isReadable()) {
final ByteBuf b = out;
out = null;
if (promise != null) {
ctx.write(b, promise);
} else {
ctx.write(b);
}
} else if (promise != null) {
ctx.write(Unpooled.EMPTY_BUFFER, promise);
}
// else out is not readable we can re-use it and so save an extra allocation

if (result.getStatus() == Status.CLOSED) {
// Make a best effort to preserve any exception that way previously encountered from the handshake
// or the transport, else fallback to a general error.
Throwable exception = handshakePromise.cause();
Expand All @@ -856,23 +875,9 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
exception = new SslClosedEngineException("SSLEngine closed already");
}
}
promise.tryFailure(exception);
promise = null;
// SSLEngine has been closed already.
// Any further write attempts should be denied.
pendingUnencryptedWrites.releaseAndFailAll(ctx, exception);
return;
} else {
if (buf.isReadable()) {
pendingUnencryptedWrites.addFirst(buf, promise);
// When we add the buffer/promise pair back we need to be sure we don't complete the promise
// later in finishWrap. We only complete the promise if the buffer is completely consumed.
promise = null;
} else {
buf.release();
}
buf = null;

switch (result.getHandshakeStatus()) {
case NEED_TASK:
if (!runDelegatedTasks(inUnwrap)) {
Expand All @@ -883,27 +888,11 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
break;
case FINISHED:
setHandshakeSuccess();
// deliberate fall-through
break;
case NOT_HANDSHAKING:
setHandshakeSuccessIfStillHandshaking();
// deliberate fall-through
case NEED_WRAP: {
ChannelPromise p = promise;

// Null out the promise so it is not reused in the finally block in the cause of
// finishWrap(...) throwing.
promise = null;
final ByteBuf b;

if (out.isReadable()) {
// There is something in the out buffer. Ensure we null it out so it is not re-used.
b = out;
out = null;
} else {
// If out is not readable we can re-use it and so save an extra allocation
b = null;
}
finishWrap(ctx, b, p, inUnwrap, false);
break;
case NEED_WRAP:
// If we are expected to wrap again and we produced some data we need to ensure there
// is something in the queue to process as otherwise we will not try again before there
// was more added. Failing to do so may fail to produce an alert that can be
Expand All @@ -912,9 +901,10 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
pendingUnencryptedWrites.add(Unpooled.EMPTY_BUFFER);
}
break;
}
case NEED_UNWRAP:
needUnwrap = true;
// The underlying engine is starving so we need to feed it with more data.
// See https://github.com/netty/netty/pull/5039
readIfNeeded(ctx);
return;
default:
throw new IllegalStateException(
Expand All @@ -923,37 +913,12 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
}
}
} finally {
// Ownership of buffer was not transferred, release it.
if (buf != null) {
buf.release();
if (out != null) {
out.release();
}
if (inUnwrap) {
needsFlush = true;
}
finishWrap(ctx, out, promise, inUnwrap, needUnwrap);
}
}

private void finishWrap(ChannelHandlerContext ctx, ByteBuf out, ChannelPromise promise, boolean inUnwrap,
boolean needUnwrap) {
if (out == null) {
out = Unpooled.EMPTY_BUFFER;
} else if (!out.isReadable()) {
out.release();
out = Unpooled.EMPTY_BUFFER;
}

if (promise != null) {
ctx.write(out, promise);
} else {
ctx.write(out);
}

if (inUnwrap) {
needsFlush = true;
}

if (needUnwrap) {
// The underlying engine is starving so we need to feed it with more data.
// See https://github.com/netty/netty/pull/5039
readIfNeeded(ctx);
}
}

Expand All @@ -977,7 +942,6 @@ private boolean wrapNonAppData(final ChannelHandlerContext ctx, boolean inUnwrap
out = allocateOutNetBuf(ctx, 2048, 1);
}
SSLEngineResult result = wrap(alloc, engine, Unpooled.EMPTY_BUFFER, out);
HandshakeStatus status = result.getHandshakeStatus();
if (result.bytesProduced() > 0) {
ctx.write(out).addListener(new ChannelFutureListener() {
@Override
Expand All @@ -989,21 +953,22 @@ public void operationComplete(ChannelFuture future) {
}
});
if (inUnwrap) {
// We may be here because we read data and discovered the remote peer initiated a renegotiation
// and this write is to complete the new handshake. The user may have previously done a
// writeAndFlush which wasn't able to wrap data due to needing the pending handshake, so we
// attempt to wrap application data here if any is pending.
if (status == HandshakeStatus.FINISHED && !pendingUnencryptedWrites.isEmpty()) {
wrap(ctx, true);
}
needsFlush = true;
}
out = null;
}

HandshakeStatus status = result.getHandshakeStatus();
switch (status) {
case FINISHED:
setHandshakeSuccess();
// We may be here because we read data and discovered the remote peer initiated a renegotiation
// and this write is to complete the new handshake. The user may have previously done a
// writeAndFlush which wasn't able to wrap data due to needing the pending handshake, so we
// attempt to wrap application data here if any is pending.
if (inUnwrap && !pendingUnencryptedWrites.isEmpty()) {
wrap(ctx, true);
}
return false;
case NEED_TASK:
if (!runDelegatedTasks(inUnwrap)) {
Expand Down Expand Up @@ -1095,11 +1060,9 @@ private SSLEngineResult wrap(ByteBufAllocator alloc, SSLEngine engine, ByteBuf i
in.skipBytes(result.bytesConsumed());
out.writerIndex(out.writerIndex() + result.bytesProduced());

switch (result.getStatus()) {
case BUFFER_OVERFLOW:
if (result.getStatus() == Status.BUFFER_OVERFLOW) {
out.ensureWritable(engine.getSession().getPacketBufferSize());
break;
default:
} else {
return result;
}
}
Expand Down
13 changes: 11 additions & 2 deletions handler/src/test/java/io/netty/handler/ssl/ApplicationProtocolNegotiationHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
*/
package io.netty.handler.ssl;

import io.netty.buffer.ByteBuf;
import io.netty.channel.ChannelHandler;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.embedded.EmbeddedChannel;
import io.netty.handler.codec.DecoderException;
import io.netty.util.CharsetUtil;
import org.junit.Test;

import javax.net.ssl.SSLContext;
Expand Down Expand Up @@ -66,13 +68,20 @@ protected void configurePipeline(ChannelHandlerContext ctx, String protocol) {
};

SSLEngine engine = SSLContext.getDefault().createSSLEngine();
engine.setUseClientMode(false);
// This test is mocked/simulated and doesn't go through full TLS handshake. Currently only JDK SSLEngineImpl
// client mode will generate a close_notify.
engine.setUseClientMode(true);

EmbeddedChannel channel = new EmbeddedChannel(new SslHandler(engine), alpnHandler);
channel.pipeline().fireUserEventTriggered(SslHandshakeCompletionEvent.SUCCESS);
assertNull(channel.pipeline().context(alpnHandler));
// Should produce the close_notify messages
assertTrue(channel.finishAndReleaseAll());
channel.releaseOutbound();
channel.close();
ByteBuf close_notify = channel.readOutbound();
assertTrue("close_notify: " + close_notify.toString(CharsetUtil.UTF_8), close_notify.readableBytes() >= 7);
close_notify.release();
channel.finishAndReleaseAll();
assertTrue(configureCalled.get());
}

Expand Down
Loading

0 comments on commit 6b48e69

Please sign in to comment.