OpenSslEngine support unwrap plaintext greater than 2^14 and avoid

infinite loop

Motivation:
If SslHandler sets jdkCompatibilityMode to false and ReferenceCountedOpenSslEngine sets jdkCompatibilityMode to true there is a chance we will get stuck in an infinite loop if the peer sends a TLS packet with length greater than 2^14 (the maximum length allowed in the TLS 1.2 RFC [1]). However there are legacy implementations which actually send larger TLS payloads than 2^14 (e.g. OpenJDK's SSLSessionImpl [2]) and in this case ReferenceCountedOpenSslEngine will return BUFFER_OVERFLOW in an attempt to notify that a larger buffer is to be used, but if the buffer is already at max size this process will repeat indefinitely.

[1] https://tools.ietf.org/html/rfc5246#section-6.2.1
[2] http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/d5a00b1e8f78/src/share/classes/sun/security/ssl/SSLSessionImpl.java#l793

Modifications:
- Support TLS payload sizes greater than 2^14 in ReferenceCountedOpenSslEngine
- ReferenceCountedOpenSslEngine should throw an exception if a
BUFFER_OVERFLOW is impossible to rectify

Result:
No more infinite loop in ReferenceCountedOpenSslEngine due to
BUFFER_OVERFLOW and large TLS payload lengths.

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

@@ -58,15 +58,18 @@ import javax.net.ssl.SSLSessionContext;
 import javax.security.cert.X509Certificate;
 
 import static io.netty.handler.ssl.OpenSsl.memoryAddress;
-import static io.netty.util.internal.EmptyArrays.EMPTY_CERTIFICATES;
-import static io.netty.util.internal.EmptyArrays.EMPTY_JAVAX_X509_CERTIFICATES;
-import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import static io.netty.handler.ssl.SslUtils.PROTOCOL_SSL_V2;
 import static io.netty.handler.ssl.SslUtils.PROTOCOL_SSL_V2_HELLO;
 import static io.netty.handler.ssl.SslUtils.PROTOCOL_SSL_V3;
 import static io.netty.handler.ssl.SslUtils.PROTOCOL_TLS_V1;
 import static io.netty.handler.ssl.SslUtils.PROTOCOL_TLS_V1_1;
 import static io.netty.handler.ssl.SslUtils.PROTOCOL_TLS_V1_2;
+import static io.netty.handler.ssl.SslUtils.SSL_RECORD_HEADER_LENGTH;
+import static io.netty.internal.tcnative.SSL.SSL_MAX_PLAINTEXT_LENGTH;
+import static io.netty.internal.tcnative.SSL.SSL_MAX_RECORD_LENGTH;
+import static io.netty.util.internal.EmptyArrays.EMPTY_CERTIFICATES;
+import static io.netty.util.internal.EmptyArrays.EMPTY_JAVAX_X509_CERTIFICATES;
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import static java.lang.Integer.MAX_VALUE;
 import static java.lang.Math.min;
 import static javax.net.ssl.SSLEngineResult.HandshakeStatus.FINISHED;
@@ -119,7 +122,11 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
     /**
      * Depends upon tcnative ... only use if tcnative is available!
      */
-    static final int MAX_PLAINTEXT_LENGTH = SSL.SSL_MAX_PLAINTEXT_LENGTH;
+    static final int MAX_PLAINTEXT_LENGTH = SSL_MAX_PLAINTEXT_LENGTH;
+    /**
+     * Depends upon tcnative ... only use if tcnative is available!
+     */
+    private static final int MAX_RECORD_SIZE = SSL_MAX_RECORD_LENGTH;
 
     private static final AtomicIntegerFieldUpdater<ReferenceCountedOpenSslEngine> DESTROYED_UPDATER =
             AtomicIntegerFieldUpdater.newUpdater(ReferenceCountedOpenSslEngine.class, "destroyed");
@@ -944,7 +951,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
             // are multiple records or partial records this may reduce thrashing events through the pipeline.
             // [1] https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html
             if (jdkCompatibilityMode) {
-                if (len < SslUtils.SSL_RECORD_HEADER_LENGTH) {
+                if (len < SSL_RECORD_HEADER_LENGTH) {
                     return newResultMayFinishHandshake(BUFFER_UNDERFLOW, status, 0, 0);
                 }
 
@@ -953,15 +960,27 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
                     throw new NotSslRecordException("not an SSL/TLS record");
                 }
 
-                if (packetLength - SslUtils.SSL_RECORD_HEADER_LENGTH > capacity) {
+                final int packetLengthDataOnly = packetLength - SSL_RECORD_HEADER_LENGTH;
-                    // No enough space in the destination buffer so signal the caller
+                if (packetLengthDataOnly > capacity) {
-                    // that the buffer needs to be increased.
+                    // Not enough space in the destination buffer so signal the caller that the buffer needs to be
+                    // increased.
+                    if (packetLengthDataOnly > MAX_RECORD_SIZE) {
+                        // The packet length MUST NOT exceed 2^14 [1]. However we do accommodate more data to support
+                        // legacy use cases which may violate this condition (e.g. OpenJDK's SslEngineImpl). If the max
+                        // length is exceeded we fail fast here to avoid an infinite loop due to the fact that we
+                        // won't allocate a buffer large enough.
+                        // [1] https://tools.ietf.org/html/rfc5246#section-6.2.1
+                        throw new SSLException("Illegal packet length: " + packetLengthDataOnly + " > " +
+                                                session.getApplicationBufferSize());
+                    } else {
+                        session.tryExpandApplicationBufferSize(packetLengthDataOnly);
+                    }
                     return newResultMayFinishHandshake(BUFFER_OVERFLOW, status, 0, 0);
                 }
 
                 if (len < packetLength) {
-                    // We either have no enough data to read the packet length at all or not enough for reading
+                    // We either don't have enough data to read the packet length or not enough for reading the whole
-                    // the whole packet.
+                    // packet.
                     return newResultMayFinishHandshake(BUFFER_UNDERFLOW, status, 0, 0);
                 }
             } else if (len == 0 && sslPending <= 0) {
@@ -1018,10 +1037,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
                             }
 
                             int bytesRead = readPlaintextData(dst);
-                            // We are directly using the ByteBuffer memory for the write, and so we only know what
+                            // We are directly using the ByteBuffer memory for the write, and so we only know what has
-                            // has been consumed after we let SSL decrypt the data. At this point we should update
+                            // been consumed after we let SSL decrypt the data. At this point we should update the
-                            // the number of bytes consumed, update the ByteBuffer position, and release temp
+                            // number of bytes consumed, update the ByteBuffer position, and release temp ByteBuf.
-                            // ByteBuf.
                             int localBytesConsumed = pendingEncryptedBytes - SSL.bioLengthByteBuffer(networkBIO);
                             bytesConsumed += localBytesConsumed;
                             packetLength -= localBytesConsumed;
@@ -1040,12 +1058,10 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
                                                 newResultMayFinishHandshake(isInboundDone() ? CLOSED : OK, status,
                                                                             bytesConsumed, bytesProduced);
                                     }
-                                } else if (packetLength == 0) {
+                                } else if (packetLength == 0 || jdkCompatibilityMode) {
-                                    // We read everything return now.
+                                    // We either consumed all data or we are in jdkCompatibilityMode and have consumed
+                                    // a single TLS packet and should stop consuming until this method is called again.
                                     break srcLoop;
-                                } else if (jdkCompatibilityMode) {
-                                    // try to write again to the BIO. stop reading from it by break out of the readLoop.
-                                    break;
                                 }
                             } else {
                                 int sslError = SSL.getError(ssl, bytesRead);
@@ -1847,6 +1863,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
         private String cipher;
         private byte[] id;
         private long creationTime;
+        private volatile int applicationBufferSize = MAX_PLAINTEXT_LENGTH;
 
         // lazy init for memory reasons
         private Map<String, Object> values;
@@ -2184,7 +2201,19 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
 
         @Override
         public int getApplicationBufferSize() {
-            return MAX_PLAINTEXT_LENGTH;
+            return applicationBufferSize;
+        }
+
+        /**
+         * Expand (or increase) the value returned by {@link #getApplicationBufferSize()} if necessary.
+         * <p>
+         * This is only called in a synchronized block, so no need to use atomic operations.
+         * @param packetLengthDataOnly The packet size which exceeds the current {@link #getApplicationBufferSize()}.
+         */
+        void tryExpandApplicationBufferSize(int packetLengthDataOnly) {
+            if (packetLengthDataOnly > MAX_PLAINTEXT_LENGTH && applicationBufferSize != MAX_RECORD_SIZE) {
+                applicationBufferSize = MAX_RECORD_SIZE;
+            }
         }
     }
 }

handler/src/main/java/io/netty/handler/ssl/SslHandler.java

@@ -66,6 +66,7 @@ import javax.net.ssl.SSLEngineResult;
 import javax.net.ssl.SSLEngineResult.HandshakeStatus;
 import javax.net.ssl.SSLEngineResult.Status;
 import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLSession;
 
 import static io.netty.buffer.ByteBufUtil.ensureWritableSuccess;
 import static io.netty.handler.ssl.SslUtils.getEncryptedPacketLength;
@@ -1223,6 +1224,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
         final int originalLength = length;
         boolean wrapLater = false;
         boolean notifyClosure = false;
+        int overflowReadableBytes = -1;
         ByteBuf decodeOut = allocate(ctx, length);
         try {
             // Only continue to loop if the handler was not removed in the meantime.
@@ -1240,7 +1242,9 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
 
                 switch (status) {
                 case BUFFER_OVERFLOW:
-                    int readableBytes = decodeOut.readableBytes();
+                    final int readableBytes = decodeOut.readableBytes();
+                    final int previousOverflowReadableBytes = overflowReadableBytes;
+                    overflowReadableBytes = readableBytes;
                     int bufferSize = engine.getSession().getApplicationBufferSize() - readableBytes;
                     if (readableBytes > 0) {
                         firedChannelRead = true;
@@ -1260,6 +1264,13 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
                         decodeOut.release();
                         decodeOut = null;
                     }
+                    if (readableBytes == 0 && previousOverflowReadableBytes == 0) {
+                        // If there is two consecutive loops where we overflow and are not able to consume any data,
+                        // assume the amount of data exceeds the maximum amount for the engine and bail
+                        throw new IllegalStateException("Two consecutive overflows but no content was consumed. " +
+                                 SSLSession.class.getSimpleName() + " getApplicationBufferSize: " +
+                                 engine.getSession().getApplicationBufferSize() + " maybe too small.");
+                    }
                     // Allocate a new buffer which can hold all the rest data and loop again.
                     // TODO: We may want to reconsider how we calculate the length here as we may
                     // have more then one ssl message to decode.
@@ -1268,8 +1279,10 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
                 case CLOSED:
                     // notify about the CLOSED state of the SSLEngine. See #137
                     notifyClosure = true;
+                    overflowReadableBytes = -1;
                     break;
                 default:
+                    overflowReadableBytes = -1;
                     break;
                 }
 

pom.xml

@@ -186,7 +186,7 @@
     <!-- Fedora-"like" systems. This is currently only used for the netty-tcnative dependency -->
     <os.detection.classifierWithLikes>fedora</os.detection.classifierWithLikes>
     <tcnative.artifactId>netty-tcnative</tcnative.artifactId>
-    <tcnative.version>2.0.6.Final</tcnative.version>
+    <tcnative.version>2.0.7.Final</tcnative.version>
     <tcnative.classifier>${os.detected.classifier}</tcnative.classifier>
     <conscrypt.groupId>org.conscrypt</conscrypt.groupId>
     <conscrypt.artifactId>conscrypt-openjdk-uber</conscrypt.artifactId>