Bug 10957 – HttpOnly Cookie Option

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Robert Sesek]

In the web development arena, HttpOnly cookies have become quite the buzz. It basically disallows JavaScript access to a cookie when it is sent with the HttpOnly flag. Currently only IE supports it, but Firefox is looking into it. I think it'd be very helpful to have (especially since PHP 5.2 will now support it--meaning lots of developers will use it). I've linked to Microsoft's spec on the flag.

[Alexey Proskuryakov]

Mozilla bug: <https://bugzilla.mozilla.org/show_bug.cgi?id=178993>.

[Alexey Proskuryakov]

This is now implemented in Firefox 2 and Opera 9.5.

This looks like a pretty important feature to me, but we need support from underlying system frameworks to implement it. A request to add such support is tracked by <rdar://problem/4154226>.

[Alexey Proskuryakov]

Created attachment 17845 [details]
test case (for http/tests/security)

[David Kilzer (:ddkilzer)]

<rdar://problem/5642992>

[Jim Manico]

Safari is the last browser to lack HttpOnly support.  I for one think it's a critical need to bring Safari's browser security in line with the rest of the industry.

[Jim Manico]

(from Robert)

The problem isn't entirely with WebKit itself, but with the underlying implementation of HTTP. Safari uses CFNetwork on the mac to parse HTTP headers into objects. Unfortunately, this code isn't public, and it's this code that needs to be changed in order for HTTPOnly to be implemented. While preventing DOM/JavaScript access to cookies needs to be done in the WebKit source, until the underlying network implementation is updated to parse the HTTPOnly flag, there's no way this is possible.

So in short, Apple is blocking this bug from being fixed. Once they update CFNetwork, progress on this patch can be made. Until then, no dice. I recommend you file a bug in RadarWeb if you're interested in getting HTTPOnly/CFNetwork done.

[Alexey Proskuryakov]

As mentioned above, there is already a Radar bug filed about this (which was closed as a duplicate of <rdar://problem/4154226>). As always, you are welcome to file another bug, citing your specific use case; please include this original bug number if you decide to file it, to ensure that it is handled right when screening.

[Jim Manico]

(In reply to comment #7)
> As mentioned above, there is already a Radar bug filed about this (which was
> closed as a duplicate of <rdar://problem/4154226>). As always, you are welcome
> to file another bug, citing your specific use case; please include this
> original bug number if you decide to file it, to ensure that it is handled
> right when screening.
> 

Alex, my apologies. I'm not certain how to access rdar://problem/5642992 - is there an Internet facing URL for this problem, or is rdar for Apple employees only?

[Alexey Proskuryakov]

The latter is correct - it's only accessible to Apple folks.

[Jim Manico]

So, a closed source proprietary library is blocking an open source project from adding a key security feature that the rest of the browser industry supports? I smell a rat. It might be at rdar but it sure sounds "off the radar" to me.

[Robert Sesek]

Apple isn't the only company we're waiting on to get HttpOnly support. Qt/Trolltech is also lacking HttpOnly support in QNetworkCookie. I filed bug #205188 with them to get QNetworkCookie::isHttpOnly() added.

[Jim Manico]

Robert, thank you very kindly for taking to time to submit a bug to Qt/Trolltech.

[Robert Sesek]

Here's the public issue for Qt: http://trolltech.com/developer/task-tracker/index_html?id=206125&method=entry

[Jim Manico]

The Java EE Servlet 3.0 spec now includes support for the HttpOnly cookie. This is just more evidence that HttpOnly is becoming a widespread non-MS standard and is worthy of Safari support. 

[Darin Adler]

It should definitely be supported by Safari!

I'm not sure why having a WebKit bug report open for this is helpful, though. It's not something handled by WebKit, and adding this to the CFNetwork library used on Mac OS X won't require any WebKit code changes.

[Jim Manico]

Thank you Darin! I'm grateful that someone at Apple agrees that we need HttpOnly support in Safari. Safari is the last holdout - Opera, IE and FireFox all support HttpOnly!

Will the trolltech issue posted in comment #13 cover this issue? Of is there another place we can post a bug?

[Darin Adler]

(In reply to comment #16)
> Thank you Darin! I'm grateful that someone at Apple agrees that we need
> HttpOnly support in Safari. Safari is the last holdout - Opera, IE and FireFox
> all support HttpOnly!
> 
> Will the trolltech issue posted in comment #13 cover this issue? Of is there
> another place we can post a bug?

It's actually comment #7 that covers the Safari issue. What Alexey said is exactly right.

[Robert Sesek]

(In reply to comment #15)
> I'm not sure why having a WebKit bug report open for this is helpful, though.
> It's not something handled by WebKit, and adding this to the CFNetwork library
> used on Mac OS X won't require any WebKit code changes.

I think a code change to the WebCore/platform still needs to be made. Looking at the various CookieJar classes, it looks like it will be up to WebCore to disregard HttpOnly cookies -- they will still be passed to us from the underlying network layer. So I do think this bug needs to be open; however, maybe the component should be changed to WebCore?

(In reply to comment #16)
> Will the trolltech issue posted in comment #13 cover this issue? Of is there
> another place we can post a bug?

Trolltech fixed this for the upcoming v4.5 of Qt. I'm currently working on compiling Qt and testing a patch I have made for the Qt-backed WebKit.

[Darin Adler]

(In reply to comment #18)
> I think a code change to the WebCore/platform still needs to be made. Looking
> at the various CookieJar classes, it looks like it will be up to WebCore to
> disregard HttpOnly cookies -- they will still be passed to us from the
> underlying network layer.

I see. You're right -- it might turn out that way. Another design would be to have HttpOnly cookies entirely invisible to the old API and add new API that allows clients to see them. I guess we won't know what's required until this gets implemented by the networking layer.

[Robert Sesek]

(In reply to comment #19)
> (In reply to comment #18)
> > I think a code change to the WebCore/platform still needs to be made. Looking
> > at the various CookieJar classes, it looks like it will be up to WebCore to
> > disregard HttpOnly cookies -- they will still be passed to us from the
> > underlying network layer.
> 
> I see. You're right -- it might turn out that way. Another design would be to
> have HttpOnly cookies entirely invisible to the old API and add new API that
> allows clients to see them. I guess we won't know what's required until this
> gets implemented by the networking layer.
> 

Looking at Qt's implementation in the 4.5 snapshot, you get all the cookies and then have to call isHttpOnly() on QNetworkCookie to figure out whether or not you keep it in the CookieJar.

[Alexey Proskuryakov]

Something else that will need to be done at WebCore level is filtering out HttpOnly cookies from XMLHttpRequest getResponseHeader results.

[Alexey Proskuryakov]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=380418

[Jim Manico]

http://www.owasp.org/index.php/HTTPOnly is also a great reference. Complete implementation includes read and write prevention though document.cookie, as well prevention of reading or writing the session cookie via a XMLHTTPRequest.

[Jim Manico]

Let me correct that:

http://www.owasp.org/index.php/HTTPOnly is also a great reference. Complete
implementation includes read and write prevention of HttpOnly cookies though document.cookie, as well as prevention of reading or writing HttpOnly cookies via a XMLHTTPRequest.

[Darin Adler]

(In reply to comment #24)
> http://www.owasp.org/index.php/HTTPOnly is also a great reference. Complete
> implementation includes read and write prevention of HttpOnly cookies though
> document.cookie, as well as prevention of reading or writing HttpOnly cookies
> via a XMLHTTPRequest.

That page doesn't mention prevention of writing HttpOnly cookies vis XMLHttpRequest. Should that really be prevented? Does any browser currently do that?

[Darin Adler]

Created attachment 25220 [details]
patch; someone needs to help me make more regression tests

[Darin Adler]

Loose ends that need to be resolved:

    1) include test cases in the patch
    2) figure out how to compile the CFNetwork version
    3) [optional] add implementations for other platforms that have HTTP-only support in the networking layers (e.g. Qt)

[Darin Adler]

Created attachment 25222 [details]
patch

Windows side is not compiled or tested yet.

[Jim Manico]

Sorry, my mistake.

I was referring to preventing XMLHTTPRequest access to set-cookie and set-cookie2 response headers - for HTTPOnly cookies.

There is no need to prevent write prevention via XMLHTTPRequest response headers. (no such thing) 

[Jim Manico]

Actually, the HttpOnly RFC working group says:

When the server sends a Set-Cookie or Set-Cookie2 header with the HTTPOnly flag set, the client should:

(1) Prevent client-side scripts from reading Cookie or Cookie2 values
(2) Prevent client-side scripts from writing Cookie or Cookie2 values
(3) Prevent client-side scripts from reading Set-Cookie or Set-Cookie2 response headers (via XHR)
(4) Prevent client-side scripts from writing Cookie or Cookie2 request headers (via XHR)

Let me get more info and post back asap.

[Adam Barth]

> Actually, the HttpOnly RFC working group says:

It would be useful to test the behavior of Firefox and Internet Explorer to make sure we have identical semantics.

In some sense, it is impossible to prevent writes because the script could always exhaust the browser's cookie store and then add a new non-HTTPOnly versions of the cookie.

[Jim Manico]

> In some sense, it is impossible to prevent writes because the script could
> always exhaust the browser's cookie store and then add a new non-HTTPOnly
> versions of the cookie.

(From Bill Cory)

That is an interesting problem.  They're referring to the hard limit that governs the number of cookies a site can have stored on the browser -- there's a limit to prevent a malicious site from filling up your hard drive with numerous cookies.  So for most browsers, the limit is 20 cookies max, and when a browser receives cookie #21, it will discard the least used (or oldest?).  This talks a bit about it:

	http://www.cookiecentral.com/faq/#2.5

So if an attacker sets 20 cookies, they can force the HTTPOnly-protected cookie out of the cookie store, then set a non-HTTPOnly cookie.

It would be tempting to change the browser behavior to NOT bump out existing cookies when the cookie store is full, but that would then allow an attacker to populate the store with bogus cookies, and wouldn't allow the legitimate website to set any additional cookies (a cookie DoS attack of sorts).  Of course, the server could just iterate through the cookies and cause the bogus ones to expire, but it's not ideal.

One solution might be that HTTPOnly cookies can't be bumped out of the cookie store.  Another solution might be that the Cookie header sent to the server somehow identifies the cookies that are HTTPOnly, which would allow the server to detect tampering (or two headers, one for regular cookies and one for HTTPOnly cookies).

Or other ideas?

The spec should probably state that the HTTPOnly flag can only be set by the server via a Set-Cookie or Set-Cookie2 header -- scripts are not allowed to set the flag or modify the incoming Set-Cookie/2 header.

[Adam Barth]

> (From Bill Cory)

I really wouldn't be that concerned about it.  Secure cookies have all the same problems and IMHO are more critical to secure.  Playing with the eviction strategy is very fragile and hard to get right.  Altering the Cookie header is unlikely to succeed (there have already been several failed attempts for Secure cookies).  In any case, it's not clear to me what attack you're trying to prevent.

[Alexey Proskuryakov]

Comment on attachment 25222 [details]
patch

> -    // <rdar://problem/5632883> On 10.5, NSHTTPCookieStorage would happily store an empty cookie, which would be sent as "Cookie: =".
> +    // <rdar://problem/5632883> On 10.5, NSHTTPCookieStorage would happily store an empty cookie,

You removed "happily" elsewhere, but not here :-)

> Index: WebCore/platform/network/soup/CookieJarSoup.h
> +#include <wtf/Platform.h>

Why is this include needed? I think that we rely on cpp files to include config.h, which includes Platform.h.

> +    return reinterpret_cast<IsHTTPOnlyFunction>(GetProcAddress(GetModuleHandleA("CFNetwork"), "CFHTTPCookieIsHTTPOnly"));

Just to confirm: weak linking doesn't work with MSVC and/or CFNetwork.dll, correct?

> +    frame->domWindow()->console()->addMessage(JSMessageSource, ErrorMessageLevel, message, 1, String());

I really wish we could get rid of these source-less error messages everywhere, but this patch is obviously not when it should be done.

> +    if (isSetCookieHeader(name) && !document()->securityOrigin()->canLoadLocalResources()) {
> +        reportUnsafeUsage(document(), "Refused to get unsafe header \"" + name + "\"");

I think this could have a comment explaining why we are doing this (which is that we need to filter out HTTPOnly cookies, but that's hard, and Firefox trunk also doesn't try to). Similarly, it may be better to explicitly test for HTTPOnly cookies being hidden from XHR, or at least to explain that some failures of the test are not catastrophic.

r=me, assuming this builds and works on Windows.

[Darin Adler]

(In reply to comment #34)
> You removed "happily" elsewhere, but not here :-)

Will do.

> > Index: WebCore/platform/network/soup/CookieJarSoup.h
> > +#include <wtf/Platform.h>
> 
> Why is this include needed? I think that we rely on cpp files to include
> config.h, which includes Platform.h.

I guess it's not.

> > +    return reinterpret_cast<IsHTTPOnlyFunction>(GetProcAddress(GetModuleHandleA("CFNetwork"), "CFHTTPCookieIsHTTPOnly"));
> 
> Just to confirm: weak linking doesn't work with MSVC and/or CFNetwork.dll,
> correct?

Weak linking might work, but it's more complicated to get that right with project files changes and such, so I'd prefer to do it this way.

> > +    frame->domWindow()->console()->addMessage(JSMessageSource, ErrorMessageLevel, message, 1, String());
> 
> I really wish we could get rid of these source-less error messages everywhere,
> but this patch is obviously not when it should be done.

I'd love to hear more about how to fix that.

> > +    if (isSetCookieHeader(name) && !document()->securityOrigin()->canLoadLocalResources()) {
> > +        reportUnsafeUsage(document(), "Refused to get unsafe header \"" + name + "\"");
> 
> I think this could have a comment explaining why we are doing this (which is
> that we need to filter out HTTPOnly cookies, but that's hard, and Firefox trunk
> also doesn't try to). Similarly, it may be better to explicitly test for
> HTTPOnly cookies being hidden from XHR, or at least to explain that some
> failures of the test are not catastrophic.

I believe that hiding the Set-Cookie header field entirely is a design decision and not just a compromise. I don't think we'd want to undo this later even if someone donated code to filter out the HTTP-only cookies. However, I'm not an expert in this area, so perhaps my belief is wrong.

[Darin Adler]

(In reply to comment #30)
> (1) Prevent client-side scripts from reading Cookie or Cookie2 values
> (2) Prevent client-side scripts from writing Cookie or Cookie2 values

Roughly speaking, I believe these are prevented by the changes my patch makes to the CookieJar implementations for the Foundation and CFNetwork networking layers. Other ports will have to do similar changes.

> (3) Prevent client-side scripts from reading Set-Cookie or Set-Cookie2 response
> headers (via XHR)

This is prevented by code added to XMLHttpRequest in my patch.

> (4) Prevent client-side scripts from writing Cookie or Cookie2 request headers
> (via XHR)

This was already impossible with TOT WebKit because those header field names are not on the white list in isSafeRequestHeader.

[Alexey Proskuryakov]

> I believe that hiding the Set-Cookie header field entirely is a design decision
> and not just a compromise. I don't think we'd want to undo this later even if
> someone donated code to filter out the HTTP-only cookies.

What worries me is that nothing (apart from svn log) will speak about the rationale for this behavior - neither code comments, not tests, really. I do not have an opinion on whether this is a final solution (more likely, it is).

[Darin Adler]

(In reply to comment #37)
> What worries me is that nothing (apart from svn log) will speak about the
> rationale for this behavior - neither code comments, not tests, really. I do
> not have an opinion on whether this is a final solution (more likely, it is).

Oh, sorry, I wasn't clear. I do intend to add a comment based on your remarks!

[Darin Adler]

http://trac.webkit.org/changeset/38566

We can use new separate bugs to track any problems with the implementation. Testing the Mac port and CFNetwork-based Windows port implementation will be limited to people with the new CFNetwork version on their computer.

[Jim Manico]

> In any case, it's not clear to me what attack you're trying to
> prevent.

(From Bil Cory)

Cookie eviction allows an attacker to remove legitimate cookies, it also allows an attacker to replace legitimate cookies with their own -- most useful for session fixation attacks.

If only a server can create HTTPOnly cookies, and HTTPOnly cookies can never be evicted, then it would prevent a cookie eviction attack.  The cookie will still expire at some point, or the server can update the cookie to remove the HTTPOnly flag, and/or set the cookie to immediately expire, so control over the cookie is kept with the server.

[Jim Manico]

Can you please make sure your patch includes protection from both Set-Cookie and Set-Cookie2 per the suggestion of the HTTPOnly working group document, entry A-2? http://docs.google.com/View?docid=dxxqgkd_0cvcqhsdw

-- (WebCore::XMLHttpRequest::getAllResponseHeaders): Hide Set-Cookie headers from
 28 clients that don't have local-resource privileges.
++ (WebCore::XMLHttpRequest::getAllResponseHeaders): Hide Set-Cookie and Set-Cookie2 headers, in a case insensitive way, from 28 clients that don't have local-resource privileges.

[Darin Adler]

(In reply to comment #41)
> Can you please make sure your patch includes protection from both Set-Cookie
> and Set-Cookie2 per the suggestion of the HTTPOnly working group document,
> entry A-2?

Jim, I'm not sure what "make sure" means in your request. The patch does include what you're asking for. The patch is <http://trac.webkit.org/changeset/38566>. A relevant function is:

    static bool isSetCookieHeader(const String& name)
    {
        return equalIgnoringCase(name, "set-cookie") || equalIgnoringCase(name, "set-cookie2"); 
    } 

Is there something specific you're worried about?

[Jim Manico]

> Jim, I'm not sure what "make sure" means in your request.

The code you submitted covers that request. I was looking at what I think are just the function comments, which did not seem to address set-cookie2 

(WebCore::XMLHttpRequest::getAllResponseHeaders): Hide Set-Cookie headers
from 28 clients that don't have local-resource privileges.

Sorry for the hassle.

[Michael Gilbert]

Hello, we are looking at whether we need to adopt these patches in debian stable, which includes webkit 1.0.1 dated 2008-06-15; well before revision 38566.  Looking at the code, it looks like these fixes primarily apply to the windows- and mac-specific cookie handling with some clean-up of the libsoup code (but nothing soup-related specifically fixed).

Note that the version of webkit in stable does not make use of libsoup, so does that mean that it is falling back on the windows-specific cookie handling?  Note also that this version appears to pass the regression test attached.

Also, could libsoup itself be vulnerable to these problems, and if so have there any patches been issued?

Please let me know if you think that we need to adopt these patches.