Skip to content

Commit

Permalink
Feed only a single SSL record to SSLEngine.unwrap()
Browse files Browse the repository at this point in the history 
Motivation:

Some SSLEngine implementations violate the contract and raises an
exception when SslHandler feeds an input buffer that contains multiple
SSL records to SSLEngine.unwrap(), while the expected behavior is to
decode the first record and return.

Modification:

- Modify SslHandler.decode() to keep the lengths of each record and feed
  SSLEngine.unwrap() record by record to work around the forementioned
  issue.
- Rename unwrap() to unwrapMultiple() and unwrapNonApp()
- Rename unwrap0() to unwrapSingle()

Result:

SslHandler now works OpenSSLEngine from finagle-native.  Performance
impact remains unnoticeable.  Slightly better readability. Fixes netty#2116.
  • Loading branch information
@trustin
trustin committed Apr 20, 2014
1 parent 104faba commit e74fa1b
Showing 1 changed file with 69 additions and 27 deletions.
96 changes: 69 additions & 27 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 @@ -34,7 +34,6 @@
import io.netty.util.internal.EmptyArrays;
import io.netty.util.internal.PendingWrite;
import io.netty.util.internal.PlatformDependent;
import io.netty.util.internal.RecyclableArrayList;
import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory;

Expand All @@ -50,6 +49,7 @@
import java.nio.channels.DatagramChannel;
import java.nio.channels.SocketChannel;
import java.util.ArrayDeque;
import java.util.Arrays;
import java.util.Deque;
import java.util.List;
import java.util.concurrent.ScheduledFuture;
Expand Down Expand Up @@ -448,7 +448,8 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
case NEED_UNWRAP:
return;
default:
throw new IllegalStateException("Unknown handshake status: " + result.getHandshakeStatus());
throw new IllegalStateException(
"Unknown handshake status: " + result.getHandshakeStatus());
}
}
}
Expand Down Expand Up @@ -505,7 +506,7 @@ private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) throws
break;
case NEED_UNWRAP:
if (!inUnwrap) {
unwrap(ctx);
unwrapNonApp(ctx);
}
break;
case NEED_WRAP:
Expand All @@ -515,7 +516,7 @@ private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) throws
// Workaround for TLS False Start problem reported at:
// https://github.com/netty/netty/issues/1108#issuecomment-14266970
if (!inUnwrap) {
unwrap(ctx);
unwrapNonApp(ctx);
}
break;
default:
Expand Down Expand Up @@ -742,6 +743,11 @@ private static int getEncryptedPacketLength(ByteBuf buffer, int offset) {

@Override
protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws SSLException {

// Keeps the list of the length of every SSL record in the input buffer.
int[] recordLengths = null;
int nRecords = 0;

final int startOffset = in.readerIndex();
final int endOffset = in.writerIndex();
int offset = startOffset;
Expand All @@ -751,6 +757,10 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
if (endOffset - startOffset < packetLength) {
return;
} else {
recordLengths = new int[4];
recordLengths[0] = packetLength;
nRecords = 1;

offset += packetLength;
packetLength = 0;
}
Expand Down Expand Up @@ -778,11 +788,22 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
break;
}

// We have a whole packet.
// Remember the length of the current packet.
if (recordLengths == null) {
recordLengths = new int[4];
}
if (nRecords == recordLengths.length) {
recordLengths = Arrays.copyOf(recordLengths, recordLengths.length << 1);
}
recordLengths[nRecords ++] = packetLength;

// Increment the offset to handle the next packet.
offset += packetLength;
}

final int length = offset - startOffset;
if (length > 0) {
final int totalLength = offset - startOffset;
if (totalLength > 0) {
// The buffer contains one or more full SSL records.
// Slice out the whole packet so unwrap will only be called with complete packets.
// Also directly reset the packetLength. This is needed as unwrap(..) may trigger
Expand All @@ -793,9 +814,9 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
// 4) unwrapLater(...) calls decode(...)
//
// See https://github.com/netty/netty/issues/1534
in.skipBytes(length);
ByteBuffer buffer = in.nioBuffer(startOffset, length);
unwrap(ctx, buffer, out);
in.skipBytes(totalLength);
ByteBuffer buffer = in.nioBuffer(startOffset, totalLength);
unwrapMultiple(ctx, buffer, totalLength, recordLengths, nRecords, out);
}

if (nonSslRecord) {
Expand All @@ -817,26 +838,54 @@ public void channelReadComplete(ChannelHandlerContext ctx) throws Exception {
super.channelReadComplete(ctx);
}

private void unwrap(ChannelHandlerContext ctx) throws SSLException {
RecyclableArrayList out = RecyclableArrayList.newInstance();
/**
* Calls {@link SSLEngine#unwrap(ByteBuffer, ByteBuffer)} with an empty buffer to handle handshakes, etc.
*/
private void unwrapNonApp(ChannelHandlerContext ctx) throws SSLException {
try {
unwrap(ctx, Unpooled.EMPTY_BUFFER.nioBuffer(), out);
final int size = out.size();
for (int i = 0; i < size; i++) {
ctx.fireChannelRead(out.get(i));
}
unwrapSingle(ctx, Unpooled.EMPTY_BUFFER.nioBuffer(), 0);
} finally {
out.recycle();
ByteBuf decodeOut = this.decodeOut;
if (decodeOut != null && decodeOut.isReadable()) {
this.decodeOut = null;
ctx.fireChannelRead(decodeOut);
}
}
}

/**
* Unwraps multiple inbound SSL records.
*/
private void unwrapMultiple(
ChannelHandlerContext ctx, ByteBuffer packet, int totalLength,
int[] recordLengths, int nRecords, List<Object> out) throws SSLException {

for (int i = 0; i < nRecords; i ++) {
packet.limit(packet.position() + recordLengths[i]);
try {
unwrapSingle(ctx, packet, totalLength);
assert !packet.hasRemaining();
} finally {
ByteBuf decodeOut = this.decodeOut;
if (decodeOut != null && decodeOut.isReadable()) {
this.decodeOut = null;
out.add(decodeOut);
}
}
}
}

private void unwrap(ChannelHandlerContext ctx, ByteBuffer packet, List<Object> out) throws SSLException {
/**
* Unwraps a single SSL record.
*/
private void unwrapSingle(
ChannelHandlerContext ctx, ByteBuffer packet, int initialOutAppBufCapacity) throws SSLException {

boolean wrapLater = false;
int totalProduced = 0;
try {
for (;;) {
if (decodeOut == null) {
decodeOut = ctx.alloc().buffer(packet.remaining());
decodeOut = ctx.alloc().buffer(initialOutAppBufCapacity);
}

final SSLEngineResult result = unwrap(engine, packet, decodeOut);
Expand All @@ -845,7 +894,6 @@ private void unwrap(ChannelHandlerContext ctx, ByteBuffer packet, List<Object> o
final int produced = result.bytesProduced();
final int consumed = result.bytesConsumed();

totalProduced += produced;
if (status == Status.CLOSED) {
// notify about the CLOSED state of the SSLEngine. See #137
sslCloseFuture.trySuccess(ctx.channel());
Expand Down Expand Up @@ -886,12 +934,6 @@ private void unwrap(ChannelHandlerContext ctx, ByteBuffer packet, List<Object> o
} catch (SSLException e) {
setHandshakeFailure(e);
throw e;
} finally {
if (totalProduced > 0) {
ByteBuf decodeOut = this.decodeOut;
this.decodeOut = null;
out.add(decodeOut);
}
}
}

Expand Down

0 comments on commit e74fa1b

Please sign in to comment.