Open Bug 1605305 Opened 2 years ago Updated 6 days ago

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

Categories

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

Product:
Core
Component:
DOM: Networking
Type:
enhancement

Tracking

()

Status:
REOPENED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
firefox85 --- fixed
firefox88 --- wontfix

People

(Reporter: nekomanma, Assigned: sstreich, NeedInfo)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [reporter-external] [client-bounty-form] [verif?] [necko-triaged])

Attachments

(2 files, 2 obsolete files)

origin.patch [Original patch for reference]
18.45 KB, patch
Details | Diff | Splinter Review
Bug 1605305 - Origin header field is not set to HTTP request in cases where it is required, r=ckerschb
47 bytes, text/x-phabricator-request
Details | Review
Attached patch origin.patch (obsolete) — — Splinter Review

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.

Flags: sec-bounty?

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

Group: firefox-core-security → dom-core-security
Type: task → defect
Component: Security → DOM: Networking
Flags: needinfo?(kershaw)
Product: Firefox → Core
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.

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

Flags: needinfo?(kershaw) → needinfo?(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
Attachment #9117176 - Flags: review-
Attached patch origin.patch (obsolete) — — Splinter Review

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

Attachment #9117176 - Attachment is obsolete: true
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"?)
Flags: needinfo?(tnguyen)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(nhnguyen)
Flags: needinfo?(nhnguyen) → needinfo?(honzab.moz)
Assignee: nobody → nekomanma
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(honzab.moz)
Priority: -- → P2
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?] [necko-triaged]

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

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.

Group: dom-core-security
Type: defect → enhancement

Fixes existing tests related to Origin header field.

Attachment #9120683 - Attachment is obsolete: true

(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.

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.

Flags: needinfo?(honzab.moz)

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.

Flags: sec-bounty? → sec-bounty-

Working on this now.

Assignee: nekomanma → honzab.moz
Flags: needinfo?(honzab.moz)
Attachment #9121788 - Attachment description: origin.patch → origin.patch [Original patch for reference]

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

FWIW I file bug 1649848 for unstable test_ext_webRequest_filterResponseData

(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.

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

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.

Blocks: 1649888

basti agreed to take over this, thanks, basti!

Assignee: honzab.moz → sstreich
Pushed by rmaries@mozilla.com:
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

Updated the patch, test should not fail now :)

Flags: needinfo?(sstreich)
Pushed by rmaries@mozilla.com:
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

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

Flags: needinfo?(sstreich)
Pushed by btara@mozilla.com:
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

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

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
status-firefox83: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch

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.

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

(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.

Flags: needinfo?(kevin)

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

Please file a new bug.

Sure thing. Filed as Bug 1667214.

Flags: needinfo?(kevin)
Regressions: 1667214
Status: RESOLVED → REOPENED
status-firefox83: fixed → ---
Resolution: FIXED → ---
Target Milestone: 83 Branch → ---

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.

Flags: needinfo?(sstreich)
Pushed by btara@mozilla.com:
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

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

Status: REOPENED → RESOLVED
Closed: 8 months ago → 6 months ago
status-firefox85: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
Flags: needinfo?(sstreich)
Regressions: 1685570
Blocks: 1692241
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Patch was backed out in bug 1685570.

Sebastian, can you take a look at this?

there is also this bug 1649888.

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