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

Strengthen requirements on CORS-safelisted request-headers #736

Merged
merged 3 commits into from Aug 16, 2018

Conversation

4 participants
@annevk
Member

annevk commented May 25, 2018

This should reduce the attack surface of non-preflighted requests quite a bit.

Fixes #382. Closes #313.


Preview | Diff

@annevk annevk added the needs tests label May 25, 2018

@annevk

This comment has been minimized.

Member

annevk commented May 25, 2018

@johnwilander @mikewest @ckerschb @bzbarsky I'd appreciate your review. @sicking this goes against what you advocated for in #313, but given Safari's success in locking this down and given that there continue to be security bugs in this area due to it not being locked down I think this is the responsible path forward.

</dl>
<li><p>If <var>value</var>'s <a for="byte sequence">length</a> is greater than 128, then return
false.

This comment has been minimized.

@annevk

annevk May 25, 2018

Member

This is one step that Safari does not have, but it would help to have it to make it harder for attackers to determine cookie size and such.

This comment has been minimized.

@yutakahirano

yutakahirano May 25, 2018

Member

Don't we need to think about the total length of all headers, rather than the length of each header?

This comment has been minimized.

@annevk

annevk May 25, 2018

Member

Yeah, if we keep adding headers to this safelist that would indeed become a concern. Otherwise I was thinking that a low enough individual limit would make that unneeded.

@annevk

This comment has been minimized.

Member

annevk commented May 28, 2018

@yutakahirano I looked at all callers of CORS-safelisted request-header and considered an alternative design whereby the input is a header list and the output is a list of headers that are not safelisted. That way we could have an overall size limit and if it is reached we'd just not safelist anything.

However, the one problem with this approach is the Headers object and its "request-no-cors" guard. That fundamentally requires per-header decisions. I don't think we can delay that check and there's no CORS-preflight fetch fallback either.

Perhaps the solution to that is that we freeze "request-no-cors" to Accept, Accept-Language, Content-Language, and Content-Type, with the 128 header value byte limit. That seems good as I don't think we want to continue to expand the API surface of no-cors. And then for the CORS protocol we impose an overall 1024 byte limit (and perhaps have the per-header limit as well?).

@annevk annevk force-pushed the annevk/cors-safelisted-request-header branch 3 times, most recently from 8c2a14f to bcca834 May 30, 2018

@annevk

This comment has been minimized.

Member

annevk commented May 30, 2018

I've pushed some commits that illustrate my above comment. I also filed #748 as a follow-up.

Would love feedback from @yutakahirano @wanderview @chenjj and folks I tagged earlier.

@annevk annevk force-pushed the annevk/cors-safelisted-request-header branch from 121c13d to 6a8e844 May 30, 2018

@annevk annevk added the topic: cors label May 30, 2018

@wanderview

Seems ok to me, but I defer to @ckerschb @bzbarsky here.

@@ -414,38 +414,92 @@ each other by 0x2C 0x20, in order.
<hr>
<p id=simple-header>A <dfn export>CORS-safelisted request-header</dfn> is a <a for=/>header</a>
whose <a for=header>name</a> is a <a>byte-case-insensitive</a> match for one of
<p id=simple-header>To determine whether a <a for=/>header</a> <var>header</var> is a

This comment has been minimized.

@wanderview

wanderview May 31, 2018

Member

Did you intent to produce text that says "header header" with the first header linked?

This comment has been minimized.

@wanderview

wanderview May 31, 2018

Member

Oh, I guess this is just <type> <var name>. Its just confusing to me when the type and variable name are identical.

@annevk

This comment has been minimized.

Member

annevk commented Jun 8, 2018

I wrote tests for this at web-platform-tests/wpt#11432. I'm assuming at this point that Chrome and Firefox are on board with implementing this, but I'll give it some more time before landing all the things.

@annevk annevk force-pushed the annevk/cors-safelisted-request-header branch from 6800438 to 603ca21 Jun 18, 2018

@annevk

This comment has been minimized.

Member

annevk commented Jun 18, 2018

I think this is good now. I'm planning on keeping this as 3 distinct commits since they're somewhat separate features, but all the tests are grouped.

@domfarolino

This comment has been minimized.

Member

domfarolino commented Jun 19, 2018

This looks good to me

@annevk annevk removed the needs tests label Aug 16, 2018

@annevk annevk merged commit e2d62ff into master Aug 16, 2018

2 checks passed

Participation annevk participates on behalf of Mozilla Corporation
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annevk annevk deleted the annevk/cors-safelisted-request-header branch Aug 16, 2018

annevk added a commit to web-platform-tests/wpt that referenced this pull request Aug 16, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 20, 2018

Bug 1467886 [wpt PR 11432] - Fetch: tests for further CORS restrictio…
…ns, a=testonly

Automatic update from web-platform-testsFetch: tests for further CORS restrictions

For whatwg/fetch#736.
--

wpt-commits: a70e655d979df85b59e977b61361c0d6d8bf2bf2
wpt-pr: 11432

xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Aug 20, 2018

Bug 1467886 [wpt PR 11432] - Fetch: tests for further CORS restrictio…
…ns, a=testonly

Automatic update from web-platform-testsFetch: tests for further CORS restrictions

For whatwg/fetch#736.
--

wpt-commits: a70e655d979df85b59e977b61361c0d6d8bf2bf2
wpt-pr: 11432

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Aug 27, 2018

annevk added a commit to web-platform-tests/wpt that referenced this pull request Sep 5, 2018

aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 6, 2018

Strengthen requirements on CORS-safelisted request-headers
With this CL, some request headers that used to be treated as
CORS-safelisted are not CORS-safelisted any more. Specifically,

 - "accept", "accept-language" and "content-language" have a stronger
   check on its value
 - All headers whose value exceeds 128 bytes are treated as not
   CORS-safelisted
 - If the sum of value length of CORS-safelisted headers exceeds 1024,
   then all of them are treated as not CORS-safelisted.

This CL also implements
https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header.

This is for whatwg/fetch#736.

Bug: 824130
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ib12a7dbff6367717a43130ae59304dca55b7bf4e
Reviewed-on: https://chromium-review.googlesource.com/1196563
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589153}

aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 6, 2018

Revert "Strengthen requirements on CORS-safelisted request-headers"
This reverts commit 074455d.

Reason for revert: Suspected of causing check failures across a variety of tests on the Android CFI bot. See https://crbug.com/881538 for more details.

Original change's description:
> Strengthen requirements on CORS-safelisted request-headers
> 
> With this CL, some request headers that used to be treated as
> CORS-safelisted are not CORS-safelisted any more. Specifically,
> 
>  - "accept", "accept-language" and "content-language" have a stronger
>    check on its value
>  - All headers whose value exceeds 128 bytes are treated as not
>    CORS-safelisted
>  - If the sum of value length of CORS-safelisted headers exceeds 1024,
>    then all of them are treated as not CORS-safelisted.
> 
> This CL also implements
> https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header.
> 
> This is for whatwg/fetch#736.
> 
> Bug: 824130
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: Ib12a7dbff6367717a43130ae59304dca55b7bf4e
> Reviewed-on: https://chromium-review.googlesource.com/1196563
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589153}

TBR=toyoshim@chromium.org,yhirano@chromium.org

Change-Id: I9952df291ff0aeaab0b50c6cff3de418b2272e71
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 824130, 881538 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1211958
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589323}

aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 7, 2018

Reland "Strengthen requirements on CORS-safelisted request-headers"
This is a reland of 074455d

Original change's description:
> Strengthen requirements on CORS-safelisted request-headers
>
> With this CL, some request headers that used to be treated as
> CORS-safelisted are not CORS-safelisted any more. Specifically,
>
>  - "accept", "accept-language" and "content-language" have a stronger
>    check on its value
>  - All headers whose value exceeds 128 bytes are treated as not
>    CORS-safelisted
>  - If the sum of value length of CORS-safelisted headers exceeds 1024,
>    then all of them are treated as not CORS-safelisted.
>
> This CL also implements
> https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header.
>
> This is for whatwg/fetch#736.
>
> Bug: 824130
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: Ib12a7dbff6367717a43130ae59304dca55b7bf4e
> Reviewed-on: https://chromium-review.googlesource.com/1196563
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589153}

Bug: 824130
Change-Id: Ia5caad12a51ee44713cf4cf11f42b1fc9ab831a9
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Tbr: toyoshim@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1212425
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589434}

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 10, 2018

Bug 1488133 [wpt PR 12797] - Update cors/simple-requests.htm for extr…
…a "safelisted headers" rules, a=testonly

Automatic update from web-platform-testsFetch: align CORS test with new "safelisted headers" rules

See whatwg/fetch#736 for context.
--

wpt-commits: efa5cebbd4a2b5df055a12212c534b13052154b8
wpt-pr: 12797

jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Sep 10, 2018

Bug 1488133 [wpt PR 12797] - Update cors/simple-requests.htm for extr…
…a "safelisted headers" rules, a=testonly

Automatic update from web-platform-testsFetch: align CORS test with new "safelisted headers" rules

See whatwg/fetch#736 for context.
--

wpt-commits: efa5cebbd4a2b5df055a12212c534b13052154b8
wpt-pr: 12797
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment