Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict <meta http-equiv=set-cookie> #3011

Closed
wants to merge 1 commit into from
Closed

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 6, 2017

Tests: ..?

Fixes #1950.

@annevk
Copy link
Member Author

annevk commented Sep 6, 2017

@mikewest how would I add the CSP inline script protection? Something like this perhaps:

  1. If the Should element's inline behavior be blocked by Content Security Policy? algorithm returns "Blocked" when executed upon the meta element, "script", and the meta element's content attribute value, then return.

@annevk
Copy link
Member Author

annevk commented Sep 6, 2017

I suspect @bsittler's suggestion was to add a check for https://html.spec.whatwg.org/#concept-n-script as well. I don't think it's expected that if you disable scripting you also disable set-cookie, although I suppose this is a bit of a mixed bag.

@bsittler
Copy link

bsittler commented Sep 6, 2017

Do CSP and meta http-equiv=set-cookie ever actually co-occur in the wild outside of test suites? If not, perhaps presence of CSP could prevent the element's cookie-setting behavior entirely

@bsittler
Copy link

bsittler commented Sep 6, 2017

(I meant: mere presence of the content-security-policy header or meta element; however that would still, in the meta-supplied-csp case, give document authors a chance to set cookies this way prior to the CSP-supplying element)

@annevk
Copy link
Member Author

annevk commented Sep 6, 2017

I'm not sure, but https://www.chromestatus.com/metrics/feature/timeline/popularity/1549 is nonzero and that is about a subset.

We should probably also consider sandboxing restricting this as you suggested, though I'm not sure if I want to tightly couple all those changes.

@mikewest
Copy link
Member

I would enjoy getting rid of <meta http-equiv='set-cookie' ...> entirely. However, usage is hovering around 0.02%, which is higher than I'd like to see. I get ~298 hits on that in HTTP Archive's 2017-07-15 run, with no real pattern to the few I spot-checked. shrug

I think it's conceptually tied to script-src, since it seems to me that setting a cookie from a page should require script access, but I'm not sure the impact is worth the churn. Perhaps @dveditz or @johnwilander has an opinion from Firefox/Safari respectively?

@annevk
Copy link
Member Author

annevk commented Sep 14, 2017

It sounds like we should maybe make the change I proposed here first and then file a follow-up if anyone is interested in further restrictions.

@mikewest
Copy link
Member

SGTM! :)

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just waiting on tests.

@domenic domenic added the needs tests Moving the issue forward requires someone to write tests label Sep 21, 2017
@annevk
Copy link
Member Author

annevk commented Sep 21, 2017

FWIW, I looked into tests a bit, but there appear to be none for this feature and existing cookie tests weren't super clear either...

I wonder if @bsittler has looked into improving that for his cookie work?

@dveditz
Copy link
Member

dveditz commented Sep 21, 2017

@mikewest could you get a sense whether the documents you saw using this were "old" or created more recently? Was it on documents that were trying to be "no-scripty", or do you think it was just folks who didn't have the control of Set-Cookie headers on their server?

I'd be fine killing this in script-disabled contexts, or treating it like inline-script for CSP purposes.

If you want to kill it more generally (which I'm OK with) perhaps it should be part of a re-examination of the entire mechanism. Does it really make sense to allow content to set arbitrary headers? Maybe we should enumerate the ones we have to support for historical reasons and disallow the rest.

@annevk
Copy link
Member Author

annevk commented Sep 21, 2017

@dveditz if by content you mean HTML, <meta http-equiv> is already restricted to a limited list in the standard. set-cookie just happens to be one of the allowed values.

@mikewest
Copy link
Member

@dveditz: The usage graph is spikey https://www.chromestatus.com/metrics/feature/timeline/popularity/1547. I suspect that's because Cloudflare uses it on their gateway error page (e.g. http://www.xuebang.com.cn/ contains <meta http-equiv="set-cookie" content="cf_use_ob=0; expires=Fri, 22-Sep-17 08:57:01 GMT; path=/"> (note: That might be a porn site if/when it ever starts loading, so, you know, browse carefully)). Poking them about it again in https://twitter.com/mikewest/status/911151559173689344.

@mikewest
Copy link
Member

mikewest commented Sep 22, 2017

Spot-checking HTTP Archive, I'm going to try to deprecate this in Chrome. Cloudflare seems to account for a substantial amount of usage, and they seem amenable to dropping it from their error pages. If they drop it, and the usage numbers go down accordingly, I'll try for removal.

Intent to Deprecate at https://groups.google.com/a/chromium.org/d/msg/blink-dev/0sJ8GUJO0Dw/iMmcXLIGBAAJ

@mikewest
Copy link
Member

@annevk: Should we repurpose #1950 for general deprecation/removal of http-equiv="set-cookie"? Or would you prefer filing a new issue? In addition to Dan's support above, Edge has expressed support for general removal in https://twitter.com/patrickkettner/status/911282308337983489.

@annevk
Copy link
Member Author

annevk commented Sep 27, 2017

I filed #3076 for you, seems a little cleaner.

mikewest added a commit to web-platform-tests/wpt that referenced this pull request Sep 27, 2017
`<meta http-equiv="set-cookie" ...>` is an attack vector, and doesn't
seem to be used widely enough to justify keeping it in the platform.
This patch contains a tentative test for removing that functionality,
as discussed in whatwg/html#3076.

Chrome has approved an [intent to deprecate][1], and [Edge][2] and
[Firefox][3] have both expressed support.

[1]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/0sJ8GUJO0Dw/iMmcXLIGBAAJ
[2]: https://twitter.com/patrickkettner/status/911282308337983489
[3]: whatwg/html#3011 (comment)
@bsittler
Copy link

@annevk i do have some coverage for <meta http-equiv="set-cookie" ... > in pending https://crrev.com/c/594890 as part of the async cookies API effort; i just uploaded a snapshot that reverses the expectations (i.e. expects the meta element to have no effect) as that behavior is now default in chromium's WPT runner

@bsittler
Copy link

bsittler commented Dec 6, 2017

There is coverage of this from the point of view of the still-tentative "async cookies API" in web-platform-tests/wpt@cd3a926 - I intend to create parallel cases not relying on that API but have not yet done so. /cc @pwnall

Actually it looks to me like @mikewest already added the needed test: https://github.com/w3c/web-platform-tests/blob/master/cookies/meta-blocked.html

annevk added a commit that referenced this pull request Apr 27, 2018
Tests: ...

Closes #1950, closes #3011, and fixes #3076.
@annevk annevk closed this in #3649 Apr 27, 2018
annevk added a commit that referenced this pull request Apr 27, 2018
Tests: cookies/meta-blocked.html in web-platform-tests.

Closes #1950, closes #3011, and fixes #3076.
@annevk annevk deleted the annevk/meta-set-cookie branch April 27, 2018 16:47
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
Tests: cookies/meta-blocked.html in web-platform-tests.

Closes whatwg#1950, closes whatwg#3011, and fixes whatwg#3076.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests topic: cookie
Development

Successfully merging this pull request may close these issues.

None yet

5 participants