Bug 32085 – WebSocket should block the same ports that are blocked for resource loading

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

The spec says: "If port is a port to which the user agent is configured to block access, then throw a SECURITY_ERR exception. (User agents typically block access to well-known ports like SMTP.)"

[Alexey Proskuryakov]

Created attachment 44178 [details]
proposed patch

[Alexey Proskuryakov]

Comment on attachment 44178 [details]
proposed patch

Dave Levin reminded me that SecurityOrigin is not in platform/, so this is a layering violation. Thinking...

[Alexey Proskuryakov]

Created attachment 44191 [details]
proposed patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44191 [details] without any errors.

[Darin Adler]

Comment on attachment 44191 [details]
proposed patch

> +        Move isDefaultPortForProtocol() to KURL, because that's a better place for it (SecurityOrigin
> +        is not even in WebCore/platform directory).

I think KURL.h is a good source file for this. But I would slightly prefer a free function to a static member function.

> +    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);
> +    }

Is it safe to use a case-sensitive map for this? Do callers all lowercase the protocol first? Should we assert that the passed in string has no uppercase ASCII letters in it?

> +bool KURL::portAllowed(const KURL& url)

This should be an ordinary member function, not a static member function. Or alternatively it could be a free function.

> +    // If the port is not in the blocked port list, allow it.
> +    if (!std::binary_search(blockedPortList, blockedPortListEnd, port))
> +        return true;

I'd like to see "using namespace std" rather than "std::binary_search".

I'd like to see an assertion that the list is sorted properly, because it seems it would be easy to accidentally add a new port and not keep the list sorted.

I'm going to say review- because I think you should do at least one of the things I suggest above. If you don't agree, feel free to put this same patch up for review again.

[Alexey Proskuryakov]

Created attachment 44255 [details]
updated patch

> Is it safe to use a case-sensitive map for this? Do callers all lowercase the
> protocol first? Should we assert that the passed in string has no uppercase
> ASCII letters in it?

They do, but other protocol-related functions in KURL.h allow non-lowercase input, so for consistency, this one should likely do so, too.

An assertion would be a very weak defense, as it won't fire before the problem actually occurs, which is unlikely to happen in testing.

> I'm going to say review- because I think you should do at least one of the
> things I suggest above.

Most or all of these comments are about moved code, but I'm cool with addressing them, as long as the patch doesn't get rejected for having changes not related to its main purpose :)

[WebKit Review Bot]

Attachment 44255 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/KURL.cpp:637:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1

[Alexey Proskuryakov]

I think that strcmp comparisons should be an exception to the rule, but I can change this when landing if a reviewer tells me to.

[Adam Barth]

style-queue false positive filed: https://bugs.webkit.org/show_bug.cgi?id=32125.  Thanks.

[Darin Adler]

Comment on attachment 44255 [details]
updated patch

>      // 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(strcasecmp(protocol, "javascript") != 0);

I think that it's not good for us to use strcasecmp since, like the <ctype.h> functions, it depends on the POSIX locale.

While I don't think it's urgent to do this, I think we should eventually outlaw strcasecmp in WebKit code with a technique similar to the functions like islower.

On way to rewrite our existing uses of strcasecmp would be to overload the equalIgnoringCase function in PlatformString.h so it can work on two C-style strings. However, this slightly conflicts with a different idea. Dan Bernstein and I discussed making the C-style string arguments to functions like equalIgnoringCase be designed for string literals. So instead of case folding such strings, it would assert they have no non-ASCII characters or uppercase ASCII characters in them, along the lines of what is done in the protocolIs function in KURL.cpp.

Sorry for the long aside -- not really relevant to this bug.

> +    DEFINE_STATIC_LOCAL(DefaultPortsMap, defaultPorts, ());
> +    if (defaultPorts.isEmpty()) {

Sometimes I wish we had a better way to initialize maps than an explicit isEmpty check.

> +bool isDefaultPortForProtocol(unsigned short port, const String& protocol);
> +bool portAllowed(const KURL&); // Blacklist ports that should never be used for Web resources.

Two thoughts about these:

   1) Longer term we might want to move these URL policies *slightly* higher level than then URL class itself. Another source file in the platform directory rather than URL.h itself. Perhaps even in the networking subdirectory of platform.

   2) The second function probably is easier to understand if its sense is reversed, since the concept is a blacklist. The name could be something like hasForbiddenPort or portShouldBeBlocked; maybe even just portBlocked. One other subtle point is that it's not ports that are blocked, it's specific port/protocol combinations, so ideally the very short name would reflect this. Maybe hasForbiddenPortProtocolPair -- well, I'm sure we could do better than that. We might think of some even better name. Since the function is still used in only a small number of places I think it's practical to rename it later, so I'm not to worried about landing it with any of these names.

> -        LOG_ERROR("Error: wrong url for WebSocket %s", url.string().utf8().data());
> +        LOG(Network, "Wrong url scheme for WebSocket %s", url.string().utf8().data());

This change means that you think most people are not interested in the error. LOG_ERROR is used for situations so unusual that anyone using a debug build would want to see a message on the console. Whereas the LOG(Network) variant is for things that someone explicitly wants to turn on. It's normally used for logging that occurs even in non-failure cases. I think that neither LOG_ERROR nor LOG is really what we're after. The people most interested in these types of errors are probably using the console in the web inspector, so the big win is hooking this up to that, something the people working on WebSocket have discussed. Lets make sure we do it.

I think I slightly prefer LOG_ERROR here, though.

r=me

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/51703>. Filed bug 32165 about error logging.