From: commit-queue@webkit.org Date: Thu, 12 Sep 2013 20:00:17 +0000 (+0000) Subject: Web Inspector: Do not try to parse incomplete HTTP requests X-Git-Url: https://git.webkit.org/?p=WebKit-https.git;a=commitdiff_plain;h=4272369db487ead12d5decce0e7218e123578f85 Web Inspector: Do not try to parse incomplete HTTP requests https://bugs.webkit.org/show_bug.cgi?id=121123 Patch by Andre Moreira Magalhaes on 2013-09-12 Reviewed by Darin Adler. When working on a patch for bug #121121 I found an issue with the InspectorServer where it would try to parse an HTTP message before receiving the full message and thus fail connecting with the chromedevtools plugin. What happens is that the WebSocketServerConnection receives buffers on WebSocketServerConnection::didReceiveSocketStreamData and calls WebSocketServerConnection::readHTTPMessage which then checks if we have a valid request by calling HTTPRequest::parseHTTPRequestFromBuffer. If the request is valid it tries to parse the message and clears the buffer, otherwise it continues adding data to the internal buffer until we have a valid request. The problem is that currently HTTPRequest::parseHTTPRequestFromBuffer considers the request as valid before receiving the full message. To solve this we should make the method check if the request headers end with a blank line otherwise we consider the request as invalid (see also http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html). * UIProcess/API/gtk/tests/TestInspectorServer.cpp: (sendIncompleteRequest): (beforeAll): Add GTK specific test to check if the inspector server replies to incomplete requests. * UIProcess/InspectorServer/HTTPRequest.cpp: (WebKit::HTTPRequest::parseHeaders): Do not consider request valid if headers didn't end with a blank line. git-svn-id: https://svn.webkit.org/repository/webkit/trunk@155642 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog index 9719ae67e2a8..77518dfcb1a0 100644 --- a/Source/WebKit2/ChangeLog +++ b/Source/WebKit2/ChangeLog @@ -1,3 +1,34 @@ +2013-09-12 Andre Moreira Magalhaes + + Web Inspector: Do not try to parse incomplete HTTP requests + https://bugs.webkit.org/show_bug.cgi?id=121123 + + Reviewed by Darin Adler. + + When working on a patch for bug #121121 I found an issue with the InspectorServer where it would try + to parse an HTTP message before receiving the full message and thus fail connecting with the + chromedevtools plugin. + + What happens is that the WebSocketServerConnection receives buffers on + WebSocketServerConnection::didReceiveSocketStreamData and calls + WebSocketServerConnection::readHTTPMessage which then checks if we have a valid request by calling + HTTPRequest::parseHTTPRequestFromBuffer. If the request is valid it tries to parse the message and + clears the buffer, otherwise it continues adding data to the internal buffer until we have a valid + request. + + The problem is that currently HTTPRequest::parseHTTPRequestFromBuffer considers the request as valid + before receiving the full message. To solve this we should make the method check if the request + headers end with a blank line otherwise we consider the request as invalid (see also + http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html). + + * UIProcess/API/gtk/tests/TestInspectorServer.cpp: + (sendIncompleteRequest): + (beforeAll): + Add GTK specific test to check if the inspector server replies to incomplete requests. + * UIProcess/InspectorServer/HTTPRequest.cpp: + (WebKit::HTTPRequest::parseHeaders): + Do not consider request valid if headers didn't end with a blank line. + 2013-09-12 Anders Carlsson SharedBuffer::createNSData should return a RetainPtr diff --git a/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp b/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp index 35fb3fcdca4e..0b2354f94ee3 100644 --- a/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp +++ b/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp @@ -240,6 +240,33 @@ static void openRemoteDebuggingSession(InspectorServerTest* test, gconstpointer) g_assert_cmpstr(webkit_web_view_get_title(test->m_webView), ==, "Web Inspector - http://127.0.0.1:2999/"); } +static void sendIncompleteRequest(InspectorServerTest* test, gconstpointer) +{ + GOwnPtr error; + + // Connect to the inspector server. + GSocketClient* client = g_socket_client_new(); + GSocketConnection* connection = g_socket_client_connect_to_host(client, "127.0.0.1", 2999, NULL, &error.outPtr()); + g_assert(!error.get()); + + // Send incomplete request (missing blank line after headers) and check if inspector server + // replies. The server should not reply to an incomplete request and the test should timeout + // on read. + GOutputStream* ostream = g_io_stream_get_output_stream(G_IO_STREAM(connection)); + // Request missing blank line after headers. + const gchar* incompleteRequest = "GET /devtools/page/1 HTTP/1.1\r\nHost: Localhost\r\n"; + g_output_stream_write(ostream, incompleteRequest, strlen(incompleteRequest), NULL, &error.outPtr()); + g_assert(!error.get()); + + GInputStream* istream = g_io_stream_get_input_stream(G_IO_STREAM(connection)); + char response[16]; + memset(response, 0, sizeof(response)); + g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, NULL, NULL, NULL); + // Give a chance for the server to reply. + test->wait(2); + // If we got any answer it means the server replied to an incomplete request, lets fail. + g_assert(String(response).isEmpty()); +} void beforeAll() { @@ -250,6 +277,7 @@ void beforeAll() InspectorServerTest::add("WebKitWebInspectorServer", "test-page-list", testInspectorServerPageList); InspectorServerTest::add("WebKitWebInspectorServer", "test-remote-debugging-message", testRemoteDebuggingMessage); InspectorServerTest::add("WebKitWebInspectorServer", "test-open-debugging-session", openRemoteDebuggingSession); + InspectorServerTest::add("WebKitWebInspectorServer", "test-incomplete-request", sendIncompleteRequest); } diff --git a/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp b/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp index 734926c6c560..5d9c4b2ea3cc 100644 --- a/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp +++ b/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp @@ -93,6 +93,13 @@ size_t HTTPRequest::parseHeaders(const char* data, size_t length, String& failur break; m_headerFields.add(name, value); } + + // If we got here and "name" is empty, it means the headers are valid and ended with a + // blank line (parseHTTPHeader returns "name" as empty if parsing a blank line), otherwise + // the headers didn't end with a blank line and we have an invalid request. + // See also http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html + if (!name.isEmpty()) + return 0; return p - data; }