2010-08-11 Fumitoshi Ukai <ukai@chromium.org>
authorukai@chromium.org <ukai@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Aug 2010 08:23:56 +0000 (08:23 +0000)
committerukai@chromium.org <ukai@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Aug 2010 08:23:56 +0000 (08:23 +0000)
        Reviewed by Alexey Proskuryakov.

        Fix length calculation to be more robust.
        https://bugs.webkit.org/show_bug.cgi?id=43777

        * websocket/tests/frame-length-overflow-expected.txt: Added.
        * websocket/tests/frame-length-overflow.html: Added.
        * websocket/tests/frame-length-overflow_wsh.py: Added.
2010-08-11  Fumitoshi Ukai  <ukai@chromium.org>

        Reviewed by Alexey Proskuryakov.

        Fix length calculation to be more robust.
        https://bugs.webkit.org/show_bug.cgi?id=43777

        Test: websocket/tests/frame-length-overflow.html

        * websockets/WebSocketChannel.cpp:
        (WebCore::WebSocketChannel::appendToBuffer): len is size_t.
         - add sanity check for integer wraps.
        (WebCore::WebSocketChannel::skipBuffer): len is size_t.
        (WebCore::WebSocketChannel::processBuffer): length is size_t.
         - add sanity check for integer wraps.
        * websockets/WebSocketChannel.h: change m_bufferSize and len to size_t.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@65135 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/websocket/tests/frame-length-overflow-expected.txt [new file with mode: 0644]
LayoutTests/websocket/tests/frame-length-overflow.html [new file with mode: 0644]
LayoutTests/websocket/tests/frame-length-overflow_wsh.py [new file with mode: 0644]
WebCore/ChangeLog
WebCore/websockets/WebSocketChannel.cpp
WebCore/websockets/WebSocketChannel.h

index d0232d1..eca970e 100644 (file)
@@ -1,3 +1,14 @@
+2010-08-11  Fumitoshi Ukai  <ukai@chromium.org>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Fix length calculation to be more robust.
+        https://bugs.webkit.org/show_bug.cgi?id=43777
+
+        * websocket/tests/frame-length-overflow-expected.txt: Added.
+        * websocket/tests/frame-length-overflow.html: Added.
+        * websocket/tests/frame-length-overflow_wsh.py: Added.
+
 2010-08-10  Eric Seidel  <eric@webkit.org>
 
         Reviewed by Adam Barth.
diff --git a/LayoutTests/websocket/tests/frame-length-overflow-expected.txt b/LayoutTests/websocket/tests/frame-length-overflow-expected.txt
new file mode 100644 (file)
index 0000000..1a15207
--- /dev/null
@@ -0,0 +1,11 @@
+Make sure WebSocket does not crash and report error when it sees length overflow
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+WebSocket is open
+WebSocket received error frame
+WebSocket is closed
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/websocket/tests/frame-length-overflow.html b/LayoutTests/websocket/tests/frame-length-overflow.html
new file mode 100644 (file)
index 0000000..627043a
--- /dev/null
@@ -0,0 +1,38 @@
+<html>
+<head>
+<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
+<script src="../../fast/js/resources/js-test-pre.js"></script>
+<script src="../../fast/js/resources/js-test-post-function.js"></script>
+</head>
+<body>
+<div id="description"></div>
+<div id="console"></div>
+<script>
+description("Make sure WebSocket does not crash and report error when it sees length overflow");
+if (window.layoutTestController)
+    layoutTestController.waitUntilDone();
+
+function finish() {
+    isSuccessfullyParsed();
+    if (window.layoutTestController)
+        layoutTestController.notifyDone();
+}
+
+var ws = new WebSocket("ws://127.0.0.1:8880/websocket/tests/frame-length-overflow");
+ws.onopen = function () {
+    debug("WebSocket is open");
+};
+ws.onmessage = function (evt) {
+    debug("WebSocket received:" + evt.data);
+};
+ws.onerror = function () {
+    debug("WebSocket received error frame");
+};
+ws.onclose = function () {
+    debug("WebSocket is closed");
+    finish();
+};
+
+var successfullyParsed = true;
+</script>
+
diff --git a/LayoutTests/websocket/tests/frame-length-overflow_wsh.py b/LayoutTests/websocket/tests/frame-length-overflow_wsh.py
new file mode 100644 (file)
index 0000000..0f518a8
--- /dev/null
@@ -0,0 +1,6 @@
+def web_socket_do_extra_handshake(request):
+  pass
+
+def web_socket_transfer_data(request):
+  msg = 16 * '\xff'
+  request.connection.write(msg)
index 0585b5e..b3cc4ae 100644 (file)
@@ -1,3 +1,20 @@
+2010-08-11  Fumitoshi Ukai  <ukai@chromium.org>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Fix length calculation to be more robust.
+        https://bugs.webkit.org/show_bug.cgi?id=43777
+
+        Test: websocket/tests/frame-length-overflow.html
+
+        * websockets/WebSocketChannel.cpp:
+        (WebCore::WebSocketChannel::appendToBuffer): len is size_t.
+         - add sanity check for integer wraps.
+        (WebCore::WebSocketChannel::skipBuffer): len is size_t.
+        (WebCore::WebSocketChannel::processBuffer): length is size_t.
+         - add sanity check for integer wraps.
+        * websockets/WebSocketChannel.h: change m_bufferSize and len to size_t.
+
 2010-08-10  Eric Seidel  <eric@webkit.org>
 
         Reviewed by Adam Barth.
index b8d6e8a..e924b91 100644 (file)
@@ -198,23 +198,28 @@ void WebSocketChannel::didCancelAuthenticationChallenge(SocketStreamHandle*, con
 {
 }
 
-bool WebSocketChannel::appendToBuffer(const char* data, int len)
+bool WebSocketChannel::appendToBuffer(const char* data, size_t len)
 {
+    size_t newBufferSize = m_bufferSize + len;
+    if (newBufferSize < m_bufferSize) {
+        LOG(Network, "WebSocket buffer overflow (%lu+%lu)", m_bufferSize, len);
+        return false;
+    }
     char* newBuffer = 0;
-    if (tryFastMalloc(m_bufferSize + len).getValue(newBuffer)) {
+    if (tryFastMalloc(newBufferSize).getValue(newBuffer)) {
         if (m_buffer)
             memcpy(newBuffer, m_buffer, m_bufferSize);
         memcpy(newBuffer + m_bufferSize, data, len);
         fastFree(m_buffer);
         m_buffer = newBuffer;
-        m_bufferSize += len;
+        m_bufferSize = newBufferSize;
         return true;
     }
-    m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, String::format("WebSocket frame (at %d bytes) is too long.", m_bufferSize + len), 0, m_handshake.clientOrigin());
+    m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, String::format("WebSocket frame (at %lu bytes) is too long.", newBufferSize), 0, m_handshake.clientOrigin());
     return false;
 }
 
-void WebSocketChannel::skipBuffer(int len)
+void WebSocketChannel::skipBuffer(size_t len)
 {
     ASSERT(len <= m_bufferSize);
     m_bufferSize -= len;
@@ -268,27 +273,50 @@ bool WebSocketChannel::processBuffer()
 
     unsigned char frameByte = static_cast<unsigned char>(*p++);
     if ((frameByte & 0x80) == 0x80) {
-        int length = 0;
+        size_t length = 0;
+        bool errorFrame = false;
         while (p < end) {
-            if (length > std::numeric_limits<int>::max() / 128) {
-                LOG(Network, "frame length overflow %d", length);
-                skipBuffer(p + length - m_buffer);
-                m_client->didReceiveMessageError();
-                if (!m_client)
-                    return false;
-                if (!m_closed)
-                    m_handle->close();
-                return false;
+            if (length > std::numeric_limits<size_t>::max() / 128) {
+                LOG(Network, "frame length overflow %lu", length);
+                errorFrame = true;
+                break;
             }
-            char msgByte = *p;
-            length = length * 128 + (msgByte & 0x7f);
+            size_t newLength = length * 128;
+            unsigned char msgByte = static_cast<unsigned char>(*p);
+            unsigned int lengthMsgByte = msgByte & 0x7f;
+            if (newLength > std::numeric_limits<size_t>::max() - lengthMsgByte) {
+                LOG(Network, "frame length overflow %lu+%u", newLength, lengthMsgByte);
+                errorFrame = true;
+                break;
+            }
+            newLength += lengthMsgByte;
+            if (newLength < length) { // sanity check
+                LOG(Network, "frame length integer wrap %lu->%lu", length, newLength);
+                errorFrame = true;
+                break;
+            }
+            length = newLength;
             ++p;
             if (!(msgByte & 0x80))
                 break;
         }
+        if (p + length < p) {
+            LOG(Network, "frame buffer pointer wrap %p+%lu->%p", p, length, p + length);
+            errorFrame = true;
+        }
+        if (errorFrame) {
+            m_client->didReceiveMessageError();
+            if (!m_client)
+                return false;
+            if (!m_closed)
+                m_handle->close();
+            return false;
+        }
+        ASSERT(p + length >= p);
         if (p + length < end) {
             p += length;
             nextFrame = p;
+            ASSERT(nextFrame > m_buffer);
             skipBuffer(nextFrame - m_buffer);
             m_client->didReceiveMessageError();
             return m_buffer;
index 893b4c6..06be50e 100644 (file)
@@ -79,8 +79,8 @@ namespace WebCore {
     private:
         WebSocketChannel(ScriptExecutionContext*, WebSocketChannelClient*, const KURL&, const String& protocol);
 
-        bool appendToBuffer(const char* data, int len);
-        void skipBuffer(int len);
+        bool appendToBuffer(const char* data, size_t len);
+        void skipBuffer(size_t len);
         bool processBuffer();
         void resumeTimerFired(Timer<WebSocketChannel>* timer);
 
@@ -89,7 +89,7 @@ namespace WebCore {
         WebSocketHandshake m_handshake;
         RefPtr<SocketStreamHandle> m_handle;
         char* m_buffer;
-        int m_bufferSize;
+        size_t m_bufferSize;
 
         Timer<WebSocketChannel> m_resumeTimer;
         bool m_suspended;