Missing ORIGIN header breaks CORS in Tor Browser 9.0

[complexparadox]

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)

[gk]

#32271 seems to be a duplicate.

[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?

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

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

[pili]

Moving tickets to November 2019

[sega01]

This is breaking doublemixwcfx4wadeuvuygpxej5jpu7uleesh3yptopnbj5kshnlrid.onion for me.

Do you know of any workarounds?

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

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

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

[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?

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

[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?

[sega01]

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

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

[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?

[pili]

Assigning more tickets to acat for the next few months

[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?

---

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.

[gk]

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.

[acat]

Bug is ​https://bugzilla.mozilla.org/show_bug.cgi?id=1598647.

[acat]

Backported in ​https://github.com/acatarineu/tor-browser/commits/32255_backport.

[gk]

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