Repository: netty/netty
Issue: SSLHandler throws NotSslRecordException on partial data
Labels: defect

In Netty 3, SSLHandler read the SSL packet length and aggregated the data until the entire packet had been received:

if (buffer.readableBytes() < packetLength) {
    return null;
}

In Netty 4, SSLHandler computes the packet length, but then calls unwrap without aggregating the data. If the engine reads the available data and aggregates internally, it will consume the header, and on receiving the rest of the packet, SSLHandler will try to compute the packet length again and throw an exception.

SSLHandler should either aggregate the data as it did in Netty 3, or track the packetLength and the bytesConsumed and only try to compute the packet length after completely consuming the previous packet.


Issue comments:
Tested with netty-handler-4.0.0.CR10-20130707.150539-39.jar -- still occurs but with much less frequency.

Hmm.. Maybe a way to reproduce?

Am 07.07.2013 um 18:14 schrieb Jeff Pinner notifications@github.com:

> Tested with netty-handler-4.0.0.CR10-20130707.150539-39.jar -- still occurs but with much less frequency.
> 
> —
> Reply to this email directly or view it on GitHub.

Trying to get a reproducible case -- strangely enough it appears to always happen on split packets, after reading the first 57 bytes of the split packet -- no idea where 57 comes from.

My guess is that the fix doesn't handle the case where the buffer contains multiple TLS records whose last record is truncated.  I didn't have a chance to confirm this by myself though.

Maybe I can check this in a few.. Just time for dinner first

Am 07.07.2013 um 18:39 schrieb Trustin Lee notifications@github.com:

> My guess is that the fix doesn't handle the case where the buffer contains multiple TLS records whose last record is truncated. I didn't have a chance to confirm this by myself though.
> 
> —
> Reply to this email directly or view it on GitHub.

In each case, the first packet contained a 32-byte TLS record (not including header) and then the first 314-bytes of the second TLS record (also not including header) -- so the first packet was 356 bytes total. Then the second packet consumed the first 57 bytes and the next time through caused the exception.

@trustin, just took a look at your fix, you're right -- you shouldn't just reset packetLength to 0 in the finally block, you need to add:

packetLength -= result.bytesConsumed()

after the unwrap call and read the next header once it reaches 0.

Not necessarily related but looks like another bug: `decode0()` calls `ctx.fireMessageReceived()` where `out.add(...)` should be called.  i.e. we should not extend ByteToMessageDecoder really.

I'll take a closer look in the morning.

Hmm... We used FrameDecoder in 3.x so I think it should be ok to use ByteToMessageDecoder in 4.x

Am 07.07.2013 um 19:03 schrieb Trustin Lee notifications@github.com:

> Not necessarily related but looks like another bug: decode0() calls ctx.fireMessageReceived() where out.add(...) should be called. i.e. we should not extend ByteToMessageDecoder really.
> 
> I'll take a closer look in the morning.
> 
> —
> Reply to this email directly or view it on GitHub.

I need to have a deeper look but at least in 3.x we do the same (reset the packetLength)

Am 07.07.2013 um 18:59 schrieb Jeff Pinner notifications@github.com:

> @trustin, just took a look at your fix, you're right -- you shouldn't just reset packetLength to 0 in the finally block, you need to add:
> 
> packetLength -= result.bytesConsumed()
> 
> after the unwrap call and read the next header once it reaches 0.
> 
> —
> Reply to this email directly or view it on GitHub.

@trustin now I see.. I think you are right we should better not extend ByteToMessageDecoder at all and just handle it by ourself. 

@normanmaurer In Netty 3.x we skipped the decoded packet length and returned the unwrapped buffer. So we never processed more than 1 frame at a time.

@trustin how did you generate your ssl test data ? just capture it via wireshark ?

@jpinner I did think about this again and I don't see why we need to "update" the packetLength as we also skip the bytes here too. This is done in the "unwrap(...)" method.

I create a pull-req to make it easier to follow the fix #1541 of this problem.

I already fixed there some issues which could I think can lead do "corrupt" data. Unfortunally I was not able to write a test-case yet. 

@trustin please also review

@jpinner nevermind... Now I see it. Let me fix it in a few

> @trustin how did you generate your ssl test data ? just capture it via wireshark ?

Just wrote a dummy packet based on the RFC.  You could just add LoggingHandler to the pipeline of SecureChatHandler to get some?

@trustin good idea... will take a stab on this after swimming to "re-gather" some energy.

@normanmaurer let me know if you need any help with this

@jpinner @trustin fixed... I was able to reproduce it with niosmtp and so finally fixed it.. yay!


Pull request review comments:
Why was this moved before the instantiation of the exception?

Isn't `this.packetLength` already `0`?


Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
index 01ef75131a2..ee8ae7554a1 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -19,6 +19,7 @@
 import io.netty.buffer.ByteBufUtil;
 import io.netty.buffer.Unpooled;
 import io.netty.channel.Channel;
+import io.netty.channel.ChannelException;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerContext;
@@ -183,6 +184,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
     private final Queue<PendingWrite> pendingUnencryptedWrites = new ArrayDeque<PendingWrite>();
 
     private int packetLength;
+    private ByteBuf decodeOut;
 
     private volatile long handshakeTimeoutMillis = 10000;
     private volatile long closeNotifyTimeoutMillis = 3000;
@@ -343,9 +345,17 @@ public Future<Channel> sslCloseFuture() {
     }
 
     @Override
-    protected void handlerRemoved0(ChannelHandlerContext ctx) throws Exception {
+    public void handlerRemoved0(ChannelHandlerContext ctx) throws Exception {
         if (decodeOut != null) {
             decodeOut.release();
+            decodeOut = null;
+        }
+        for (;;) {
+            PendingWrite write = pendingUnencryptedWrites.poll();
+            if (write == null) {
+                break;
+            }
+            write.fail(new ChannelException("Pending write on removal of SslHandler"));
         }
     }
 
@@ -495,7 +505,7 @@ private void flush0(ChannelHandlerContext ctx) throws SSLException {
             }
 
             if (unwrapLater) {
-                decode0(ctx);
+                unwrapLater(ctx);
             }
         } catch (SSLException e) {
             setHandshakeFailure(e);
@@ -517,6 +527,16 @@ private void flush0(ChannelHandlerContext ctx) throws SSLException {
         }
     }
 
+    private void unwrapLater(ChannelHandlerContext ctx) throws SSLException {
+            MessageList<Object> messageList = MessageList.newInstance();
+            decode(ctx, internalBuffer(),  messageList);
+            if (messageList.isEmpty()) {
+                messageList.recycle();
+            } else {
+                ctx.fireMessageReceived(messageList);
+            }
+    }
+
     private void flushNonAppData0(ChannelHandlerContext ctx) throws SSLException {
         boolean unwrapLater = false;
         ByteBuf out = null;
@@ -567,7 +587,7 @@ private void flushNonAppData0(ChannelHandlerContext ctx) throws SSLException {
             }
 
             if (unwrapLater) {
-                decode0(ctx);
+                unwrapLater(ctx);
             }
         } catch (SSLException e) {
             setHandshakeFailure(e);
@@ -782,26 +802,19 @@ private static int getEncryptedPacketLength(ByteBuf buffer) {
     }
 
     @Override
-    public void decode(final ChannelHandlerContext ctx, ByteBuf in, MessageList<Object> out) throws Exception {
-        decode0(ctx);
-    }
-
-    private ByteBuf decodeOut;
-
-    private void decode0(final ChannelHandlerContext ctx) throws SSLException {
-        final ByteBuf in = internalBuffer();
-
-        // Check if the packet length was parsed yet, if so we can skip the parsing
-        final int readableBytes = in.readableBytes();
+    protected void decode(ChannelHandlerContext ctx, ByteBuf in, MessageList<Object> out) throws SSLException {
         int packetLength = this.packetLength;
         if (packetLength == 0) {
+            // the previous packet was consumed so try to read the length of the next packet
+            final int readableBytes = in.readableBytes();
             if (readableBytes < 5) {
+                // not enough bytes readable to read the packet length
                 return;
             }
 
             packetLength = getEncryptedPacketLength(in);
             if (packetLength == -1) {
-                // Bad data - discard the buffer and raise an exception.
+                // Not an SSL/TLS packet
                 NotSslRecordException e = new NotSslRecordException(
                         "not an SSL/TLS record: " + ByteBufUtil.hexDump(in));
                 in.skipBytes(readableBytes);
@@ -814,10 +827,22 @@ private void decode0(final ChannelHandlerContext ctx) throws SSLException {
             this.packetLength = packetLength;
         }
 
-        if (readableBytes < packetLength) {
+        if (in.readableBytes() < packetLength) {
+            // wait until the whole packet can be read
             return;
         }
 
+        try {
+            // slice out the whole packet so unwrap will only be called with complete packets
+            int readerIndex = in.readerIndex();
+            in.skipBytes(packetLength);
+            unwrap(ctx, in.nioBuffer(readerIndex, packetLength), out);
+        } finally {
+            this.packetLength = 0;
+        }
+    }
+
+    private void unwrap(ChannelHandlerContext ctx, ByteBuffer packet, MessageList<Object> out) throws SSLException {
         boolean wrapLater = false;
         int bytesProduced = 0;
         try {
@@ -826,9 +851,8 @@ private void decode0(final ChannelHandlerContext ctx) throws SSLException {
                 if (decodeOut == null) {
                     decodeOut = ctx.alloc().buffer();
                 }
-                SSLEngineResult result = unwrap(engine, in, decodeOut);
+                SSLEngineResult result = unwrap(engine, packet, decodeOut);
                 bytesProduced += result.bytesProduced();
-
                 switch (result.getStatus()) {
                     case CLOSED:
                         // notify about the CLOSED state of the SSLEngine. See #137
@@ -870,23 +894,18 @@ private void decode0(final ChannelHandlerContext ctx) throws SSLException {
             setHandshakeFailure(e);
             throw e;
         } finally {
-            // reset the packet length so it will be parsed again on the next call
-            this.packetLength = 0;
-
             if (bytesProduced > 0) {
                 ByteBuf decodeOut = this.decodeOut;
                 this.decodeOut = null;
-                ctx.fireMessageReceived(decodeOut);
+                out.add(decodeOut);
             }
         }
     }
 
-    private static SSLEngineResult unwrap(SSLEngine engine, ByteBuf in, ByteBuf out) throws SSLException {
-        ByteBuffer in0 = in.nioBuffer();
+    private static SSLEngineResult unwrap(SSLEngine engine, ByteBuffer in, ByteBuf out) throws SSLException {
         for (;;) {
             ByteBuffer out0 = out.nioBuffer(out.writerIndex(), out.writableBytes());
-            SSLEngineResult result = engine.unwrap(in0, out0);
-            in.skipBytes(result.bytesConsumed());
+            SSLEngineResult result = engine.unwrap(in, out0);
             out.writerIndex(out.writerIndex() + result.bytesProduced());
             switch (result.getStatus()) {
             case BUFFER_OVERFLOW:


Test patch: