Bug 169356 – [SOUP] Implement strict secure cookies specification

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Insecure webpages should be prevented from setting secure cookies. See:

https://bugzilla.mozilla.org/show_bug.cgi?id=976073

[Patrick Griffis]

This appears to already be implemented if I'm reading it right: https://github.com/WebKit/webkit/commit/ca55797f9037f4655f6f673b3ebb91b904899a5c#diff-0260b688ce1742d2664124764a517056R503

I don't know if there are any tests we have for this to ensure they run with Soup.

[Patrick Griffis]

Hmm, I guess that snippet is only accessing cookies not setting them.

[Michael Catanzaro]

Yeah. Basically this eature boils down to an if statement somewhere to make sure it's impossible for an http:// connection to set secure cookies. Just block the cookie if so. Taking a quick look at the code, I guess that needs handled inside NetworkStorageSession::setCookies and NetworkStorageSession::setCookiesFromDOM, then audit all calls to NetworkStorageSession::setCookie....

Then there's a change to ensure existing secure cookies can't be replaced with insecure cookies, and a bit about expiration policy: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01. All that needs to be tested.

Writing tests will probably be harder than implementing the change itself. I think they need to go under LayoutTests/http/tests/ssl as those are the only tests that run under an HTTPS server. You could add a directory LayoutTests/http/tests/ssl/cookies. Alternatively, instead of writing your own, it might be better to try importing the relevant WPT tests like https://github.com/web-platform-tests/wpt/blob/master/cookies/secure/set-from-http.sub.html and its friends, although I've never done this before and I'm not certain whether all these tests are actually compatible with our infrastructure....

[Patrick Griffis]

Initial work in libsoup: https://gitlab.gnome.org/GNOME/libsoup/merge_requests/47

[Patrick Griffis]

Created attachment 371127 [details]
Patch

[Michael Catanzaro]

Tests?

[EWS Watchlist]

Comment on attachment 371127 [details]
Patch

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

New failing tests:
http/wpt/service-workers/service-worker-networkprocess-crash.html

[EWS Watchlist]

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

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

[Patrick Griffis]

This is covered by tests from WPT, considering how simple this is would that be acceptable or should I import them over?

[Michael Catanzaro]

They should be imported under LayoutTests/imported/w3c/web-platform-tests.

[Carlos Alberto Lopez Perez]

BTW, don't import the test by manually copying them. Instead use the importer script that does the required rewrites of resources. Something like this should work:

$ Tools/Scripts/import-w3c-tests -t  web-platform-tests/cookies/samesite

See also: https://trac.webkit.org/wiki/WebKitW3CTesting

[Carlos Alberto Lopez Perez]

I also find this easier for everybody doing the process of importing tests and implementing a feature in two steps (two different bugs):

1. First import the WPT tests in another bug and mark the new imported tests as failing on the TestExpectation.

2. Then land the bug with the implementation and remove the failures from the TestExpectation file that now pass and/or adjusting test expectation results.


See for example r251163 and r251182

[Patrick Griffis]

Created attachment 385472 [details]
Patch

[Michael Catanzaro]

Comment on attachment 385472 [details]
Patch

Nice.

[WebKit Commit Bot]

Comment on attachment 385472 [details]
Patch

Clearing flags on attachment: 385472

Committed r253431: <https://trac.webkit.org/changeset/253431>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.