Cap lifetime of persistent cookies created client-side through document.cookie

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Wilander]

As pointed out in https://github.com/mikewest/http-state-tokens:

1) Cookies are available to JavaScript by default via document.cookie, which enables a smooth upgrade from one-time XSS to theft of persistent credentials and also makes cookies available to Spectre-like attacks on memory.

2) Though the HttpOnly attribute was introduced well over a decade ago, only ~8.31% of Set-Cookie operations use it today (stats from Chrome). We need developer incentives to put proper protections in place.

3) The median (uncompressed) Cookie request header is 409 bytes, while the 90th percentile is 1,589 bytes, the 95th 2,549 bytes, the 99th 4,601 bytes, and ~0.1% of Cookie headers are over 10kB (stats from Chrome). This is bad for load performance.

In addition to this, third-party scripts running in first-party contexts can read user data through document.cookie and even store cross-site tracking data in them.

Authentication cookies should be HttpOnly and thus not be affected by restrictions to document.cookie. Cookies that persist for a long time should be Secure, HttpOnly, and SameSite to provide good security and privacy.

By capping the lifetime of persistent cookies set through document.cookie we embark on a journey towards better cookie management on the web.

[John Wilander]

<rdar://problem/44741888>

[John Wilander]

Created attachment 350703 [details]
Patch

[John Wilander]

Created attachment 350704 [details]
Patch

[John Wilander]

Moved to RetainPtr<> and adoptNS() based on feedback from David Kilzer.

[Chris Dumez]

Comment on attachment 350703 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350703&action=review

> Source/WebCore/ChangeLog:3
> +        Cap lifetime of persistent cookies created client-side through document.cookie

Should this be behind a setting? Also, I am unclear why we want to do this?

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:269
> +    static NeverDestroyed<NSTimeInterval> secondsPerWeek = 7 * 24 * 60 * 60;

NSTimeInterval is a double so you do not need a NeverDestroyed<>. const NSTimeInterval secondsPerWeek = 7 * 24 * 60 * 60; would do.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:285
> +            NSDate* dateInAWeek = [[[NSDate alloc] initWithTimeIntervalSinceNow:secondsPerWeek] autorelease];

Not super familiar with autorelease? Is this how we like to do things now? I thought we were used RetainPtrs.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:286
> +            if ([dateInAWeek earlierDate:[cookie expiresDate]] == dateInAWeek) {

I assume dateInAWeek will be returned if [cookie expiresDate] returns nil? Might be clearer to check this case explicitly rather than relying on the NSDate behavior?

Also, I think we can avoid constructing dateInAWeek by doing something like:
if (!cookie.expiresDate || cookie.expiresDate.timeIntervalSinceNow > secondsPerWeek)

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:287
> +                NSMutableDictionary<NSHTTPCookiePropertyKey, id>* properties = [[[cookie properties] mutableCopy] autorelease];

ditto.

> Source/WebCore/testing/Internals.cpp:4765
> +Vector<Internals::CookieData> Internals::getCookies() const

FYI, the following also works:
auto Internals::getCookies() const -> Vector<CookieData>

> Source/WebCore/testing/Internals.cpp:4772
> +    if (getRawCookies(*document, document->cookieURL(), cookies)) {

I do not think you need a if check. You can just use the same code with an empty vector.

> Source/WebCore/testing/Internals.cpp:4773
> +        Vector<Internals::CookieData> cookieData;

Vector<CookieData>

> Source/WebCore/testing/Internals.cpp:4775
> +        for (Cookie& cookie : cookies)

auto&

> Source/WebCore/testing/Internals.cpp:4776
> +            cookieData.uncheckedAppend(CookieData(cookie));

cookieData.uncheckedAppend(cookie); should work since your constructor is not explicit.

Alternatively, this would be written as:
auto cookieDatas = WTF::map(cookies, [](auto& cookie) {
    return CookieData { cookie };
});

Seems shorter.

> LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:10
> +    jsTestIsAsync = true;

This test looks synchronous to me?

[Chris Dumez]

Comment on attachment 350704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350704&action=review

> Source/WebCore/ChangeLog:3
> +        Cap lifetime of persistent cookies created client-side through document.cookie

See my comments on previous iteration.

[Chris Dumez]

Comment on attachment 350704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350704&action=review

>> Source/WebCore/ChangeLog:3
>> +        Cap lifetime of persistent cookies created client-side through document.cookie
> 
> See my comments on previous iteration.

Also, why are we putting more restrictions on document.cookie setter than Set-Cookie HTTP response header?

[John Wilander]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 350703 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350703&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Cap lifetime of persistent cookies created client-side through document.cookie
> 
> Should this be behind a setting? Also, I am unclear why we want to do this?

We want the feedback from STP and people living on nightlies/trunk. As for the motivation, see bug description.

> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:269
> > +    static NeverDestroyed<NSTimeInterval> secondsPerWeek = 7 * 24 * 60 * 60;
> 
> NSTimeInterval is a double so you do not need a NeverDestroyed<>. const
> NSTimeInterval secondsPerWeek = 7 * 24 * 60 * 60; would do.

Will fix.

> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:285
> > +            NSDate* dateInAWeek = [[[NSDate alloc] initWithTimeIntervalSinceNow:secondsPerWeek] autorelease];
> 
> Not super familiar with autorelease? Is this how we like to do things now? I
> thought we were used RetainPtrs.

I already switched to that for the patch that's up.

> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:286
> > +            if ([dateInAWeek earlierDate:[cookie expiresDate]] == dateInAWeek) {
> 
> I assume dateInAWeek will be returned if [cookie expiresDate] returns nil?
> Might be clearer to check this case explicitly rather than relying on the
> NSDate behavior?
> 
> Also, I think we can avoid constructing dateInAWeek by doing something like:
> if (!cookie.expiresDate || cookie.expiresDate.timeIntervalSinceNow >
> secondsPerWeek)

Much cleaner. Will fix.

> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:287
> > +                NSMutableDictionary<NSHTTPCookiePropertyKey, id>* properties = [[[cookie properties] mutableCopy] autorelease];
> 
> ditto.

This one is also changed to RetainPtr and adoptNS.

> > Source/WebCore/testing/Internals.cpp:4765
> > +Vector<Internals::CookieData> Internals::getCookies() const
> 
> FYI, the following also works:
> auto Internals::getCookies() const -> Vector<CookieData>

OK. Might as well try it.

> > Source/WebCore/testing/Internals.cpp:4772
> > +    if (getRawCookies(*document, document->cookieURL(), cookies)) {
> 
> I do not think you need a if check. You can just use the same code with an
> empty vector.

OK. Will fix.

> > Source/WebCore/testing/Internals.cpp:4773
> > +        Vector<Internals::CookieData> cookieData;
> 
> Vector<CookieData>

Hmm. It was complaining at some point. Will test.

> > Source/WebCore/testing/Internals.cpp:4775
> > +        for (Cookie& cookie : cookies)
> 
> auto&

Got it.

> > Source/WebCore/testing/Internals.cpp:4776
> > +            cookieData.uncheckedAppend(CookieData(cookie));
> 
> cookieData.uncheckedAppend(cookie); should work since your constructor is
> not explicit.

Got it.

> Alternatively, this would be written as:
> auto cookieDatas = WTF::map(cookies, [](auto& cookie) {
>     return CookieData { cookie };
> });
> 
> Seems shorter.

I actually had that in the back of my mind. That there was some convenience function in WTF. Will fix.

> > LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:10
> > +    jsTestIsAsync = true;
> 
> This test looks synchronous to me?

Yes, but for some reason it hangs without this. Maybe I'm missing something that needs to be called in synchronous cases?

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 350704 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350704&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +        Cap lifetime of persistent cookies created client-side through document.cookie
> > 
> > See my comments on previous iteration.
> 
> Also, why are we putting more restrictions on document.cookie setter than
> Set-Cookie HTTP response header?

Just read your bug description, I guess we can try this. Please put the full description in your patch changelog.

[John Wilander]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 350704 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350704&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +        Cap lifetime of persistent cookies created client-side through document.cookie
> > 
> > See my comments on previous iteration.
> 
> Also, why are we putting more restrictions on document.cookie setter than
> Set-Cookie HTTP response header?

Proper Set-Cookie headers are controlled by the first-party server whereas document.cookie is accessible by third-party scripts running in the first-party context. Therefore the benefit is the highest in restricting document.cookie first.

[John Wilander]

(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #7)
> > Comment on attachment 350704 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=350704&action=review
> > 
> > >> Source/WebCore/ChangeLog:3
> > >> +        Cap lifetime of persistent cookies created client-side through document.cookie
> > > 
> > > See my comments on previous iteration.
> > 
> > Also, why are we putting more restrictions on document.cookie setter than
> > Set-Cookie HTTP response header?
> 
> Just read your bug description, I guess we can try this. Please put the full
> description in your patch changelog.

Will do.

[John Wilander]

Created attachment 350709 [details]
Patch

[Chris Dumez]

Comment on attachment 350709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350709&action=review

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:284
> +        if (![cookie isSessionOnly]) {

Don't we need to update the CFNet implementation as well?

> Source/WebCore/testing/Internals.cpp:4769
> +        return { { } };

Why not return { };

> Source/WebCore/testing/Internals.cpp:4773
> +    auto cookieData = WTF::map(cookies, [](auto& cookie) {

you can return the result of WTF::map() right away. We do not really need this local variable.

> Source/WebCore/testing/Internals.h:765
> +        CookieData(Cookie cookie)

Maybe a blank line before this?

> Source/WebCore/testing/Internals.h:778
> +        CookieData()

Do we really need this? With the return { }; I suggested earlier, this may no longer be needed.

> LayoutTests/ChangeLog:11
> +        1) Cookies are available to JavaScript by default via document.cookie, which

You do not need to repeat the whole changelog in the Layout tests one. This changelog should merely be something like "Added layout test coverage.".

> LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:39
> +    const overTwoDaysInSeconds = twoDaysInSeconds + 10;

Can 10 lead to flakiness on slow bots? Why not 30? (I think this is the default timeout value).

> LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:40
> +    const overOneWeekInSeconds = oneWeekInSeconds + 10;

ditto

> LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:43
> +        if (!cookies)

if (!cookies.length) no?

[John Wilander]

(In reply to Chris Dumez from comment #13)
> Comment on attachment 350709 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350709&action=review
> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:284
> > +        if (![cookie isSessionOnly]) {
> 
> Don't we need to update the CFNet implementation as well?

I don't think so. But I can do that in a follow-up patch if we think it's needed.

> > Source/WebCore/testing/Internals.cpp:4769
> > +        return { { } };
> 
> Why not return { };

Will fix.

> > Source/WebCore/testing/Internals.cpp:4773
> > +    auto cookieData = WTF::map(cookies, [](auto& cookie) {
> 
> you can return the result of WTF::map() right away. We do not really need
> this local variable.

True. Will fix.

> > Source/WebCore/testing/Internals.h:765
> > +        CookieData(Cookie cookie)
> 
> Maybe a blank line before this?

Sure.

> > Source/WebCore/testing/Internals.h:778
> > +        CookieData()
> 
> Do we really need this? With the return { }; I suggested earlier, this may
> no longer be needed.

I tried and it is needed even with the return { }.

> > LayoutTests/ChangeLog:11
> > +        1) Cookies are available to JavaScript by default via document.cookie, which
> 
> You do not need to repeat the whole changelog in the Layout tests one. This
> changelog should merely be something like "Added layout test coverage.".

OK.

> > LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:39
> > +    const overTwoDaysInSeconds = twoDaysInSeconds + 10;
> 
> Can 10 lead to flakiness on slow bots? Why not 30? (I think this is the
> default timeout value).

I'm seeing a test failure on the mac-wk2 bot. Waiting for the results archive. Maybe the shorter time addition is the problem. Will fix either way.

> > LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:40
> > +    const overOneWeekInSeconds = oneWeekInSeconds + 10;
> 
> ditto

Will fix.

> > LayoutTests/http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html:43
> > +        if (!cookies)
> 
> if (!cookies.length) no?

Yup. Will fix.

Thanks for the review! I will figure out the test failure before landing.

[EWS Watchlist]

Comment on attachment 350709 [details]
Patch

Attachment 350709 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9337602

New failing tests:
http/tests/cookies/capped-lifetime-for-cookie-set-in-js.html

[EWS Watchlist]

Created attachment 350718 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[John Wilander]

:( The failure is due to array ordering.

[John Wilander]

Created attachment 350721 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 350721 [details]
Patch for landing

Clearing flags on attachment: 350721

Committed r236448: <https://trac.webkit.org/changeset/236448>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Comment on attachment 350721 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=350721&action=review

> Source/WebCore/testing/Internals.h:754
> +    struct CookieData {

Why didn't we just WebCore::Cookie here?

> LayoutTests/http/tests/cookies/resources/cookie-utilities.js:246
> +    date.setTime(date.getTime() + (maxAgeInSeconds * 1000));

Why are we using kiloseconds?

[Chris Dumez]

Comment on attachment 350721 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=350721&action=review

>> LayoutTests/http/tests/cookies/resources/cookie-utilities.js:246
>> +    date.setTime(date.getTime() + (maxAgeInSeconds * 1000));
> 
> Why are we using kiloseconds?

Do you mean milliseconds? If so, because this is what Date.setTime() expects:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/setTime

[Chris Dumez]

Comment on attachment 350721 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=350721&action=review

>> Source/WebCore/testing/Internals.h:754
>> +    struct CookieData {
> 
> Why didn't we just WebCore::Cookie here?

If we did, we would have to introduce an IDL interface for WebCore::Cookie. This is purely used to help with bindings conversion.

[Alexey Proskuryakov]

Does this patch mean that my search preferences in Bugzilla won't stick any more?

[John Wilander]

(In reply to Alexey Proskuryakov from comment #24)
> Does this patch mean that my search preferences in Bugzilla won't stick any
> more?

I don't know the specifics of how Bugzilla persists search preferences but I have some plans that might make this change more targeted and not affect sites like Bugzilla.