1605305 - Origin header field is not set to HTTP request in cases where it is required

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Akihiko Odaki]

Attachment: origin.patch

WHATWG's Fetch Standard requests to set Origin header field with the request's origin or string "null" in most cases. However, Mozilla may fail to set the field, depending on the user preference and the referrer's URI scheme. This leads to cross-site request forgery (CSRF) vulnerabilities in case the Web server utilizes the field for the protection, expecting it is set accordingly to the specification.

The issue can be reproduced with the latest revision, 46336170cd327a6951aa7b386fa712829b3a488c.
https://hg.mozilla.org/mozilla-unified/rev/46336170cd327a6951aa7b386fa712829b3a488c

mozilla::net::nsHttpChannel::SetOriginHeader is expected to perform the procedure specified by the standard, which is stated with a inline comment:

// Step 10 of HTTP-network-or-cache fetch
void nsHttpChannel::SetOriginHeader() {

These lines come from a commit which says the field was implemented for CSRF mitigation.
https://hg.mozilla.org/mozilla-unified/rev/7d2792c6423a8857e7de3ba12c7b3fa39362e338#l5.30

The specification can be accessed via the URL below:
https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch

Mozilla fails to set the field in the following cases:

1. Referrer is unavailable.
2. Referrer's scheme is not allowed to be set to the field. (e.g. data URI scheme)
3. User denied to send the origin value via network.http.sendOriginHeader preference.

In those cases, the specification says to set string "null" to the field. In fact, Mozilla overwrites the field with "null" if it is already set but matches one of these conditions. However, it does not set the field if there is no existing Origin field.

If the user agent conforms to the specification, a Web server can implement CSRF mitigation by assuming:
a. Origin is not set if the request is from the same origin, or the method is no-CORS GET or HEAD.
b. Origin is set to the origin of the request if it is different.
c. Origin is set to "null" if it is not allowed to send the actual origin.

A server can safely perform an operation with side effect if:
i. the method is not GET nor HEAD and
ii. Origin is the same origin or not set.

It is notable that a server can assume it is safe to have side effect when Origin is not set, according to the specification. The behavior is useful when supporting clients which are not Web browsers (so-called "apps") and do not send Origin nor suffer from CSRF. Firefox breaks the assumption.

Sinatra's rack-protection is an example of a popular library for Web servers making such an assumption. The relevant code is below:
https://github.com/sinatra/sinatra/blob/66786bac2b30f7a55f8cd7b2ce992962b2ebe9bf/rack-protection/lib/rack/protection/http_origin.rb#L32

The data URI below is a proof of concept. It makes a POST request to https://example.com/ without Origin header field.
data:text/html;base64,PCFET0NUWVBFIGh0bWw+DQo8Zm9ybSBhY3Rpb249Imh0dHBzOi8vZXhhbXBsZS5jb20vIiBtZXRob2Q9InBvc3QiPjwvZm9ybT4NCjxzY3JpcHQ+DQogIGRvY3VtZW50LmdldEVsZW1lbnRzQnlUYWdOYW1lKCdmb3JtJylbMF0uc3VibWl0KCk7DQo8L3NjcmlwdD4=

I contribute and suggest a fix, origin.patch, attaching to this bug.

I woud not like to receive bounty since I investigated this bug as a part of my employment and my employer does not have a policy for bounty.

Comment 1

[:Gijs (out for now; he/him)]

Kershaw, can you (pass this to someone to) take a look at the description and/or patch? Thanks!

Comment 2

[Kershaw Chang [:kershaw]]

Comment on attachment 9117176 [details] [diff] [review]
origin.patch

I think Christoph is the right reviewer for this patch. Unfortunately, Christoph is on holiday right now.
I'll keep this ni and redirect this to him when he is back.

Comment 3

[Kershaw Chang [:kershaw]]

It seems that Christoph's queue is quite long. I'll ask Thomas to take a look.

Comment 4

[Thomas Nguyen (:tnguyen)]

Comment on attachment 9117176 [details] [diff] [review]
origin.patch

Review of attachment 9117176 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. Just a quick look and this breaks the rule that origin header should follow Referrer Policy (in no-referrer and same-origin).
For example, if referrer policy is no-referrer and origin should be null

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +9838,5 @@
> +        nsContentUtils::GetASCIIOrigin(referrer, origin);
> +      }
> +    } else if (sSendOriginHeader != 0) {
> +      nsContentUtils::GetASCIIOrigin(referrer, origin);
> +    }

We are removing the relation between referrer policy and origin header and I am pretty sure it will break mochitest and wpt test

Comment 5

[Akihiko Odaki]

Attachment: origin.patch

It takes ReferrerInfo::ShouldSetNullOriginHeader(this, referrer) into account, respecting referrer policy.

Comment 6

[Thomas Nguyen (:tnguyen)]

Comment on attachment 9120683 [details] [diff] [review]
origin.patch

Review of attachment 9120683 [details] [diff] [review]:
-----------------------------------------------------------------

Could you please add some tests about that? And please fix the existed test failures with your patch.
and it would be better if you add some comments point out the specs we are following (I could see step 10 of http-network-or-cache-fetch could not explain why we should choose "null", maybe "serialisation-of-an-origin"?)

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:nhi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Honza Bambas (:mayhemer)]

(this bug was hidden from out triage because of a long outstanding needinfo request)

Comment 9

[Daniel Veditz [:dveditz]]

Looks like the Web Platform Tests don't match the spec (or more likely are incomplete) since we get a good score on Origin support:
https://wpt.fyi/results/fetch/origin?label=master&label=experimental&aligned&q=origin

This bug doesn't need to be hidden because it's not a direct attack on Firefox. It also doesn't increase vulnerability for a secure site, it just doesn't give them a modern tool in some cases. If there's no Origin: header then they still need to do something secure because that's what an old browser will give them. I suspect this is a dupe because better Origin support keeps showing up on the seceng roadmap. In any case we know we don't present an Origin header in as many situations as Chrome does. The spec changed and we're still implementing an older version.

Comment 10

[Akihiko Odaki]

Attachment: origin.patch [Original patch for reference]

Fixes existing tests related to Origin header field.

Comment 11

[Akihiko Odaki]

(In reply to Thomas Nguyen (:tnguyen) from comment #6)

Comment on attachment 9120683 [details] [diff] [review]
origin.patch

Review of attachment 9120683 [details] [diff] [review]:

Could you please add some tests about that? And please fix the existed test
failures with your patch.
and it would be better if you add some comments point out the specs we are
following (I could see step 10 of http-network-or-cache-fetch could not
explain why we should choose "null", maybe "serialisation-of-an-origin"?)

According to the spec, "append a request Origin header", which is permed in step 10, always sets the header field unless request's method is GET or HEAD. Therefore it is violating the spec to leave the header field unset.
Now the question is why it should be "null". "append a request Origin header sets "the result of byte-serializing a request origin with request" or null. "The result of byte-serializing a request origin with request" can also be "null" or origin serialized in "scheme://host:port" syntax. In this way you have two possible values of the header field: "null" and serialization in "scheme://host:port" syntax.

Comment 12

[:Gijs (out for now; he/him)]

Looks like there's no review flags set here and sadly Thomas is no longer with Mozilla; Honza, can you help make sure the patch here does not fall through the cracks? Thanks.

Comment 13

[Tom Ritter [:tjr]]

bounty- per

I would not like to receive bounty since I investigated this bug as a part of my employment and my employer does not have a policy for bounty.

Comment 14

[Honza Bambas (:mayhemer)]

Working on this now.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: Bug 1605305 - Origin header field is not set to HTTP request in cases where it is required, r=ckerschb

Comment 16

[Honza Bambas (:mayhemer)]

Fails on try with:

TEST-UNEXPECTED-FAIL | browser/components/search/test/browser/browser_contextmenu.js | Ensure there were no errors loading the search page - false == true - got false, expected true (operator ==) 
TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_webRequest_filterResponseData.js | xpcshell return code: -11

Comment 17

[Junior [inactive]]

FWIW I file bug 1649848 for unstable test_ext_webRequest_filterResponseData

Comment 18

[Honza Bambas (:mayhemer)]

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

FWIW I file bug 1649848 for unstable test_ext_webRequest_filterResponseData

Thanks for referring this, Junior! I'll rebase the patch to the latest m-c and see how this test is going to behave. I have a simple fix for the browser_contextmenu.js test already.

Comment 19

[Junior [inactive]]

The return code: -11 might be bug 1647978, which is in review.
I don't think it's related to Origin header

Comment 20

[Honza Bambas (:mayhemer)]

Thanks.

Looks like the latest version perma fails:

TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_referrerDefaultPolicy.js | The correct referrer must be read from DOM - Got http://example.net/browser/toolkit/components/antitracking/test/browser/page.html, expected http://example.net/

which is a rare intermittent bug 1530943.

Comment 21

[Honza Bambas (:mayhemer)]

basti agreed to take over this, thanks, basti!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1047cef8519b
Origin header field is not set to HTTP request in cases where it is required, r=ckerschb,necko-reviewers,JuniorHsu

Comment 23

[Razvan Maries]

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=XVo6GHq7TzqywPP7iGBtNg.0&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=os%2Cx%2C10.14%2Cdebug%2Cxpcshell%2Ctests%2Ctest-macosx1014-64%2Fdebug-xpcshell-e10s%2Cx1&revision=1047cef8519b32384eb290c813ca436f1f78f0cd

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313329576&repo=autoland&lineNumber=5379

Backout: https://hg.mozilla.org/integration/autoland/rev/a752892ed85b12d643f0dc2ec207483ef51c6f09

Comment 24

[Sebastian Streich [:sstreich]]

Updated the patch, test should not fail now :)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cad986c81a5
Origin header field is not set to HTTP request in cases where it is required, r=ckerschb,necko-reviewers,JuniorHsu

Comment 26

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_trr.js

backout: https://hg.mozilla.org/integration/autoland/rev/d96e9f03428da7d6dc1844d23ae853c65fa3de52

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cxpcshell%2Ctests%2Cwith%2Cnetworking%2Con%2Csocket%2Cprocess%2Ctest-linux1804-64%2Fdebug-xpcshell-spi-nw-e10s%2Cx2&revision=7cad986c81a593e313c359c2cd10d27e0f4fd630&selectedTaskRun=DNlefoknSI6DkGzzHkdsvA.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=313354816&repo=autoland&lineNumber=4017

[task 2020-08-18T20:31:22.405Z] 20:31:22 INFO - TEST-START | netwerk/test/unit/test_trr.js
[task 2020-08-18T20:36:22.401Z] 20:36:22 WARNING - TEST-UNEXPECTED-TIMEOUT | netwerk/test/unit/test_trr.js | Test timed out
[task 2020-08-18T20:36:22.402Z] 20:36:22 INFO - TEST-INFO took 300000ms
[task 2020-08-18T20:36:22.402Z] 20:36:22 INFO - >>>>>>>
[task 2020-08-18T20:36:22.402Z] 20:36:22 INFO - PID 31974 | [31974, Unnamed thread 7fd87e56e040] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp, line 202
[task 2020-08-18T20:36:22.403Z] 20:36:22 INFO - PID 31974 | [31974, Unnamed thread 7fd87e56e040] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/checkouts/gecko/xpcom/base/nsTraceRefcnt.cpp, line 202
[task 2020-08-18T20:36:22.403Z] 20:36:22 INFO - PID 31974 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2020-08-18T20:36:22.404Z] 20:36:22 INFO - PID 31974 | [31974, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp, line 2914
[task 2020-08-18T20:36:22.404Z] 20:36:22 INFO - PID 31974 | [31974, Main Thread] WARNING: Couldn't get the user appdata directory, crash dumps will go in an unusual location: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp, line 2973
[task 2020-08-18T20:36:22.404Z] 20:36:22 INFO - PID 31974 | start!
[task 2020-08-18T20:36:22.404Z] 20:36:22 INFO - TEST-PASS | netwerk/test/unit/test_trr.js | setup - [setup : 37] "36064" != null

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f12c895fc5e7
Origin header field is not set to HTTP request in cases where it is required, r=ckerschb,necko-reviewers,JuniorHsu

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f12c895fc5e7

Comment 29

[Kevin Locke]

For reference, I just found this bug via mozregression:

Last good revision: d7c68a3161242a0442d01f114ca9eebd9e200f8a
First bad revision: f12c895fc5e764414175efe737f8b2f256ea86a7
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d7c68a3161242a0442d01f114ca9eebd9e200f8a&tochange=f12c895fc5e764414175efe737f8b2f256ea86a7

It appears to have broken DuckDuckGo HTML (i.e. non-JavaScript) search from the browser. To reproduce:

1. Visit https://html.duckduckgo.com
2. "Add Search Engine" from page actions menu.
3. Search for anything using the newly added "DuckDuckGo HTML".
4. The response is text/plain "forbidden" served with code 403.

To confirm the Origin header is the issue:

$ curl -isS -H 'Origin: null' -d q=test 'https://html.duckduckgo.com/html/' | head -n1
HTTP/2 403

$ curl -isS -d q=test 'https://html.duckduckgo.com/html/' | head -n1
HTTP/2 200

Unless anyone has a better communication channel, I'll raise the issue on https://reddit.com/r/duckduckgo so the DuckDuckGo developers are aware.

Comment 30

[Kevin Locke]

Possibly related to Bug 1615901 which disabled sending Origin for OpenSearch POST requests (also DuckDuckGo-related).

Comment 31

[:Gijs (out for now; he/him)]

(In reply to Kevin Locke from comment #29)

It appears to have broken DuckDuckGo HTML (i.e. non-JavaScript) search from the browser. To reproduce:

1. Visit https://html.duckduckgo.com
2. "Add Search Engine" from page actions menu.
3. Search for anything using the newly added "DuckDuckGo HTML".
4. The response is text/plain "forbidden" served with code 403.

Please file a new bug. If it turns out to be a duckduckgo issue we can move it to our web compat components and contact them about it. Either way we should avoid shipping this regression to beta/release users if we can avoid it.

Comment 32

[Kevin Locke]

(In reply to :Gijs (he/him) from comment #31)

Please file a new bug.

Sure thing. Filed as Bug 1667214.

Comment 33

[Natalia Csoregi [:nataliaCs]]

Backed out for causing bustage on nsHttpChannel.cpp on mozilla-central

backout: https://hg.mozilla.org/integration/autoland/rev/27a003fc545c71e48b3695227e3560044782614d

m-c push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedTaskRun=PzvQPuAKROWYZkCKSd4g5Q.1&revision=4846ccf88574b5b66de5d5a8f40edfd3f3390b8b&searchStr=linux%2Cx64%2Copt%2Cbuild-linux64-base-toolchains%2Fopt%2Cbb

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76c1d9cd91e36b2a4b658a0e55b8dce95e0ea084&group_state=expanded

Comment 34

[Natalia Csoregi [:nataliaCs]]

Merged to mozilla-central: https://hg.mozilla.org/mozilla-central/rev/27a003fc545c71e48b3695227e3560044782614d

Comment 35

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:sstreich, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92d71744323a
Origin header field is not set to HTTP request in cases where it is required, r=ckerschb,necko-reviewers,JuniorHsu,valentin

Comment 37

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/92d71744323a

Comment 38

[Tom S [:evilpie]]

Patch was backed out in bug 1685570.

Comment 39

[Dragana Damjanovic [:dragana]]

Sebastian, can you take a look at this?

there is also this bug 1649888.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Hey Greg, Basti moved to a different team and I don't think he is going to finish up the work here. Nevertheless it would be nice if we would have correct origin header support. I am clearing the priority as well so it shows up your next triage meeting. Most likely it's not just this bug, but also bugs in the dependency chain that would need to be evaluated/addressed/fixed.

Comment 42

[Tom Schuster]

I rebased the patch and I can't actually reproduce the DuckDuckGo problem described in bug 1667214. We don't try to set the origin in SetOriginHeader because the triggering principal is a SystemPrincipal.

I think we might be able to work around the extension issue described in bug 1685570 by also not modifying requests triggered by extensions.

Comment 43

[Tom Schuster]

Attachment: Bug 1605305 - Consistently provide an Origin header for normal requests. r?freddyb,kershaw

This patch is more conservative for requests initiated by add-on and prefers
to send no Origin header instead of Origin: null.

Comment 44

[Tom Schuster]

Attachment: Bug 1605305 - WebExtension test to make sure there is no Origin with POST. r?robwu

I think this is only relevant for Manifest v2 at the moment.

Depends on D147091

Comment 45

[Phabricator Automation]

Comment on attachment 9278139 [details]
Bug 1605305 - WebExtension test to make sure there is no Origin with POST. r?robwu

Revision D147296 was moved to bug 1772485. Setting attachment 9278139 [details] to obsolete.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28cf8d7e9723
Consistently provide an Origin header for normal requests. r=freddyb,necko-reviewers,kershaw,robwu

Comment 47

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/28cf8d7e9723

Comment 48

[Mathew Hodson]

Can you fix the milestone on this bug?