Reviewed by Darin Adler.
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Dec 2009 19:16:15 +0000 (19:16 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Dec 2009 19:16:15 +0000 (19:16 +0000)
        https://bugs.webkit.org/show_bug.cgi?id=32085
        WebSocket should block the same ports that are blocked for resource loading

        Test: websocket/tests/url-parsing.html

        * page/SecurityOrigin.cpp:
        (WebCore::SecurityOrigin::SecurityOrigin):
        (WebCore::SecurityOrigin::localURLSchemes):
        * page/SecurityOrigin.h:
        Move isDefaultPortForProtocol() to KURL, because that's a better place for it (SecurityOrigin
        is not even in WebCore/platform directory).

        * html/HTMLAnchorElement.cpp:
        (WebCore::HTMLAnchorElement::host):
        (WebCore::HTMLAnchorElement::setHost):
        (WebCore::HTMLAnchorElement::setPort):
        Updated for the new location of isDefaultPortForProtocol().

        * platform/KURL.cpp:
        (WebCore::KURL::protocolIs): In an assertion, compare to "javascript" case-insensitively,
        since the function deosn't require lower case input.
        (WebCore::isDefaultPortForProtocol): Moved from SecurityOrigin.
        (WebCore::portAllowed): Moved from ResourceHandle.
        * platform/KURL.h:

        * platform/network/ResourceHandle.cpp: (WebCore::ResourceHandle::create): Updated for the
        new location of portAllowed().

        * websockets/WebSocket.cpp: (WebCore::WebSocket::connect): Per the spec, raise a SECURITY_ERR
        if trying to connect to a blocked port.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/websocket/tests/script-tests/url-parsing.js [new file with mode: 0644]
LayoutTests/websocket/tests/url-parsing-expected.txt [new file with mode: 0644]
LayoutTests/websocket/tests/url-parsing.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/html/HTMLAnchorElement.cpp
WebCore/page/SecurityOrigin.cpp
WebCore/page/SecurityOrigin.h
WebCore/platform/KURL.cpp
WebCore/platform/KURL.h
WebCore/platform/network/ResourceHandle.cpp
WebCore/websockets/WebSocket.cpp

index 78422558e926abda86633fbff54c4de5abc7ecb0..992a69bfa528edd28d0aaf51aa9d9809aeed0ac0 100644 (file)
@@ -1,3 +1,17 @@
+2009-12-04  Alexey Proskuryakov  <ap@apple.com>
+
+        Reviewed by Darin Adler.
+
+        https://bugs.webkit.org/show_bug.cgi?id=32085
+        WebSocket should block the same ports that are blocked for resource loading
+
+        Added some tests for URL parsing (one of them expects current WebKit behavior, not what the
+        spec says).
+
+        * websocket/tests/script-tests/url-parsing.js: Added.
+        * websocket/tests/url-parsing-expected.txt: Added.
+        * websocket/tests/url-parsing.html: Added.
+
 2009-12-04  Benjamin Poulain  <benjamin.poulain@nokia.com>
 
         Reviewed by Kenneth Rohde Christiansen.
diff --git a/LayoutTests/websocket/tests/script-tests/url-parsing.js b/LayoutTests/websocket/tests/script-tests/url-parsing.js
new file mode 100644 (file)
index 0000000..59f2a83
--- /dev/null
@@ -0,0 +1,14 @@
+description("Test WebSocket URL parsing.");
+
+// Can't use relative URLs - because spec says so, and because the scheme is different anyway.
+shouldThrow('new WebSocket("/applet")');
+
+// UA is allowed to block access to some ports, which we do.
+shouldThrow('new WebSocket("ws://127.0.0.1:25/")');
+
+// This is what we currently do, but not what the spec says (as of Editor's Draft 1 December 2009).
+// The spec says that the string passed to WebScoket constructor should be returned unchanged.
+shouldBe('(new WebSocket("ws://127.0.0.1/a/../")).URL', '"ws://127.0.0.1/"');
+
+var successfullyParsed = true;
+isSuccessfullyParsed();
diff --git a/LayoutTests/websocket/tests/url-parsing-expected.txt b/LayoutTests/websocket/tests/url-parsing-expected.txt
new file mode 100644 (file)
index 0000000..d43a689
--- /dev/null
@@ -0,0 +1,11 @@
+Test WebSocket URL parsing.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+PASS new WebSocket("/applet") threw exception Error: SYNTAX_ERR: DOM Exception 12.
+PASS new WebSocket("ws://127.0.0.1:25/") threw exception Error: SECURITY_ERR: DOM Exception 18.
+PASS (new WebSocket("ws://127.0.0.1/a/../")).URL is "ws://127.0.0.1/"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/websocket/tests/url-parsing.html b/LayoutTests/websocket/tests/url-parsing.html
new file mode 100644 (file)
index 0000000..4d6dde3
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<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 src="script-tests/url-parsing.js"></script>
+</body>
+</html>
index 4f4864503796fcd35220e0178aeadfb986bdf63a..c6c3fc26f02bf8baba92173c95c2620441793514 100644 (file)
@@ -1,3 +1,38 @@
+2009-12-04  Alexey Proskuryakov  <ap@apple.com>
+
+        Reviewed by Darin Adler.
+
+        https://bugs.webkit.org/show_bug.cgi?id=32085
+        WebSocket should block the same ports that are blocked for resource loading
+
+        Test: websocket/tests/url-parsing.html
+
+        * page/SecurityOrigin.cpp:
+        (WebCore::SecurityOrigin::SecurityOrigin):
+        (WebCore::SecurityOrigin::localURLSchemes):
+        * page/SecurityOrigin.h:
+        Move isDefaultPortForProtocol() to KURL, because that's a better place for it (SecurityOrigin
+        is not even in WebCore/platform directory).
+
+        * html/HTMLAnchorElement.cpp:
+        (WebCore::HTMLAnchorElement::host):
+        (WebCore::HTMLAnchorElement::setHost):
+        (WebCore::HTMLAnchorElement::setPort):
+        Updated for the new location of isDefaultPortForProtocol().
+
+        * platform/KURL.cpp:
+        (WebCore::KURL::protocolIs): In an assertion, compare to "javascript" case-insensitively,
+        since the function doesn't require lower case input.
+        (WebCore::isDefaultPortForProtocol): Moved from SecurityOrigin.
+        (WebCore::portAllowed): Moved from ResourceHandle.
+        * platform/KURL.h:
+
+        * platform/network/ResourceHandle.cpp: (WebCore::ResourceHandle::create): Updated for the
+        new location of portAllowed().
+
+        * websockets/WebSocket.cpp: (WebCore::WebSocket::connect): Per the spec, raise a SECURITY_ERR
+        if trying to connect to a blocked port.
+
 2009-12-04  Benjamin Poulain  <benjamin.poulain@nokia.com>
 
         Reviewed by Kenneth Rohde Christiansen.
index 446d4e0f3fe8105d509cf7f2dd3c373719b43203..58d39c175e9e07d917b1dc66a942099ab781ec20 100644 (file)
@@ -386,7 +386,7 @@ String HTMLAnchorElement::host() const
     const KURL& url = href();
     if (url.hostEnd() == url.pathStart())
         return url.host();
-    if (SecurityOrigin::isDefaultPortForProtocol(url.port(), url.protocol()))
+    if (isDefaultPortForProtocol(url.port(), url.protocol()))
         return url.host();
     return url.host() + ":" + String::number(url.port());
 }
@@ -414,7 +414,7 @@ void HTMLAnchorElement::setHost(const String& value)
             // requires setting the port to "0" if it is set to empty string.
             url.setHostAndPort(value.substring(0, separator + 1) + "0");
         } else {
-            if (SecurityOrigin::isDefaultPortForProtocol(port, url.protocol()))
+            if (isDefaultPortForProtocol(port, url.protocol()))
                 url.setHostAndPort(value.substring(0, separator));
             else
                 url.setHostAndPort(value.substring(0, portEnd));
@@ -482,7 +482,7 @@ void HTMLAnchorElement::setPort(const String& value)
     // specifically goes against RFC 3986 (p3.2) and
     // requires setting the port to "0" if it is set to empty string.
     unsigned port = value.toUInt();
-    if (SecurityOrigin::isDefaultPortForProtocol(port, url.protocol()))
+    if (isDefaultPortForProtocol(port, url.protocol()))
         url.removePort();
     else
         url.setPort(port);
index 6d55d3ef7f59fddb9f231df690c82b07825217b7..f53dbf146cc19104a5fb71956d9b3a74a2073196 100644 (file)
@@ -75,22 +75,6 @@ static URLSchemesMap& noAccessSchemes()
     return noAccessSchemes;
 }
 
-bool SecurityOrigin::isDefaultPortForProtocol(unsigned short port, const String& protocol)
-{
-    if (protocol.isEmpty())
-        return false;
-
-    typedef HashMap<String, unsigned> DefaultPortsMap;
-    DEFINE_STATIC_LOCAL(DefaultPortsMap, defaultPorts, ());
-    if (defaultPorts.isEmpty()) {
-        defaultPorts.set("http", 80);
-        defaultPorts.set("https", 443);
-        defaultPorts.set("ftp", 21);
-        defaultPorts.set("ftps", 990);
-    }
-    return defaultPorts.get(protocol) == port;
-}
-
 SecurityOrigin::SecurityOrigin(const KURL& url)
     : m_protocol(url.protocol().isNull() ? "" : url.protocol().lower())
     , m_host(url.host().isNull() ? "" : url.host().lower())
@@ -411,7 +395,7 @@ void SecurityOrigin::removeURLSchemeRegisteredAsLocal(const String& scheme)
     localSchemes().remove(scheme);
 }
 
-const URLSchemesMap&  SecurityOrigin::localURLSchemes()
+const URLSchemesMap& SecurityOrigin::localURLSchemes()
 {
     return localSchemes();
 }
index bf3221f0846449d745480078054a5ba5e3d556ca..af83f02109af87e83c667d2f9de139fde4f1400d 100644 (file)
@@ -179,8 +179,6 @@ namespace WebCore {
         static void whiteListAccessFromOrigin(const SecurityOrigin& sourceOrigin, const String& destinationProtocol, const String& destinationDomains, bool allowDestinationSubdomains);
         static void resetOriginAccessWhiteLists();
 
-        static bool isDefaultPortForProtocol(unsigned short port, const String& protocol);
-
     private:
         explicit SecurityOrigin(const KURL&);
         explicit SecurityOrigin(const SecurityOrigin*);
index 3c93952cc14dccc69ca2ebe21ca99c14d50cc5cb..12e34bd8efc71b61a52d528dfddef9bb8622d6dd 100644 (file)
@@ -30,7 +30,7 @@
 #include "KURL.h"
 
 #include "CString.h"
-#include "PlatformString.h"
+#include "StringHash.h"
 #include "TextEncoding.h"
 #include <wtf/StdLibExtras.h>
 
@@ -633,7 +633,7 @@ bool KURL::protocolIs(const char* protocol) const
 
     // JavaScript URLs are "valid" and should be executed even if KURL decides they are invalid.
     // The free function protocolIsJavaScript() should be used instead. 
-    ASSERT(strcmp(protocol, "javascript") != 0);
+    ASSERT(!equalIgnoringCase(protocol, String("javascript")));
 
     if (!m_isValid)
         return false;
@@ -1640,6 +1640,120 @@ bool isValidProtocol(const String& protocol)
     return true;
 }
 
+bool isDefaultPortForProtocol(unsigned short port, const String& protocol)
+{
+    if (protocol.isEmpty())
+        return false;
+
+    typedef HashMap<String, unsigned, CaseFoldingHash> DefaultPortsMap;
+    DEFINE_STATIC_LOCAL(DefaultPortsMap, defaultPorts, ());
+    if (defaultPorts.isEmpty()) {
+        defaultPorts.set("http", 80);
+        defaultPorts.set("https", 443);
+        defaultPorts.set("ftp", 21);
+        defaultPorts.set("ftps", 990);
+    }
+    return defaultPorts.get(protocol) == port;
+}
+
+bool portAllowed(const KURL& url)
+{
+    unsigned short port = url.port();
+
+    // Since most URLs don't have a port, return early for the "no port" case.
+    if (!port)
+        return true;
+
+    // This blocked port list matches the port blocking that Mozilla implements.
+    // See http://www.mozilla.org/projects/netlib/PortBanning.html for more information.
+    static const unsigned short blockedPortList[] = {
+        1,    // tcpmux
+        7,    // echo
+        9,    // discard
+        11,   // systat
+        13,   // daytime
+        15,   // netstat
+        17,   // qotd
+        19,   // chargen
+        20,   // FTP-data
+        21,   // FTP-control
+        22,   // SSH
+        23,   // telnet
+        25,   // SMTP
+        37,   // time
+        42,   // name
+        43,   // nicname
+        53,   // domain
+        77,   // priv-rjs
+        79,   // finger
+        87,   // ttylink
+        95,   // supdup
+        101,  // hostriame
+        102,  // iso-tsap
+        103,  // gppitnp
+        104,  // acr-nema
+        109,  // POP2
+        110,  // POP3
+        111,  // sunrpc
+        113,  // auth
+        115,  // SFTP
+        117,  // uucp-path
+        119,  // nntp
+        123,  // NTP
+        135,  // loc-srv / epmap
+        139,  // netbios
+        143,  // IMAP2
+        179,  // BGP
+        389,  // LDAP
+        465,  // SMTP+SSL
+        512,  // print / exec
+        513,  // login
+        514,  // shell
+        515,  // printer
+        526,  // tempo
+        530,  // courier
+        531,  // Chat
+        532,  // netnews
+        540,  // UUCP
+        556,  // remotefs
+        563,  // NNTP+SSL
+        587,  // ESMTP
+        601,  // syslog-conn
+        636,  // LDAP+SSL
+        993,  // IMAP+SSL
+        995,  // POP3+SSL
+        2049, // NFS
+        3659, // apple-sasl / PasswordServer [Apple addition]
+        4045, // lockd
+        6000, // X11
+    };
+    const unsigned short* const blockedPortListEnd = blockedPortList + sizeof(blockedPortList) / sizeof(blockedPortList[0]);
+
+#ifndef NDEBUG
+    // The port list must be sorted for binary_search to work.
+    static bool checkedPortList = false;
+    if (!checkedPortList) {
+        for (const unsigned short* p = blockedPortList; p != blockedPortListEnd - 1; ++p)
+            ASSERT(*p < *(p + 1));
+        checkedPortList = true;
+    }
+#endif
+
+    // If the port is not in the blocked port list, allow it.
+    if (!binary_search(blockedPortList, blockedPortListEnd, port))
+        return true;
+
+    // Allow ports 21 and 22 for FTP URLs, as Mozilla does.
+    if ((port == 21 || port == 22) && url.protocolIs("ftp"))
+        return true;
+
+    // Allow any port number in a file URL, since the port number is ignored.
+    if (url.protocolIs("file"))
+        return true;
+
+    return false;
+}
+
 String mimeTypeFromDataURL(const String& url)
 {
     ASSERT(protocolIs(url, "data"));
index 993842e2d3721857ef85f1b503ddc5abcdd7aa6c..647330df09c687a064bd5f68dfb8b3900272e800 100644 (file)
@@ -264,6 +264,9 @@ bool protocolIs(const String& url, const char* protocol);
 bool protocolIsJavaScript(const String& url);
 bool isValidProtocol(const String& protocol);
 
+bool isDefaultPortForProtocol(unsigned short port, const String& protocol);
+bool portAllowed(const KURL&); // Blacklist ports that should never be used for Web resources.
+
 String mimeTypeFromDataURL(const String& url);
 
 // Unescapes the given string using URL escaping rules, given an optional
index d3ab582b4a84a97cfc384d9e0a3ab2ce2d0f47ec..7c205614f158fc6057a41bc97179c8d2eef48672 100644 (file)
@@ -36,8 +36,6 @@ namespace WebCore {
 
 static bool shouldForceContentSniffing;
 
-static bool portAllowed(const ResourceRequest&);
-
 ResourceHandle::ResourceHandle(const ResourceRequest& request, ResourceHandleClient* client, bool defersLoading,
          bool shouldContentSniff, bool mightDownloadFromHandle)
     : d(new ResourceHandleInternal(this, request, client, defersLoading, shouldContentSniff, mightDownloadFromHandle))
@@ -57,7 +55,7 @@ PassRefPtr<ResourceHandle> ResourceHandle::create(const ResourceRequest& request
         return newHandle.release();
     }
 
-    if (!portAllowed(request)) {
+    if (!portAllowed(request.url())) {
         newHandle->scheduleFailure(BlockedFailure);
         return newHandle.release();
     }
@@ -113,95 +111,6 @@ void ResourceHandle::clearAuthentication()
 #endif
     d->m_currentWebChallenge.nullify();
 }
-
-static bool portAllowed(const ResourceRequest& request)
-{
-    unsigned short port = request.url().port();
-
-    // Since most URLs don't have a port, return early for the "no port" case.
-    if (!port)
-        return true;
-
-    // This blocked port list matches the port blocking that Mozilla implements.
-    // See http://www.mozilla.org/projects/netlib/PortBanning.html for more information.
-    static const unsigned short blockedPortList[] = {
-        1,    // tcpmux
-        7,    // echo
-        9,    // discard
-        11,   // systat
-        13,   // daytime
-        15,   // netstat
-        17,   // qotd
-        19,   // chargen
-        20,   // FTP-data
-        21,   // FTP-control
-        22,   // SSH
-        23,   // telnet
-        25,   // SMTP
-        37,   // time
-        42,   // name
-        43,   // nicname
-        53,   // domain
-        77,   // priv-rjs
-        79,   // finger
-        87,   // ttylink
-        95,   // supdup
-        101,  // hostriame
-        102,  // iso-tsap
-        103,  // gppitnp
-        104,  // acr-nema
-        109,  // POP2
-        110,  // POP3
-        111,  // sunrpc
-        113,  // auth
-        115,  // SFTP
-        117,  // uucp-path
-        119,  // nntp
-        123,  // NTP
-        135,  // loc-srv / epmap
-        139,  // netbios
-        143,  // IMAP2
-        179,  // BGP
-        389,  // LDAP
-        465,  // SMTP+SSL
-        512,  // print / exec
-        513,  // login
-        514,  // shell
-        515,  // printer
-        526,  // tempo
-        530,  // courier
-        531,  // Chat
-        532,  // netnews
-        540,  // UUCP
-        556,  // remotefs
-        563,  // NNTP+SSL
-        587,  // ESMTP
-        601,  // syslog-conn
-        636,  // LDAP+SSL
-        993,  // IMAP+SSL
-        995,  // POP3+SSL
-        2049, // NFS
-        3659, // apple-sasl / PasswordServer [Apple addition]
-        4045, // lockd
-        6000, // X11
-    };
-    const unsigned short* const blockedPortListEnd = blockedPortList
-        + sizeof(blockedPortList) / sizeof(blockedPortList[0]);
-
-    // If the port is not in the blocked port list, allow it.
-    if (!std::binary_search(blockedPortList, blockedPortListEnd, port))
-        return true;
-
-    // Allow ports 21 and 22 for FTP URLs, as Mozilla does.
-    if ((port == 21 || port == 22) && request.url().protocolIs("ftp"))
-        return true;
-
-    // Allow any port number in a file URL, since the port number is ignored.
-    if (request.url().protocolIs("file"))
-        return true;
-
-    return false;
-}
   
 bool ResourceHandle::shouldContentSniff() const
 {
index c2b0042e8eb41c97f5d412433657c7342520f5e5..da6002540ba4dec0fc62691b4856a65d1ffb3578 100644 (file)
@@ -43,7 +43,6 @@
 #include "Logging.h"
 #include "MessageEvent.h"
 #include "ScriptExecutionContext.h"
-#include "SecurityOrigin.h"
 #include "WebSocketChannel.h"
 #include <wtf/StdLibExtras.h>
 
@@ -126,18 +125,23 @@ void WebSocket::connect(const KURL& url, const String& protocol, ExceptionCode&
     m_protocol = protocol;
 
     if (!m_url.protocolIs("ws") && !m_url.protocolIs("wss")) {
-        LOG_ERROR("Error: wrong url for WebSocket %s", url.string().utf8().data());
+        LOG(Network, "Wrong url scheme for WebSocket %s", url.string().utf8().data());
         m_state = CLOSED;
         ec = SYNTAX_ERR;
         return;
     }
     if (!isValidProtocolString(m_protocol)) {
-        LOG_ERROR("Error: wrong protocol for WebSocket %s", m_protocol.utf8().data());
+        LOG(Network, "Wrong protocol for WebSocket %s", m_protocol.utf8().data());
         m_state = CLOSED;
         ec = SYNTAX_ERR;
         return;
     }
-    // FIXME: if m_url.port() is blocking port, raise SECURITY_ERR.
+    if (!portAllowed(url)) {
+        LOG(Network, "WebSocket port %d blocked", url.port());
+        m_state = CLOSED;
+        ec = SECURITY_ERR;
+        return;
+    }
 
     m_channel = WebSocketChannel::create(scriptExecutionContext(), this, m_url, m_protocol);
     m_channel->connect();