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 gk

Keywords: tbb-9.0-issues tbb-9.0.1-can added; cors removed
Severity: BlockerNormal

comment:2 Changed 7 weeks ago by gk

Cc: stridentbean added
Keywords: TorBrowserTeam201910 added

#32271 seems to be a duplicate.

comment:3 Changed 7 weeks ago by gk

Keywords: tbb-regression added

comment:4 Changed 6 weeks ago by acat

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 gk

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 acat

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 pili

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201910 removed

Moving tickets to November 2019

comment:8 Changed 6 weeks ago by pili

Points: 2

comment:9 Changed 5 weeks ago by sega01

This is breaking doublemixwcfx4wadeuvuygpxej5jpu7uleesh3yptopnbj5kshnlrid.onion for me.

Do you know of any workarounds?

comment:10 in reply to:  9 ; Changed 5 weeks ago by 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.

comment:11 in reply to:  10 Changed 5 weeks ago by sega01

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 Changed 5 weeks ago by 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.

comment:13 in reply to:  12 Changed 5 weeks ago by gk

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?

Last edited 5 weeks ago by gk (previous) (diff)

comment:14 Changed 5 weeks ago by gk

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 Changed 5 weeks ago by 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 the nsCORSListenerProxy path or am I missing something here?

comment:16 Changed 5 weeks ago by sega01

Was the intention to only strip origin when going between .onion and clearnet, or even .onion to .onion?

comment:17 in reply to:  16 ; Changed 5 weeks ago by 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.

comment:18 in reply to:  17 Changed 5 weeks ago by sega01

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 pili

Cc: tbb-team added
Owner: changed from tbb-team to acat
Status: newassigned

Assigning more tickets to acat for the next few months

comment:20 in reply to:  15 ; Changed 3 weeks ago by acat

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: assignedneeds_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 the nsCORSListenerProxy 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 in reply to:  20 Changed 3 weeks ago by gk

Keywords: tbb-backport added
Resolution: fixed
Status: needs_reviewclosed

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 the nsCORSListenerProxy 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 acat

Resolution: fixed
Status: closedreopened

comment:24 Changed 3 weeks ago by acat

Status: reopenedneeds_review

comment:25 Changed 3 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me. Merged to tor-browser-68.2.0esr-9.5-1 (commit 9de14da5e2a82b6e1ea4a4c68328996cbcc847ce and 1b5b65b5c8f11e3f2fd30ca0604fde741bc2c749)

Note: See TracTickets for help on using tickets.