Closed Bug 1424076 Opened 7 years ago Closed 5 years ago

Enable network.http.sendOriginHeader by default

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: francois, Assigned: CuveeHsu)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

(Whiteboard: [domsecurity-backlog3])

Attachments

(4 files)

Bug 1424076 - P1 send Origin headers for all eligible requests
(deleted), text/x-phabricator-request
Details
Bug 1424076 - P2 adjust Origin header comparison for POST request
(deleted), text/x-phabricator-request
Details
Bug 1424076 - P3 Enable testing Origin header in wpt
(deleted), text/x-phabricator-request
Details
Bug 1424076 - P4 Update sendBeacon origin header test
(deleted), text/x-phabricator-request
Details
There is some work to be done before we can enable network.http.sendOriginHeader by default. See https://public.etherpad-mozilla.org/p/bug446344 for the current list.
Blocks: 446344
Whiteboard: [domsecurity-active]
Assignee: francois → nobody
Status: ASSIGNED → NEW
Whiteboard: [domsecurity-active]
Snapshot of the etherpad on 2018-05-03 Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=446344 WIP patch: https://hg.mozilla.org/users/fmarier_mozilla.com/mozilla-unified/rev/origin-header-446344 Implementation Should the Origin header be set in AsyncOpen2(), like the Referer header, instead of BeginConnect()? https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#398 Should redirected requests contain the first URI or the last one as the "referring" origin? this only affects 307/308 since 302s get turned into GET when redirected https://github.com/whatwg/fetch/issues/593 Honour network.http.referer.hideOnionSource (or file follow-up bug). Add the Origin to XHR and fetch() requests (or file follow-up bug). Add the Origin to websocket requests (or file follow-up bug). Testing The Origin header is not subject to the Referrer trimming/spoofing/suppressing pref The Origin header is not subject to the Referrer Policy. The Origin header is controlled by its own pref. XHR should not be able to overwrite the Origin header. https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/dom/xhr/XMLHttpRequestMainThread.cpp#3065 fetch() should not be able to overwrite the Origin header. https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/dom/fetch/InternalHeaders.cpp#260 XHR and fetch() should include the header on POST requests. HTTPS to HTTP loads should get an Origin header with a value of "null". That's the Chrome behavior, but the OWASP wiki says it's not supposed to be like that for some reason. https://www.owasp.org/index.php/CSRF_Prevention_Cheat_Sheet#Checking_the_Origin_Header HTTPS to http://127.0.0.1 shouldgam et the full origin since 127.0.0.1 is considered a potentially secure Origin. IDN domains are shown in the header as punnycode. The Origin header should not interfere with CORS. The Origin header should be sent for cached requests too. Requests from a file:/// URI (i.e. not whitelisted) should get an Origin header with a value of "null". Requests from a data: URI (i.e. not whitelisted) should get an Origin header with a value of "null". Tests wyciwyg:// channels (check with Christoph for details) Look into the web platform tests: fetch/api/basic/request-headers.js fetch/api/cors/cors-redirect.js Specification https://tools.ietf.org/html/rfc6454 https://fetch.spec.whatwg.org/#origin-header https://fetch.spec.whatwg.org/#http-network-or-cache-fetch (Step 11) Clarify the relationship between the Referrer Policy and the Origin header. Clarify that HTTPS to HTTP loads won't include the Origin header. Clarify that file URIs should not be included in the Origin header. When should "null" be sent instead of an empty string (or no header at all)? Should the serialization of the origin in the header be UTF-8 or ASCII? Documentation https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin The header can take a "null" value on Chrome. It's not only sent in POST requests but in all non-GET and non-HEAD requests.
Priority: P2 → P3
Whiteboard: [domsecurity-backlog3]
I assume this is enabled by default on Firefox Nightly? I found another failure: https://github.com/w3c/web-platform-tests/pull/11164. However, that also required a specification change: https://github.com/whatwg/fetch/pull/594. Chrome already passes this. > Should the serialization of the origin in the header be UTF-8 or ASCII? It should be ASCII. Fetch used "utf-8 encode" to get bytes out of a string containing only ASCII code points. I had not defined "isomorphic encode" yet, which makes it clearer it's a much more restricted transform than it might seem when it says UTF-8.
(In reply to Anne (:annevk) from comment #2) > I assume this is enabled by default on Firefox Nightly? It's default OFF everywhere. You need to flip the pref manually if you want to test it out.
I don't get a different result setting it to 1 (you cannot flip this pref seemingly). And FWIW, the reason I assumed it was enabled on Nightly is that my test uses a "no-cors" fetch and Firefox is including the Origin header. That means Firefox already includes it on fetches beyond those required for "cors"...
Blocks: csrf, WBGP, 1272302, 1341689, 1422519
No longer blocks: 446344
Depends on: 446344
(In reply to Anne (:annevk) from comment #4) > I don't get a different result setting it to 1 (you cannot flip this pref > seemingly). And FWIW, the reason I assumed it was enabled on Nightly is that > my test uses a "no-cors" fetch and Firefox is including the Origin header. > That means Firefox already includes it on fetches beyond those required for > "cors"... Fetch [1] and WebSocket [2] sends Origin Headers before Bug 446344 landed [1] https://searchfox.org/mozilla-central/rev/5612e2c75bfde2f5e9512b5b3ffc952ddb74a6dc/dom/fetch/FetchDriver.cpp#303-312 [2] https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/netwerk/protocol/websocket/WebSocketChannel.cpp#2893-2897
Depends on: 1503736
Depends on: 1504085
As a potential anti-CSRF tool the proposed Sec-Metadata header might have better properties than trying to use Origin. Contains more contextual information but less specific about the origin so it resolves some of the issues about privacy or having to spoof/redact info. https://mikewest.github.io/sec-metadata/

Monitoring, just in case.

Keywords: dev-doc-needed

This is a serious web compatibility issue. Should be higher priority.

Plan on next release (68)

Assignee: nobody → juhsu
Priority: P3 → P2
Blocks: 1528800

Hello :ckerschb,
I see we have a test [1] for not sending Origin header for Beacon
Per [2], I guess it's time to flip the test since Origin header is needed here.
OTOH, Chrome sends the Origin header as well.

What do you think?

[1] https://hg.mozilla.org/mozilla-central/diff/670d4c99c1c7310525a5820e3dfd65569b063e7d/dom/tests/mochitest/beacon/test_beaconOriginHeader.html
[2] https://w3c.github.io/beacon/#sec-processing-model

Flags: needinfo?(ckerschb)

We'd like to enable the Origin header for all eligible reqeusts, which include POST.
The test should be adjusted.

Now we send the Origin header!

(In reply to Junior [:junior] from comment #12)

Hello :ckerschb,
I see we have a test [1] for not sending Origin header for Beacon
Per [2], I guess it's time to flip the test since Origin header is needed here.
OTOH, Chrome sends the Origin header as well.

What do you think?

Sorry for the lag here - just looking at your phabricator patch
https://phabricator.services.mozilla.com/D33386

Flags: needinfo?(ckerschb)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)

Sorry for the lag here - just looking at your phabricator patch
https://phabricator.services.mozilla.com/D33386

FWIW, it seems our implementation of Beacon is outdated in a way that we are still applying CORS even though the spec says not to apply CORS. I filed Bug 1557386 to get the Beacon bits updated. The CORS problem with beacon however is orthogonal to what we are trying to accomplish within this bug, so it's not a blocker but I thought it's wort mentioning it here.

Hello Daniel and Anne,
We suffer from web-compat issues for Origin: headers ([1][2] and many votes in bug 446344)
Here's current status:
(a) we pass current web-platform tests AFAIK, and treeherder is green [3]
(b) we're going to merge a clear spec [4] for "Origin header honors ReferrerPolicy" soon, and the gecko implementation is in nightly now (Bug 1504085)
(c) Bug 1535795 needs some attention. It has effect in all non-cors requests. But I don't think it's a blocker.
(d) New wpt for honoring RP has some failures. We file Bug 1560076 which is about the referrer-policy and Bug 1560079 for a POST fetch from about:blank, which is rare.

Based on these analysis, I'd like to enable network.http.sendOriginHeader to 2 by default.
needinfo? to confirm if I don't miss anything.
If it's good to go, I'll ask :ckerschb to sign off the pref change.

Thanks.
[1] https://webcompat.com/issues/19248
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1272302
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e02c5cc3fcfe17e8a6adfcc6f4be3908146322ad
[4] https://github.com/whatwg/fetch/pull/908

Flags: needinfo?(dveditz)
Flags: needinfo?(annevk)

That seems good to me, I'm a little worried that our tests did not hit bug 1535795. We should add tests for that, but that should not need to block this I think.

Flags: needinfo?(annevk)

(In reply to Anne (:annevk) from comment #21)

That seems good to me, I'm a little worried that our tests did not hit bug 1535795. We should add tests for that, but that should not need to block this I think.

We're going to have one test for bug 1535795, although which is one of the corner case.
https://github.com/web-platform-tests/wpt/pull/14260/files#diff-4ac023be6fea526c56e3c384fe32fa98R5

I'm concerned that in bug 1504085 we differ between cors and no-cors requests. Why should cors requests continue to leak the Origin if there's a ReferrerPolicy of noreferrer? Sure, it's hard for the site to decide what to do with a null Origin, but those can come up from redirects so the server has to know how to deal with it anyway. I hope we're adding WPT tests for the spec change for both options (and the various ways of getting a cors request because bug 1535795 shows we have multiple paths).

That issue aside (which is a spec issue we should discuss in WASWG) please do set network.http.sendOriginHeader to 2 by default. Don't need to wait for the referrerpolicy issue resolution: it's an improvement either way.

Flags: needinfo?(dveditz)

We have tests for CORS and Referrer Policy in https://github.com/web-platform-tests/wpt/pull/14260 and we have discussed that issue several times, last time at https://github.com/whatwg/fetch/pull/908. It's an explicit choice from the fetcher to use CORS so it seems okay to "leak" in that case. I'd also expect things to break down if we retroactively started applying this there. I suppose I'm okay with revisiting it some more (and we should take into account WebSocket too then).

All patch are r+.
Soft code freeze for Firefox 69 starts. Let's take the next train.

(In reply to Junior [:junior] from comment #25)

All patch are r+.
Soft code freeze for Firefox 69 starts. Let's take the next train.

Yes, that's a smart decision.

The patch is one month old, so I rebase them and see if treeherder is currently happy.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9e7368aabb66824972fff62bb78d33b3268bd6b

Pushed by juhsu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/743605a9b0ac P1 send Origin headers for all eligible requests r=ckerschb https://hg.mozilla.org/integration/autoland/rev/6127a13715c9 P2 adjust Origin header comparison for POST request r=Honza https://hg.mozilla.org/integration/autoland/rev/f567bf2904c9 P3 Enable testing Origin header in wpt r=Ehsan https://hg.mozilla.org/integration/autoland/rev/c6299be2356c P4 Update sendBeacon origin header test r=ckerschb

Fix. Thanks!

Flags: needinfo?(juhsu)
Pushed by juhsu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/50fbc0cec01a P1 send Origin headers for all eligible requests r=ckerschb https://hg.mozilla.org/integration/autoland/rev/c002ddb976e9 P2 adjust Origin header comparison for POST request r=Honza https://hg.mozilla.org/integration/autoland/rev/2b4487c6403d P3 Enable testing Origin header in wpt r=Ehsan https://hg.mozilla.org/integration/autoland/rev/67239c7df87c P4 Update sendBeacon origin header test r=ckerschb

FWIW I think those are my bad (from bug 1218456).

(In reply to Oana Pop-Rus from comment #32)

Backed out 4 changesets (bug 1424076) for mochitests failures in test_iframe_sandbox_navigation.html

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=67239c7df87c120521c11925f7404ff22e8e89b2

I don't see failure in test_iframe_sandbox_navigation.html with the Push with failure

false alarm?

Flags: needinfo?(opoprus)

Yeah, what happened here was that:

  • My patches in bug 1218456 caused a failure on a bogus test, and thus was backed out.
  • But a later push had landed that relied on that patch to remove a workaround in a test included from test_iframe_navigation, which was not backed out, and thus caused the timeouts.

I've relanded all that stuff, will reland your patches Junior, sorry for the hassle.

Flags: needinfo?(opoprus)

Yes, I'm sorry wrong backout. I will reland in a minute.

Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/51140597ddb7 P1 send Origin headers for all eligible requests r=ckerschb https://hg.mozilla.org/integration/autoland/rev/e85a0177b2f7 P2 adjust Origin header comparison for POST request r=Honza https://hg.mozilla.org/integration/autoland/rev/86406127033a P3 Enable testing Origin header in wpt r=Ehsan https://hg.mozilla.org/integration/autoland/rev/49b44a7f1148 P4 Update sendBeacon origin header test r=ckerschb

Can someone help to check bug 1566295? This patch seems to break evernote web clipboard...

Let's move the discussion to Bug 1566295

Flags: needinfo?(juhsu)
Regressions: 1615901
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: