Attachment 78408 Details for Bug 51253 – addressed review comments

[patch] addressed review comments

bug51253-2.patch (text/plain), 14.87 KB, created by Joe Mason on 2011-01-10 10:30:16 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Joe Mason

Created: 2011-01-10 10:30:16 PST

Size: 14.87 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 75380)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,19 @@
>+2011-01-09  Joe Mason  <jmason@rim.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        WebSockets: unbounded buffer growth when server sends bad data
>+        https://bugs.webkit.org/show_bug.cgi?id=51253
>+
>+        Fail a websocket handshake after 1024 bytes without a newline, or if it
>+        contains a null byte before the first newline.
>+
>+        Tests: http/tests/websocket/tests/handshake-fail-by-maxlength.html
>+               http/tests/websocket/tests/handshake-fail-by-prepended-null.html
>+
>+        * websockets/WebSocketHandshake.cpp:
>+        (WebCore::WebSocketHandshake::readStatusLine):
>+
> 2011-01-10  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         Reviewed by Martin Robinson.
>Index: Source/WebCore/websockets/WebSocketHandshake.cpp
>===================================================================
>--- Source/WebCore/websockets/WebSocketHandshake.cpp	(revision 75380)
>+++ Source/WebCore/websockets/WebSocketHandshake.cpp	(working copy)
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2009 Google Inc.  All rights reserved.
>+ * Copyright (C) Research In Motion Limited 2011. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions are
>@@ -427,6 +428,10 @@ KURL WebSocketHandshake::httpURLForAuthe
> // statusCode and statusText will be set to -1 and a null string, respectively.
> int WebSocketHandshake::readStatusLine(const char* header, size_t headerLength, int& statusCode, String& statusText)
> {
>+    // Arbitrary size limit to prevent the server from sending an unbounded
>+    // amount of data with no newlines and forcing us to buffer it all.
>+    static const int maximumLength = 1024;
>+
>     statusCode = -1;
>     statusText = String();
> 
>@@ -441,6 +446,12 @@ int WebSocketHandshake::readStatusLine(c
>                 space1 = p;
>             else if (!space2)
>                 space2 = p;
>+        } else if (*p == '\0') {
>+            // The caller isn't prepared to deal with null bytes in status
>+            // line. WebSockets specification doesn't prohibit this, but HTTP
>+            // does, so we'll just treat this as an error. 
>+            m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Status line contains embedded null", 0, clientOrigin());
>+            return p + 1 - header;
>         } else if (*p == '\n')
>             break;
>     }
>@@ -448,9 +459,9 @@ int WebSocketHandshake::readStatusLine(c
>         return -1; // We have not received '\n' yet.
> 
>     const char* end = p + 1;
>-    if (end - header > INT_MAX) {
>-        m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Status line is too long: " + trimConsoleMessage(header, maxConsoleMessageSize + 1), 0, clientOrigin());
>-        return INT_MAX;
>+    if (end - header > maximumLength) {
>+        m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Status line is too long", 0, clientOrigin());
>+        return maximumLength;
>     }
>     int lineLength = end - header;
> 
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 75380)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,20 @@
>+2011-01-09  Joe Mason  <jmason@rim.com>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        WebSockets: unbounded buffer growth when server sends bad data
>+        https://bugs.webkit.org/show_bug.cgi?id=51253
>+
>+        Tests that a websocket handshake should fail after 1024 bytes without a
>+        newline, or if it contains a null byte before the first newline.
>+        
>+        * http/tests/websocket/tests/handshake-fail-by-maxlength-expected.txt: Added.
>+        * http/tests/websocket/tests/handshake-fail-by-maxlength.html: Added.
>+        * http/tests/websocket/tests/handshake-fail-by-maxlength_wsh.py: Added.
>+        * http/tests/websocket/tests/handshake-fail-by-prepended-null-expected.txt: Added.
>+        * http/tests/websocket/tests/handshake-fail-by-prepended-null.html: Added.
>+        * http/tests/websocket/tests/handshake-fail-by-prepended-null_wsh.py: Added.
>+
> 2011-01-10  Adam Roben  <aroben@apple.com>
> 
>         Add some new tests to the Windows Skipped file
>Index: LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength-expected.txt
>===================================================================
>--- LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength-expected.txt	(revision 0)
>+++ LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength-expected.txt	(revision 0)
>@@ -0,0 +1,12 @@
>+CONSOLE MESSAGE: line 0: Status line is too long
>+Connection should fail immediately, rather than succeeding or staying in limbo until timeout, if handshake is longer than 1024 bytes.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+PASS timedOut is false
>+PASS connected is false
>+PASS origin is undefined.
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>Index: LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength.html
>===================================================================
>--- LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength.html	(revision 0)
>+++ LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength.html	(revision 0)
>@@ -0,0 +1,64 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<html>
>+<head>
>+<link rel="stylesheet" href="../../../js-test-resources/js-test-style.css">
>+<script src="../../../js-test-resources/js-test-pre.js"></script>
>+<script src="../../../js-test-resources/js-test-post-function.js"></script>
>+</head>
>+<body>
>+<div id="description"></div>
>+<div id="console"></div>
>+<script>
>+description('Connection should fail immediately, rather than succeeding or staying in limbo until timeout, if handshake is longer than 1024 bytes.');
>+
>+if (window.layoutTestController)
>+    layoutTestController.waitUntilDone()
>+
>+var timedOut = false;
>+var connected = false;
>+var origin;
>+
>+function endTest() {
>+    shouldBeFalse('timedOut');
>+    shouldBeFalse('connected');
>+    shouldBeUndefined('origin');
>+    clearTimeout(timeoutID);
>+    isSuccessfullyParsed();
>+    if (window.layoutTestController)
>+        layoutTestController.notifyDone();
>+}
>+
>+var url = 'ws://localhost:8880/websocket/tests/handshake-fail-by-maxlength';
>+var ws = new WebSocket(url);
>+
>+ws.onopen = function()
>+{
>+    debug('Connected');
>+    connected = true;
>+}
>+
>+ws.onmessage = function(messageEvent)
>+{
>+    origin = messageEvent.data;
>+    debug('origin = ' + origin);
>+    ws.close();
>+}
>+
>+ws.onclose = function()
>+{
>+    endTest();
>+}
>+
>+function timeoutCallback()
>+{
>+    debug('Timed out (state = ' + ws.readyState + ')');
>+    timedOut = true;
>+    endTest();
>+}
>+
>+var timeoutID = setTimeout(timeoutCallback, 3000);
>+
>+var successfullyParsed = true;
>+</script>
>+</body>
>+</html>
>Index: LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength_wsh.py
>===================================================================
>--- LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength_wsh.py	(revision 0)
>+++ LayoutTests/http/tests/websocket/tests/handshake-fail-by-maxlength_wsh.py	(revision 0)
>@@ -0,0 +1,44 @@
>+# Copyright (C) Research In Motion Limited 2011. All rights reserved.
>+#
>+# Redistribution and use in source and binary forms, with or without modification, are permitted provided that the
>+# following conditions are met:
>+#
>+# 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following
>+# disclaimer.
>+#
>+# 2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the
>+# following disclaimer in the documentation and/or other materials provided with the distribution.
>+#
>+# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS âAS ISâ AND ANY EXPRESS OR
>+# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
>+# MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
>+# APPLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>+# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>+# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
>+# TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>+# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+
>+import time
>+
>+def web_socket_do_extra_handshake(request):
>+    # This will cause the handshake to fail because it pushes the length of the
>+    # status line past 1024 characters
>+    msg = ""
>+    for i in range(0, 1024):
>+        msg += "."
>+    msg += 'HTTP/1.1 101 WebSocket Protocol Handshake\r\n'
>+    msg += 'Upgrade: WebSocket\r\n'
>+    msg += 'Connection: Upgrade\r\n'
>+    msg += 'Sec-WebSocket-Location: ' + request.ws_location + '\r\n'
>+    msg += 'Sec-WebSocket-Origin: ' + request.ws_origin + '\r\n'
>+    msg += '\r\n'
>+    msg += request.ws_challenge_md5
>+    request.connection.write(msg)
>+    # continue writing data until the client disconnects
>+    while True:
>+        time.sleep(1)
>+        request.connection.write('keepalive\n')
>+
>+def web_socket_transfer_data(request):
>+    pass
>Index: LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null-expected.txt
>===================================================================
>--- LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null-expected.txt	(revision 0)
>+++ LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null-expected.txt	(revision 0)
>@@ -0,0 +1,12 @@
>+CONSOLE MESSAGE: line 0: Status line contains embedded null
>+Connection should fail immediately, rather than succeeding or staying in limbo until timeout, if a null byte is received before the handshake.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+PASS timedOut is false
>+PASS connected is false
>+PASS origin is undefined.
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>Index: LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null.html
>===================================================================
>--- LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null.html	(revision 0)
>+++ LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null.html	(revision 0)
>@@ -0,0 +1,64 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<html>
>+<head>
>+<link rel="stylesheet" href="../../../js-test-resources/js-test-style.css">
>+<script src="../../../js-test-resources/js-test-pre.js"></script>
>+<script src="../../../js-test-resources/js-test-post-function.js"></script>
>+</head>
>+<body>
>+<div id="description"></div>
>+<div id="console"></div>
>+<script>
>+description('Connection should fail immediately, rather than succeeding or staying in limbo until timeout, if a null byte is received before the handshake.');
>+
>+if (window.layoutTestController)
>+    layoutTestController.waitUntilDone()
>+
>+var timedOut = false;
>+var connected = false;
>+var origin;
>+
>+function endTest() {
>+    shouldBeFalse('timedOut');
>+    shouldBeFalse('connected');
>+    shouldBeUndefined('origin');
>+    clearTimeout(timeoutID);
>+    isSuccessfullyParsed();
>+    if (window.layoutTestController)
>+        layoutTestController.notifyDone();
>+}
>+
>+var url = 'ws://localhost:8880/websocket/tests/handshake-fail-by-prepended-null';
>+var ws = new WebSocket(url);
>+
>+ws.onopen = function()
>+{
>+    debug('Connected');
>+    connected = true;
>+}
>+
>+ws.onmessage = function(messageEvent)
>+{
>+    origin = messageEvent.data;
>+    debug('origin = ' + origin);
>+    ws.close();
>+}
>+
>+ws.onclose = function()
>+{
>+    endTest();
>+}
>+
>+function timeoutCallback()
>+{
>+    debug('Timed out (state = ' + ws.readyState + ')');
>+    timedOut = true;
>+    endTest();
>+}
>+
>+var timeoutID = setTimeout(timeoutCallback, 3000);
>+
>+var successfullyParsed = true;
>+</script>
>+</body>
>+</html>
>Index: LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null_wsh.py
>===================================================================
>--- LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null_wsh.py	(revision 0)
>+++ LayoutTests/http/tests/websocket/tests/handshake-fail-by-prepended-null_wsh.py	(revision 0)
>@@ -0,0 +1,49 @@
>+# Copyright (C) Research In Motion Limited 2011. All rights reserved.
>+#
>+# Redistribution and use in source and binary forms, with or without modification, are permitted provided that the
>+# following conditions are met:
>+#
>+# 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following
>+# disclaimer.
>+#
>+# 2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the
>+# following disclaimer in the documentation and/or other materials provided with the distribution.
>+#
>+# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS âAS ISâ AND ANY EXPRESS OR
>+# IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
>+# MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
>+# APPLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>+# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>+# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
>+# TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>+# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+
>+import time
>+
>+def web_socket_do_extra_handshake(request):
>+    # This simulates a broken server that sends a WebSocket frame before the
>+    # handshake, and more frames afterwards.  It is important that if this
>+    # happens the client does not buffer all the frames as the server continues
>+    # to send more data - it should abort after reading a reasonable number of
>+    # bytes (set arbitrarily to 1024).
>+    frame = '\0Frame-contains-thirty-two-bytes'
>+
>+    msg = frame
>+    msg += 'HTTP/1.1 101 WebSocket Protocol Handshake\r\n'
>+    msg += 'Upgrade: WebSocket\r\n'
>+    msg += 'Connection: Upgrade\r\n'
>+    msg += 'Sec-WebSocket-Location: ' + request.ws_location + '\r\n'
>+    msg += 'Sec-WebSocket-Origin: ' + request.ws_origin + '\r\n'
>+    msg += '\r\n'
>+    msg += request.ws_challenge_md5
>+    request.connection.write(msg)
>+    # continue writing data until the client disconnects
>+    while True:
>+        time.sleep(1)
>+        numFrames = 1024 / len(frame) # write over 1024 bytes including the above handshake
>+        for i in range(0, numFrames):
>+            request.connection.write(frame)
>+
>+def web_socket_transfer_data(request):
>+    pass

Actions: View | Formatted Diff | Diff

Attachments on bug 51253: 76879 | 78375 | 78376 | 78377 | 78408