Reviewed by Darin.
authorap <ap@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Jan 2007 18:00:30 +0000 (18:00 +0000)
committerap <ap@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Jan 2007 18:00:30 +0000 (18:00 +0000)
        http://bugs.webkit.org/show_bug.cgi?id=6272
        XMLHttpRequest freezes on getting a missing document with overridden Content-Length

        http://bugs.webkit.org/show_bug.cgi?id=6212
        Investigate disallowing some XMLHttpRequest headers from being set via setRequestHeader.

        Tests:
        - http/tests/xmlhttprequest/connection-error-sync.html
        - http/tests/xmlhttprequest/set-dangerous-headers.html

        * bindings/js/kjs_binding.cpp:
        (KJS::):
        (KJS::setDOMException): Added support for NETWORK_ERR. Changed the temporary
        PERMISSION_DENIED error into a special case.

        * loader/FrameLoader.cpp:
        (WebCore::FrameLoader::loadResourceSynchronously):
        * loader/FrameLoader.h:
        Return the error object to the caller. Removed an extra local variable for error,
        which shadowed the one from enclosing block.

        * dom/XMLTokenizer.cpp:
        (WebCore::openFunc):
        * xml/XSLTProcessor.cpp:
        (WebCore::docLoaderFunc):
        Updated to pass an error object (currently ignored).

        * xml/xmlhttprequest.cpp:
        (WebCore::canSetRequestHeader): Added. The headers to block include the ones from
        the current XMLHttpRequest draft plus "Via", which is blocked by Firefox.
        (WebCore::XMLHttpRequest::send): Raise an exception if a sync request results in
        an error.
        (WebCore::XMLHttpRequest::setRequestHeader): Call canSetRequestHeader().
        * xml/xmlhttprequest.h:
        (WebCore::): Added NETWORK_ERR and a comment about PERMISSION_DENIED.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/xmlhttprequest/connection-error-sync-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/xmlhttprequest/connection-error-sync.html [new file with mode: 0644]
LayoutTests/http/tests/xmlhttprequest/resources/infinite-loop.php [new file with mode: 0644]
LayoutTests/http/tests/xmlhttprequest/resources/print-headers.cgi [new file with mode: 0755]
LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/bindings/js/kjs_binding.cpp
WebCore/dom/XMLTokenizer.cpp
WebCore/loader/FrameLoader.cpp
WebCore/loader/FrameLoader.h
WebCore/xml/XSLTProcessor.cpp
WebCore/xml/xmlhttprequest.cpp
WebCore/xml/xmlhttprequest.h

index 3aa46b7..86117c1 100644 (file)
@@ -1,3 +1,20 @@
+2007-01-15  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        http://bugs.webkit.org/show_bug.cgi?id=6272
+        XMLHttpRequest freezes on getting a missing document with overridden Content-Length
+
+        http://bugs.webkit.org/show_bug.cgi?id=6212
+        Investigate disallowing some XMLHttpRequest headers from being set via setRequestHeader.
+
+        * http/tests/xmlhttprequest/connection-error-sync-expected.txt: Added.
+        * http/tests/xmlhttprequest/connection-error-sync.html: Added.
+        * http/tests/xmlhttprequest/resources/infinite-loop.php: Added.
+        * http/tests/xmlhttprequest/resources/print-headers.cgi: Added.
+        * http/tests/xmlhttprequest/set-dangerous-headers-expected.txt: Added.
+        * http/tests/xmlhttprequest/set-dangerous-headers.html: Added.
+
 2007-01-15  Rob Buis  <buis@kde.org>
 
         Reviewed by Eric.
diff --git a/LayoutTests/http/tests/xmlhttprequest/connection-error-sync-expected.txt b/LayoutTests/http/tests/xmlhttprequest/connection-error-sync-expected.txt
new file mode 100644 (file)
index 0000000..b7818e4
--- /dev/null
@@ -0,0 +1,3 @@
+Test the behavior of a sync XMLHttpRequest that encounters an infinite redirection loop.
+
+Exception Error; code=101; number=undefined (0); message='NETWORK_ERR: XMLHttpRequest Exception 101'
diff --git a/LayoutTests/http/tests/xmlhttprequest/connection-error-sync.html b/LayoutTests/http/tests/xmlhttprequest/connection-error-sync.html
new file mode 100644 (file)
index 0000000..a1522f8
--- /dev/null
@@ -0,0 +1,32 @@
+<html>
+<head>
+<body>
+<p>Test the behavior of a sync XMLHttpRequest that encounters an infinite redirection loop.</p>
+<script>
+
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+
+    try {
+        if (window.XMLHttpRequest) {
+            req = new XMLHttpRequest();
+        } else {
+            try {
+                req = new ActiveXObject("Msxml2.XMLHTTP");
+            } catch (ex) {
+                req = new ActiveXObject("Microsoft.XMLHTTP");
+            }
+        }
+        
+        req.open('GET', 'resources/infinite-loop.php', false);
+        req.send(null);
+
+        document.write("Status: " + req.status);
+        
+    } catch (ex) {
+        document.write("Exception " + ex.name + "; code=" + ex.code + "; number=" + ex.number + " (" + (ex.number & 0xFFFF) + "); message='" + ex.message + "'");
+    }
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/tests/xmlhttprequest/resources/infinite-loop.php b/LayoutTests/http/tests/xmlhttprequest/resources/infinite-loop.php
new file mode 100644 (file)
index 0000000..b812efc
--- /dev/null
@@ -0,0 +1,4 @@
+<?php
+  header('HTTP/1.0 302 Found');
+  header('Location: infinite-loop.php');
+?>
diff --git a/LayoutTests/http/tests/xmlhttprequest/resources/print-headers.cgi b/LayoutTests/http/tests/xmlhttprequest/resources/print-headers.cgi
new file mode 100755 (executable)
index 0000000..32d0f42
--- /dev/null
@@ -0,0 +1,11 @@
+#!/usr/bin/perl -wT
+use strict;
+
+print "Content-Type: text/plain\n";
+print "Cache-Control: no-store\n\n";
+
+foreach (keys %ENV) {
+    if ($_ =~ "HTTP_") {
+        print $_ . ": " . $ENV{$_} . "\n";
+    }
+}
diff --git a/LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers-expected.txt b/LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers-expected.txt
new file mode 100644 (file)
index 0000000..5e8cfa2
--- /dev/null
@@ -0,0 +1,4 @@
+Test that setRequestHeader cannot be used to alter security-sensitive headers.
+
+SUCCESS
+
diff --git a/LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers.html b/LayoutTests/http/tests/xmlhttprequest/set-dangerous-headers.html
new file mode 100644 (file)
index 0000000..6b7e459
--- /dev/null
@@ -0,0 +1,37 @@
+<html>
+<body>
+<p>Test that setRequestHeader cannot be used to alter security-sensitive headers.</p>
+<pre id=result>FAIL: script didn't run or raised an unexpected exception.</pre>
+<script>
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+    
+    req = new XMLHttpRequest;
+    req.open("GET", "resources/print-headers.cgi", false);
+
+    req.setRequestHeader("ACCEPT-CHARSET", "foobar");
+    req.setRequestHeader("ACCEPT-ENCODING", "foobar");
+    req.setRequestHeader("CONTENT-LENGTH", "123456");
+    req.setRequestHeader("EXPECT", "100-continue");
+    req.setRequestHeader("DATE", "foobar");
+    req.setRequestHeader("HOST", "foobar");
+    req.setRequestHeader("KEEP-ALIVE", "foobar");
+    req.setRequestHeader("REFERER", "foobar");
+    req.setRequestHeader("TE", "foobar");
+    req.setRequestHeader("TRAILER", "foobar");
+    req.setRequestHeader("TRANSFER-ENCODING", "foobar");
+    req.setRequestHeader("UPGRADE", "foobar");
+    req.setRequestHeader("VIA", "foobar");
+
+    try {
+        req.send("");
+        if (req.responseText.match("100-continue|foobar|123456"))
+            document.getElementById("result").textContent = req.responseText;
+        else
+            document.getElementById("result").textContent = "SUCCESS";
+    } catch (ex) {
+        document.getElementById("result").textContent = ex;
+    }
+</script>
+</body>
+</html>
index 598fd07..85205d5 100644 (file)
@@ -1,3 +1,43 @@
+2007-01-15  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin.
+
+        http://bugs.webkit.org/show_bug.cgi?id=6272
+        XMLHttpRequest freezes on getting a missing document with overridden Content-Length
+
+        http://bugs.webkit.org/show_bug.cgi?id=6212
+        Investigate disallowing some XMLHttpRequest headers from being set via setRequestHeader.
+
+        Tests:
+        - http/tests/xmlhttprequest/connection-error-sync.html
+        - http/tests/xmlhttprequest/set-dangerous-headers.html
+
+        * bindings/js/kjs_binding.cpp:
+        (KJS::):
+        (KJS::setDOMException): Added support for NETWORK_ERR. Changed the temporary 
+        PERMISSION_DENIED error into a special case.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadResourceSynchronously):
+        * loader/FrameLoader.h:
+        Return the error object to the caller. Removed an extra local variable for error, 
+        which shadowed the one from enclosing block.
+
+        * dom/XMLTokenizer.cpp:
+        (WebCore::openFunc): 
+        * xml/XSLTProcessor.cpp:
+        (WebCore::docLoaderFunc): 
+        Updated to pass an error object (currently ignored).
+
+        * xml/xmlhttprequest.cpp:
+        (WebCore::canSetRequestHeader): Added. The headers to block include the ones from 
+        the current XMLHttpRequest draft plus "Via", which is blocked by Firefox.
+        (WebCore::XMLHttpRequest::send): Raise an exception if a sync request results in 
+        an error.
+        (WebCore::XMLHttpRequest::setRequestHeader): Call canSetRequestHeader().
+        * xml/xmlhttprequest.h:
+        (WebCore::): Added NETWORK_ERR and a comment about PERMISSION_DENIED.
+
 2007-01-15  Zack Rusin  <zack@kde.org>
 
         Optimizing rendering on the Qt platform. Mainly
index 57a73b4..6f9f741 100644 (file)
@@ -353,7 +353,7 @@ static const char* const eventExceptionNames[] = {
 };
 
 static const char* const xmlHttpRequestExceptionNames[] = {
-    "Permission denied"
+    "NETWORK_ERR"
 };
 
 #ifdef XPATH_SUPPORT
@@ -395,15 +395,17 @@ void setDOMException(ExecState* exec, ExceptionCode ec)
         nameIndex = code;
         nameTable = eventExceptionNames;
         nameTableSize = sizeof(eventExceptionNames) / sizeof(eventExceptionNames[0]);
-    } else if (code >= XMLHttpRequestExceptionOffset && code <= XMLHttpRequestExceptionMax) {
-        // XMLHttpRequest case is special, because there is no exception code assigned (yet?).
+    } else if (code == XMLHttpRequestExceptionOffset) {
+        // FIXME: this exception should be replaced with DOM SECURITY_ERR when it finds its way to the spec.
+        throwError(exec, GeneralError, "Permission denied");
+        return;
+    } else if (code > XMLHttpRequestExceptionOffset && code <= XMLHttpRequestExceptionMax) {
+        type = "XMLHttpRequest";
+        // XMLHttpRequest exception codes start with 101 and we don't want 100 empty elements in the name array
+        nameIndex = code - NETWORK_ERR;
         code -= XMLHttpRequestExceptionOffset;
-        nameIndex = code;
         nameTable = xmlHttpRequestExceptionNames;
         nameTableSize = sizeof(xmlHttpRequestExceptionNames) / sizeof(xmlHttpRequestExceptionNames[0]);
-
-        throwError(exec, GeneralError, nameIndex < nameTableSize ? nameTable[nameIndex] : 0);
-        return;
 #ifdef XPATH_SUPPORT
     } else if (code >= XPathExceptionOffset && code <= XPathExceptionMax) {
         type = "DOM XPath";
@@ -427,7 +429,7 @@ void setDOMException(ExecState* exec, ExceptionCode ec)
         nameTableSize = sizeof(exceptionNames) / sizeof(exceptionNames[0]);
     }
 
-    const char* name = nameIndex < nameTableSize ? nameTable[nameIndex] : 0;
+    const char* name = (nameIndex < nameTableSize && nameIndex >= 0) ? nameTable[nameIndex] : 0;
 
     // 100 characters is a big enough buffer, because there are:
     //   13 characters in the message
index d21a6f2..76dbf1f 100644 (file)
@@ -373,11 +373,12 @@ static void* openFunc(const char* uri)
     if (!globalDocLoader || !shouldAllowExternalLoad(uri))
         return &globalDescriptor;
 
+    ResourceError error;
     ResourceResponse response;
     Vector<char> data;
     
     if (globalDocLoader->frame()) 
-        globalDocLoader->frame()->loader()->loadResourceSynchronously(KURL(uri), response, data);
+        globalDocLoader->frame()->loader()->loadResourceSynchronously(KURL(uri), error, response, data);
 
     return new OffsetBuffer(data);
 }
index 76b1a16..2480a54 100644 (file)
@@ -2964,7 +2964,7 @@ void FrameLoader::loadEmptyDocumentSynchronously()
     load(request);
 }
 
-void FrameLoader::loadResourceSynchronously(const ResourceRequest& request, ResourceResponse& response, Vector<char>& data)
+void FrameLoader::loadResourceSynchronously(const ResourceRequest& request, ResourceError& error, ResourceResponse& response, Vector<char>& data)
 {
     // Since this is a subresource, we can load any URL (we ignore the return value).
     // But we still want to know whether we should hide the referrer or not, so we call the canLoad method.
@@ -2988,17 +2988,13 @@ void FrameLoader::loadResourceSynchronously(const ResourceRequest& request, Reso
     initialRequest.setMainDocumentURL(m_frame->page()->mainFrame()->loader()->documentLoader()->request().url());
     initialRequest.setHTTPUserAgent(client()->userAgent());
     
-    ResourceError error;
     unsigned long identifier = 0;    
     ResourceRequest newRequest(initialRequest);
     requestFromDelegate(newRequest, identifier, error);
 
     if (error.isNull()) {
         ASSERT(!newRequest.isNull());
-        ResourceError error;
-        
         didTellBridgeAboutLoad(newRequest.url().url());
-
         ResourceHandle::loadResourceSynchronously(newRequest, error, response, data);
     }
     
index 4e039c9..fad0a88 100644 (file)
@@ -156,7 +156,7 @@ namespace WebCore {
 
         Frame* createWindow(const FrameLoadRequest&, const WindowFeatures&);
 
-        void loadResourceSynchronously(const ResourceRequest& request, ResourceResponse& r, Vector<char>& data);
+        void loadResourceSynchronously(const ResourceRequest&, ResourceError&, ResourceResponse&, Vector<char>& data);
 
         bool canHandleRequest(const ResourceRequest&);
 
index 6e1aad2..c282a17 100644 (file)
@@ -89,6 +89,7 @@ static xmlDocPtr docLoaderFunc(const xmlChar *uri,
             xmlChar *base = xmlNodeGetBase(context->document->doc, context->node);
             KURL url((const char*)base, (const char*)uri);
             xmlFree(base);
+            ResourceError error;
             ResourceResponse response;
             xmlGenericErrorFunc oldErrorFunc = xmlGenericError;
             void *oldErrorContext = xmlGenericErrorContext;
@@ -96,7 +97,7 @@ static xmlDocPtr docLoaderFunc(const xmlChar *uri,
             Vector<char> data;
                 
             if (globalDocLoader->frame()) 
-                globalDocLoader->frame()->loader()->loadResourceSynchronously(url, response, data);
+                globalDocLoader->frame()->loader()->loadResourceSynchronously(url, error, response, data);
 
             xmlSetGenericErrorFunc(0, parseErrorFunc);
             // We don't specify an encoding here. Neither Gecko nor WinIE respects
index c0ca66f..2088611 100644 (file)
@@ -1,7 +1,7 @@
 /*
  *  This file is part of the KDE libraries
  *  Copyright (C) 2004, 2006 Apple Computer, Inc.
- *  Copyright (C) 2005, 2006 Alexey Proskuryakov <ap@nypop.com>
+ *  Copyright (C) 2005-2007 Alexey Proskuryakov <ap@webkit.org>
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -33,6 +33,7 @@
 #include "Frame.h"
 #include "FrameLoader.h"
 #include "HTMLDocument.h"
+#include "Page.h"
 #include "PlatformString.h"
 #include "RegularExpression.h"
 #include "ResourceHandle.h"
@@ -141,6 +142,29 @@ static String getCharset(const String& contentTypeString)
     return String();
 }
 
+static bool canSetRequestHeader(const String& name)
+{
+    static HashSet<String, CaseInsensitiveHash<String> > forbiddenHeaders;
+    
+    if (forbiddenHeaders.isEmpty()) {
+        forbiddenHeaders.add("accept-charset");
+        forbiddenHeaders.add("accept-encoding");
+        forbiddenHeaders.add("content-length");
+        forbiddenHeaders.add("expect");
+        forbiddenHeaders.add("date");
+        forbiddenHeaders.add("host");
+        forbiddenHeaders.add("keep-alive");
+        forbiddenHeaders.add("referer");
+        forbiddenHeaders.add("te");
+        forbiddenHeaders.add("trailer");
+        forbiddenHeaders.add("transfer-encoding");
+        forbiddenHeaders.add("upgrade");
+        forbiddenHeaders.add("via");
+    }
+    
+    return !forbiddenHeaders.contains(name);
+}
+
 XMLHttpRequestState XMLHttpRequest::getReadyState() const
 {
     return m_state;
@@ -400,17 +424,21 @@ void XMLHttpRequest::send(const String& body, ExceptionCode& ec)
 
     if (!m_async) {
         Vector<char> data;
+        ResourceError error;
         ResourceResponse response;
 
         {
             // avoid deadlock in case the loader wants to use JS on a background thread
             KJS::JSLock::DropAllLocks dropLocks;
             if (m_doc->frame()) 
-                m_doc->frame()->loader()->loadResourceSynchronously(request, response, data);
+                m_doc->frame()->loader()->loadResourceSynchronously(request, error, response, data);
         }
 
         m_loader = 0;
-        processSyncLoadResults(data, response);
+        if (error.isNull())
+            processSyncLoadResults(data, response);
+        else
+            ec = NETWORK_ERR;
     
         return;
     }
@@ -466,6 +494,12 @@ void XMLHttpRequest::setRequestHeader(const String& name, const String& value, E
         return;
     }
 
+    if (!canSetRequestHeader(name)) {
+        if (m_doc && m_doc->frame() && m_doc->frame()->page())
+            m_doc->frame()->page()->chrome()->addMessageToConsole("Refused to set unsafe header " + name, 1, String());
+        return;
+    }
+
     if (!m_requestHeaders.contains(name)) {
         m_requestHeaders.set(name, value);
         return;
index bceca5c..beb0fdc 100644 (file)
@@ -44,8 +44,11 @@ class String;
 typedef int ExceptionCode;
 
 const int XMLHttpRequestExceptionOffset = 500;
-const int XMLHttpRequestExceptionMax = 599;
-enum XMLHttpRequestExceptionCode { PERMISSION_DENIED = XMLHttpRequestExceptionOffset };
+const int XMLHttpRequestExceptionMax = 699;
+enum XMLHttpRequestExceptionCode {
+    PERMISSION_DENIED = XMLHttpRequestExceptionOffset, // Use SECURITY_ERR when that's in DOM Core, http://bugs.webkit.org/show_bug.cgi?id=12182
+    NETWORK_ERR = XMLHttpRequestExceptionOffset + 101
+};
 
 // these exact numeric values are important because JS expects them
 enum XMLHttpRequestState {