1444278 - CORS fetches redirected across origins fail because the Origin header contains the wrong value

Status: RESOLVED WORKSFORME

(Core :: Networking: HTTP, defect, P2)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

I created a simple HTML web page with both a simple Fetch and XHR request linked to buttons. The request in both cases was to url_a on the same virtualhost.

Both requests were specified withCredentials.

url_a returns a 302 to url_b on a second virtual host.

url_b retuns a 302 to url_c on a third virtual host.

url_c returns a 302 to url_d on the original virtual host.

url_d returns a 200 and a response body which says 'Helloooo!!!!'

The procedure was repeated with url_b returning a 303.

All virtual hosts are configured to respond with Access-Control-Allow-Origin headers matching the requested Origin value. Other standard headers used to allow CORS have also been included in the response.

This test was carried out on 58.0.2 on osx. It was also carried out on safari and chrome.

Nginx was used to set up and run the virtualhosts. Each virtaul hosts's urls included the same cors add_headers file.

Tested on osx and linux builds of 58.



Actual results:

The browser followed the redirect from url_a to url_b. 

The browser blocked the redirect from url_b to url_c and raised a Cross-Origin Request Blocked error mesaage in the console. This stated that the CORS header Access-Control-Allow-Origin does not match a string literal which has the same value as tghe origin header for the request made to url_c.

However, this requested Origin value is identical to the Access-Control-Allow-Origin value shown in url_c's blocked response header.

The network page of the developer window shows only three 302 requests. 

There are no pre-flight requests being made. 

The third 302 line corresponds to the blocked response from url_c. I've inspected the raw request and response headers from this line and the origin values match exactly. The error messsage does not inform the user of the Access-Control-Allow-Origin value which reportedly failed the comparison.

Chrome and Safari followed all the 302s and 303s, returning the results of the call to url_d - and displaying the cheerful greeting in the web page.
  


Expected results:

The error message should not have been displayed and and the redirect response to the call to url_c should not have been blocked.

All 302 CORS requests should have been followed - resulting in the results of url_d being returned by the fetch and xhr calls. 

If a legitimate mismatch had caused the request to be blocked, both values being compared should be reported in the error message.

Happy to provide more details if this isn't enough to go on.

Thanks for looking!

Comment 1

[Stefan [:StefanG_QA]]

Based on other similar bug reports I think the correct component is Firefox::Developer Tools:Console. Please correct if this is not the right component.

Comment 2

[Brian Grinstead [:bgrins]]

Thanks for the report - I'm going to move this into the networking component since this sounds like an issue with CORS itself. It would be very helpful if you could provide a link to a demo web page that demonstrates this issue, or your nginx configuration plus the simple web page needed to reproduce.

Comment 3

[[e-mail]]

Attachment: nginx.dummy.login.conf

Here's an example nginx config which will spin up a number of virtual hosts on the same hosts - with paths that redirect to each other.

Comment 4

[[e-mail]]

Attachment: CORS add_header include for nginx config

CORS add header include file for nginx.

Comment 5

[[e-mail]]

Attachment: ajax_test.html

Thanks, w3schools! :-)

Comment 6

[[e-mail]]

Attachment: hello.txt

Helooo!!

Comment 7

[[e-mail]]

I've added the files as requested. You may need to adjust some of the paths slightly. I'm sure you don't need that jquery include in the html file either.  

The version of nginx will also matter when testing. Older (probably only *really* old) versions don't consider the add_header directives when making 303s (possibly 302s too) - and send these without them added. 

Many thanks!

Acky.

Comment 8

[Michal Novotny [:michal]]

I cannot reproduce it with nginx 1.10.3 on debian Stretch. I tried 58.0.2 and 60.0b3 on linux. Please, provide a log, maybe we can see something in it: https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging

Comment 9

[[e-mail]]

Hi yes,

Sorry, my javascript skills are  a bit rusty. I just checked this in a completely different linux 16.04 environment with nginx 1.10.3 and a less buggy ajax_test.html. 

XHR is working as expected on firefox 59.0. Sorry for wasting your time on that.

Fetch is still looking a bit odd, though. It's possible I was eroneously only testing fetch before. I'll do a bit of digging and post some http logging output - and an improved test page - if I can't work out what's going on there. 

Might just be my tragically bad javascript skill again. :-)

Comment 10

[[e-mail]]

Yeah it looks like fetch is the feller with the problem. It's failing on the call to c with:
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at http://127.0.0.1:1062/c. (Reason: CORS header ‘Access-Control-Allow-Origin’ does not match ‘http://127.0.0.1:1060’).

I'll add a better test case tarball. It can be run like so with a bit of hard coded path tweaking: 
nginx -c /home/whomever/app_server/nginx.dummy.login.conf 

http://127.0.0.1:1060/ajax_test.html

Comment 11

[[e-mail]]

Attachment: ajax_test.tar.gz

Comment 12

[[e-mail]]

Attachment: ajax_test_logs.tar.gz

Html logs.

Cheers.

Acky.

Comment 13

[Michal Novotny [:michal]]

In case of XHR test, when downloading http://host:1062/c we send request header "Origin: null" and we get "Access-Control-Allow-Origin: null". In case of fetch test we send "Origin: http://host:1060" and we get "Access-Control-Allow-Origin: http://host:1060", but we expect "null" at https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/netwerk/protocol/http/nsCORSListenerProxy.cpp#619 because mOriginHeaderPrincipal was replaced at https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/netwerk/protocol/http/nsCORSListenerProxy.cpp#778 on previous redirect (b->c).

Jason, you've reviewed this code in the past. Can you have a look at it or find somebody who knows this code?

Comment 14

[Anne (:annevk)]

FYI: this is tested by fetch/api/cors/cors-redirect.any.html on wpt.

Comment 16

[Anne (:annevk)]

To be clear, this is a problem for both preflighted and non-preflighted CORS fetches with redirects that cross the origin boundary. Once we cross origin boundaries the request's origin is supposed to become a unique opaque identifier (which serializes to null). It seems we do expect the server to return such a value in Access-Control-Allow-Origin, but the Origin header we send out still contains a non-null origin.

It'd be good to fix this as this is a rather subtle hard-to-debug bug for web developers and quite a major incompatibility point with respect to other browsers.

Comment 17

[Frederik Braun [:freddy]]

Christoph, you said you were interested in tackling this.
Comment 14 has a simple test case.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Frederik Braun [:freddyb] from comment #17)
> Christoph, you said you were interested in tackling this.
> Comment 14 has a simple test case.

It seems test from comment 14 is not in our tree. If we have a good testcase I am happy to debug and help find the problem.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Anne (:annevk) from comment #14)
> FYI: this is tested by fetch/api/cors/cors-redirect.any.html on wpt.

Ah, finally found it, it's in our tree actually: fetch/api/cors/cors-redirect.any.js is the right name of the file. Thanks Anne!
I'll take a look.

Comment 20

[kdevarakonda]

I was wondering when this will be available for testing.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(In reply to kdevarakonda from comment #20)
> I was wondering when this will be available for testing.

I am swamped with other things at the moment unfortunately. I'll see if I can have someone else look into this - sorry.

Comment 22

[Jason Duell]

Dragana

Comment 23

[Jason Duell]

Dragana: see esp comment 10 and comment 13

Comment 24

[Thomas Nguyen (:tnguyen)]

I could see the header is changed to "null" here :
https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/netwerk/protocol/http/nsCORSListenerProxy.cpp#1001

But not sure after that we still send original origin header

Comment 25

[Thomas Nguyen (:tnguyen)]

Well, the reason is we set the original origin header back here:
https://searchfox.org/mozilla-central/rev/5612e2c75bfde2f5e9512b5b3ffc952ddb74a6dc/dom/fetch/FetchDriver.cpp#303

I don't see any specs about "force Origin header flag". Hi annek, is that deprecated so I can remove the related codes?

Comment 26

[Thomas Nguyen (:tnguyen)]

https://archive.li/ecTeJ points me to old spec and it was removed. I am intending to remove the deprecated code then it could fix this problem. Plz feel free let me know if you have any objection.

Comment 27

[Anne (:annevk)]

Yeah, that's gone. The new logic for when and how to include the Origin header is defined by step 5.10 of https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch. Note that it is a somewhat involved step, with redirects being able to influence the serialized value and such.

Bug 1424076 is also relevant here, I think.

Comment 28

[Junior [inactive]]

Bug 1502904 is going to take away ForceOriginHeader

Comment 30

[Thomas Nguyen (:tnguyen)]

Attachment: WIP-0001-Bug-1444278-Correct-Origin-Header-when-redirect-fetc.patch

Comment 31

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1444278 - Send unique opaque origin (null) when CORS fetches redirected cross origins

According to specs, we should send a "null" origin header in CORS
redirect cross origin. Add an opaque origin check in httpChannelInternal
to do that

MozReview-Commit-ID: 2lfVoo72YXp

Comment 32

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b46f8029847865b7c86e55cde0e7ddc3656368d

Comment 33

[Thomas Nguyen (:tnguyen)]

I am canceling the review requests because there's still something to do with CORS flag. We don't have CORS flag now and it's better to have it.
I had a short talked with Annek, we may do it in Cors listener or necko Http implementation.

Comment 34

[Thomas Nguyen (:tnguyen)]

Junior should take this bug, sorry I cannot take a look at this bug at the moment. Junior, although the original issue in this bug is fixed in bug 1508661, there're still some works to make origin header works follow CORS flag spec. Such as
/fetch/api/redirect/redirect-origin.any.html
/fetch/api/redirect/redirect-origin.any.worker.html
/fetch/api/cors/cors-cookies-redirect.any.worker.html
/fetch/api/cors/cors-cookies-redirect.any.html
/cookies/samesite/fetch.html
Thanks

Comment 35

[Junior [inactive]]

Attachment: Bug 1444278 - Send unique opaque origin (null) when CORS fetches redirected cross origins

Comment 36

[Junior [inactive]]

(In reply to Thomas Nguyen from comment #34)

Junior should take this bug, sorry I cannot take a look at this bug at the moment. Junior, although the original issue in this bug is fixed in bug 1508661, there're still some works to make origin header works follow CORS flag spec. Such as
/fetch/api/redirect/redirect-origin.any.html
/fetch/api/redirect/redirect-origin.any.worker.html
/fetch/api/cors/cors-cookies-redirect.any.worker.html
/fetch/api/cors/cors-cookies-redirect.any.html
/cookies/samesite/fetch.html
Thanks

Note: works well except /cookies/samesite/fetch.html with current central and nightly.

Comment 37

[Junior [inactive]]

The culprit is not origin header for the samesite.
The remaining issue is the CORS Flag, mentioned by spec and Comment 33

Comment 38

[Junior [inactive]]

WORKSFORME since the issue is gone. The tainted origin flag is indicated by [1]. That is, the Origin: is set to null for the null principal. See 4.4.10 [2]

However, 4.5.5.10 [3] isn't implemented good enough. For example, api/cors/cors-redirect.any.html is passed by luck. The Origin: is set to null in [4] since nsCORSListenerProxy follows the tainted rule. Later the code path go through [5]. It doesn't follow the tainted rule. It works since the GET is bypassed. In other words, POST doesn't work well because [5] will overwrite the Origin: header.

Moreover, Section 4 specifies the behavior of HttpFetch, instead of Fetch API only. IMO nsCORSListenerProxy is response for passing the CORS flags and tainted origin flag to channel. And let the nsHttpChannel add the Origin: [6]

[1] https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/netwerk/protocol/http/nsCORSListenerProxy.cpp#718-747
[2] https://fetch.spec.whatwg.org/#http-redirect-fetch
[3] https://fetch.spec.whatwg.org/#http-network-or-cache-fetch
[4] https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/netwerk/protocol/http/nsCORSListenerProxy.cpp#940-968
[5] https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/dom/fetch/FetchDriver.cpp#1458-1468
[6] https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/netwerk/protocol/http/nsHttpChannel.cpp#9439