Strip referrer and origin headers in xorigin requests from a .onion #10760

fmarier · 2021-10-29T03:32:03Z

Resolves brave/brave-browser#18071

Security report: https://hackerone.com/reports/1337624

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Open http://ixrdj3iwwhkuau5tby5jh3a536a2rdhpbdbu6ldhng43r47kim7a3lid.onion/referrer/onion.html in a Tor window
Open the Network tab of the devtools.
Follow the instructions on that test page.

fmarier · 2021-10-29T03:34:21Z

The extra patches are unfortunate, but I could not find a way to get our existing hooks to cover all of the test cases in my test page.

Note that I am planning to upstream (at least) the referrer patch into the Referrer Policy spec and then Chromium, given that the .onion scheme has been formally described in RFC 7686.

patches/services-network-cors-cors_url_loader.cc.patch

patches/net-url_request-url_request_job.cc.patch

chromium_src/services/network/cors/cors_url_loader.cc

chromium_src/net/url_request/url_request_job.cc

iefremov · 2021-10-29T13:28:34Z

chromium_src/net/url_request/url_request_job.cc

+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+#define BRAVE_COMPUTE_REFERRER_FOR_POLICY                                      \


why don't we do this in MaybeChangeReferrer in brave_shields_util.cc?

Because it doesn't cover all cases,sadly. That was my first attempt, but none of our existing hooks were even called in the case of a redirected CORS request for example (test case #6 on my test page).

Changing this function also takes care of the Origin header in most cases (though not the CORS ones).

if it doesn't cover all cases, do we have a problem with the referrer capping that we do in MaybeChangeReferrer?..

We might, I haven't gotten around to testing that yet.

fmarier · 2021-10-30T00:17:19Z

chromium_src/services/network/cors/cors_url_loader.cc

+          url::Origin::Create(request_.url))) {                    \
+    request_.headers.SetHeader(net::HttpRequestHeaders::kOrigin,   \
+                               url::Origin().Serialize());         \
+  } else /* NOLINT */


Needed, otherwise the linter complains about a missing brace.

chromium_src/net/url_request/url_request_job.cc

iefremov · 2021-11-03T13:48:02Z

If the fix is not super urgent we should invest into a browsertest, the url loader patch looks pretty fragile

fmarier · 2021-11-16T08:05:34Z

@iefremov I have added a browsertest which covers all of the test cases I have manually tested. Could you please review again?

components/tor/onion_location_navigation_throttle.h

…ave-browser#18071)

…ve/brave-browser#18071)

kjozwiak · 2021-11-26T04:49:08Z

Verification PASSED on Win 11 x64 using the following build:

Brave | 1.34.38 Chromium: 96.0.4664.45 (Official Build) nightly (64-bit)
-- | --
Revision | 76e4c1bb2ab4671b8beba3444e61c0f17584b2fc-refs/branch-heads/4664@{#947}
OS | Windows 11 Version 21H2 (Build 22000.348)

`Sub-resources`

`Same-origin`

Test Case #1

onion16_1.png - was loaded with the full Referer header and the origin of this page in the Origin header.

Origin: http://ixrdj3iwwhkuau5tby5jh3a536a2rdhpbdbu6ldhng43r47kim7a3lid.onion
Referer: http://ixrdj3iwwhkuau5tby5jh3a536a2rdhpbdbu6ldhng43r47kim7a3lid.onion/referrer/onion.html

Test Case #2

onion16_2.png - was loaded with a full Referer header and without an Origin header.

Referer: http://ixrdj3iwwhkuau5tby5jh3a536a2rdhpbdbu6ldhng43r47kim7a3lid.onion/referrer/onion.html

Test Case #3

onion16_3.png - was loaded with a full Referer header and the origin of this page in the Origin header.

Origin: http://ixrdj3iwwhkuau5tby5jh3a536a2rdhpbdbu6ldhng43r47kim7a3lid.onion
Referer: http://ixrdj3iwwhkuau5tby5jh3a536a2rdhpbdbu6ldhng43r47kim7a3lid.onion/referrer/onion.html

`Cross-origin`

Test Case #1

onion16_4.png - was loaded without a Referer header and with a value of null in the Origin header.

origin: null

Test Case #2

onion16_5.png - was loaded without Referer or Origin headers.

Test Case #3

onion16_6.png - was loaded without a Referer header and a value of null in the Origin header.

`Navigations`

`Same-origin`

The Referer header should be present (full URL of this page) in this example:

Test Case #1 - after a same-origin GET navigation

Test Case #2 - after a same-origin GET navigation ending up in a redirect

The Referer and Origin headers should be present (full URL, and same hostname as this page, respectively) in all of these examples:

Test Case #1 - after a same-origin POST navigation

Test Case #2 - after a POST navigation ending up in a redirect

`Cross-origin`

Neither the Referer not the Origin header should be present in these examples:

Test Case #1 - after a cross-origin GET navigation

Test Case #2 - after a same-origin GET navigation ending up in a cross-origin redirect

Test Case #3 - after a cross-origin GET navigation ending up in a same-origin redirect

The Referer header should not be present and the Origin header should be null in all of these examples:

Test Case #1 - after a cross-origin POST navigation

Test Case #2 - after a same-origin POST navigation ending up in a cross-origin redirect

Test Case #3 - after a cross-origin POST navigation ending up in a same-origin redirect

fmarier requested review from mariospr and iefremov October 29, 2021 03:32

fmarier self-assigned this Oct 29, 2021

fmarier requested review from a team as code owners October 29, 2021 03:32

fmarier changed the title ~~Strip referrer and origin in xorigin requests from a .onion~~ Strip referrer and origin headers in xorigin requests from a .onion Oct 29, 2021

goodov reviewed Oct 29, 2021

View reviewed changes

iefremov reviewed Oct 29, 2021

View reviewed changes

fmarier force-pushed the referrer-onion-18071 branch from 4ef37e0 to e04ea51 Compare October 30, 2021 00:10

fmarier requested a review from a team as a code owner October 30, 2021 00:10

fmarier force-pushed the referrer-onion-18071 branch from e04ea51 to 3253526 Compare October 30, 2021 00:14

fmarier commented Oct 30, 2021

View reviewed changes

fmarier force-pushed the referrer-onion-18071 branch from 3253526 to 088cbb9 Compare October 30, 2021 00:34

goodov reviewed Nov 1, 2021

View reviewed changes

chromium_src/net/url_request/url_request_job.cc Outdated Show resolved Hide resolved

goodov approved these changes Nov 1, 2021

View reviewed changes

fmarier force-pushed the referrer-onion-18071 branch from 088cbb9 to ffa2c9b Compare November 1, 2021 18:32

fmarier requested a review from iefremov November 1, 2021 18:33

iefremov reviewed Nov 3, 2021

View reviewed changes

chromium_src/net/url_request/url_request_job.cc Show resolved Hide resolved

fmarier force-pushed the referrer-onion-18071 branch 2 times, most recently from 15bf75c to fe3b682 Compare November 9, 2021 02:51

fmarier mentioned this pull request Nov 10, 2021

Send "null" Origin header on cross-origin .onion requests whatwg/fetch#1351

Draft

3 tasks

fmarier force-pushed the referrer-onion-18071 branch from fe3b682 to 0f3e7cd Compare November 16, 2021 07:56

fmarier requested a review from iefremov November 16, 2021 08:04

iefremov reviewed Nov 18, 2021

View reviewed changes

components/tor/onion_location_navigation_throttle.h Outdated Show resolved Hide resolved

iefremov approved these changes Nov 18, 2021

View reviewed changes

fmarier force-pushed the referrer-onion-18071 branch from 0f3e7cd to 030bf22 Compare November 19, 2021 00:02

Strip referrer header in xorigin requests from .onion (fixes brave/br…

5f931c8

…ave-browser#18071)

fmarier added 2 commits November 18, 2021 16:03

Nullify Origin header in xorigin CORS requests from .onion (fixes bra…

1784f7b

…ve/brave-browser#18071)

Add browsertest for cross-origin .onion requests.

5e415ed

fmarier force-pushed the referrer-onion-18071 branch from 030bf22 to 5e415ed Compare November 19, 2021 00:03

fmarier merged commit 38b8fc1 into master Nov 19, 2021

fmarier deleted the referrer-onion-18071 branch November 19, 2021 01:56

fmarier added this to the 1.34.x - Nightly milestone Nov 19, 2021

brave-builds pushed a commit that referenced this pull request Nov 19, 2021

Uplift of #10760 (squashed) to beta

a035f37

brave-builds mentioned this pull request Nov 19, 2021

Strip referrer and origin headers in xorigin requests from a .onion (uplift to 1.33.x) #11186

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strip referrer and origin headers in xorigin requests from a .onion #10760

Strip referrer and origin headers in xorigin requests from a .onion #10760

fmarier commented Oct 29, 2021 •

edited

Loading

fmarier commented Oct 29, 2021

iefremov Oct 29, 2021

fmarier Oct 29, 2021

iefremov Nov 3, 2021

fmarier Nov 4, 2021

fmarier Oct 30, 2021

iefremov commented Nov 3, 2021

fmarier commented Nov 16, 2021

kjozwiak commented Nov 26, 2021

Strip referrer and origin headers in xorigin requests from a .onion #10760

Strip referrer and origin headers in xorigin requests from a .onion #10760

Conversation

fmarier commented Oct 29, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

fmarier commented Oct 29, 2021

iefremov Oct 29, 2021

Choose a reason for hiding this comment

fmarier Oct 29, 2021

Choose a reason for hiding this comment

iefremov Nov 3, 2021

Choose a reason for hiding this comment

fmarier Nov 4, 2021

Choose a reason for hiding this comment

fmarier Oct 30, 2021

Choose a reason for hiding this comment

iefremov commented Nov 3, 2021

fmarier commented Nov 16, 2021

kjozwiak commented Nov 26, 2021

Sub-resources

Same-origin

Cross-origin

Navigations

Same-origin

Cross-origin

fmarier commented Oct 29, 2021 •

edited

Loading

`Sub-resources`

`Same-origin`

`Cross-origin`

`Navigations`

`Same-origin`

`Cross-origin`