Opened 7 weeks ago
Closed 3 weeks ago
#32255 closed defect (fixed)
Missing ORIGIN header breaks CORS in Tor Browser 9.0
Reported by: | complexparadox | Owned by: | acat |
---|---|---|---|
Priority: | Medium | Milestone: | |
Component: | Applications/Tor Browser | Version: | |
Severity: | Normal | Keywords: | tbb-9.0-issues, tbb-9.0.1-can, tbb-regression, TorBrowserTeam201911R, tbb-backport |
Cc: | stridentbean, tbb-team | Actual Points: | |
Parent ID: | Points: | 2 | |
Reviewer: | Sponsor: |
Description
Looks like there is an issue on Tor Browser 9.0 which affects our CORS allowance setup, at least with the dependency django-cors-headers, because it fails to send the expected header ORIGIN in the OPTIONS preflight. It works fine using the latest 8 version. We've noticed this only happens when the CORS request source is a .onion address, otherwise it works as usual.
Example:
public.com XHR OPTIONS >> publicapi.com (ORIGIN HEADER INCLUDED, WORKS)
hidden.onion XHR OPTIONS >> publicapi.com (MISSING ORIGIN HEADER, BREAKS)
hidden.onion XHR OPTIONS >> hiddenapi.onion (MISSING ORIGIN HEADER, BREAKS)
Child Tickets
Change History (25)
comment:1 Changed 7 weeks ago by
Keywords: | tbb-9.0-issues tbb-9.0.1-can added; cors removed |
---|---|
Severity: | Blocker → Normal |
comment:2 Changed 7 weeks ago by
Cc: | stridentbean added |
---|---|
Keywords: | TorBrowserTeam201910 added |
comment:3 Changed 7 weeks ago by
Keywords: | tbb-regression added |
---|
comment:4 Changed 6 weeks ago by
It seems this is now controlled by network.http.referer.hideOnionSource
(changed in https://bugzilla.mozilla.org/show_bug.cgi?id=1503736). Was this pref supposed to affect all requests or just hiding the referer when navigating from an onion website to a non-onion one?
comment:5 Changed 6 weeks ago by
Hm! The idea behind the pref was avoiding information leakage when coming from an .onion domain while requesting a non-onion one. See: #22320 for a scenario.
The patch that got uplifted (https://bugzilla.mozilla.org/show_bug.cgi?id=1305144) strips the Referer header if the domain in that header would be a .onion one (it does not matter whether the target domain is a .onion or not)
So, the question here is: should the Origin header follow that model to prevent information leakage or is the usecase a different one (or better: are there use-cases that are different enough from the Referer one that we need a more nuanced approach here)?
I am not sure yet, to be honest.
comment:6 Changed 6 weeks ago by
If I understand it correctly, if we talk about Referer
headers this patch is currently only making it easier to not leak the .onion referrer by default, but it should be possible to achieve the same via the right Referrer-Policy
, right?
With Origin
I think it's different, because the patch allows something that *I think* is not possible in regular browsers: to issue xhr (fetch, xmlhttprequest) requests without the Origin
header. Well, it's possible to do with fetch
+ mode: no-cors
option, but you only get an "opaque" response.
I'm also not sure about what we should do here. One possibility would be to simply go back to previous esr60 behaviour and not strip the Origin
header for xhr requests. This however would make it not possible to do fetch
requests without Origin
. While that's what happens in regular browsers, I think being able to do fetch requests without Origin
can be useful for .onion websites.
If we want to keep the current default behaviour, one possibility for people that need CORS in .onions could be to make Origin/Referer
headers opt-in based on the page (or fetch API) Referrer-Policy
. While linking the Origin
header to the Referrer-Policy
might be surprising (and non-standard), I think it would be safe to assume that a website that has an explicit policy like no-referrer-when-downgrade
would be fine to have both Referer
and Origin
header in requests. If this approach would work, I guess we could change the default Referrer-Policy
from no-referrer-when-downgrade
to something like same-origin
.
comment:7 Changed 6 weeks ago by
Keywords: | TorBrowserTeam201911 added; TorBrowserTeam201910 removed |
---|
Moving tickets to November 2019
comment:8 Changed 6 weeks ago by
Points: | → 2 |
---|
comment:9 follow-up: 10 Changed 5 weeks ago by
This is breaking doublemixwcfx4wadeuvuygpxej5jpu7uleesh3yptopnbj5kshnlrid.onion for me.
Do you know of any workarounds?
comment:10 follow-up: 11 Changed 5 weeks ago by
Replying to sega01:
This is breaking doublemixwcfx4wadeuvuygpxej5jpu7uleesh3yptopnbj5kshnlrid.onion for me.
Do you know of any workarounds?
What does break mean? I just loaded the .onion and it works fine for me.
comment:11 Changed 5 weeks ago by
Replying to gk:
Replying to sega01:
This is breaking doublemixwcfx4wadeuvuygpxej5jpu7uleesh3yptopnbj5kshnlrid.onion for me.
Do you know of any workarounds?
What does break mean? I just loaded the .onion and it works fine for me.
Try to do a mix and watch the network tab. It will retry OPTIONS over and over. Just enter a Bitcoin address like 1xm4vFerV3pSgvBFkyzLgT1Ew3HQYrS1V and click on mix.
The Bitmix API requires the Origin header to return the correct CORS response. Foxmixer is fine because it returns for *. Bitmix is selective.
comment:12 follow-up: 13 Changed 5 weeks ago by
BTW, I mentioned this issue in the uplift meeting, and tom did not see any problem with just reverting the https://bugzilla.mozilla.org/show_bug.cgi?id=1503736 patch and go to the previous esr60 behaviour. It's not clear why that change was done.
comment:13 Changed 5 weeks ago by
Replying to acat:
BTW, I mentioned this issue in the uplift meeting, and tom did not see any problem with just reverting the https://bugzilla.mozilla.org/show_bug.cgi?id=1503736 patch and go to the previous esr60 behaviour. It's not clear why that change was done.
Maybe the authors of the patch read
Whenever a user agent issues an HTTP request from a "privacy- sensitive" context, the user agent MUST send the value "null" in the Origin header field.
and arguably .onion sites could be seen as a privacy-sensitive context. Now, the question is whether we could just avoid stripping the header and set it to "null" instead?
comment:14 Changed 5 weeks ago by
Oh, and that seems to be the same behavior in the fetch
spec: https://fetch.spec.whatwg.org/#origin-header: If we omit the Referer, send Origin: null
. But it seems to me the patch in 1503736 is outright omitting it which does not seem to be compliant with the spec.
comment:15 follow-up: 20 Changed 5 weeks ago by
Hm,
+ if (!currentOrgin.EqualsIgnoreCase(origin.get()) && + StringEndsWith(potentialOnionHost, NS_LITERAL_CSTRING(".onion"))) { + origin.Truncate(); + } + } + rv = http->SetRequestHeader(nsDependentCString(net::nsHttp::Origin), origin, false); NS_ENSURE_SUCCESS(rv, rv);
and
+ if (!origin.EqualsIgnoreCase(currentOrigin.get())) { + // Origin header is suppressed by .onion + return; + } + } } rv = mRequestHead.SetHeader(nsHttp::Origin, origin, false /* merge */);
does not even seem to be the same behavior depending on whether the code takes the nsHttpChannel
or the nsCORSListenerProxy
path or am I missing something here?
comment:16 follow-up: 17 Changed 5 weeks ago by
Was the intention to only strip origin when going between .onion and clearnet, or even .onion to .onion?
comment:17 follow-up: 18 Changed 5 weeks ago by
Replying to sega01:
Was the intention to only strip origin when going between .onion and clearnet, or even .onion to .onion?
Hard to say as no one from us wrote the patch. But if the intention was to follow the Referer-related patch then Origin
would be empty.
comment:18 Changed 5 weeks ago by
Replying to gk:
Replying to sega01:
Was the intention to only strip origin when going between .onion and clearnet, or even .onion to .onion?
Hard to say as no one from us wrote the patch. But if the intention was to follow the Referer-related patch then
Origin
would be empty.
I see, thank you. What do you think the best fix is from here?
comment:19 Changed 5 weeks ago by
Cc: | tbb-team added |
---|---|
Owner: | changed from tbb-team to acat |
Status: | new → assigned |
Assigning more tickets to acat for the next few months
comment:20 follow-up: 21 Changed 3 weeks ago by
Keywords: | TorBrowserTeam201911R added; TorBrowserTeam201911 removed |
---|---|
Status: | assigned → needs_review |
Replying to gk:
Hm,
+ if (!currentOrgin.EqualsIgnoreCase(origin.get()) && + StringEndsWith(potentialOnionHost, NS_LITERAL_CSTRING(".onion"))) { + origin.Truncate(); + } + } + rv = http->SetRequestHeader(nsDependentCString(net::nsHttp::Origin), origin, false); NS_ENSURE_SUCCESS(rv, rv);and
+ if (!origin.EqualsIgnoreCase(currentOrigin.get())) { + // Origin header is suppressed by .onion + return; + } + } } rv = mRequestHead.SetHeader(nsHttp::Origin, origin, false /* merge */);does not even seem to be the same behavior depending on whether the code takes the
nsHttpChannel
or thensCORSListenerProxy
path or am I missing something here?
Do you mean that one truncates the origin and the other just not sets it? Or that the 'is .onion' check is done differently in both cases?
---
I checked with doublemixwcfx4wadeuvuygpxej5jpu7uleesh3yptopnbj5kshnlrid.onion and apparently they fixed it already, so we cannot tell if setting Origin: null
would have fixed the original issue or not. But if we are going to keep the current behaviour, with .onion website being "privacy-sensitive context", I guess it's better to set it to null rather than removing the header and be more compliant with the spec.
For esr68 I think this would do it: https://github.com/acatarineu/tor-browser/commit/32255. I can file a bugzilla issue for this.
comment:21 Changed 3 weeks ago by
Keywords: | tbb-backport added |
---|---|
Resolution: | → fixed |
Status: | needs_review → closed |
Replying to acat:
Replying to gk:
Hm,
+ if (!currentOrgin.EqualsIgnoreCase(origin.get()) && + StringEndsWith(potentialOnionHost, NS_LITERAL_CSTRING(".onion"))) { + origin.Truncate(); + } + } + rv = http->SetRequestHeader(nsDependentCString(net::nsHttp::Origin), origin, false); NS_ENSURE_SUCCESS(rv, rv);and
+ if (!origin.EqualsIgnoreCase(currentOrigin.get())) { + // Origin header is suppressed by .onion + return; + } + } } rv = mRequestHead.SetHeader(nsHttp::Origin, origin, false /* merge */);does not even seem to be the same behavior depending on whether the code takes the
nsHttpChannel
or thensCORSListenerProxy
path or am I missing something here?
Do you mean that one truncates the origin and the other just not sets it? Or that the 'is .onion' check is done differently in both cases?
The former.
---
I checked with doublemixwcfx4wadeuvuygpxej5jpu7uleesh3yptopnbj5kshnlrid.onion and apparently they fixed it already, so we cannot tell if setting
Origin: null
would have fixed the original issue or not. But if we are going to keep the current behaviour, with .onion website being "privacy-sensitive context", I guess it's better to set it to null rather than removing the header and be more compliant with the spec.
For esr68 I think this would do it: https://github.com/acatarineu/tor-browser/commit/32255. I can file a bugzilla issue for this.
Please do. The patch looks good to me. Merged to tor-browser-68.2.0esr-9.5-1
(commit f26fb9c17d71f3373c8ccb91ae74c438d9e13f80). Marking for possible backport.
comment:23 Changed 3 weeks ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:24 Changed 3 weeks ago by
Status: | reopened → needs_review |
---|
comment:25 Changed 3 weeks ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
Looks good to me. Merged to tor-browser-68.2.0esr-9.5-1
(commit 9de14da5e2a82b6e1ea4a4c68328996cbcc847ce and 1b5b65b5c8f11e3f2fd30ca0604fde741bc2c749)
#32271 seems to be a duplicate.