From e71beb4a071c996372807d25d23708e2ab7b7683 Mon Sep 17 00:00:00 2001
From: Jake Wharton <jw@squareup.com>
Date: Tue, 3 Mar 2015 13:09:55 -0700
Subject: [PATCH] Payload length continuations are unsigned values.

The short and long continuations of payload length should be treated as unsigned values. Due to the sheer size of an unsigned long, any value over the signed Long.MAX_VALUE is forbidden.
---
 .../internal/ws/WebSocketReaderTest.java      | 22 +++++++++++++++++++
 .../internal/ws/WebSocketWriterTest.java      |  4 ++--
 .../okhttp/internal/ws/WebSocketProtocol.java |  9 ++++++--
 .../okhttp/internal/ws/WebSocketReader.java   |  6 ++++-
 .../okhttp/internal/ws/WebSocketWriter.java   |  2 +-
 5 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketReaderTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketReaderTest.java
index 2f9dda8be..ed9413942 100644
--- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketReaderTest.java
+++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketReaderTest.java
@@ -123,6 +123,28 @@ public class WebSocketReaderTest {
     callback.assertTextMessage("Hello");
   }
 
+  @Test public void clientFramePayloadShort() throws IOException {
+    data.write(ByteString.decodeHex("817E000548656c6c6f")); // Hello
+    clientReader.processNextFrame();
+    callback.assertTextMessage("Hello");
+  }
+
+  @Test public void clientFramePayloadLong() throws IOException {
+    data.write(ByteString.decodeHex("817f000000000000000548656c6c6f")); // Hello
+    clientReader.processNextFrame();
+    callback.assertTextMessage("Hello");
+  }
+
+  @Test public void clientFramePayloadTooLongThrows() throws IOException {
+    data.write(ByteString.decodeHex("817f8000000000000000"));
+    try {
+      clientReader.processNextFrame();
+      fail();
+    } catch (ProtocolException e) {
+      assertEquals("Frame length 0x8000000000000000 > 0x7FFFFFFFFFFFFFFF", e.getMessage());
+    }
+  }
+
   @Test public void serverHelloTwoChunks() throws IOException {
     data.write(ByteString.decodeHex("818537fa213d7f9f4d")); // Hel
 
diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketWriterTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketWriterTest.java
index 141d9ca4a..3ee47905e 100644
--- a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketWriterTest.java
+++ b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/ws/WebSocketWriterTest.java
@@ -100,9 +100,9 @@ public class WebSocketWriterTest {
   }
 
   @Test public void serverSendBinaryShort() throws IOException {
-    byte[] payload = binaryData(1000);
+    byte[] payload = binaryData(0xffff);
     serverWriter.sendMessage(BINARY, new Buffer().write(payload));
-    assertData("827e03e8");
+    assertData("827effff");
     assertData(payload);
   }
 
diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketProtocol.java b/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketProtocol.java
index b4d17cc50..2b93398fa 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketProtocol.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketProtocol.java
@@ -72,9 +72,14 @@ public final class WebSocketProtocol {
    * {@link #PAYLOAD_SHORT} or {@link #PAYLOAD_LONG}.
    */
   static final int PAYLOAD_MAX = 125;
-  /** Value for {@link #B1_MASK_LENGTH} which indicates the next two bytes are the length. */
+  /**
+   * Value for {@link #B1_MASK_LENGTH} which indicates the next two bytes are the unsigned length.
+   */
   static final int PAYLOAD_SHORT = 126;
-  /** Value for {@link #B1_MASK_LENGTH} which indicates the next eight bytes are the length. */
+  /**
+   * Value for {@link #B1_MASK_LENGTH} which indicates the next eight bytes are the unsigned
+   * length.
+   */
   static final int PAYLOAD_LONG = 127;
 
   static void toggleMask(byte[] buffer, long byteCount, byte[] key, long frameBytesRead) {
diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketReader.java b/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketReader.java
index 294854a97..052122254 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketReader.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketReader.java
@@ -135,9 +135,13 @@ public final class WebSocketReader {
     // Get frame length, optionally reading from follow-up bytes if indicated by special values.
     frameLength = b1 & B1_MASK_LENGTH;
     if (frameLength == PAYLOAD_SHORT) {
-      frameLength = source.readShort();
+      frameLength = source.readShort() & 0xffffL; // Value is unsigned.
     } else if (frameLength == PAYLOAD_LONG) {
       frameLength = source.readLong();
+      if (frameLength < 0) {
+        throw new ProtocolException(
+            "Frame length 0x" + Long.toHexString(frameLength) + " > 0x7FFFFFFFFFFFFFFF");
+      }
     }
     frameBytesRead = 0;
 
diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketWriter.java b/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketWriter.java
index 16d269bbc..207ab24c6 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketWriter.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/internal/ws/WebSocketWriter.java
@@ -215,7 +215,7 @@ public final class WebSocketWriter {
       if (byteCount <= PAYLOAD_MAX) {
         b1 |= (int) byteCount;
         sink.writeByte(b1);
-      } else if (byteCount <= Short.MAX_VALUE) {
+      } else if (byteCount <= 0xffffL) { // Unsigned short.
         b1 |= PAYLOAD_SHORT;
         sink.writeByte(b1);
         sink.writeShort((int) byteCount);