From 821501717ba3215a81863ad1c329ab8cb130dec4 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Thu, 12 Jan 2017 20:10:22 +0100
Subject: [PATCH] Fix possible IOOBE when calling
 ReferenceCountedSslEngine.unwrap(...) with heap buffers.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Motivation:

fc3c9c9523150190760801dd0fbf014909519942 introduced a bug which will have ReferenceCountedSslEngine.unwrap(...) produce an IOOBE when be called with an BŷteBuffer as src that contains multiple SSLRecords and has a position != 0.

Modification:

- Correctly set the limit on the ByteBuffer and so fix the IOOBE.
- Add test-case to verify the fix

Result:

Correctly handle heap buffers as well.
---
 .../ssl/ReferenceCountedOpenSslEngine.java    | 12 +--
 .../io/netty/handler/ssl/SSLEngineTest.java   | 75 +++++++++++++++++++
 2 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
index 23c666796f..2b7b25df4d 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -422,12 +422,14 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
             final ByteBuf buf = alloc.directBuffer(len);
             try {
                 final long addr = memoryAddress(buf);
+
+                // Set the limit depending on the length of the allocated buffer and restore the original
+                // after we copied bytes.
+                int limit = src.limit();
                 int newLimit = pos + len;
-                if (newLimit != src.remaining()) {
-                    buf.setBytes(0, (ByteBuffer) src.duplicate().position(pos).limit(newLimit));
-                } else {
-                    buf.setBytes(0, src);
-                }
+                src.limit(newLimit);
+                buf.setBytes(0, src);
+                src.limit(limit);
 
                 netWrote = SSL.writeToBIO(networkBIO, addr, len);
                 if (netWrote >= 0) {
diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
index 8e401b0e46..914cd6e476 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
@@ -1323,4 +1323,79 @@ public abstract class SSLEngineTest {
             cleanupServerSslEngine(server);
         }
     }
+
+    @Test
+    public void testMultipleRecordsInOneBufferWithNonZeroPosition() throws Exception {
+        SelfSignedCertificate cert = new SelfSignedCertificate();
+
+        clientSslCtx = SslContextBuilder
+                .forClient()
+                .trustManager(cert.cert())
+                .sslProvider(sslClientProvider())
+                .build();
+        SSLEngine client = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);
+
+        serverSslCtx = SslContextBuilder
+                .forServer(cert.certificate(), cert.privateKey())
+                .sslProvider(sslServerProvider())
+                .build();
+        SSLEngine server = serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);
+
+        try {
+            // Choose buffer size small enough that we can put multiple buffers into one buffer and pass it into the
+            // unwrap call without exceed MAX_ENCRYPTED_PACKET_LENGTH.
+            ByteBuffer plainClientOut = ByteBuffer.allocate(1024);
+            ByteBuffer plainServerOut = ByteBuffer.allocate(server.getSession().getApplicationBufferSize());
+
+            ByteBuffer encClientToServer = ByteBuffer.allocate(client.getSession().getPacketBufferSize());
+
+            int positionOffset = 1;
+            // We need to be able to hold 2 records + positionOffset
+            ByteBuffer combinedEncClientToServer = ByteBuffer.allocate(
+                    encClientToServer.capacity() * 2 + positionOffset);
+            combinedEncClientToServer.position(positionOffset);
+
+            handshake(client, server);
+
+            plainClientOut.limit(plainClientOut.capacity());
+            SSLEngineResult result = client.wrap(plainClientOut, encClientToServer);
+            assertEquals(plainClientOut.capacity(), result.bytesConsumed());
+            assertTrue(result.bytesProduced() > 0);
+
+            encClientToServer.flip();
+
+            // Copy the first record into the combined buffer
+            combinedEncClientToServer.put(encClientToServer);
+
+            plainClientOut.clear();
+            encClientToServer.clear();
+
+            result = client.wrap(plainClientOut, encClientToServer);
+            assertEquals(plainClientOut.capacity(), result.bytesConsumed());
+            assertTrue(result.bytesProduced() > 0);
+
+            encClientToServer.flip();
+
+            int encClientToServerLen = encClientToServer.remaining();
+
+            // Copy the first record into the combined buffer
+            combinedEncClientToServer.put(encClientToServer);
+
+            encClientToServer.clear();
+
+            combinedEncClientToServer.flip();
+            combinedEncClientToServer.position(positionOffset);
+
+            // Ensure we have the first record and a tiny amount of the second record in the buffer
+            combinedEncClientToServer.limit(
+                    combinedEncClientToServer.limit() - (encClientToServerLen - positionOffset));
+            result = server.unwrap(combinedEncClientToServer, plainServerOut);
+            assertEquals(encClientToServerLen, result.bytesConsumed());
+            assertTrue(result.bytesProduced() > 0);
+        } finally {
+            cert.delete();
+            cleanupClientSslEngine(client);
+            cleanupServerSslEngine(server);
+        }
+    }
 }