Repository: netty/netty
Issue: SslHandler does not work with little endian buffers
Labels: defect

### Expected behavior
SslHandler to work regardless of LE or BE buffers
### Actual behavior
SslHandler uses [SslUtils#getEncryptedPacketLength](https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/SslUtils.java#L123) which does not work with little endian buffers.

Looks like the issue was fixed in [16750ff](https://github.com/netty/netty/commit/16750ffc2802e6098e624df56ea6fbbe5c1055c3) but the change was not carried forward. I suspect there is an alternative then?

From the doc in the fix:
> Reads a big-endian short integer from the buffer.  Please note that we do not use
> {@link ChannelBuffer#getShort(int)} because it might be a little-endian buffer.

### Minimal yet complete reproducer code (or URL to code)
Code to reproduce the issue is here: [8b415c1](https://github.com/sudheeshkatkam/netty/commit/8b415c1949e0320b4fa1b7a2c7430c1af4eabd52). This bootstraps the chat server and client with a little endian bytebuf allocator, and adds details to console output.

To reproduce the failure (handshake timeout), start server using this command (in root directory):
> ./run-example.sh -Dlebba=true securechat-server

And then the client, using this command:

> ./run-example.sh -Dlebba=true securechat-client

To fix the use, apply a modified version of [16750ff](https://github.com/netty/netty/commit/16750ffc2802e6098e624df56ea6fbbe5c1055c3) which is here: [f7da006](https://github.com/sudheeshkatkam/netty/commit/f7da006152eaaee40b3482b7126e2f1f2bd90b3d).
### Netty version
4.1
### JVM version (e.g. `java -version`)
java version "1.8.0_77"
Java(TM) SE Runtime Environment (build 1.8.0_77-b03)
Java HotSpot(TM) 64-Bit Server VM (build 25.77-b03, mixed mode)


Issue comments:
@sudheeshkatkam will check... thanks!
@sudheeshkatkam PTAL https://github.com/netty/netty/pull/6713
LGTM!
Fixed by https://github.com/netty/netty/pull/6713

Pull request review comments:
nit: add static import for `SslUtils.getEncryptedPacketLength`
done...

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java
index dbb22609eea..bdf65672485 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslUtils.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslUtils.java
@@ -17,11 +17,13 @@
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufAllocator;
+import io.netty.buffer.ByteBufUtil;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.base64.Base64;
 import io.netty.handler.codec.base64.Base64Dialect;
 
 import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
 import javax.net.ssl.SSLHandshakeException;
 
 /**
@@ -120,7 +122,7 @@ static int getEncryptedPacketLength(ByteBuf buffer, int offset) {
             int majorVersion = buffer.getUnsignedByte(offset + 1);
             if (majorVersion == 3) {
                 // SSLv3 or TLS
-                packetLength = buffer.getUnsignedShort(offset + 3) + SSL_RECORD_HEADER_LENGTH;
+                packetLength = unsignedShortBE(buffer, offset + 3) + SSL_RECORD_HEADER_LENGTH;
                 if (packetLength <= SSL_RECORD_HEADER_LENGTH) {
                     // Neither SSLv3 or TLSv1 (i.e. SSLv2 or bad data)
                     tls = false;
@@ -137,11 +139,8 @@ static int getEncryptedPacketLength(ByteBuf buffer, int offset) {
             int majorVersion = buffer.getUnsignedByte(offset + headerLength + 1);
             if (majorVersion == 2 || majorVersion == 3) {
                 // SSLv2
-                if (headerLength == 2) {
-                    packetLength = (buffer.getShort(offset) & 0x7FFF) + 2;
-                } else {
-                    packetLength = (buffer.getShort(offset) & 0x3FFF) + 3;
-                }
+                packetLength = headerLength == 2 ?
+                        (shortBE(buffer, offset) & 0x7FFF) + 2 : (shortBE(buffer, offset) & 0x3FFF) + 3;
                 if (packetLength <= headerLength) {
                     return NOT_ENOUGH_DATA;
                 }
@@ -152,12 +151,33 @@ static int getEncryptedPacketLength(ByteBuf buffer, int offset) {
         return packetLength;
     }
 
+    // Reads a big-endian unsigned short integer from the buffer
+    @SuppressWarnings("deprecation")
+    private static int unsignedShortBE(ByteBuf buffer, int offset) {
+        return buffer.order() == ByteOrder.BIG_ENDIAN ?
+                buffer.getUnsignedShort(offset) : buffer.getUnsignedShortLE(offset);
+    }
+
+    // Reads a big-endian short integer from the buffer
+    @SuppressWarnings("deprecation")
+    private static short shortBE(ByteBuf buffer, int offset) {
+        return buffer.order() == ByteOrder.BIG_ENDIAN ?
+                buffer.getShort(offset) : buffer.getShortLE(offset);
+    }
+
     private static short unsignedByte(byte b) {
         return (short) (b & 0xFF);
     }
 
-    private static int unsignedShort(short s) {
-        return s & 0xFFFF;
+    // Reads a big-endian unsigned short integer from the buffer
+    private static int unsignedShortBE(ByteBuffer buffer, int offset) {
+        return shortBE(buffer, offset) & 0xFFFF;
+    }
+
+    // Reads a big-endian short integer from the buffer
+    private static short shortBE(ByteBuffer buffer, int offset) {
+        return buffer.order() == ByteOrder.BIG_ENDIAN ?
+                buffer.getShort(offset) : ByteBufUtil.swapShort(buffer.getShort(offset));
     }
 
     static int getEncryptedPacketLength(ByteBuffer[] buffers, int offset) {
@@ -207,7 +227,7 @@ private static int getEncryptedPacketLength(ByteBuffer buffer) {
             int majorVersion = unsignedByte(buffer.get(pos + 1));
             if (majorVersion == 3) {
                 // SSLv3 or TLS
-                packetLength = unsignedShort(buffer.getShort(pos + 3)) + SSL_RECORD_HEADER_LENGTH;
+                packetLength = unsignedShortBE(buffer, pos + 3) + SSL_RECORD_HEADER_LENGTH;
                 if (packetLength <= SSL_RECORD_HEADER_LENGTH) {
                     // Neither SSLv3 or TLSv1 (i.e. SSLv2 or bad data)
                     tls = false;
@@ -224,11 +244,8 @@ private static int getEncryptedPacketLength(ByteBuffer buffer) {
             int majorVersion = unsignedByte(buffer.get(pos + headerLength + 1));
             if (majorVersion == 2 || majorVersion == 3) {
                 // SSLv2
-                if (headerLength == 2) {
-                    packetLength = (buffer.getShort(pos) & 0x7FFF) + 2;
-                } else {
-                    packetLength = (buffer.getShort(pos) & 0x3FFF) + 3;
-                }
+                packetLength = headerLength == 2 ?
+                        (shortBE(buffer, pos) & 0x7FFF) + 2 : (shortBE(buffer, pos) & 0x3FFF) + 3;
                 if (packetLength <= headerLength) {
                     return NOT_ENOUGH_DATA;
                 }


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/SslUtilsTest.java b/handler/src/test/java/io/netty/handler/ssl/SslUtilsTest.java
new file mode 100644
index 00000000000..c1de33dd6e3
--- /dev/null
+++ b/handler/src/test/java/io/netty/handler/ssl/SslUtilsTest.java
@@ -0,0 +1,66 @@
+/*
+ * Copyright 2017 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.handler.ssl;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import org.junit.Test;
+
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
+import java.security.NoSuchAlgorithmException;
+
+import static io.netty.handler.ssl.SslUtils.getEncryptedPacketLength;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class SslUtilsTest {
+
+    @SuppressWarnings("deprecation")
+    @Test
+    public void testPacketLength() throws SSLException, NoSuchAlgorithmException {
+        SSLEngine engineLE = newEngine();
+        SSLEngine engineBE = newEngine();
+
+        ByteBuffer empty = ByteBuffer.allocate(0);
+        ByteBuffer cTOsLE = ByteBuffer.allocate(17 * 1024).order(ByteOrder.LITTLE_ENDIAN);
+        ByteBuffer cTOsBE = ByteBuffer.allocate(17 * 1024);
+
+        assertTrue(engineLE.wrap(empty, cTOsLE).bytesProduced() > 0);
+        cTOsLE.flip();
+
+        assertTrue(engineBE.wrap(empty, cTOsBE).bytesProduced() > 0);
+        cTOsBE.flip();
+
+        ByteBuf bufferLE = Unpooled.buffer().order(ByteOrder.LITTLE_ENDIAN).writeBytes(cTOsLE);
+        ByteBuf bufferBE = Unpooled.buffer().writeBytes(cTOsBE);
+
+        // Test that the packet-length for BE and LE is the same
+        assertEquals(getEncryptedPacketLength(bufferBE, 0), getEncryptedPacketLength(bufferLE, 0));
+        assertEquals(getEncryptedPacketLength(new ByteBuffer[] { bufferBE.nioBuffer() }, 0),
+                getEncryptedPacketLength(new ByteBuffer[] { bufferLE.nioBuffer().order(ByteOrder.LITTLE_ENDIAN) }, 0));
+    }
+
+    private static SSLEngine newEngine() throws SSLException, NoSuchAlgorithmException  {
+        SSLEngine engine = SSLContext.getDefault().createSSLEngine();
+        engine.setUseClientMode(true);
+        engine.beginHandshake();
+        return engine;
+    }
+}