Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Difference between spec and practice in mixed content upgrade #63

Closed
moztomer opened this issue Jan 5, 2023 · 16 comments
Closed

Difference between spec and practice in mixed content upgrade #63

moztomer opened this issue Jan 5, 2023 · 16 comments

Comments

@moztomer
Copy link
Contributor

moztomer commented Jan 5, 2023

We plan to ship the automatic upgrading of passive mixed content (as per Mixed Lvl2) in Firefox Nightly.

We checked the behavior of mixed-content display upgrade mechanism of Firefox Nightly and Chrome release related to CORS.
E.g., how does mixed content interacts with CORS request where CORS is not allowed.
It appears that such CORS requests get upgraded by mixed content before they get blocked in both browser.

STR for chrome and nightly: open an https site and type the following in the webconsole:

let video12 = document.createElement("video")
video12.crossOrigin = "Anonymous"
video12.src = "http://www.youtube.com/watch?v=q5Tp1ZUbAFQ"
document.body.appendChild(video12)

Expected error message: something like: "Access to video at 'http: //videoURL' from origin 'https://someSite ... blocked by Mixed-Content Blocker'
Actual error message: "Access to video at 'https://videoURL' from origin 'https://someSite ... blocked by CORS policy"

But the specs says we DON'T upgrade mixed content:

If one or more of the following conditions is met, return without modifying request:
...
3. request’s mode is CORS.

My question is, should we change the mixed-content spec or the browsers implementation according to mixed-content (level 2) behavior with CORS requests?

@mikewest
Copy link
Member

mikewest commented Jan 5, 2023

cc @estark37 and @carlosjoan91

@annevk
Copy link
Member

annevk commented Jan 5, 2023

(Expected and Actual error message in OP are the same.)

It's not clear to me why we'd exclude "cors" requests here. Would be interested in the rationale.

It's also not clear to me why "imageset" initiator requests are excluded. (Potentially a separate issue.)

@moztomer
Copy link
Contributor Author

moztomer commented Jan 5, 2023

(Expected and Actual error message in OP are the same.)

The difference is the upper letter S in the protocol of the blocked urls. I will edit it to highlight it more :)

@annevk
Copy link
Member

annevk commented Jan 6, 2023

@moztomer ah true, but wouldn't you expect it to be blocked by Mixed Content as opposed to CORS?

@moztomer
Copy link
Contributor Author

moztomer commented Jan 6, 2023

Yes, I think you are right.
So, in case mixed-content doesn't upgrade the media/ image element to https it should block it unrelated to CORS.
My initially expected outcome was incorrect then. Thank you I will correct it.

@mozfreddyb
Copy link

We want to align here, but are not sure whether this is incorrect in spec or in Chrome: There are no tests and Chrome is upgrading regardless of the request mode.

@estark37: Can you remember the rationale for not upgrading when the request mode is cors?

@annevk
Copy link
Member

annevk commented Jan 12, 2023

It's the same as with "imageset", prior behavior: https://www.w3.org/TR/2016/CR-mixed-content-20160802/#should-block-fetch.

@moztomer
Copy link
Contributor Author

@annevk I think the difference here is that either chrome as firefox nightly are upgrading requests that enable CORS.

So I would assume, here we have situation where we need to decide whether we update the browser implementations or the spec?

@annevk
Copy link
Member

annevk commented Jan 12, 2023

Was the old behavior just never implemented? I'm pretty sure Firefox used to block HTTPS -> HTTP CORS requests.

@moztomer
Copy link
Contributor Author

It is blocking CORS request if upgrade_display_content pref is disabled. If it is enabled ( which is currently the case in Firefox Nightly) it does upgrade display mixed content even if the request has the cors mode. Latter is also true for the default mixed content implementation of Chrome

@annevk
Copy link
Member

annevk commented Jan 13, 2023

I guess @carlosjoan91 and @estark37 need to weigh in why the decision was made differently here. And perhaps you know why Firefox did this differently?

This would potentially result in requests being made that have been blocked for a long time now. Probably no big security downside, but it might impact performance. And it's also rather inconsistent with the "imageset" decision.

@mozfreddyb
Copy link

To be clear, upgrading CORS requests is only in Firefox Nightly. We are currently tracking this in bug 1806114, as part of our road to improving the implementation in alignment with the spec and the wider ecosystem. Hence the question..

@estark37 is Chrome going to keep upgrading CORS requests or do you intent to align with the spec?

@mozfreddyb
Copy link

After some additional testing, we have learned that both Firefox (before upgrading any mixed content) and Safari Tech Preview 161 allow passive mixed content even if the request mode is CORS.

For those who wish to follow along, here's how I tested

  • on an HTTPS web page (e.g., https://example.com)
  • insert into DevTools Console
  • document.body.insertAdjacentHTML('beforeend', '<img src="http://httpbin.org/image/jpeg" crossorigin="anonymous" alt="HTTP loaded image - mixed passive content with CORS=anonymous">')
  • Note how the image is loading as passive mixed content

I therefore propose that we change the algorithm "4.1. Upgrade a mixed content request to a potentially trustworthy URL, if appropriate" and remove its 4th step ("request’s mode is CORS.`"), so that it will start allowing an upgrade of CORS requests.

moztomer added a commit to moztomer/webappsec-mixed-content that referenced this issue Jan 18, 2023
As mentioned in w3c#63 (comment). 
It might be worth to update the spec's upgrading algorithm. Because exempting CORS requests could lead to insecure loads via HTTP. 

Also, if we were blocking CORS request we wouldn't break any CORS request with trying to autoupgrade it.
@mikewest
Copy link
Member

I'd like @carlosjoan91 and @estark37 to weigh in, but FWIW: if we're upgrading and not blocking, I think it's reasonable to reconsider both the CORS and imageset carveouts. Neither make as much sense as carveouts to drive mixed content down over time if we're going to correct them (and therefore eradicate mixed content entirely).

@estark37
Copy link
Collaborator

I'm sorry I missed this discussion!

First of all I strongly suspect that we didn't make any conscious decision about upgrading CORS requests when implementing autoupgrading in Chrome, and whatever behavior exists today is just what our implementation happened to do. In other words, it's probably a bug.

I agree with @mozfreddyb that it's fine to change the spec to upgrade passive mixed CORS requests. When we shipped autoupgrading, we did want to avoid upgrading requests that had previously been blocked, because it seemed like that could introduce the risk of unexpected behavior -- possibly even security consequences -- without much benefit. But since it sounds like browsers have been already allowing passive mixed CORS requests to load, I don't think there's any harm in upgrading them. I think active mixed CORS requests should stay blocked (hopefully all browsers are already blocking them) and not upgraded.

I believe Chrome at least is correctly blocking and not upgrading imageset, so I think we should leave that one as it is, under the principle that we shouldn't start upgrading and loading a type of request that is already blocked.

@mozfreddyb
Copy link

Looks like there is wide agreement then. FYI: I've been curious about the Webkit position so I filed a standards position request in WebKit/standards-positions#124.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants