Bug 189933

Summary: Cap lifetime of persistent cookies created client-side through document.cookie
Product: WebKit Reporter: John Wilander <wilander>
Component: WebCore Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, bfulgham, cdumez, commit-queue, ews-watchlist, ggaren, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=190687
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Patch for landing none

John Wilander
Reported 2018-09-24 15:26:50 PDT
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.
Attachments
Patch (15.45 KB, patch)
2018-09-24 15:42 PDT, John Wilander 		no flags
DetailsFormatted DiffDiff
Patch (15.49 KB, patch)
2018-09-24 15:50 PDT, John Wilander 		no flags
DetailsFormatted DiffDiff
Patch (18.18 KB, patch)
2018-09-24 16:38 PDT, John Wilander 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.50 MB, application/zip)
2018-09-24 17:36 PDT, EWS Watchlist 		no flags
Details
Patch for landing (16.89 KB, patch)
2018-09-24 17:46 PDT, John Wilander 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
John Wilander
Comment 1 2018-09-24 15:27:10 PDT
<rdar://problem/44741888>
John Wilander
Comment 2 2018-09-24 15:42:07 PDT
Created attachment 350703 [details] Patch
John Wilander
Comment 3 2018-09-24 15:50:21 PDT
Created attachment 350704 [details] Patch
John Wilander
Comment 4 2018-09-24 15:50:54 PDT
Moved to RetainPtr<> and adoptNS() based on feedback from David Kilzer.
Chris Dumez
Comment 5 2018-09-24 16:10:22 PDT
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 6 2018-09-24 16:10:50 PDT
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 7 2018-09-24 16:14:54 PDT
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
Comment 8 2018-09-24 16:17:36 PDT
(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
Comment 9 2018-09-24 16:19:00 PDT
(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
Comment 10 2018-09-24 16:19:04 PDT
(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
Comment 11 2018-09-24 16:19:22 PDT
(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
Comment 12 2018-09-24 16:38:12 PDT
Created attachment 350709 [details] Patch
Chris Dumez
Comment 13 2018-09-24 16:55:38 PDT
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
Comment 14 2018-09-24 17:28:41 PDT
(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 15 2018-09-24 17:36:48 PDT
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
Comment 16 2018-09-24 17:36:50 PDT
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
Comment 17 2018-09-24 17:38:06 PDT
:( The failure is due to array ordering.
John Wilander
Comment 18 2018-09-24 17:46:43 PDT
Created attachment 350721 [details] Patch for landing
WebKit Commit Bot
Comment 19 2018-09-24 18:13:33 PDT
Comment on attachment 350721 [details] Patch for landing Clearing flags on attachment: 350721 Committed r236448: <https://trac.webkit.org/changeset/236448>
WebKit Commit Bot
Comment 20 2018-09-24 18:13:35 PDT
All reviewed patches have been landed. Closing bug.
Alex Christensen
Comment 21 2018-09-25 11:31:06 PDT
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 22 2018-09-25 12:00:04 PDT
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 23 2018-09-25 12:01:29 PDT
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
Comment 24 2018-09-26 09:27:53 PDT
Does this patch mean that my search preferences in Bugzilla won't stick any more?
John Wilander
Comment 25 2018-10-01 10:27:00 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.