1781772 - HTTPS-only mode drops origin header from media requests with crossorigin=anonymous

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Dennis Schubert [:denschub]]

Attachment: testcase.html

The attached testcase is building an <img> with crossOrigin = "anonymous" and is attaching a HTTP URL:

let image = new Image();
image.crossOrigin = "anonymous";
image.src = "http://placekitten.com/200/300";

In Firefox with HTTPS-only enabled, the request to the image file is sent with the header

Origin: null

And unfortunately, this breaks a governmental archive site in Denmark...

Comment 1

[Daniel Veditz [:dveditz]]

If you set a request to anonymous aren't we supposed to drop the Origin header for a GET? We shouldn't sent Origin: null in that case though, since that indicates a CORS request but one that can't use the actual origin for various reasons (e.g. redirect chains)

Is this a regression? Tom did some recent work in this area.

From the webcompat bug

The site, which is served via HTTPS, is trying to load the image via HTTP. In Chrome, it gets upgraded by their mixed-content auto-upgrade-magic, and it works correctly. In a fresh Firefox profile with HTTPS-only mode diasabled, it works just fine for me. However, with HTTPS-only mode enabled, it breaks as shown here.

The original breaking URL:

Description: Cannot show pictures from the scanned church records on the page
Steps to Reproduce:
Go to any subpage (eg. https://www.sa.dk/ao-soegesider/da/billedviser?epid=17114393#148846,24601867 ) that is meant to show an image of a chruch record and you will get an error. Test the same in any other browser, you will see the image of the page.

Comment 2

[Daniel Veditz [:dveditz]]

This does work in ESR-91.11 so it is a regression (probably more recent than that, it was handy).

Tom: can you take a look?

Comment 3

[Tom Schuster (MoCo)]

This is related to the redirect tainting we do since bug 1605305. Disabling the pref network.http.origin.redirectTainted fixes this.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1605305

Comment 5

[Tom Schuster (MoCo)]

We could potentially check if the request was upgraded by HTTPS only mode and not apply tainting in that case?

Comment 6

[Dennis Schubert [:denschub]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

If you set a request to anonymous aren't we supposed to drop the Origin header for a GET?

If that's the case, this would also be a bug worth filing for Chrome, because they set the origin header in the request.

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Tom Schuster (MoCo) from comment #5)

We could potentially check if the request was upgraded by HTTPS only mode and not apply tainting in that case?

This sounds reasonable to me.

Comment 8

[Tom Schuster (MoCo)]

If you set a request to anonymous aren't we supposed to drop the Origin header for a GET?

I don't think that is correct. The spec wants us to send the Origin header for all "CORS tainted" requests. Anonymous or use-credentials isn't even considered. (https://fetch.spec.whatwg.org/#append-a-request-origin-header Step 2.)

Comment 9

[Tom Schuster (MoCo)]

Attachment: WIP: Bug 1781772 - Don't redirect-taint HTTPS-only mode upgrades

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, this bug has a P1 WebCompat priority.
:tschuster, could you consider increasing the severity of this web compatibility bug?

For more information, please visit auto_nag documentation.

Comment 11

[Tom Schuster (MoCo)]

No. HTTPS-only mode isn't enabled by default as far as I know.

Comment 12

[Dennis Schubert [:denschub]]

HTTPS-only mode isn't enabled by default as far as I know.

This is true, dropping the WebCompat priority accordingly.

Comment 13

[Tom Schuster (MoCo)]

Could you take a look at the patch? I think the approach should work, but I have some open questions.

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to Tom Schuster (MoCo) from comment #13)

Could you take a look at the patch? I think the approach should work, but I have some open questions.

I've replied on phab.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1605305

Comment 16

[Tom Schuster (MoCo)]

Hi Kershaw. I am a bit stuck on this, because it seems like we now also ignore redirects from one HTTP to another HTTP site that are different origin, but were both upgraded.

Comment 17

[Kershaw Chang [:kershaw]]

(In reply to Tom Schuster (MoCo) from comment #16)

Hi Kershaw. I am a bit stuck on this, because it seems like we now also ignore redirects from one HTTP to another HTTP site that are different origin, but were both upgraded.

Sorry, I don't quite understand.
Can you provide a concrete example?

I imagine the case you described would be like:

1. Go to http://example.com
2. Upgrade to https://example.com because of HTTPS-only mode.
   We don't mark this request as tainted.
3. We connect to the server and the server redirects to http://example.org
   At this point, this request should be marked as tainted.
4. Upgrade to https://example.org.
   We should keep the tainted flag.

Do I understand your question correctly?

Comment 18

[Tom Schuster (MoCo)]

I think you've got the idea. I have to following test case that shows that we don't apply tainting properly, because we treat everything as an upgrade with the current patch.

Go to example.com (with HTTPS only mode enabled) and execute the following in the web console:

let image = new Image();
image.crossOrigin = "anonymous";
image.src = "http://httpbin.org/redirect-to?url=http%3A%2F%2Fplacekitten.com%2F200%2F300";

ShouldTaintReplacementChannelOrigin
 mURI = http://httpbin.org/redirect-to?url=http%3A%2F%2Fplacekitten.com%2F200%2F300 aNewURI = https://httpbin.org/redirect-to?url=http%3A%2F%2Fplacekitten.com%2F200%2F300
  detected HTTPS Only
  NO taint!
ShouldTaintReplacementChannelOrigin
 mURI = http://httpbin.org/redirect-to?url=http%3A%2F%2Fplacekitten.com%2F200%2F300 aNewURI = https://httpbin.org/redirect-to?url=http%3A%2F%2Fplacekitten.com%2F200%2F300
  detected HTTPS Only
  NO taint!
ShouldTaintReplacementChannelOrigin
 mURI = https://httpbin.org/redirect-to?url=http%3A%2F%2Fplacekitten.com%2F200%2F300 aNewURI = http://placekitten.com/200/300
  detected HTTPS Only
  NO taint!
ShouldTaintReplacementChannelOrigin
 mURI = https://httpbin.org/redirect-to?url=http%3A%2F%2Fplacekitten.com%2F200%2F300 aNewURI = http://placekitten.com/200/300
  detected HTTPS Only
  NO taint!
ShouldTaintReplacementChannelOrigin
 mURI = http://placekitten.com/200/300 aNewURI = https://placekitten.com/200/300
  detected HTTPS Only
  NO taint!
ShouldTaintReplacementChannelOrigin
 mURI = http://placekitten.com/200/300 aNewURI = https://placekitten.com/200/300
  detected HTTPS Only
  NO taint!

Comment 19

[Tom Schuster (MoCo)]

Could you look at this? I am not an expert in the networking code so fixing this myself is quite a bit harder.

Comment 20

[Kershaw Chang [:kershaw]]

(In reply to Tom Schuster (MoCo) from comment #19)

Could you look at this? I am not an expert in the networking code so fixing this myself is quite a bit harder.

I think this is fixed by bug 1800990, but I'll still try to add a test for this.
Hope you don't mind that I take this bug from you.

Comment 21

[Kershaw Chang [:kershaw]]

Attachment: Bug 1781772 - Test redirect tainting with Https-only mode, r=tschuster

Comment 22

[Tom Schuster (MoCo)]

Thank you for picking this up!

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:kershaw, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 24

[Kershaw Chang [:kershaw]]

I'll land this.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8da627ca762f
Test redirect tainting with Https-only mode, r=tschuster

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8da627ca762f