Bug 34289 – WebSocket ignores HttpOnly cookies, but should use in Handshake.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

Current implementation ignores HttpOnly cookies in WebSocket handshake.
But in practical use case, Web Sockets will be used in an environment where users are authenticated, and that in many cases the Web Socket will be established once the user has logged into a page via HTTP/HTTPS. Assume that a server may track the logged-in-ness of the client using a HttpOnly cookie, and that the server-side logic to check whether a user is already logged in could easily be leveraged for Web Sockets, since it starts as an HTTP connection that includes cookies and is then upgraded.

[Fumitoshi Ukai]

Created attachment 47825 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 47825 [details]
Patch

+# Copyright (C) 2009 Google Inc. All rights reserved.

2010.

+Should say PASS:
+
+WebSocket open
+WebSocket closed
+PASS cookie is "WK-test=1; WK-test-httponly=1"
+PASS successfullyParsed is true

The "should say pass" phrase is slightly misguiding - it's a series of PASS messages, followed by "TEST COMPLETE".

+        "-x", "/websocket/tests/cookies",

It's confusing to have .html files that are processed as CGIs, and it's confusing to have directories that are magically different from others. The run-webkit-tests script has a built-in list of extensions for tests, which includes .pl and .php, among others.

+    shouldBe("cookie", '"WK-test=1; WK-test-httponly=1"');

I'm always nervous when cookie names are reused. We already have WK-test used by xmlhttprequest/cookies.html test, and I don't see how this new test can not make it fail (other than if it's more than a 1000 tests away, and gets a new DRT instance).

Please use unique cookie names, ideally ones that clearly correspond to the test (I should have set a good example of the latter with xmlhttprequest/cookies.html!)

+static String getCookiesForWebSocket(Document *document, KURL& url)

WebSocket code should not attempt to build Cookie headers itself. For example, we should use +[NSHTTPCookie requestHeaderFieldsWithCookies:] on Mac OS X, just like cookies() function in CookieJar does.

This should be trivial to implement by adapting cookie() code, the only minor challenge is coming up with a nice name for the new CookieJar function. I'm thinking of cookieRequestHeaderFieldValue().

+SECURITY WARNING: This uses CGIHTTPServer and CGIHTTPServer is not secure.

I had some difficulty parsing this phrase. And a more detailed explanation of how this is insecure would help - Apache also executes CGIs, but this doesn't automatically make it insecure. 

+It may execute arbitrary Python code or external programs. It should not be
+used outside a firewall.

I think it's actually more common that insecure servers run outside of firewall (more precisely, in "DMZ"). That way, one can't use them as trampoline for bypassing firewall.

[Alexey Proskuryakov]

I don't really buy the explanation that WebSocket "starts as an HTTP connection" - I don't think that the spec says so. But I agree that HttpOnly cookies should be sent.

[Fumitoshi Ukai]

Created attachment 48608 [details]
Patch

[Fumitoshi Ukai]

(In reply to comment #2)
> (From update of attachment 47825 [details])
> +# Copyright (C) 2009 Google Inc. All rights reserved.
> 
> 2010.
> 
> +Should say PASS:
> +
> +WebSocket open
> +WebSocket closed
> +PASS cookie is "WK-test=1; WK-test-httponly=1"
> +PASS successfullyParsed is true
> 
> The "should say pass" phrase is slightly misguiding - it's a series of PASS
> messages, followed by "TEST COMPLETE".
> 
> +        "-x", "/websocket/tests/cookies",
> 
> It's confusing to have .html files that are processed as CGIs, and it's
> confusing to have directories that are magically different from others. The
> run-webkit-tests script has a built-in list of extensions for tests, which
> includes .pl and .php, among others.
> 
> +    shouldBe("cookie", '"WK-test=1; WK-test-httponly=1"');
> 
> I'm always nervous when cookie names are reused. We already have WK-test used
> by xmlhttprequest/cookies.html test, and I don't see how this new test can not
> make it fail (other than if it's more than a 1000 tests away, and gets a new
> DRT instance).

I think this is because these belong to different port.
Anyway, changed them to WK-websocket-test-* to make it clear.

> 
> Please use unique cookie names, ideally ones that clearly correspond to the
> test (I should have set a good example of the latter with
> xmlhttprequest/cookies.html!)
> 
> +static String getCookiesForWebSocket(Document *document, KURL& url)
> 
> WebSocket code should not attempt to build Cookie headers itself. For example,
> we should use +[NSHTTPCookie requestHeaderFieldsWithCookies:] on Mac OS X, just
> like cookies() function in CookieJar does.
> 
> This should be trivial to implement by adapting cookie() code, the only minor
> challenge is coming up with a nice name for the new CookieJar function. I'm
> thinking of cookieRequestHeaderFieldValue().
> 
> +SECURITY WARNING: This uses CGIHTTPServer and CGIHTTPServer is not secure.
> 
> I had some difficulty parsing this phrase. And a more detailed explanation of
> how this is insecure would help - Apache also executes CGIs, but this doesn't
> automatically make it insecure. 
> 
> +It may execute arbitrary Python code or external programs. It should not be
> +used outside a firewall.
> 
> I think it's actually more common that insecure servers run outside of firewall
> (more precisely, in "DMZ"). That way, one can't use them as trampoline for
> bypassing firewall.

I just copied it from python's CGIHTTPServer warning phrase.
I think it's unsecure because original CGIHTTPServer might run any executable outside of document root. 
pywebsocket checks .. should not be used for request path for cgi, so it might be ok.
Do you think we should remove this warning?

[Alexey Proskuryakov]

Comment on attachment 48608 [details]
Patch

     String cookies(const Document*, const KURL&);
+    String cookieRequestHeaderFieldValue(const Document*, const KURL&);

Looking at this, I think that there should be a comment explaining that cookies() omits HttpOnly cookies.

+        "-x", "/websocket/tests/cookies",

Ideally, we should be able to set his to "/websocket/tests". That way, no one will get surprised by trying to add a .pl test to another subdirectory. Of course, pywebsocket would need to learn how to distinguish .html and .pl files.

>I think this is because these belong to different port.

Indeed, I keep forgetting about this!

> Do you think we should remove this warning?

It seems confusing, as we're passing a specific directory for CGIs.

This warning is not necessary for WebKit, since it's fairly clear that a machine running Apache on LayoutTests/http/tests on an external interface is vulnerable to attacks (by default, it only binds to 127.0.0.1 loopback). Websocket tests do not seem to add much to this.

r=me

[Fumitoshi Ukai]

Committed r54707: <http://trac.webkit.org/changeset/54707>

[Fumitoshi Ukai]

(In reply to comment #6)
> (From update of attachment 48608 [details])
>      String cookies(const Document*, const KURL&);
> +    String cookieRequestHeaderFieldValue(const Document*, const KURL&);
> 
> Looking at this, I think that there should be a comment explaining that
> cookies() omits HttpOnly cookies.
> 
> +        "-x", "/websocket/tests/cookies",
> 
> Ideally, we should be able to set his to "/websocket/tests". That way, no one
> will get surprised by trying to add a .pl test to another subdirectory. Of
> course, pywebsocket would need to learn how to distinguish .html and .pl files.

I see.  File another bug. https://bugs.webkit.org/show_bug.cgi?id=34879

> 
> >I think this is because these belong to different port.
> 
> Indeed, I keep forgetting about this!
> 
> > Do you think we should remove this warning?
> 
> It seems confusing, as we're passing a specific directory for CGIs.
> 
> This warning is not necessary for WebKit, since it's fairly clear that a
> machine running Apache on LayoutTests/http/tests on an external interface is
> vulnerable to attacks (by default, it only binds to 127.0.0.1 loopback).
> Websocket tests do not seem to add much to this.
> 
> r=me

[Darin Fisher (:fishd, Google)]

We're going to have a problem supporting cookieRequestHeaderFieldValue in Chromium.  We intentionally deny the WebKit process access to HTTP-only cookies because we can add them in within the browser process later on.

Ukai, maybe we can preserve that isolation by writing a special token into the stream that the WebSocketStreamHandle can replace w/ the real cookie stream?

Also using getRawCookies to implement cookieRequestHeaderFieldValue is not necessarily correct.  getRawCookies fails to mark the cookies as visited, which does not bump their priority in the cookie "cache replacement policy" algorithm.  getRawCookies was only designed to be used by devtools to support its UI.

[Darin Fisher (:fishd, Google)]

ukai:  please note http://trac.webkit.org/changeset/54742, which is a Chromium WebKit API change, but as part of that change I made ChromiumBridge::cookieRequestHeaderFieldValue just call cookies() for now.  We can fix that by implementing WebCookieJar on the Chromium side.  Also, note that pfeldman has a patch that will make getRawCookies fail for any non-inspector process.